Bug 86827 - [8 Regression] -Warray-bounds produces negative indicies
Summary: [8 Regression] -Warray-bounds produces negative indicies
Status: RESOLVED DUPLICATE of bug 88273
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 8.1.0
: P2 normal
Target Milestone: 8.4
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks:
 
Reported: 2018-08-02 08:25 UTC by Wei Liu
Modified: 2020-02-25 16:39 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-08-02 00:00:00


Attachments
Test code (364 bytes, text/x-csrc)
2018-08-02 08:25 UTC, Wei Liu
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Wei Liu 2018-08-02 08:25:39 UTC
Created attachment 44484 [details]
Test code

The attached program fails to build with gcc 8.1 (Debian 8.1.0-12).

$ gcc  -m32 -march=i686 -std=gnu99 -Wall -O2   -Werror   -c -o t.o t.c                                                                                        
t.c: In function 'func':                                                                                                                                      
t.c:41:9: error: 'memcpy' offset [-204, -717] is out of the bounds [0, 216] of object 'ctrl' with type 'struct kdd_ctrl' [-Werror=array-bounds]
         memcpy(buf, ((uint8_t *)&ctrl.c32) + offset, len);         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
t.c:27:21: note: 'ctrl' declared here
     struct kdd_ctrl ctrl; 

And to quote Martin in a thread to gcc-help:

It looks like a bug in the implementation of the warning.
The offset is determined not to be in the range [-205, -716]
(pointer offsets are in ptrdiff_t) or (since the variable is
unsigned) in [4294966580, 4294967091].  That means that it
can be either in the range [0, 4294966579] or in [4294967092,
UINT_MAX].  But the warning code seems to get this anti-range
wrong and treats it as [-204, -717].
Comment 1 Richard Biener 2018-08-02 08:31:54 UTC
hand-rolled range handling code again :/
Comment 2 Martin Sebor 2018-08-02 15:43:59 UTC
Confirmed (gcc-help thread: https://gcc.gnu.org/ml/gcc-help/2018-08/msg00006.html).
Comment 3 Alexandre Oliva 2018-11-15 10:23:54 UTC
builtin_memref::offset_out_of_bounds has code to handle such anti-ranges, that would avoid this warning, but it's only active when base has an array type, not a compound type containing an array type.  Can't we just activate that code more often?
--- a/gcc/gimple-ssa-warn-restrict.c
+++ b/gcc/gimple-ssa-warn-restrict.c
@@ -479,7 +479,7 @@ builtin_memref::offset_out_of_bounds (int strict, offset_int ooboff[2]) const
   /* A temporary, possibly adjusted, copy of the offset range.  */
   offset_int offrng[2] = { offrange[0], offrange[1] };
 
-  if (DECL_P (base) && TREE_CODE (TREE_TYPE (base)) == ARRAY_TYPE)
+  if (true)
     {
       /* Check for offset in an anti-range with a negative lower bound.
 	 For such a range, consider only the non-negative subrange.  */
Comment 4 Alexandre Oliva 2018-11-15 10:40:11 UTC
Erhm, but even that doesn't handle all anti-ranges properly; right below, we state:

  /* The bounds need not be ordered.  Set HIB to use as the index
     of the larger of the bounds and LOB as the opposite.  */

which "corrects" the anti-range into the complementary range whenever we haven't taken the correction above, e.g., when given an anti-range with nonnegative boundaries.
Comment 5 Jakub Jelinek 2018-11-27 15:07:43 UTC
Martin, you've changed status to ASSIGNED, but not Assignee.  Are you working on this, or is Alex working on it?
Comment 6 Martin Sebor 2018-11-27 16:06:12 UTC
I'm not working on this at the moment.
Comment 7 Jakub Jelinek 2018-12-05 12:04:05 UTC
Adjusted testcase that fails also with -m64:
struct A { unsigned char a[84]; };
struct B { unsigned char b[216]; };
struct C { union { struct A c; struct B d; }; };
struct D { unsigned char e[65536]; unsigned int f; __SIZE_TYPE__ g; };

void
foo (struct D *s)
{
  struct C t;
  unsigned char *e = s->e + 64;
  unsigned int l = s->f;
  __SIZE_TYPE__ o = s->g;
  if (o > 512)
    o -= 512;
  o -= 204;
  if (o > sizeof t.c.a || o + l > sizeof t.c.a)
    l = 0;
  else
    __builtin_memcpy (e, t.c.a + o, l);
}

If this warning would be done during vrp2 when there are still asserts available rather than in a separate pass right after it, or if it used evrp analyzer, guess it could easily find out from the guarding condition that the range is narrower.
But even if it doesn't, I think we shouldn't warn even for:
void
bar (struct D *s)
{
  struct C t;
  unsigned char *e = s->e + 64;
  unsigned int l = s->f;
  __SIZE_TYPE__ o = s->g;
  if (o > 512)
    o -= 512;
  o -= 204;
  __builtin_memcpy (e, t.c.a + o, l);
}

  # RANGE ~[18446744073709550900, 18446744073709551411]
  o_10 = o_3 + 18446744073709551412;
  # RANGE [0, 4294967295] NONZERO 4294967295
  _1 = (long unsigned int) l_7;
  _2 = &t.D.1913.c.a + o_10;
  __builtin_memcpy (e_5, _2, _1);

we should treat at least anti-ranges where both min and max are completely outside of the bounds of the object as effectively VARYING, it doesn't tell us any interesting information.
Comment 8 Richard Biener 2018-12-05 12:33:48 UTC
(In reply to Jakub Jelinek from comment #7)
> Adjusted testcase that fails also with -m64:
> struct A { unsigned char a[84]; };
> struct B { unsigned char b[216]; };
> struct C { union { struct A c; struct B d; }; };
> struct D { unsigned char e[65536]; unsigned int f; __SIZE_TYPE__ g; };
> 
> void
> foo (struct D *s)
> {
>   struct C t;
>   unsigned char *e = s->e + 64;
>   unsigned int l = s->f;
>   __SIZE_TYPE__ o = s->g;
>   if (o > 512)
>     o -= 512;
>   o -= 204;
>   if (o > sizeof t.c.a || o + l > sizeof t.c.a)
>     l = 0;
>   else
>     __builtin_memcpy (e, t.c.a + o, l);
> }
> 
> If this warning would be done during vrp2 when there are still asserts
> available rather than in a separate pass right after it, or if it used evrp
> analyzer, guess it could easily find out from the guarding condition that
> the range is narrower.
> But even if it doesn't, I think we shouldn't warn even for:
> void
> bar (struct D *s)
> {
>   struct C t;
>   unsigned char *e = s->e + 64;
>   unsigned int l = s->f;
>   __SIZE_TYPE__ o = s->g;
>   if (o > 512)
>     o -= 512;
>   o -= 204;
>   __builtin_memcpy (e, t.c.a + o, l);
> }
> 
>   # RANGE ~[18446744073709550900, 18446744073709551411]
>   o_10 = o_3 + 18446744073709551412;
>   # RANGE [0, 4294967295] NONZERO 4294967295
>   _1 = (long unsigned int) l_7;
>   _2 = &t.D.1913.c.a + o_10;
>   __builtin_memcpy (e_5, _2, _1);
> 
> we should treat at least anti-ranges where both min and max are completely
> outside of the bounds of the object as effectively VARYING, it doesn't tell
> us any interesting information.

It tells us that the index is always out-of-bounds in case one is above
and one is below the range of valid values.  But yes, the quality
of the diagnostic is lacking.

Note we never reported ranges that overlap the valid indexes and the cited
anti-range above definitely does.
Comment 9 Jakub Jelinek 2019-02-22 15:19:54 UTC
GCC 8.3 has been released.
Comment 10 Jeffrey A. Law 2020-02-25 12:42:14 UTC
Martin Sebor fixed this in gcc-9
Comment 11 Martin Sebor 2020-02-25 16:39:22 UTC
It was fixed in r268048 committed for bug 88273 into GCC 9 and backported to GCC 8.

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