Bug 58270 - Wrong code while accessing trailing array elements in a global common structure
Summary: Wrong code while accessing trailing array elements in a global common structure
Status: RESOLVED DUPLICATE of bug 50463
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.6.4
: P3 critical
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2013-08-29 10:48 UTC by Krzysztof Strasburger
Modified: 2021-11-29 02:32 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.5.4
Known to fail: 4.6.4, 4.9.0
Last reconfirmed: 2013-09-02 00:00:00


Attachments
Isolated code giving wrong results (329 bytes, text/plain)
2013-08-29 10:48 UTC, Krzysztof Strasburger
Details
Second example, not working also with -fno-tree-dse -fno-dse (354 bytes, text/plain)
2013-08-29 12:21 UTC, Krzysztof Strasburger
Details
File containing main() for the second example (187 bytes, text/plain)
2013-08-29 12:23 UTC, Krzysztof Strasburger
Details
Example of failing FORTRAN code, with assembler output from gfortran 4.6.4 (728 bytes, application/octet-stream)
2013-09-02 08:24 UTC, Krzysztof Strasburger
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Krzysztof Strasburger 2013-08-29 10:48:45 UTC
Created attachment 30717 [details]
Isolated code giving wrong results

Array elements are accessed incorretly, if the array is passed in a global structure.
This bug exists in 4.6 and 4.7 up to 4.7.2, no info about newer 4.7, 4.8 and development.
Comment 1 Florian Weimer 2013-08-29 10:58:06 UTC
The compiler is free to assume that both i1 and i2 are zero and the first store is dead (because this is the only valid array index).  So if the buggy() function stores a value of 1.0 at mem.dmem[0] unconditionally, this is still correct.

struct {
    double dmem[1]; /* Change to dmem[2] and the bug disappears */
} mem;

void buggy(int i1, int i2) {
    mem.dmem[i1] = 0.5;
    mem.dmem[i2] = 1.;
}
Comment 2 Krzysztof Strasburger 2013-08-29 11:15:07 UTC
OK, I'm not and expert, but mem is a global structure and it can be of different size in other object file. The linker should assume the biggest of all, correct?
The example I posted comes from f2c-translated FORTRAN77 code (it is cleared from f2c references). It was a normal practice to mix C with FORTRAN for dynamic memory allocation. The memory allocated via malloc() was referenced to a small (one-element) static array. There was nothing illegal with this.
And how can the compiler assume freely that both i1 and i2 are zeros, if they are passed as actual arguments?
Comment 3 Krzysztof Strasburger 2013-08-29 11:29:01 UTC
The compiler option -fno-tree-dse does the job for me. Florian - thank you for using the term "dead store" ;). I'm not sure whether it should be enabled by default for a C compiler, but I'm not competent enough even to suggest a solution.
Comment 4 Krzysztof Strasburger 2013-08-29 12:19:40 UTC
Unfortunately, this is not the end of story. I'm going to attach a little more complicated example, for which even using -fno-dse -fno-tree-dse does not help.
Comment 5 Krzysztof Strasburger 2013-08-29 12:21:28 UTC
Created attachment 30719 [details]
Second example, not working also with -fno-tree-dse -fno-dse
Comment 6 Krzysztof Strasburger 2013-08-29 12:23:25 UTC
Created attachment 30720 [details]
File containing main() for the second example
Comment 7 Mikael Pettersson 2013-08-29 12:39:29 UTC
Your examples are invalid C.  In one module you present the compiler with a specific declaration, and complain when it utilizes constraints derived from that declaration.  Then in another module you have an _incompatible_ declaration for the same object.  You can't expect to get away with that, even if it seemed to work with an older compiler.

You should use a C99 "flexible array member", or a pointer (to an array of unknown size).
Comment 8 Krzysztof Strasburger 2013-08-29 13:32:47 UTC
Mikael,
I cannot agree. Do not look at main.c, as the compiler doesn't know anything about it while compiling buggy.c (this is the reason for which I keep main() separately) and doesn't know that i1, i2 and i3 may be set to something > 0 at runtime.
If it would be so much strict about declarations, it wouldn't also allow to modify mem.dmem[1] - everything would go into mem.dmem[0].
However, it writes mem.dmem[1] only (!) if compiled without -fno-tree-dse and mem.dmem[0] plus mem.dmem[1] with -fno-tree-dse.
The problem is that the compiler does not work predictably. BTW, correct size of the mem structure (global variable) is ensured by the linker:
$ nm buggy.o
00000000 T buggy
0000000c C loc
00000008 C mem
I would also expect that if the compiler is instructed explicitly to release some constraints, then these will be released.
Comment 9 Jakub Jelinek 2013-08-29 14:48:09 UTC
You are confusing how C/C++ commons work with how Fortran commons work.
Your examples are simply invalid C/C++.
Comment 10 Krzysztof Strasburger 2013-08-29 16:23:50 UTC
Jakub,
I do not care about C++ (never understood it), but commons are just commons. I see them from linker's perspective. How does the compiler treat variables belonging to that common - this is a different story.
I examined the assembler outputs and I think that the real problem is that the compiler treats one-element array (dmem) in buggy.c as ordinary variable. somewhere.
If dmem is declared as two-element array (so that nobody can assume blindly to which element data should go), then everything works correctly, regardless how it is declared elsewhere.
It is an overoptimization IMHO, but I'm just a user.
Comment 11 Krzysztof Strasburger 2013-09-02 08:20:39 UTC
I'm not going to discuss whether my example is a valid C code or not, but in FORTRAN it goes a similarly wrong way. The compiler treats incorrectly the one-element array in a common.
Comment 12 Krzysztof Strasburger 2013-09-02 08:24:51 UTC
Created attachment 30740 [details]
Example of failing FORTRAN code, with assembler output from gfortran 4.6.4
Comment 13 Dominique d'Humieres 2013-09-02 12:21:43 UTC
> Example of failing FORTRAN code, with assembler output from gfortran 4.6.4

This code is invalid:

5.7.2.5 Differences between named common and blank common

A blank common block has the same properties as a named common block, except for the following.

...
 Named common blocks of the same name shall be of the same size in all scoping units of a program in which they appear, but blank common blocks may be of dierent sizes.
...

If you put the two *.f files in the same one and compile the result, you get the following waring:

Warning: Named COMMON block 'mem' at (1) shall be of the same size as elsewhere (24 vs 8 bytes)

and the executable gives the result you expect.
Comment 14 Krzysztof Strasburger 2013-09-02 13:14:20 UTC
Marking the report as invalid doesn't solve the real problem.
I changed the common to unnamed and the situation is still the same.
main.f:
C Compile and link this file with buggy.f, using gfortran 4.6 (and probably
C any newer version), with optimization enabled (at least -O1).
C Run with: echo 1 2 3 | ./a.out
C expected (correct) result: 1. 2. 2.
      program main
      integer*4 i1,i2,i3
      real*8 dmem
      common dmem(3)
      read (*,*) i1,i2,i3
      call buggy(i1,i2,i3)
      write (*,*) dmem(1),dmem(2),dmem(3)
      end 
buggy.f:
      subroutine buggy(i1, i2, i3)
      integer*4 i1, i2, i3
      real*8 dmem
      common dmem(1)
      dmem(i1)=1.
      dmem(i2)=2.
      dmem(i3)=2.
      return
      end
Better?
Comment 15 Dominique d'Humieres 2013-09-02 14:39:33 UTC
It is invalid to use

      subroutine buggy(i1, i2, i3)
      integer*4 i1, i2, i3
      real*8 dmem
      common dmem(1)
      dmem(i1)=1.
      dmem(i2)=2.
      dmem(i3)=2.
      return
      end

with any i* different from 1. If you compile the code with -fbounds-check (or for recent gfortran, -fcheck=bounds) you get for 'echo 1 2 3'

Fortran runtime error: Index '2' of dimension 1 of array 'dmem' above upper bound of 1

As the code is invalid if one of the i* is not one, the compiler can do whatever it finds appropriate, e.g., set i1=i2=i3=1 (only valid case) and discard the other assignments.

AFAICT, the following works as I expect (4.0, 2.0, 3.0):

[macbook] dominiq/Downloads% cat buggy.f90
      subroutine buggy(i1, i2, i3)
      integer*4 i1, i2, i3
      real*8 dmem
      common dmem(1)
      dmem(i1)=4.
!      dmem(i2)=2.
!      dmem(i3)=2.
      return
      end
[macbook] dominiq/Downloads% cat main.f90
! Compile and link this file with buggy.f, using gfortran 4.6 (and probably
! any newer version), with optimization enabled (at least -O1).
! Run with: echo 1 2 3 | ./a.out
! expected (correct) result: 1. 2. 2.
      program main
      implicit none
      integer*4 i1,i2,i3
      real*8 dmem
      common dmem(3)
      read (*,*) i1,i2,i3
      dmem(i1) = 1.0
      dmem(i2) = 2.0
      dmem(i3) = 3.0
      print *, dmem
      call buggy(i1,i2,i3)
      write (*,*) dmem(1),dmem(2),dmem(3)
      end

I let you close the PR as INVALID.
Comment 16 Krzysztof Strasburger 2013-09-02 16:32:07 UTC
Dear Dominique,
I cannot agree with you. You are interpreting the code that may access the array beyond declared bounds as invalid, which is simply not true.
As you pointed it out before, unnamed common block may be declared larger elsewhere, so writing the dmem array beyond its first element may be a design decision and therefore may be perfectly legal. The compiler has no clue about real size of unnamed common while compiling buggy.f and bounds checking is optional.
I would also like to point it out that interpreting things this way you do, you exclude some older FORTRAN77 software (for example: quantum chemistry GAMESS), in which the lack of dynamic memory allocation was overcome using the trick we are discussing here (mixing with C was needed). BTW, change the size of dmem to 2 in buggy.f and things start to work correctly, although "out of bounds" memory accesses still do happen. The problem occurs only if dmem is of size 1.
Of course you (developers) may decide to ignore this problem anyway, so if you do so, feel free to close this bug. I'm not going to reopen it again, because I'm out of arguments. I'm also not competent enough to tamper with the compiler.
Comment 17 Dominique d'Humieres 2013-09-02 17:53:26 UTC
> I cannot agree with you. You are interpreting the code that may access the
> array beyond declared bounds as invalid, which is simply not true.

6.5 Use of data objects

... The value of a subscript in an array element shall be within the bounds for its dimension.

where the "shall" means (as elsewhere in the standard) that it is the coder responsibility to honor the constraint at run time.

> As you pointed it out before, unnamed common block may be declared larger
> elsewhere, so writing the dmem array beyond its first element may be a design
> decision and therefore may be perfectly legal. 

As above this is a wrong assumption: the design decision must be standard conforming. When you write "common dmem(1)" you tell the compiler that 'dmem' has only one element.

> The compiler has no clue about real size of unnamed common while compiling 
> buggy.f and bounds checking is optional.

When you write "common dmem(1)" you tell the compiler that 'dmem' has only one element.

> I would also like to point it out that interpreting things this way 
> you do, 

This is not my interpretation, it is what the Fortran standard says.

> you exclude some older FORTRAN77 software (for example: quantum chemistry
> GAMESS), in which the lack of dynamic memory allocation was overcome using 
> the trick we are discussing here (mixing with C was needed). 

Well, I have used safer tricks to overcome the limitation.

> BTW, change the size of dmem to 2 in buggy.f and things start to work 
> correctly, although "out of bounds" memory accesses still do happen. 
> The problem occurs only if dmem is of size 1.

Because only for size 1 the optimizer can infer that valid uses will provide the (1,1,1) triplet.

> Of course you (developers) may decide to ignore this problem anyway, 
> so if you do so, feel free to close this bug. I'm not going to reopen 
> it again, because I'm out of arguments. I'm also not competent enough 
> to tamper with the compiler.

What you can do is to look at the GCC manual and try to find the optimization pass than enable the optimization you don't like and disable it. Note that most optimizations are not part of the gfortran front-end.
Comment 18 Krzysztof Strasburger 2013-09-03 07:56:40 UTC
I have found that tree-fre, tree-pre and tree-dse have to be disabled in order to generate correctly working code at all optimization levels (both C and FORTRAN).
I'm happy with this workaround, so thank you for all suggestions.
Comment 19 Richard Biener 2013-09-03 09:22:56 UTC
DECL_COMMONs could be special-cased in places that look at DECL_SIZE (I'll declare this a QOI issue).  Or all decls that do not bind locally.  The
place that would "fix" tree-fre, tree-pre and tree-dse is in get_ref_base_and_extent where it does

  if (DECL_P (exp))
    {
      /* If maxsize is unknown adjust it according to the size of the
         base decl.  */
      if (maxsize == -1
          && host_integerp (DECL_SIZE (exp), 1))
        maxsize = TREE_INT_CST_LOW (DECL_SIZE (exp)) - hbit_offset;
    }
  else if (CONSTANT_CLASS_P (exp))
    {
      /* If maxsize is unknown adjust it according to the size of the
         base type constant.  */
      if (maxsize == -1
          && host_integerp (TYPE_SIZE (TREE_TYPE (exp)), 1))
        maxsize = TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp))) - hbit_offset;
    }

In its general processing the function treats all trailing arrays as possibly
of undefined size.

Note that because -fcommon is still the default for all C dialects the
impact of changing the above for example in the simple && !DECL_COMMON (exp)
way is unknown.  -fcommon is a source of interesting bugs.
Comment 20 Dominique d'Humieres 2013-09-03 09:41:10 UTC
This looks like a duplicate of pr50463 (and may be more).
Comment 21 Dominique d'Humieres 2013-09-03 09:47:59 UTC
Duplicate of pr53086 also.
Comment 22 Andrew Pinski 2021-11-29 02:32:37 UTC
Invalid as mentioned and a dup as mentioned.

*** This bug has been marked as a duplicate of bug 50463 ***