Bug 31205 - aliased operator assignment produces wrong result
Summary: aliased operator assignment produces wrong result
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.3.0
: P3 normal
Target Milestone: ---
Assignee: Paul Thomas
URL:
Keywords: wrong-code
Depends on:
Blocks: 32834
  Show dependency treegraph
 
Reported: 2007-03-16 11:28 UTC by Joost VandeVondele
Modified: 2007-07-24 19:19 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2007-06-28 08:04:33


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Joost VandeVondele 2007-03-16 11:28:14 UTC
With a recent gfortran, the following compiles, but generates the wrong results:

MODULE TT
 TYPE data_type
   INTEGER :: I=2
 END TYPE data_type
 INTERFACE ASSIGNMENT (=)
   MODULE PROCEDURE set
 END INTERFACE
CONTAINS
  PURE SUBROUTINE set(x1,x2)
    TYPE(data_type), INTENT(IN) :: x2
    TYPE(data_type), INTENT(OUT) :: x1
    CALL S1(x1,x2)
  END SUBROUTINE
  PURE SUBROUTINE S1(x1,x2)
    TYPE(data_type), INTENT(IN) :: x2
    TYPE(data_type), INTENT(OUT) :: x1
    x1%i=x2%i
  END SUBROUTINE
END MODULE

USE TT
TYPE(data_type) :: D,E

D%I=4
D=D

E%I=4
CALL set(E,(E))

IF (D%I.NE.4) CALL ABORT()
IF (4.NE.E%I) CALL ABORT()
END
Comment 1 Paul Thomas 2007-03-17 07:26:16 UTC
The aliasing of the arguments to subroutine set results in the initialization of the intent(out) also being applied to the intent(in) argument.

I feel a temporary coming on.

Paul
Comment 2 Andrew Pinski 2007-03-17 07:38:19 UTC
This is related to PR 14771, most likely the parentheses are being ignored.
Comment 3 Tobias Burnus 2007-04-15 18:39:24 UTC
Minimal example:

program test
  implicit none
  type data_type
    integer :: i=2
  end type data_type
  type(data_type) :: d
  d%i = 4
  call set(d,d)
contains
  subroutine set(x1,x2)
    type(data_type),intent(out):: x1
    type(data_type),intent(in) :: x2
    x1%i = x2%i
  end subroutine set
end program test

Hereby,
  d%i = 4
  call set(d,d)
is translated into:
  d.i = 4;
  {
    struct data_type data_type.0;
    data_type.0.i = 2;
    d = data_type.0;
  }
  set (&d, &d);
If one removes the "intent(out)" for x1, data_type.0 is not created and everything is fine. If one creates a temporary structure, shouldn't one have something like the following? (Handcrafted)
  d.i = 4;
  {
    struct data_type data_type.0;
    data_type.0.i = 2;
    set (&data_type.0, &d);
    d = data_type.0;
  }

There is actually a difference how gfortran and how g95 and NAG f95 do the default initialization. For the following Fortran code

program test
  implicit none
  type data_type
    integer :: i=2
  end type data_type
  type(data_type) :: d
  d%i = 4
  call set(d)
  print *, d%i
contains
  subroutine set(x1)
    type(data_type),intent(out):: x1
  end subroutine set
end program test

"2" is printed (by all compilers). gfortran does:
  d.i = 4;
  {
    struct data_type data_type.0;
    data_type.0.i = 2;
    d = data_type.0;
  }
  set (&d);
set (x1)
{
  (void) 0;

whereas g95 does:
  d.i = 4;
  set_ (&d);;
set_ (x1)
{
  int4 D.470;
  D.470 = 0;
  if (x1 != 0B)
      *x1 = {.i=2};
  return D.470;;

analogously does NAG f95:
 d_.i_ = 2;
 d_.i_ = 4;
 test_IP_set(&d_);

static void test_IP_set(x1_)
     struct test_DT_data_type *x1_;
{
  x1_->i_ = 2;

Though, I don't know which method is better.
Comment 4 Paul Thomas 2007-06-27 15:56:27 UTC
(In reply to comment #2)
> This is related to PR 14771, most likely the parentheses are being ignored.

The parentheses are being ignored - in fact they disappear completely; I presume that gfc_simplify_expr is the culprit.

In addition, a temporary needs to be made for intent(out), derived types with a default initializer and the initialization applied to that, when the variable is aliassed.

I note that other compilers apply the initialization in the callee, whereas gfortran leaves that duty to the caller.  I think that the former is "cleaner" in some sense and that we should make the change.

Thus, this little beauty comprises at least two bugs and should probably be three PRs:-)  I propose that, for the sake of tractability, it should be left as it is.

Paul


Comment 5 Paul Thomas 2007-06-28 08:04:33 UTC
(In reply to comment #4)
> (In reply to comment #2)
> > This is related to PR 14771, most likely the parentheses are being ignored.
> The parentheses are being ignored - in fact they disappear completely; I
> presume that gfc_simplify_expr is the culprit.

Not so - it is matchexp.c, where there is an incorrect interpretation of the standard, such that parentheses are not applied to non-numeric expressions.  All expressions in parentheses are data entities.  Applying this (modifying gfc_get_parentheses to resolve the expression before doing anything with it.) causes one or two regressions because character lengths need to be carried over (I think!).  The second test in this PR now works.

> In addition, a temporary needs to be made for intent(out), derived types with a
> default initializer and the initialization applied to that, when the variable
> is aliassed.
> I note that other compilers apply the initialization in the callee, whereas
> gfortran leaves that duty to the caller.  I think that the former is "cleaner"

Applying gfc_get_parentheses for this case, in resolve_code at the point where module operator assignments are detected, fixes the 1st test in thi PR, as long as the initialization is shifted to the callee.  This latter was effected by writing a helper function to write an lvalue expression from a symbol; this has at least one other existing client and does not apparently cause any regressions.
 
> in some sense and that we should make the change.
> Thus, this little beauty comprises at least two bugs and should probably be
> three PRs:-)  I propose that, for the sake of tractability, it should be left
> as it is.
> Paul

I am down to 5 regressions for the overall patch, so I had better assign myself the PR!

Paul

Comment 6 patchapp@dberlin.org 2007-06-30 16:19:08 UTC
Subject: Bug number PR31205

A patch for this bug has been added to the patch tracker.
The mailing list url for the patch is http://gcc.gnu.org/ml/gcc-patches/2007-06/msg02151.html
Comment 7 patchapp@dberlin.org 2007-07-24 07:55:26 UTC
Subject: Bug number PR31205

A patch for this bug has been added to the patch tracker.
The mailing list url for the patch is http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01709.html
Comment 8 Paul Thomas 2007-07-24 19:15:42 UTC
Subject: Bug 31205

Author: pault
Date: Tue Jul 24 19:15:27 2007
New Revision: 126885

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=126885
Log:
2007-07-24 Paul Thomas <pault@gcc.gnu.org>

	PR fortran/31205
	PR fortran/32842
	* trans-expr.c (gfc_conv_function_call): Remove the default
	initialization of intent(out) derived types.
	* symbol.c (gfc_lval_expr_from_sym): New function.
	* matchexp.c (gfc_get_parentheses): Return argument, if it is
	character and posseses a ref.
	* gfortran.h : Add prototype for gfc_lval_expr_from_sym.
	* resolve.c (has_default_initializer): Move higher up in file.
	(resolve_code): On detecting an interface assignment, check
	if the rhs and the lhs are the same symbol.  If this is so,
	enclose the rhs in parenetheses to generate a temporary and
	prevent any possible aliasing.
	(apply_default_init): Remove code making the lval and call
	gfc_lval_expr_from_sym instead.
	(resolve_operator): Give a parentheses expression a type-
	spec if it has no type.
	* trans-decl.c (gfc_trans_deferred_vars): Apply the a default
	initializer, if any, to an intent(out) derived type, using
	gfc_lval_expr_from_sym and gfc_trans_assignment.  Check if
	the dummy is present.


2007-07-24 Paul Thomas <pault@gcc.gnu.org>

	PR fortran/31205
	* gfortran.dg/alloc_comp_basics_1.f90 : Restore number of
	"deallocates" to 24, since patch has code rid of much spurious
	code.
	* gfortran.dg/interface_assignment_1.f90 : New test.

	PR fortran/32842
	* gfortran.dg/interface_assignment_2.f90 : New test.

Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/gfortran.h
    trunk/gcc/fortran/matchexp.c
    trunk/gcc/fortran/resolve.c
    trunk/gcc/fortran/symbol.c
    trunk/gcc/fortran/trans-decl.c
    trunk/gcc/fortran/trans-expr.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gfortran.dg/alloc_comp_basics_1.f90

Comment 9 Paul Thomas 2007-07-24 19:16:49 UTC
Subject: Bug 31205

Author: pault
Date: Tue Jul 24 19:16:36 2007
New Revision: 126886

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=126886
Log:
2007-07-24 Paul Thomas <pault@gcc.gnu.org>

	PR fortran/31205
	PR fortran/32842
	* trans-expr.c (gfc_conv_function_call): Remove the default
	initialization of intent(out) derived types.
	* symbol.c (gfc_lval_expr_from_sym): New function.
	* matchexp.c (gfc_get_parentheses): Return argument, if it is
	character and posseses a ref.
	* gfortran.h : Add prototype for gfc_lval_expr_from_sym.
	* resolve.c (has_default_initializer): Move higher up in file.
	(resolve_code): On detecting an interface assignment, check
	if the rhs and the lhs are the same symbol.  If this is so,
	enclose the rhs in parenetheses to generate a temporary and
	prevent any possible aliasing.
	(apply_default_init): Remove code making the lval and call
	gfc_lval_expr_from_sym instead.
	(resolve_operator): Give a parentheses expression a type-
	spec if it has no type.
	* trans-decl.c (gfc_trans_deferred_vars): Apply the a default
	initializer, if any, to an intent(out) derived type, using
	gfc_lval_expr_from_sym and gfc_trans_assignment.  Check if
	the dummy is present.


2007-07-24 Paul Thomas <pault@gcc.gnu.org>

	PR fortran/31205
	* gfortran.dg/alloc_comp_basics_1.f90 : Restore number of
	"deallocates" to 24, since patch has code rid of much spurious
	code.
	* gfortran.dg/interface_assignment_1.f90 : New test.

	PR fortran/32842
	* gfortran.dg/interface_assignment_2.f90 : New test.

Added:
    trunk/gcc/testsuite/gfortran.dg/interface_assignment_1.f90
    trunk/gcc/testsuite/gfortran.dg/interface_assignment_2.f90

Comment 10 Paul Thomas 2007-07-24 19:19:36 UTC
Fixed on trunk.

Thanks, Joost.

Paul