Bug 44882 - Bogus types in references with mismatched commons
Summary: Bogus types in references with mismatched commons
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.6.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: accepts-invalid, wrong-code
Depends on:
Blocks:
 
Reported: 2010-07-08 22:46 UTC by H.J. Lu
Modified: 2020-11-06 22:10 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2010-07-09 08:23:10


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description H.J. Lu 2010-07-08 22:46:54 UTC
On Linux/x86-64, revision 161953 failed to build 416.gamess in
SPEC CPU 2006:

gfortran -c -o vibanl.fppized.o -O3 -ffast-math -funroll-loops -ffixed-form vibanl.fppized.f
trudge.fppized.f: In function 'trudia':
trudge.fppized.f:1037:0: internal compiler error: in vectorizable_store, at tree-vect-stmts.c:3378
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.
Comment 1 H.J. Lu 2010-07-09 00:28:42 UTC
It is caused by revision 161949:

http://gcc.gnu.org/ml/gcc-cvs/2010-07/msg00303.html

[hjl@gnu-32 delta-fortran]$ cat testcase-min.f
      SUBROUTINE TRUDGE(KDIR)
      COMMON /TRUPAR/ DR(10),V(10,10)
      DO 110 I=1,NDIR
  110 DR(I)=V(I,JDIR)
      END
      SUBROUTINE TRUSRC(LEAVE)
      IMPLICIT DOUBLE PRECISION (A-H,O-Z)
      COMMON /TRUPAR/ DX(10),V(10,10)
      END
[hjl@gnu-32 delta-fortran]$ /export/gnu/import/rrs/161949/usr/bin/gcc -S -O3 -ffast-math -funroll-loops -ffixed-form testcase-min.f
testcase-min.f:8.21:

      COMMON /TRUPAR/ DX(10),V(10,10)                                   
                     1
Warning: Named COMMON block 'trupar' at (1) shall be of the same size
testcase-min.f: In function ‘trudge’:
testcase-min.f:1:0: internal compiler error: in vectorizable_store, at tree-vect-stmts.c:3378
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.
[hjl@gnu-32 delta-fortran]$ 
Comment 2 Dominique d'Humieres 2010-07-09 08:15:11 UTC
Confirmed on darwin. I also see the same ICE with -O3 for the polyhedron test mdbx.f90.
Comment 3 Richard Biener 2010-07-09 08:23:10 UTC
Mine.  If I fix the warning

t.f90:8.21:

      COMMON /TRUPAR/ DX(10),V(10,10)                                   
                     1
Warning: Named COMMON block 'trupar' at (1) shall be of the same size

then the ICE vanishes.  Fix as in make DX and V implicit double precision
in trudge as well.
Comment 4 Richard Biener 2010-07-09 08:33:49 UTC
So we have

 <array_ref 0x7ffff7f0c240
    type <real_type 0x7ffff7ef1000 real(kind=4) SF
        size <integer_cst 0x7ffff7ed2708 constant 32>
        unit size <integer_cst 0x7ffff7ed2410 constant 4>
        align 32 symtab 0 alias set 2 canonical type 0x7ffff7ef1000 precision 32
        pointer_to_this <pointer_type 0x7ffff7ef11f8>>
   
    arg 0 <component_ref 0x7ffff7ee1400
        type <array_type 0x7ffff7fc8c78 type <real_type 0x7ffff7ef1000 real(kind=4)>
            type_2 BLK
            size <integer_cst 0x7ffff7fbe500 constant 320>
            unit size <integer_cst 0x7ffff7fbe668 constant 40>
            align 32 symtab 0 alias set 2 canonical type 0x7ffff7fc8c78 domain <integer_type 0x7ffff7fcb000>
            pointer_to_this <pointer_type 0x7ffff7fcba80>>
       
        arg 0 <var_decl 0x7ffff7fcc000 trupar type <record_type 0x7ffff5ad9888>
            addressable public static ignored common decl_3 BLK defer-output file t.f90 line 2 col 0
            size <integer_cst 0x7ffff7fcd2d0 constant 7040>
            unit size <integer_cst 0x7ffff7fcd280 constant 880>
            align 128 context <function_decl 0x7ffff7fc9c00 trudge> chain <function_decl 0x7ffff7fc9c00 trudge>>
        arg 1 <field_decl 0x7ffff7fc6720 dr type <array_type 0x7ffff7fc8c78>
            BLK file t.f90 line 2 col 0 size <integer_cst 0x7ffff7fbe500 320> unit size <integer_cst 0x7ffff7fbe668 40>
            align 32 offset_align 128
            offset <integer_cst 0x7ffff7ed2438 constant 0>
            bit offset <integer_cst 0x7ffff7ed2b40 constant 0> context <record_type 0x7ffff7fcb930> chain <field_decl 0x7ffff7fc67b8 v>>>
    arg 1 <ssa_name 0x7ffff7fcf528
        type <integer_type 0x7ffff7ee45e8 integer(kind=8) public DI
            size <integer_cst 0x7ffff7ed27d0 constant 64>
            unit size <integer_cst 0x7ffff7ed27f8 constant 8>
            align 64 symtab 0 alias set -1 canonical type 0x7ffff7ee45e8 precision 64 min <integer_cst 0x7ffff7ed2758 -9223372036854775808> max <integer_cst 0x7ffff7ed2780 9223372036854775807>
            pointer_to_this <pointer_type 0x7ffff7f235e8>>
        visited var <var_decl 0x7ffff7fcc6e0 D.1580>def_stmt D.1580_6 = D.1579_5 + -1;

        version 6>>

and the record type is

 <record_type 0x7ffff5ad9888 BLK
    size <integer_cst 0x7ffff7fcd2d0 type <integer_type 0x7ffff7ee40a8 bit_size_type> constant 7040>
    unit size <integer_cst 0x7ffff7fcd280 type <integer_type 0x7ffff7ee4000> constant 880>
    align 64 symtab 0 alias set 3 canonical type 0x7ffff5ad9888
    fields <field_decl 0x7ffff7fc6850 dx
        type <array_type 0x7ffff7fcbbd0 type <real_type 0x7ffff7ef10a8 real(kind=8)>


see how we are accessing trupar.dr as if it were an array of floats while
the structure type has an array of doubles as elements.

That is going to wreck TBAA.

Now, we could silently continue instead of ICEing here if using MEM_REFs.
The assert we have is basically, for the original access
trupar.dr[D.1580_6], assert that the outermost access alias-set conflicts
with the alias-set of the base object (trupar).  Which it does always
due to record_component_aliases --- iff the types were not wrecked in the
first place.

Thus, I'm simply going to remove the assert.
Comment 5 Richard Biener 2010-07-09 08:39:38 UTC
I'm going to leave the bug open after that because I think the Fortran frontend
should do better than overriding the type of the common in TRUDGE from the
unused variant in TRUSRC.

Sooner or later you'll trip into another ICE with these mismatched types in
the IL.  Why can't this be a hard error?

Thus, re-assigning to Fortran component.  (yes, I'm still going to "fix" the
middle-end ICE)
Comment 6 Richard Biener 2010-07-09 10:05:41 UTC
Subject: Bug 44882

Author: rguenth
Date: Fri Jul  9 10:05:27 2010
New Revision: 161990

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=161990
Log:
2010-07-09  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/44882
	* tree-vect-stmts.c (vectorizable_store): Do not assert alias
	sets do conflict.
	(vectorizable_load): Likewise.

	* gfortran.dg/pr44882.f90: New testcase.

Added:
    trunk/gcc/testsuite/gfortran.dg/pr44882.f90
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-vect-stmts.c

Comment 7 Richard Biener 2010-07-09 10:10:13 UTC
ICE fixed.  Fortran FE problem remains.
Comment 8 Tobias Burnus 2010-07-09 12:16:31 UTC
(In reply to comment #5)
> I'm going to leave the bug open after that because I think the Fortran
> frontend should do better than overriding the type of the common in TRUDGE
> from the unused variant in TRUSRC.

Can you be a bit more specific what the solution of the problem should be?


> Sooner or later you'll trip into another ICE with these mismatched types in
> the IL.  Why can't this be a hard error?

"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 different sizes." (5.7.2.5, Fortran 2008 (FDIS).)

Thus, for blank commons, the standard requires to support it - while for named commones (such as TRUPAR in comment 1), it does not allow it. However, that did not stop people from doing so - and the number of (legacy) code which does so is legion.
Comment 9 rguenther@suse.de 2010-07-09 12:21:26 UTC
Subject: Re:  [4.6 Regression] Bogus types in references
 with mismatched commons

On Fri, 9 Jul 2010, burnus at gcc dot gnu dot org wrote:

> ------- Comment #8 from burnus at gcc dot gnu dot org  2010-07-09 12:16 -------
> (In reply to comment #5)
> > I'm going to leave the bug open after that because I think the Fortran
> > frontend should do better than overriding the type of the common in TRUDGE
> > from the unused variant in TRUSRC.
> 
> Can you be a bit more specific what the solution of the problem should be?

Good question.  I'd say never change types like that, just leave the
original type in place.  The code is not a valid fortran program anyway
and this avoids invalid trees in the middle-end.

So for the testcase make TRUPAR DR and V real=4, if you'd change it to

      SUBROUTINE TRUSRC(LEAVE)
      IMPLICIT DOUBLE PRECISION (A-H,O-Z)
      COMMON /TRUPAR/ DX(10),V(10,10)
      END
      SUBROUTINE TRUDGE(KDIR)
      COMMON /TRUPAR/ DR(10),V(10,10)
      DO 110 I=1,NDIR
  110 DR(I)=V(I,JDIR)
      END

make it real=8.

> > Sooner or later you'll trip into another ICE with these mismatched types in
> > the IL.  Why can't this be a hard error?
> 
> "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
> different sizes." (5.7.2.5, Fortran 2008 (FDIS).)
> 
> Thus, for blank commons, the standard requires to support it - while for named
> commones (such as TRUPAR in comment 1), it does not allow it. However, that did
> not stop people from doing so - and the number of (legacy) code which does so
> is legion.

And when compilers do not reject such code it will never be fixed ;)
Does GFortran have something like -fpermissive?
Comment 10 Tobias Burnus 2010-07-09 13:22:03 UTC
(In reply to comment #9)
> And when compilers do not reject such code it will never be fixed ;)
> Does GFortran have something like -fpermissive?

  -std=legacy

The problem with fixing: That helps for actively maintained code, but if someone downloads some old program (like I did  a while ago with Cowan's Hartree Fock code, written 30 years ago), one simply wants to use it and not to fix correctness bugs. I sincerely hope that no one writes this in new code!
Comment 11 H.J. Lu 2010-07-09 13:51:16 UTC
FWIW, the original testcase, vibanl.fppized.f, doesn't
have type mismatch.
Comment 12 Dominique d'Humieres 2010-07-09 14:35:27 UTC
Reduced test case extracted from the polyhedron test mdbx.f90 that give an ICE at revision 161959:

[karma] lin/test% cat mdbx_red.f90
!*==MASTER.spg  processed by SPAG 6.55Dc at 09:26 on 23 Sep 2005
      SUBROUTINE MASTER(Nsteps,Nlist,Method,Skin,Ncorr,Nprint)
      IMPLICIT DOUBLE PRECISION(A-H,O-Z)
      PARAMETER (NM=16384)
      COMMON /SCRATC/ DUMmy1(NM) , DUMmy2(NM) , DUMmy3(NM) , DUMmy4(NM)
      END
!*==CBUILD.spg  processed by SPAG 6.55Dc at 09:26 on 23 Sep 2005
      SUBROUTINE CBUILD(Ransq,Icode)
      IMPLICIT DOUBLE PRECISION(A-H,O-Z)
      PARAMETER (NM=16384)
      PARAMETER (LL=10*NM)
      COMMON /LISCOM/ LISt(LL) , MRKr1(NM) , MRKr2(NM) , LISlen
      COMMON /SCRATC/ DUMmy1(NM) , DUMmy2(NM) , DUMmy3(NM) , DUMmy4(NM)
      INTEGER MARk(NM)
      EQUIVALENCE (MARk,DUMmy1)
      EQUIVALENCE (KNTnts,DUMmy2)
      PARAMETER (NNEMAX=512)
      INTEGER neigh(NNEMAX)
      PARAMETER (KNTSIZ=3*NM)
      INTEGER KNTnts(KNTSIZ)
      DO icell = 0 , ncells - 1
         DO jpc = ipc + 1 , npc(icell)
            neigh(nneigc+jpc-ipc) = KNTnts(jpc+icoffs)
         ENDDO
         DO icand = 1 , nneigi
            LISt(nll) = neigh(icand)
            nll = nll + MARk(icand)
         ENDDO
      ENDDO
      END
[karma] lin/test% gfc -O3 mdbx_red.f90
mdbx_red.f90: In function 'cbuild':
mdbx_red.f90:8:0: internal compiler error: in vectorizable_load, at tree-vect-stmts.c:3828
Comment 13 Richard Biener 2010-09-02 10:51:53 UTC
The bogus types are not a regression.
Comment 14 Dominique d'Humieres 2014-09-19 13:23:37 UTC
I dont see any ICE for the test in comment 12 on powerpc-apple-darwin9 (from 4.4.7 to r214282). Any reason why this PR is still open?
Comment 15 rguenther@suse.de 2014-09-22 07:33:49 UTC
On Fri, 19 Sep 2014, dominiq at lps dot ens.fr wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=44882
> 
> Dominique d'Humieres <dominiq at lps dot ens.fr> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>              Status|NEW                         |WAITING
> 
> --- Comment #14 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> I dont see any ICE for the test in comment 12 on powerpc-apple-darwin9 (from
> 4.4.7 to r214282). Any reason why this PR is still open?

See comment #5/6, the Fortran frontend still produces wrong-code or
silently accepts invalid input.
Comment 16 Dominique d'Humieres 2016-02-04 16:02:35 UTC
See also pr53086 and pr69368.
Comment 17 Eric Gallager 2017-08-04 03:02:49 UTC
(In reply to rguenther@suse.de from comment #15)
> On Fri, 19 Sep 2014, dominiq at lps dot ens.fr wrote:
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=44882
> > 
> > Dominique d'Humieres <dominiq at lps dot ens.fr> changed:
> > 
> >            What    |Removed                     |Added
> > ----------------------------------------------------------------------------
> >              Status|NEW                         |WAITING
> > 
> > --- Comment #14 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> > I dont see any ICE for the test in comment 12 on powerpc-apple-darwin9 (from
> > 4.4.7 to r214282). Any reason why this PR is still open?
> 
> See comment #5/6, the Fortran frontend still produces wrong-code or
> silently accepts invalid input.

Adding those as Keywords on this bug then
Comment 18 Dominique d'Humieres 2017-10-05 19:47:28 UTC
The code in comment 1 is invalid, thus the FE can do what it likes. Would it be enough to close this PR by emitting an error unless the code is compiled with -std=legacy (as done for pr25071)?
Comment 19 rguenther@suse.de 2017-10-06 08:09:46 UTC
On Thu, 5 Oct 2017, dominiq at lps dot ens.fr wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=44882
> 
> --- Comment #18 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> The code in comment 1 is invalid, thus the FE can do what it likes. Would it be
> enough to close this PR by emitting an error unless the code is compiled with
> -std=legacy (as done for pr25071)?

Well, with -std=legacy the FE still emits "wrong code".  See comment#4
where we have a VAR_DECL of type 0x7ffff5ad9888 but accessing it as
if it were type 0x7ffff7fcb930 (the COMPONENT_REF arg 0 has a different
, incompatible type than its arg 1 DECL_CONTEXT).  We've had tree
verification for this in place at some point but IIRC I removed it
because FEs didn't follow the rules.

A workaround for the FE would be, when it builds the COMPONENT_REF
to access the 'dr' field, do

  if (TYPE_MAIN_VARIANT (TREE_TYPE (decl)) != TYPE_MAIN_VARIANT 
(record_type))
    decl = build1 (VIEW_CONVERT_EXPR, record_type, decl);

thus wrap the variable (in this case the common, and this is probably
only ever needed for (blank) commons with -std=legacy) inside
a VIEW_CONVERT_EXPR telling the middle-end we're trying to access
the data with a type that is not the same as the declared type.

That'll also fix TBAA.
Comment 20 Eric Gallager 2018-06-22 04:32:50 UTC
(In reply to rguenther@suse.de from comment #19)
> On Thu, 5 Oct 2017, dominiq at lps dot ens.fr wrote:
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=44882
> > 
> > --- Comment #18 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> > The code in comment 1 is invalid, thus the FE can do what it likes. Would it be
> > enough to close this PR by emitting an error unless the code is compiled with
> > -std=legacy (as done for pr25071)?
> 
> Well, with -std=legacy the FE still emits "wrong code".  See comment#4
> where we have a VAR_DECL of type 0x7ffff5ad9888 but accessing it as
> if it were type 0x7ffff7fcb930 (the COMPONENT_REF arg 0 has a different
> , incompatible type than its arg 1 DECL_CONTEXT).  We've had tree
> verification for this in place at some point but IIRC I removed it
> because FEs didn't follow the rules.
> 
> A workaround for the FE would be, when it builds the COMPONENT_REF
> to access the 'dr' field, do
> 
>   if (TYPE_MAIN_VARIANT (TREE_TYPE (decl)) != TYPE_MAIN_VARIANT 
> (record_type))
>     decl = build1 (VIEW_CONVERT_EXPR, record_type, decl);
> 
> thus wrap the variable (in this case the common, and this is probably
> only ever needed for (blank) commons with -std=legacy) inside
> a VIEW_CONVERT_EXPR telling the middle-end we're trying to access
> the data with a type that is not the same as the declared type.
> 
> That'll also fix TBAA.

...so does this bug still need to be marked as WAITING then?
Comment 21 Eric Gallager 2019-09-22 04:48:13 UTC
(In reply to Eric Gallager from comment #20)
> 
> ...so does this bug still need to be marked as WAITING then?

Update: guess not