Bug 52754 - [4.7 Regression] indirect indexing broken with -fpredictive-commoning
Summary: [4.7 Regression] indirect indexing broken with -fpredictive-commoning
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: other (show other bugs)
Version: 4.7.0
: P3 normal
Target Milestone: 4.7.1
Assignee: Richard Biener
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2012-03-28 15:28 UTC by Matthias Kretz
Modified: 2012-04-03 11:55 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work: 4.7.1, 4.8.0
Known to fail: 4.7.0
Last reconfirmed: 2012-03-28 00:00:00


Attachments
testcase (288 bytes, text/x-c++src)
2012-03-28 15:28 UTC, Matthias Kretz
Details
gcc48-pr52754.patch (551 bytes, patch)
2012-03-28 17:22 UTC, Jakub Jelinek
Details | Diff
alternative patch (1.08 KB, patch)
2012-03-30 10:45 UTC, Richard Biener
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Kretz 2012-03-28 15:28:03 UTC
Created attachment 27022 [details]
testcase

The attached testcase is compiled to access the indexes array at bogus offsets. If line 8 is removed and line 7 written as
    unsigned int indexes[8] = { 0, 1, 2, 3, 4, 5, 6, 7 };
the error disappears.

% /opt/gcc-4.7.0/bin/g++ -msse2 -v -O1 -fpredictive-commoning main.cpp
Using built-in specs.
COLLECT_GCC=/opt/gcc-4.7.0/bin/g++
COLLECT_LTO_WRAPPER=/opt/gcc-4.7.0/libexec/gcc/x86_64-linux-gnu/4.7.0/lto-wrapper
Target: x86_64-linux-gnu
Configured with: ./configure --prefix=/opt/gcc-4.7.0 --build=x86_64-linux-gnu --host=x86_64-linux-gnu --enable-languages=c,c++,fortran --with-gmp=/opt/gcc-4.7.0 --with-mpfr=/opt/gcc-4.7.0 --with-ppl=/opt/gcc-4.7.0 --with-cloog=/opt/gcc-4.7.0 --with-libelf=/opt/gcc-4.7.0 --with-mpc=/opt/gcc-4.7.0 --enable-lto
Thread model: posix
gcc version 4.7.0 (GCC)
COLLECT_GCC_OPTIONS='-msse2' '-v' '-O1' '-fpredictive-commoning' '-shared-libgcc' '-mtune=generic' '-march=x86-64'
 /opt/gcc-4.7.0/libexec/gcc/x86_64-linux-gnu/4.7.0/cc1plus -quiet -v -imultiarch x86_64-linux-gnu -D_GNU_SOURCE main.cpp -quiet -dumpbase main.cpp -msse2 -mtune=generic -march=x86-64 -auxbase main -O1 -version -fpredictive-commoning -o /tmp/ccmv6Zx9.s
GNU C++ (GCC) version 4.7.0 (x86_64-linux-gnu)
        compiled by GNU C version 4.7.0, GMP version 5.0.4, MPFR version 3.1.0, MPC version 0.9
GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
ignoring nonexistent directory "/usr/local/include/x86_64-linux-gnu"
ignoring nonexistent directory "/opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0/../../../../x86_64-linux-gnu/include"
#include "..." search starts here:
#include <...> search starts here:
 /opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0/../../../../include/c++/4.7.0
 /opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0/../../../../include/c++/4.7.0/x86_64-linux-gnu
 /opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0/../../../../include/c++/4.7.0/backward
 /opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0/include
 /usr/local/include
 /opt/gcc-4.7.0/include
 /opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0/include-fixed
 /usr/include/x86_64-linux-gnu
 /usr/include
End of search list.
GNU C++ (GCC) version 4.7.0 (x86_64-linux-gnu)
        compiled by GNU C version 4.7.0, GMP version 5.0.4, MPFR version 3.1.0, MPC version 0.9
GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
Compiler executable checksum: 32eb1a24aeca8b97e0c59b4b063f668f
COLLECT_GCC_OPTIONS='-msse2' '-v' '-O1' '-fpredictive-commoning' '-shared-libgcc' '-mtune=generic' '-march=x86-64'
 /opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0/../../../../x86_64-linux-gnu/bin/as --64 -o /tmp/cca4x2R5.o /tmp/ccmv6Zx9.s
COMPILER_PATH=/opt/gcc-4.7.0/libexec/gcc/x86_64-linux-gnu/4.7.0/:/opt/gcc-4.7.0/libexec/gcc/x86_64-linux-gnu/4.7.0/:/opt/gcc-4.7.0/libexec/gcc/x86_64-linux-gnu/:/opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0/:/opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/:/opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0/../../../../x86_64-linux-gnu/bin/
LIBRARY_PATH=/opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0/:/opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0/../../../../lib64/:/lib/x86_64-linux-gnu/:/lib/../lib64/:/usr/lib/x86_64-linux-gnu/:/opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0/../../../../x86_64-linux-gnu/lib/:/opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0/../../../:/lib/:/usr/lib/
COLLECT_GCC_OPTIONS='-msse2' '-v' '-O1' '-fpredictive-commoning' '-shared-libgcc' '-mtune=generic' '-march=x86-64'
 /opt/gcc-4.7.0/libexec/gcc/x86_64-linux-gnu/4.7.0/collect2 --eh-frame-hdr -m elf_x86_64 -dynamic-linker /lib64/ld-linux-x86-64.so.2 /usr/lib/x86_64-linux-gnu/crt1.o /usr/lib/x86_64-linux-gnu/crti.o /opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0/crtbegin.o -L/opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0 -L/opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0/../../../../lib64 -L/lib/x86_64-linux-gnu -L/lib/../lib64 -L/usr/lib/x86_64-linux-gnu -L/opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0/../../../../x86_64-linux-gnu/lib -L/opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0/../../.. /tmp/cca4x2R5.o -lstdc++ -lm -lgcc_s -lgcc -lc -lgcc_s -lgcc /opt/gcc-4.7.0/lib/gcc/x86_64-linux-gnu/4.7.0/crtend.o /usr/lib/x86_64-linux-gnu/crtn.o

% ./a.out
[1]    7716 bus error  ./a.out
Comment 1 Jakub Jelinek 2012-03-28 16:05:53 UTC
Started with http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=178028
Comment 2 Jakub Jelinek 2012-03-28 16:48:51 UTC
Seems ref_at_iteration is buggy, when iter is negative, but ARRAY_REF's index is TYPE_UNSIGNED smaller than size of pointer, computing the ARRAY_REF's index in the smaller unsigned type isn't a good idea, because then we end up with those
0xfffffffdU etc. indexes while we want -3L.
Wonder if it is generally ok to do the computation in a wider type (if any),
or if we should just handle the simple cases (integer_onep (iv.step) ? ) and punt otherwise.
Comment 3 Jakub Jelinek 2012-03-28 17:22:15 UTC
Created attachment 27023 [details]
gcc48-pr52754.patch

Patch I had in mind.  Seems to fix the testcase.  Richard, what do you think about this?
Comment 4 Richard Biener 2012-03-29 09:58:02 UTC
Mine.
Comment 5 Matthias Kretz 2012-03-29 10:24:19 UTC
I just tested the patch on the last 4.7 snapshot (20120324). My unit test apparently runs without failure, but I still get incorrect warnings "warning: array subscript is above array bounds [-Warray-bounds]".
Comment 6 Richard Biener 2012-03-30 10:22:45 UTC
We indeed should not create negative array indices (well, out-of-bound array
indices).  The issue why this happens is that we transform

 const unsigned int * ii = (const unsigned int *) &indexes[i];
 *(ii + 12)

to

 MEM[(const unsigned int *)&indexes + 12B][i_3];

via

            ii = &indexes[i];
            D.3849 = ii + 12;
            D.3850 = *D.3849;

forwprop1 (still ok):

  ii_9 = &indexes[i_3];
  D.3850_11 = MEM[(const unsigned int *)ii_9 + 12B];

forwprop2 (bogus):

  D.3850_11 = MEM[(const unsigned int *)&indexes + 12B][i_3];
Comment 7 Richard Biener 2012-03-30 10:45:28 UTC
Created attachment 27042 [details]
alternative patch
Comment 8 Matthias Kretz 2012-03-30 11:13:59 UTC
(In reply to comment #7)
> alternative patch

Just tested it on the 4.7-20120324 snapshot and all bogus warnings are gone, as are the crashes. No regressions in my testsuite.
Comment 9 Jakub Jelinek 2012-03-30 11:31:58 UTC
(In reply to comment #7)
> Created attachment 27042 [details]
> alternative patch

I'm not against it, but what if the source code and/or some other pass result in similar ARRAY_REF?  To me pcom still looks potentially buggy...
Comment 10 rguenther@suse.de 2012-03-30 11:41:35 UTC
On Fri, 30 Mar 2012, jakub at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52754
> 
> --- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-03-30 11:31:58 UTC ---
> (In reply to comment #7)
> > Created attachment 27042 [details]
> > alternative patch
> 
> I'm not against it, but what if the source code and/or some other pass result
> in similar ARRAY_REF?  To me pcom still looks potentially buggy...

If that happens the other pass is broken or the source invokes undefined
behavior.  I had the following patch which fixed the issue, too:

Index: tree-predcom.c
===================================================================
--- tree-predcom.c      (revision 186007)
+++ tree-predcom.c      (working copy)
@@ -1414,9 +1414,11 @@ ref_at_iteration (struct loop *loop, tre
        }
       else
        {
-         val = fold_build2 (MULT_EXPR, type, iv.step,
-                            build_int_cst_type (type, iter));
-         val = fold_build2 (PLUS_EXPR, type, iv.base, val);
+         tree itype = signed_type_for (type);
+         val = fold_build2 (MULT_EXPR, itype, fold_convert (itype, 
iv.step),
+                            build_int_cst (itype, iter));
+         val = fold_build2 (PLUS_EXPR, itype,
+                            fold_convert (itype, iv.base), val);
        }
       *idx_p = unshare_expr (val);
     }

as we know iter is negative.  We could also change the caller so
we pass a positive count and instead subtract it from the base
value, though that's still going to get possibly negative
(and in case iv.base is not a constant we will not know this here).

The above would not be a real fix, instead we would need to cater
for a large unsigned iv.base, thus, promote val and iv.base to
[u]intptr_t.

But - we can't really "win" here for array indices that ultimatively
will be out-of-bound.  If some GCC pass re-constructs invalid
array-refs it has to be fixed.
Comment 11 Richard Biener 2012-03-30 13:41:31 UTC
Author: rguenth
Date: Fri Mar 30 13:41:24 2012
New Revision: 186012

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=186012
Log:
2012-03-30  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/52754
	* tree-ssa-forwprop.c (forward_propagate_addr_expr_1): Only
	propagate arbitrary addresses into really plain dereferences.

	* gcc.target/i386/pr52754.c: New testcase.

Added:
    trunk/gcc/testsuite/gcc.target/i386/pr52754.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-forwprop.c
Comment 12 Richard Biener 2012-03-30 13:42:14 UTC
Fixed on trunk sofar.
Comment 13 Richard Biener 2012-04-03 11:55:30 UTC
Author: rguenth
Date: Tue Apr  3 11:55:24 2012
New Revision: 186105

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=186105
Log:
2012-04-03  Richard Guenther  <rguenther@suse.de>

	Backport from mainline
	2012-03-06  Richard Guenther  <rguenther@suse.de>

	PR middle-end/52493
	* tree-ssa-alias.c (ptr_derefs_may_alias_p): Robustify.

	* gcc.dg/torture/pr52493.c: New testcase.

	2012-03-23  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/52678
	* tree-vectorizer.h (struct _stmt_vec_info): Add
	loop_phi_evolution_part member.
	(STMT_VINFO_LOOP_PHI_EVOLUTION_PART): New define.
	* tree-vect-loop.c (vect_analyze_scalar_cycles_1): Initialize
	STMT_VINFO_LOOP_PHI_EVOLUTION_PART.
	* tree-vect-loop-manip.c (vect_update_ivs_after_vectorizer):
	Use the cached evolution part and the PHI nodes value from
	the loop preheader edge instead of re-analyzing the evolution.

	* gfortran.dg/pr52678.f: New testcase.

	2012-03-26  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/52701
	* tree-vect-loop.c (vect_analyze_scalar_cycles_1): Always
	compute and set the evolution part of PHI nodes.

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

	2012-03-30  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/52754
	* tree-ssa-forwprop.c (forward_propagate_addr_expr_1): Only
	propagate arbitrary addresses into really plain dereferences.

	* gcc.target/i386/pr52754.c: New testcase.

Added:
    branches/gcc-4_7-branch/gcc/testsuite/gcc.dg/torture/pr52493.c
    branches/gcc-4_7-branch/gcc/testsuite/gcc.target/i386/pr52754.c
    branches/gcc-4_7-branch/gcc/testsuite/gfortran.dg/pr52678.f
    branches/gcc-4_7-branch/gcc/testsuite/gfortran.dg/pr52701.f90
Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_7-branch/gcc/tree-ssa-alias.c
    branches/gcc-4_7-branch/gcc/tree-ssa-forwprop.c
    branches/gcc-4_7-branch/gcc/tree-vect-loop-manip.c
    branches/gcc-4_7-branch/gcc/tree-vect-loop.c
    branches/gcc-4_7-branch/gcc/tree-vectorizer.h
Comment 14 Richard Biener 2012-04-03 11:55:56 UTC
Fixed.