Bug 71433 - [7 Regression] -Warray-bounds false positive with -O2
Summary: [7 Regression] -Warray-bounds false positive with -O2
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 7.0
: P1 normal
Target Milestone: 7.0
Assignee: Richard Biener
URL:
Keywords: diagnostic
Depends on:
Blocks:
 
Reported: 2016-06-06 11:13 UTC by Vincent Lefèvre
Modified: 2017-01-27 14:42 UTC (History)
3 users (show)

See Also:
Host:
Target: x86_64-linux-gnu
Build:
Known to work:
Known to fail:
Last reconfirmed: 2016-06-09 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Vincent Lefèvre 2016-06-06 11:13:53 UTC
When compiling GNU MPFR 3.1.4 with CFLAGS="-O2 -Werror=array-bounds" and

gcc (Debian 20160603-1) 7.0.0 20160603 (experimental) [trunk revision 237077]

I get:

get_d.c: In function 'mpfr_get_d':
get_d.c:115:24: error: array subscript is above array bounds [-Werror=array-bounds]
             d = (d + tp[i]) / MP_BASE_AS_DOUBLE;
                      ~~^~~

There was no such problem with

gcc (Debian 20160508-1) 7.0.0 20160508 (experimental) [trunk revision 236009]

so that's a recent regression.
Comment 1 Vincent Lefèvre 2016-06-06 13:57:15 UTC
Here's a simple test case:

int t[1];
int fct (long e)
{
  int d = 0, i, n = 52;
  if (e < 0)
    n += e;
  for (i = 1 ; i < n / 64 + 1 ; i++)
    d = t[i];
  return d;
}

If I replace "long" by "int" in the second line, I no longer get any warning.
Comment 2 Vincent Lefèvre 2016-06-06 15:03:13 UTC
Well, this test case also yields a warning with 20160508-1 (trunk r236009). Here's a new test case that yields no warnings with 20160508-1, but yields one with 20160603-1 (r237077).

int t[1];
int a (void);
int fct (int r, long e, int neg)
{
  int d = 0;
  if (r == 4)
    r = neg ? 3 : 2;
  if (e < -52)
    d = r == 0 && a () ? 1 : 2;
  else
    {
      int i, n = 53;
      if (e < 0)
        n += e;
      for (i = 1 ; i < n / 64 + 1 ; i++)
        d = t[i];
    }
  return d;
}
Comment 3 Vincent Lefèvre 2016-06-08 10:31:49 UTC
The bug was introduced in r236831.
Comment 4 Jeffrey A. Law 2016-06-09 19:45:35 UTC
Thanks Vincent.   Glad to have this little test.

AFAICT before the recent jump threading changes we had something like this in VRP1:


;;   basic block 9, loop depth 0, count 0, freq 900, maybe hot
;;    prev block 8, next block 10, flags: (NEW, REACHABLE)
;;    pred:       5 [50.0%]  (FALSE_VALUE,EXECUTABLE)
  e_23 = ASSERT_EXPR <e_13(D), e_13(D) >= -52>;
  if (e_23 < 0)
    goto <bb 10>;
  else
    goto <bb 11>;
;;    succ:       10 [27.0%]  (TRUE_VALUE,EXECUTABLE)
;;                11 [73.0%]  (FALSE_VALUE,EXECUTABLE)

;;   basic block 10, loop depth 0, count 0, freq 243, maybe hot
;;    prev block 9, next block 11, flags: (NEW, REACHABLE)
;;    pred:       9 [27.0%]  (TRUE_VALUE,EXECUTABLE)
  e_22 = ASSERT_EXPR <e_23, e_23 < 0>;
  _2 = (unsigned int) e_22;
  _3 = _2 + 53;
  n_16 = (int) _3;
;;    succ:       11 [100.0%]  (FALLTHRU,EXECUTABLE)

;;   basic block 11, loop depth 0, count 0, freq 900, maybe hot
;;    prev block 10, next block 12, flags: (NEW, REACHABLE)
;;    pred:       9 [73.0%]  (FALSE_VALUE,EXECUTABLE)
;;                10 [100.0%]  (FALLTHRU,EXECUTABLE)
  # n_9 = PHI <53(9), n_16(10)>
  goto <bb 13>;

Note that as written we can pretty easily derive a range for n_16 as [1..52] which ultimately allows us to remove the block with the out-of-range array index as unreachable.


With the threading before VRP things are bit more complex as BB10 has 3 predecessors (due to threading).  Now it happens that on each predecessor we have a suitable ASSERT_EXPR to narrow the range of e_13.  But we're unable to use them in that form.

Ideally we'd like to either be smarter about the initial insertion point or sink them  after insertion.  I'm pretty sure that would resolve this issue.  However, it's not compile-time friendly.  I'm going to need to think a bit more on this one.
Comment 5 Vincent Lefèvre 2016-10-20 13:21:47 UTC
Any news?

FYI, the provided test case no longer yields the warning with Debian's gcc-snapshot/20161006-1 package, but the issue is still present when compiling MPFR (get_d.c). Here's a corresponding simple test case:

int t[1];
int fct (int r, long e)
{
  int d = 0;
  if (r == 4)
    r = 1;
  if (e < -52)
    d = r == 0 ? 1 : 2;
  else
    {
      int i, n = 53;
      if (__builtin_expect (e < 0, 0))
        n += e;
      for (i = 1 ; i < n / 64 + 1 ; i++)
        t[i] = 0;
    }
  return d;
}

Surprisingly, if I replace "int t[1];" by "int t[2];", I no longer get the warning.
Comment 6 Richard Biener 2016-10-20 14:24:24 UTC
The issue is that jump-threading duplicated a tail with equal conditions so VRP is faced with

<bb 3>:
if (e_14(D) < -52)
  goto <bb 6>;
else
  goto <bb 14>;

# r_7 = PHI <r_20(13)>
if (e_14(D) < -52)
  goto <bb 5>;
else
  goto <bb 15>;

<bb 15>:
e_23 = ASSERT_EXPR <e_14(D), e_14(D) >= -52>;
goto <bb 7>;

<bb 14>:
e_22 = ASSERT_EXPR <e_14(D), e_14(D) >= -52>;

<bb 7>:
_1 = e_14(D) < 0;
_2 = (long int) _1;
if (e_14(D) < 0)


due to the fact that VRP does not insert PHIs for asserts (the assumption is
that we'll always merge to [-INF, +INF] I guess) we lose the fact that
e_14(D) >= -52 at the entry to BB 7.

So it's basically VRPs own stupidness.  The EVRP DOM-based algorithm should be
fine here (and I hope we can rip out the SSA propagator based VRP at some point,
I believe I now fixed most of its short-comings, loops still need to be dealt with though - stay tuned).
Comment 7 Richard Biener 2016-10-20 14:26:44 UTC
Before VRP we have

  # r_3 = PHI <1(2)>
  if (e_14(D) < -52)
    goto <bb 6>;
  else
    goto <bb 7>;

  <bb 4>:
  # r_7 = PHI <r_13(D)(2)>
  if (e_14(D) < -52)
    goto <bb 5>;
  else
    goto <bb 7>;

  <bb 5>:
  if (r_7 == 0)
    goto <bb 12>;
  else
    goto <bb 6>;

  <bb 6>:
  goto <bb 12>;

  <bb 7>:
  _1 = e_14(D) < 0;
  _2 = (long int) _1;
  if (e_14(D) < 0)


so _maybe_ the fix is to go over the pending asserts we want to insert and
see if we want to insert the same one on all predecessors of a block and
then simply merge them, inserting them into the block (BB 7) itself.  I
think that should be reasonably easy to do.
Comment 8 Richard Biener 2017-01-12 15:06:34 UTC
Mine.
Comment 9 Richard Biener 2017-01-17 08:39:06 UTC
Fixed.
Comment 10 Richard Biener 2017-01-17 08:39:31 UTC
Author: rguenth
Date: Tue Jan 17 08:38:59 2017
New Revision: 244520

URL: https://gcc.gnu.org/viewcvs?rev=244520&root=gcc&view=rev
Log:
2017-01-17  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/71433
	* tree-vrp.c (register_new_assert_for): Merge same asserts
	on all incoming edges.
	(process_assert_insertions_for): Handle insertions at the
	beginning of BBs.

	* gcc.dg/Warray-bounds-20.c: New testcase.

Added:
    trunk/gcc/testsuite/gcc.dg/Warray-bounds-20.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-vrp.c
Comment 11 Vincent Lefèvre 2017-01-27 00:02:06 UTC
The warning no longer occurs on the simple test case, but it still occurs on the MPFR code:

$ gcc-snapshot --version
gcc (Debian 20170125-1) 7.0.1 20170125 (experimental) [trunk revision 244909]
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ gcc-snapshot -DMPFR_WANT_ASSERT=2 -DMPFR_USE_LOGGING=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_DOUBLE_IEEE_LITTLE_ENDIAN=1 -DHAVE_LITTLE_ENDIAN=1 -DTIME_WITH_SYS_TIME=1 -DHAVE_LOCALE_H=1 -DHAVE_WCHAR_H=1 -DHAVE_STDARG=1 -DHAVE_SYS_TIME_H=1 -DHAVE_STRUCT_LCONV_DECIMAL_POINT=1 -DHAVE_STRUCT_LCONV_THOUSANDS_SEP=1 -DHAVE_ALLOCA_H=1 -DHAVE_STDINT_H=1 -DHAVE_VA_COPY=1 -DHAVE_SETLOCALE=1 -DHAVE_GETTIMEOFDAY=1 -DHAVE_SIGNAL=1 -DHAVE_SIGACTION=1 -DHAVE_LONG_LONG=1 -DHAVE_INTMAX_T=1 -DMPFR_HAVE_INTMAX_MAX=1 -DMPFR_PRINTF_MAXLM=\"j\" -DMPFR_HAVE_NORETURN=1 -DMPFR_HAVE_BUILTIN_UNREACHABLE=1 -DMPFR_HAVE_CONSTRUCTOR_ATTR=1 -DHAVE_PTHREAD=1 -DMPFR_HAVE_FESETROUND=1 -DHAVE_DENORMS=1 -DHAVE_DENORMS_FLT=1 -DHAVE_SIGNEDZ=1 -DHAVE_ROUND=1 -DHAVE_TRUNC=1 -DHAVE_FLOOR=1 -DHAVE_CEIL=1 -DHAVE_NEARBYINT=1 -DHAVE_LDOUBLE_IEEE_EXT_LITTLE=1 -DMPFR_WANT_DECIMAL_FLOATS=1 -DMPFR_WANT_FLOAT128=1 -DMPFR_USE_STATIC_ASSERT=1 -DHAVE_CLOCK_GETTIME=1 -DHAVE_ATTRIBUTE_MODE=1 -DPRINTF_L=1 -DPRINTF_T=1 -DHAVE___GMPN_SBPI1_DIVAPPR_Q=1 -DHAVE___GMPN_INVERT_LIMB=1 -DHAVE___GMPN_RSBLSH_N=1 -DMPFR_LONG_WITHIN_LIMB=1 -DHAVE_GETRUSAGE=1 -I.   -I/usr/local/gmp-6.1.0-debug/include  -O2 -Wall -Wold-style-declaration -Wold-style-definition -Wmissing-parameter-type -Wmissing-prototypes -Wmissing-declarations -Wmissing-field-initializers -Wno-error=unused-function -Wno-error=deprecated-declarations -Werror -DMPFR_COV_CHECK -c -o get_d.o get_d.c
get_d.c: In function 'mpfr_get_d':
get_d.c:115:24: error: array subscript is above array bounds [-Werror=array-bounds]
             d = (d + tp[i]) / MP_BASE_AS_DOUBLE;
                      ~~^~~
cc1: all warnings being treated as errors

One has:

          for (i = 1 ; i < np ; i++)
            d = (d + tp[i]) / MP_BASE_AS_DOUBLE;

and here np = 1 (known at compile time).
Comment 12 Vincent Lefèvre 2017-01-27 00:52:18 UTC
New test case, based on the previous one (just a __builtin_expect added):

int t[1];
int a (void);
int fct (int r, long e, int neg)
{
  int d = 0;
  if (r == 4)
    r = neg ? 3 : 2;
  if (__builtin_expect(e < -52, 0))
    d = r == 0 && a () ? 1 : 2;
  else
    {
      int i, n = 53;
      if (e < 0)
        n += e;
      for (i = 1 ; i < n / 64 + 1 ; i++)
        d = t[i];
    }
  return d;
}
Comment 13 Richard Biener 2017-01-27 09:07:27 UTC
Ok, same reason but my "simple" two-predecessor "hack" doesn't get it because we now have three...
Comment 14 Richard Biener 2017-01-27 12:31:04 UTC
Fixed again.  Thanks for the new testcase.
Comment 15 Richard Biener 2017-01-27 12:31:15 UTC
Author: rguenth
Date: Fri Jan 27 12:30:43 2017
New Revision: 244974

URL: https://gcc.gnu.org/viewcvs?rev=244974&root=gcc&view=rev
Log:
2017-01-27  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/71433
	* tree-vrp.c (register_new_assert_for): Revert earlier changes.
	(compare_assert_loc): New function.
	(process_assert_insertions): Sort and optimize assert locations
	to remove duplicates and push down identical assertions on
	edges to their destination block.

	* gcc.dg/Warray-bounds-21.c: New testcase.

Added:
    trunk/gcc/testsuite/gcc.dg/Warray-bounds-21.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-vrp.c
Comment 16 Vincent Lefèvre 2017-01-27 14:42:20 UTC
Thanks, I confirm that for r244974, there is no longer any issue on the MPFR code.