Bug 31320 - Illegal read with gfortran.dg/alloc_comp_assign_2.f90 and *_3.f90
Summary: Illegal read with gfortran.dg/alloc_comp_assign_2.f90 and *_3.f90
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.3.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2007-03-22 23:16 UTC by Tobias Burnus
Modified: 2007-07-17 17:58 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2007-07-05 14:31:03


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Burnus 2007-03-22 23:16:45 UTC
gfortran -g gfortran.dg/alloc_comp_assign_2.f90
valgrind ./a.out

==30049== Invalid read of size 1
==30049==    at 0x4C24854: memcpy (in /usr/lib64/valgrind/amd64-linux/vgpreload_memcheck.so)
==30049==    by 0x401682: MAIN__ (in /projects/tob/gcc/gcc/testsuite/gfortran.dg/a.out)
==30049==    by 0x405FAB: main (fmain.c:22)
==30049==  Address 0x404D64B is 3 bytes after a block of size 16 alloc'd
==30049==    at 0x4C22C56: malloc (in /usr/lib64/valgrind/amd64-linux/vgpreload_memcheck.so)
==30049==    by 0x4E3DD78: _gfortrani_get_mem (memory.c:53)
==30049==    by 0x400A5C: MAIN__ (in /projects/tob/gcc/gcc/testsuite/gfortran.dg/a.out)
==30049==    by 0x405FAB: main (fmain.c:22)

I checked the example itself with g95 and NAG f95 and both fail at the second, third and fourth "call abort()" whereas ifort has a segv.
Is this example (PR 20541) really valid?

A check with NAG f95, g95 and ifort with valgrind shows no problem for NAG f95, and invalid reads for g95 and ifort.

For alloc_comp_assign_3.f90: Compiles with g95, ifort and NAG f95 and gives no "call abort()", however, gfortran has:

==30454== Invalid read of size 1
==30454==    at 0x4C24854: memcpy (in /usr/lib64/valgrind/amd64-linux/vgpreload_memcheck.so)
==30454==    by 0x401223: MAIN__ (alloc_comp_assign_3.f90:22)
==30454==    by 0x40444B: main (fmain.c:22)
==30454==  Address 0x404D64B is 3 bytes after a block of size 16 alloc'd
==30454==    at 0x4C22C56: malloc (in /usr/lib64/valgrind/amd64-linux/vgpreload_memcheck.so)
==30454==    by 0x4E3DD78: _gfortrani_get_mem (memory.c:53)
==30454==    by 0x400A5C: MAIN__ (alloc_comp_assign_3.f90:19)
==30454==    by 0x40444B: main (fmain.c:22)

For alloc_comp_initializer_1.f90 the same.
Comment 1 Paul Thomas 2007-03-24 08:16:35 UTC
(In reply to comment #0)

> Is this example (PR 20541) really valid?

Lahey does not complain.

Paul 

Comment 2 Tobias Burnus 2007-04-12 10:52:18 UTC
> > Is this example (PR 20541) really valid?
> Lahey does not complain.

At compile time? Or at run time?
Comment 3 Daniel Franke 2007-07-05 14:31:03 UTC
Reduced testcase:

$> cat pr31320.f90
  type :: a
    integer, allocatable :: i(:)
  end type a
  type(a) :: x, y

  x = a ((/1, 2, 3, 4/))
  y = x
end

$> gfortran-svn -g -Wall pr31320.f90
$> valgrind --tool=memcheck --leak-check=full --show-reachable=yes a.out
[...]
==3173== Invalid read of size 1
==3173==    at 0x4022CC1: memcpy (mc_replace_strmem.c:406)
==3173==    by 0x80488AE: MAIN__ (alloc_comp_assign_2e.f90:8)
==3173==    by 0x8048938: main (fmain.c:22)
==3173==  Address 0x422C5C3 is 3 bytes after a block of size 16 alloc'd
==3173==    at 0x40213D0: malloc (vg_replace_malloc.c:149)
==3173==    by 0x8048710: MAIN__ (alloc_comp_assign_2e.f90:7)
==3173==    by 0x8048938: main (fmain.c:22)
[...]

In addition, while reducing this, I saw different kinds of other errors, most notably unnecessary free's and leaking memory.
Comment 4 Daniel Franke 2007-07-05 16:09:19 UTC
Some more examples. Uncomment either assignment to 'y' to have valgrind report the errors indicated at the end of the line:

  type :: a
    integer, allocatable :: i(:)
  end type a
  type(a) :: x, y

  x = a ((/ 1,2,3 /))
! y = a (x%i(1:3))       ! ok
! y = a (x%i(1:))        ! invalid read
! y = a (x%i(:3))        ! invalid read
! y = a (x%i(:))         ! invalid read, memory leak
! y = a (x%i)            ! invalid read, memory leak
! y = x                  ! invalid read
end
Comment 5 Daniel Franke 2007-07-05 16:32:57 UTC
Tobias, good catch :)

It seems to be another off-by-one issue. Comparing the tree dumps from different assignments as shown in comment #4, one learns (copied only the relevant lines, between any two adjacent lines any amount of other code was removed):

    struct a a.0;
      struct array1_int4 parm.2;
      parm.2.dim[0].ubound = 3;
      a.0.i = (struct array1_int4) parm.2;             /* ubound == 3 */
      a.0.i.dim[0].ubound = a.0.i.dim[0].ubound + 1;   /* ubound == 4 (!) */
    x = a.0;

Adding
  print *, ubound(x%i, 1), ubound(y%i, 1)
to the source gives 4/4 instead of 3/3 as one would expect from the initalizer.
Comment 6 Paul Thomas 2007-07-13 13:47:17 UTC
(In reply to comment #5)

>     struct a a.0;
>       struct array1_int4 parm.2;
>       parm.2.dim[0].ubound = 3;
>       a.0.i = (struct array1_int4) parm.2;             /* ubound == 3 */
>       a.0.i.dim[0].ubound = a.0.i.dim[0].ubound + 1;   /* ubound == 4 (!) */
>     x = a.0;
> Adding
>   print *, ubound(x%i, 1), ubound(y%i, 1)
> to the source gives 4/4 instead of 3/3 as one would expect from the initalizer.

This all happens in trans-expr.c(gfc_trans_subcomponent_assign):3007
The change for 0 to unity based indexing is done incorrectly - apparently, what is assumed to be zero based comes through as something else... sometimes!

I think that the right thing to do here is to use the array_spec lower bound as the base and to use the expression upper-lower as the range.

I'll see if I cannot post a fix in the next couple of days.

Paul
 
Comment 7 Tobias Burnus 2007-07-16 12:57:30 UTC
For the fix for this PR and for PR32665, see:

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/msg01417.html
Comment 8 Paul Thomas 2007-07-17 17:07:54 UTC
(In reply to comment #7)
Tobias,

Does the patch fix this, please? The testcase that you sent me breaks gfortran in other places and probably should have a PR all of its very own.  (try the FORALL part separately).

Cheers

Paul
Comment 9 Paul Thomas 2007-07-17 17:23:03 UTC
Subject: Bug 31320

Author: pault
Date: Tue Jul 17 17:22:44 2007
New Revision: 126703

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

	PR fortran/31320
	PR fortran/32665
	* trans-expr.c (gfc_trans_subcomponent_assign): Ensure that
	renormalization unity base is done independently of existing
	lbound value.
	(gfc_trans_scalar_assign): If rhs is not a variable, put
	lse->pre after rse->pre to ensure that de-allocation of lhs
	occurs after evaluation of rhs.

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

	PR fortran/31320
	PR fortran/32665
	* gfortran.dg/alloc_comp_constructor_3.f90: New test.


Added:
    trunk/gcc/testsuite/gfortran.dg/alloc_comp_constructor_3.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-expr.c
    trunk/gcc/testsuite/ChangeLog

Comment 10 Tobias Burnus 2007-07-17 17:58:10 UTC
> Does the patch fix this, please?
Yes, otherwise I would have blamed you in the patch review ;-)

> The testcase that you sent me breaks gfortran
> in other places and probably should have a PR all of its very own. (try the
> FORALL part separately).

I now filled PR 32795; I had hoped it would be something obvious, but it is seemingly not.

Thanks for fixing this PR!