Bug 34955 - transfer_assumed_size_1.f90: Valgrind error: invalid read of size 3
Summary: transfer_assumed_size_1.f90: Valgrind error: invalid read of size 3
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.3.0
: P3 normal
Target Milestone: 4.4.0
Assignee: Paul Thomas
URL:
Keywords: wrong-code
Depends on:
Blocks: 32834
  Show dependency treegraph
 
Reported: 2008-01-24 16:05 UTC by Tobias Burnus
Modified: 2009-01-30 17:36 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2008-12-18 22:29:38


Attachments
A preliminary patch for the PR (1.80 KB, patch)
2009-01-12 16:20 UTC, Paul Thomas
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Burnus 2008-01-24 16:05:47 UTC
./transfer_assumed_size_1.f90==29961== Invalid read of size 1
==29961==    at 0x4C26B50: memmove (in /usr/lib64/valgrind/amd64-linux/vgpreload_memcheck.so)
==29961==    by 0x400EA5: __transferbug_MOD_bytestostring (transfer_assumed_size_1.f90:31)
==29961==    by 0x401657: MAIN__ (transfer_assumed_size_1.f90:41)
==29961==    by 0x40174B: main (fmain.c:21)
==29961==  Address 0x58c5793 is 13 bytes before a block of size 2 free'd
==29961==    at 0x4C2430F: free (in /usr/lib64/valgrind/amd64-linux/vgpreload_memcheck.so)
Comment 1 Jerry DeLisle 2008-02-16 07:08:54 UTC
Take a look at this with valgrind after compiling with -m32.  Very interesting
Comment 2 Francois-Xavier Coudert 2008-02-28 16:01:57 UTC
Tobias, what target and options are you compiling with? I can't reproduce this on x86_64-linux...
Comment 3 Francois-Xavier Coudert 2008-02-29 19:50:29 UTC
(In reply to comment #2)
> Tobias, what target and options are you compiling with? I can't reproduce this
> on x86_64-linux...

Nor on i686-linux.
Comment 4 Daniel Franke 2008-03-16 13:01:46 UTC
$> gfortran-svn -v
Target: i686-pc-linux-gnu
gcc version 4.4.0 20080315 (experimental) (GCC)

$> gfortran-svn -g -Wall -W transfer_assumed_size_1.f90 
$> valgrind --tool=memcheck --leak-check=full a.out
[...]
==6291== Invalid read of size 1
==6291==    at 0x4023A83: memmove (mc_replace_strmem.c:516)
==6291==    by 0x8048C12: __transferbug_MOD_bytestostring (transfer_assumed_size_1.f90:31)
==6291==    by 0x80493A0: MAIN__ (transfer_assumed_size_1.f90:41)
==6291==    by 0x80494A8: main (fmain.c:21)
==6291==  Address 0x425067b is 13 bytes before a block of size 2 free'd
==6291==    at 0x402248F: free (vg_replace_malloc.c:323)
==6291==    by 0x804912E: __transferbug_MOD_stringtobytes (transfer_assumed_size_1.f90:21)
==6291==    by 0x8049383: MAIN__ (transfer_assumed_size_1.f90:41)
==6291==    by 0x80494A8: main (fmain.c:21)
==6291==
==6291== Invalid read of size 1
==6291==    at 0x4023A90: memmove (mc_replace_strmem.c:516)
==6291==    by 0x8048C12: __transferbug_MOD_bytestostring (transfer_assumed_size_1.f90:31)
==6291==    by 0x80493A0: MAIN__ (transfer_assumed_size_1.f90:41)
==6291==    by 0x80494A8: main (fmain.c:21)
==6291==  Address 0x4250679 is 15 bytes before a block of size 2 free'd
==6291==    at 0x402248F: free (vg_replace_malloc.c:323)
==6291==    by 0x804912E: __transferbug_MOD_stringtobytes (transfer_assumed_size_1.f90:21)
==6291==    by 0x8049383: MAIN__ (transfer_assumed_size_1.f90:41)
==6291==    by 0x80494A8: main (fmain.c:21)
[...]
Comment 5 Paul Thomas 2008-03-21 20:35:10 UTC
FX,

You'll be glad to know that your memory leak patch reports:

!! Memory deallocation error in the code generated by the GNU Fortran compiler.

!! Please report this issue to http://gcc.gnu.org/bugzilla/

Even on Cygwin_NT:)

Paul
Comment 6 Francois-Xavier Coudert 2008-03-24 18:43:13 UTC
OK, I now can reproduce it. Here's a reduced testcase:

module TransferBug
  type ByteType
    integer(kind=1) :: singleByte
  end type
  type(ByteType) :: BytesPrototype(1)
contains
  function StringToBytes(v) result (bytes)
    character(len=2) :: v
    type (ByteType) :: bytes(size(transfer(v, BytesPrototype)))
    bytes = transfer('Hi', BytesPrototype)
  end function

  subroutine BytesToString(bytes, string)
    type (ByteType) :: bytes(2)
    character(len=*) :: string
    string = transfer(bytes, string)
  end subroutine
end module

program main
  use TransferBug
  character(len=3) :: str
  call BytesToString( StringToBytes('Hi'), str )
end program
Comment 7 Hans-Peter Nilsson 2008-04-09 23:49:00 UTC
For cris-elf, this graduated to a runtime error equivalent to SEGV (no valgrind); worked with 134139, failed from 134147.
Comment 8 Hans-Peter Nilsson 2008-04-09 23:56:04 UTC
Try what?  I always start with a clean slate; an empty build directory!
Perhaps you mean something else?
Comment 9 Hans-Peter Nilsson 2008-04-09 23:57:03 UTC
(In reply to comment #8)
> Try what?  I always start with a clean slate; an empty build directory!
> Perhaps you mean something else?

Oops, replied in wrong PR... please ignore.

Comment 10 Hans-Peter Nilsson 2008-05-14 23:58:32 UTC
Worked again with trunk as of r134973, stopped working again as in comment #7
with r135298.  I don't see any signs of this bug actively fixed?
Comment 11 Francois-Xavier Coudert 2008-05-15 07:46:32 UTC
(In reply to comment #10)
> I don't see any signs of this bug actively fixed?

I wanted to look into it, but I can't reproduce it any more on x86_64-linux with current mainline.
Comment 12 Hans-Peter Nilsson 2008-05-15 10:42:36 UTC
(In reply to comment #11)
> (In reply to comment #10)
> > I don't see any signs of this bug actively fixed?
> 
> I wanted to look into it, but I can't reproduce it any more on x86_64-linux
> with current mainline.

The bug is apparently of the kind that hides from time to time.
If you want to look into it, I suggest you use one of the mentioned revisions. 
IIUC you once had a revision where you could repeat it?
For CRIS, the bug hid again (or with less probability, was fixed) with r135315, no surprises.
Comment 13 Jerry DeLisle 2008-05-15 13:22:03 UTC
valgrind says:

==3482== 474 bytes in 3 blocks are definitely lost in loss record 4 of 11
==3482==    at 0x4A04D1F: calloc (vg_replace_malloc.c:279)
==3482==    by 0xB3500A: xcalloc (xmalloc.c:162)
==3482==    by 0x558A47: init_emit (emit-rtl.c:5012)
==3482==    by 0x5F0B52: prepare_function_start (function.c:3903)
==3482==    by 0x5F2B68: init_function_start (function.c:3950)
==3482==    by 0x4916AD: trans_function_start (trans-decl.c:1599)
==3482==    by 0x49847D: gfc_generate_function_code (trans-decl.c:3108)
==3482==    by 0x47F921: gfc_generate_module_code (trans.c:1206)
==3482==    by 0x454AFB: gfc_parse_file (parse.c:3580)
==3482==    by 0x47C69D: gfc_be_parse_file (f95-lang.c:258)
==3482==    by 0x6E1140: toplev_main (toplev.c:962)
==3482==    by 0x3FF061E073: (below main) (libc-start.c:220)
==3482== 
==3482== 
==3482== 816 bytes in 6 blocks are definitely lost in loss record 5 of 11
==3482==    at 0x4A059F6: malloc (vg_replace_malloc.c:149)
==3482==    by 0xB34FCB: xrealloc (xmalloc.c:177)
==3482==    by 0x8C53B0: vec_heap_o_reserve_1 (vec.c:176)
==3482==    by 0x50336D: insn_locators_alloc (vecprim.h:27)
==3482==    by 0xA8FEA8: tree_expand_cfg (cfgexpand.c:1850)
==3482==    by 0x6656A1: execute_one_pass (passes.c:1252)
==3482==    by 0x6658A0: execute_pass_list (passes.c:1302)
==3482==    by 0x745C85: tree_rest_of_compilation (tree-optimize.c:421)
==3482==    by 0x8F4A01: cgraph_expand_function (cgraphunit.c:1148)
==3482==    by 0x8F67A3: cgraph_assemble_pending_functions (cgraphunit.c:514)
==3482==    by 0x8F5CF4: cgraph_finalize_function (cgraphunit.c:632)
==3482==    by 0x499705: gfc_generate_function_code (trans-decl.c:3371)
==3482== 
==3482== 
==3482== 91,856 (70,320 direct, 21,536 indirect) bytes in 1,827 blocks are definitely lost in loss record 10 of 11
==3482==    at 0x4A059F6: malloc (vg_replace_malloc.c:149)
==3482==    by 0xB35057: xmalloc (xmalloc.c:147)
==3482==    by 0x447514: gfc_getmem (misc.c:37)
==3482==    by 0x470C37: gfc_get_namespace (symbol.c:2079)
==3482==    by 0x4544B1: parse_contained (parse.c:3103)
==3482==    by 0x454C82: gfc_parse_file (parse.c:3409)
==3482==    by 0x47C69D: gfc_be_parse_file (f95-lang.c:258)
==3482==    by 0x6E1140: toplev_main (toplev.c:962)
==3482==    by 0x3FF061E073: (below main) (libc-start.c:220)
==3482== 
==3482== LEAK SUMMARY:
==3482==    definitely lost: 71,610 bytes in 1,836 blocks.
==3482==    indirectly lost: 21,536 bytes in 145 blocks.
==3482==      possibly lost: 64 bytes in 2 blocks.
==3482==    still reachable: 335,995 bytes in 1,228 blocks.
==3482==         suppressed: 0 bytes in 0 blocks.
Comment 14 Francois-Xavier Coudert 2008-05-15 13:23:24 UTC
(In reply to comment #13)
> valgrind says

What target? I don't see anything.
Comment 15 Jerry DeLisle 2008-05-15 13:30:56 UTC
x86-64-linux with valgrind --leak-check=full f951 transfer_assumed_size_1.f90

Using built-in specs.
Target: x86_64-unknown-linux-gnu
Configured with: ../gcc44/configure --prefix=/home/jerry/gcc/usr --enable-languages=c,fortran --disable-libmudflap --enable-libgomp --with-mpfr-lib=/home/jerry/gcc/usr/lib --with-mpfr-include=/home/jerry/gcc/usr/include --disable-bootstrap
Thread model: posix
gcc version 4.4.0 20080514 (experimental) [trunk revision 134151] (GCC) 
Comment 16 Francois-Xavier Coudert 2008-05-15 13:41:00 UTC
(In reply to comment #15)
> x86-64-linux with valgrind --leak-check=full f951 transfer_assumed_size_1.f90

Oh, I see. But the rest of the PR is about running the compiled program. I don't think the two are related. For what it's worth, the following is a reduced testcase for the compiler leak issue:

  type ByteType
    integer(kind=1) :: singleByte
  end type
  type(ByteType) :: bytes

  bytes = transfer(' ', bytes)
  end
Comment 17 Tobias Burnus 2008-05-15 13:55:17 UTC
On AMD Atholon64 x2 4800+ with openSUSE 11 (x86-64 beta3) I still get the following valgrind message with today's gfortran (4.4.0 20080515 [trunk revision 135326]):

$ gfortran -g gfortran.dg/transfer_assumed_size_1.f90
$ valgrind ./a.out
[...]
Invalid read of size 1
  at 0x4C26AF0: memmove (in /usr/lib64/valgrind/amd64-linux/vgpreload_memcheck.so)
  by 0x400E9F: __transferbug_MOD_bytestostring (transfer_assumed_size_1.f90:31)
  by 0x40163C: MAIN__ (transfer_assumed_size_1.f90:41)
  by 0x40172B: main (fmain.c:21)
Address 0x58e27cb is 13 bytes before a block of size 2 free'd
  at 0x4C2430F: free (in /usr/lib64/valgrind/amd64-linux/vgpreload_memcheck.so)
  by 0x4013AD: __transferbug_MOD_stringtobytes (transfer_assumed_size_1.f90:21)
  by 0x401624: MAIN__ (transfer_assumed_size_1.f90:41)
  by 0x40172B: main (fmain.c:21)
Comment 18 Mikael Morin 2008-11-17 19:43:13 UTC
The problem is in ByteToString. 
The assignment of the transfer result is changed to a memmove. 
The memmove is controlled by the size of both the lhs and the rhs. 
The size of the rhs (actually the charlen=3 in this case) is not the same as that of the actual data, as the charlen was copied from string's typespec in transfer. 
Thus, we are copying 3 bytes, instead of 2. 

I think the code is still a valid program and a wrong-code, as while the standard allow to have a result (partially) undefined/processor-dependent, it doesn't allow to read unallocated memory. 


module TransferBug
  type ByteType
    integer(kind=1) :: singleByte
  end type
  type(ByteType) :: BytesPrototype(2)
contains
  function StringToBytes(v) result (bytes)
    character(len=2) :: v
    type (ByteType) :: bytes(2)!size(transfer(v, BytesPrototype)))
    bytes = transfer('Hi', BytesPrototype)
  end function

  subroutine BytesToString(bytes, string)
    type (ByteType) :: bytes(2)
    character(len=*) :: string
    string = transfer(bytes, string)
  end subroutine
end module

program main
  use TransferBug
  character(len=3) :: str
  type(ByteType) :: i(1)
  call BytesToString( StringToBytes('Hi'), str )
end program



Valgrind doesn't complain on this testcase, but I strongly suspect that the bug is still there. Using the commented array size forces the creation of temporaries, and then valgrind catches the error. 


Now, how to fix this?
We could unconditionally create a temporary in transfer, so that we have both
the original data size and the target data size at the time of the memmove. 
That would probably mean a lot of regressions, and not only speed ones. 
We could keep track of the original data size.
I must admit I'm not satisfied with this one either. 
What else?
Comment 19 Paul Thomas 2008-12-18 22:29:38 UTC
I have noticed that the scalar TRANSFER does not respect the undefined condition for the rest of the result if the SOURCE size is less than the MOLD size.  I have a patch for this, which is probably not complete but goes in the right direction. In particular, it assumes that array types are compelete, which was not the case when array TRANSFER was written. My inclination is to combine the two to ensure complete bomb-proofness.  I will post my provisional patch tomorrow.

Paul
Comment 20 Paul Thomas 2009-01-12 16:20:41 UTC
Created attachment 17079 [details]
A preliminary patch for the PR

A slightly earlier version of this regtested OK.  Will do it to this version tonight and then will submit.

Cheers

Paul
Comment 21 Dominique d'Humieres 2009-01-14 08:15:07 UTC
On i686-apple-darwin9 I cannot test the valgrind part of this pr, however with the patch in http://gcc.gnu.org/ml/fortran/2009-01/msg00162.html the following test now succeeds:

  character(len=1)  :: string = "z"
  character(len=20) :: tmp = ""
  tmp = Upper ("abcdefgh")
  print *, tmp
 contains
  Character (len=20) Function Upper (string)
    Character(len=*) string
    integer :: ij
    print *, len(string)
    print *, size(transfer(string,"xy",len(string)))
    i = size(transfer(string,"xy",len(string)))
    if (i /= len(string)) call abort()
    Upper = ""
    Upper(1:2) =                                                                &
     transfer(merge(transfer(string,"xy",len(string)),    &
       string(1:2), .true.), "xy")
    return
  end function Upper
end

(coming from pr31608). I saw one regression on char_cast_1.f90 which needs some adjustment of the test, the following change allows it to pass:

--- ../_gcc_clean/gcc/testsuite/gfortran.dg/char_cast_1.f90	2008-05-19 14:20:35.000000000 +0200
+++ gcc/testsuite/gfortran.dg/char_cast_1.f90	2009-01-14 07:37:03.000000000 +0100
@@ -27,5 +27,5 @@
 end
 ! The sign that all is well is that [S.5][1] appears twice.
 ! Platform dependent variations are [S$5][1], [__S_5][1], [S___5][1]
-! { dg-final { scan-tree-dump-times "5\\\]\\\[1\\\]" 2 "original" } }
+! { dg-final { scan-tree-dump-times "6\\\]\\\[1\\\]" 2 "original" } }
 ! { dg-final { cleanup-tree-dump "original" } }

From the comment the test seems fragile and should probably changed to something more robust.

Thanks for the patch.
Comment 22 Paul Thomas 2009-01-17 11:32:13 UTC
Subject: Bug 34955

Author: pault
Date: Sat Jan 17 11:32:02 2009
New Revision: 143462

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

	PR fortran/34955
	* trans-intrinsic.c (gfc_conv_intrinsic_array_transfer): Has
	been absorbed into gfc_conv_intrinsic_transfer. All
	references to it in trans-intrinsic.c have been changed
	accordingly.  PR fixed by using a temporary for scalar
	character transfer, when the source is shorter than the
	destination.

2009-01-17  Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/34955
	* gfortran.dg/transfer_intrinsic_1.f90: New test.
	* gfortran.dg/transfer_intrinsic_2.f90: New test.


Added:
    trunk/gcc/testsuite/gfortran.dg/transfer_intrinsic_1.f90
    trunk/gcc/testsuite/gfortran.dg/transfer_intrinsic_2.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-intrinsic.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gfortran.dg/char_cast_1.f90

Comment 23 Paul Thomas 2009-01-17 11:34:48 UTC
Fixed on trunk.

Thanks for the report and sorry that it took so long to fix.

Paul