[Bug tree-optimization/94675] [9/10 regression] -Warray-bounds false positive with -O2 since r9-1948
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94675 --- Comment #18 from Xavier --- The lib has been recently opensourced so I can share it : https://github.com/Intersec/lib-common/blob/master/src/core/str-stream.h We have 100-200 usages of p_end/s_end/b_end so even if it's possible to patch them all, I am not a big fan of this solution.
[Bug tree-optimization/94675] [9/10 regression] -Warray-bounds false positive with -O2 since r9-1948
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94675
--- Comment #17 from Martin Sebor ---
As you observed, the warning disappears if the assert is removed, so that's one
workaround. But rather than working around it I would suggest to rewrite the
code to avoid the pointer subtraction. Chances are it will not only avoid the
warning but help improve the emitted object code. I'm not sure I understand
correctly what the test case is meant to do but the example below shows what
I'm thinking of. If modifying the code isn't feasible then #pragma GCC
diagnostic is the recommended suppression mechanism.
typedef unsigned char byte;
typedef __PTRDIFF_TYPE__ ptrdiff_t;
typedef __SIZE_TYPE__ size_t;
typedef struct pstream_t {
const byte * p;
ptrdiff_t n;
} pstream_t;
static inline _Bool ps_has (const pstream_t *ps, size_t len)
{
return (size_t)ps->n >= len;
}
static inline int __ps_skip (pstream_t * ps, size_t len)
{
if (!ps_has (ps, len))
__builtin_abort ();
ps->p += len;
ps->n -= len;
return 0;
}
static inline int ps_skip (pstream_t * ps, size_t len)
{
return ps_has (ps, len) ? __ps_skip(ps, len) : -1;
}
byte c;
int c_len;
void f(void)
{
pstream_t ps = { , c_len };
ps_skip (, 7);
}
[Bug tree-optimization/94675] [9/10 regression] -Warray-bounds false positive with -O2 since r9-1948
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94675 --- Comment #16 from Xavier --- (In reply to Martin Sebor from comment #14) > That said and codegen improvements aside, I think the submitted test case is > sufficiently tricky that I don't see issuing a warning for it as a problem. > All flow-based warnings have a non-zero rate of false positives (as do many > front-end warnings) and there are mechanisms to avoid them. Compared to > some of the other false positives we have, avoiding this one seems like a > low priority to me. Well is there a workaround ? Or do you suggest using pragma ignore ?
[Bug tree-optimization/94675] [9/10 regression] -Warray-bounds false positive with -O2 since r9-1948
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94675 --- Comment #15 from rguenther at suse dot de --- On Tue, 21 Apr 2020, law at redhat dot com wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94675 > > --- Comment #12 from Jeffrey A. Law --- > SO it's not terrible to get the key block cleaned up. but that's not > sufficient to resolve this issue. We all missed an important tidbit. > > > VRP is complaining about this: > > ps.D.2041.s = [(void *) + 7B]; > > > Note the object we reference in the expression, "c". "c" is a byte: > > typedef unsigned char byte; > byte c; > > > One could argue the problem is FRE. Prior to fre3 we had: > > ;; basic block 9, loop depth 0 > ;;pred: 7 > _34 = ps.D.2041.s; > _35 = _34 + 7; > ps.D.2041.s = _35; > > fre3 turns that into: > > ps.D.2041.s = [(void *) + 7B]; > > And we're going to lose. > > One could also argue that the warning has to be tolerant of these actions from > FRE. The question would be how to do that without totally compromising the > warning. FRE does an identity transform. That VRP does not warn about POINTER_PLUS_EXPR + 7B (I think it would?) may be another issue. That is FRE does _34 = ps.D.2041.s; -> _34 = _35 = _34 + 7; -> _35 = + 7; // propagatable form [ + 7] ps.D.2041.s = _35; -> ps.D.2041.s = [ + 7]; aka nothing wrong.
[Bug tree-optimization/94675] [9/10 regression] -Warray-bounds false positive with -O2 since r9-1948
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94675 --- Comment #14 from Martin Sebor --- I can think of only one way the warning code could avoid triggering here: by assuming that the difference between two pointers into the same array is less than or equal the size of the array (with non-array objects being viewed as arrays of a single element). That's necessarily true in any valid C/C++ program so it would be a safe assumption to make here as well. Since there is no array subscripting involved here, issuing -Warray-bounds might also be considered inappropriate, and the warning could be made not to trigger. But that would just mask the problem which would come back if __ps_skip attempted to access ps->s[len]. It would also come back if/when GCC started to warn more diligently for forming out-of-bounds pointers (I already submitted one patch to do that and the work I'm doing in the path isolation pass is an opportunity to revisit the feature). So we're back to deriving assumptions about the results of pointer arithmetic based the sizes of pointed-to objects. The warning would need to work quite hard to figure this out in general, so hard that I don't think it would be worth the effort unless it benefited code generation as well, or at least all other warnings like it (-Warray-bounds isn't the only one that can be triggered -- a number of others could be made to). Which was also the point of my comment #1 (and related to Richard's observation in comment #4 about an missed optimization opportunity). That said and codegen improvements aside, I think the submitted test case is sufficiently tricky that I don't see issuing a warning for it as a problem. All flow-based warnings have a non-zero rate of false positives (as do many front-end warnings) and there are mechanisms to avoid them. Compared to some of the other false positives we have, avoiding this one seems like a low priority to me.
[Bug tree-optimization/94675] [9/10 regression] -Warray-bounds false positive with -O2 since r9-1948
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94675 Jeffrey A. Law changed: What|Removed |Added CC||drahflow at gmx dot de --- Comment #13 from Jeffrey A. Law --- *** Bug 94655 has been marked as a duplicate of this bug. ***
[Bug tree-optimization/94675] [9/10 regression] -Warray-bounds false positive with -O2 since r9-1948
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94675 Jeffrey A. Law changed: What|Removed |Added Priority|P3 |P2
[Bug tree-optimization/94675] [9/10 regression] -Warray-bounds false positive with -O2 since r9-1948
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94675 --- Comment #12 from Jeffrey A. Law --- SO it's not terrible to get the key block cleaned up. but that's not sufficient to resolve this issue. We all missed an important tidbit. VRP is complaining about this: ps.D.2041.s = [(void *) + 7B]; Note the object we reference in the expression, "c". "c" is a byte: typedef unsigned char byte; byte c; One could argue the problem is FRE. Prior to fre3 we had: ;; basic block 9, loop depth 0 ;;pred: 7 _34 = ps.D.2041.s; _35 = _34 + 7; ps.D.2041.s = _35; fre3 turns that into: ps.D.2041.s = [(void *) + 7B]; And we're going to lose. One could also argue that the warning has to be tolerant of these actions from FRE. The question would be how to do that without totally compromising the warning.
[Bug tree-optimization/94675] [9/10 regression] -Warray-bounds false positive with -O2 since r9-1948
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94675
--- Comment #11 from Martin Sebor ---
(In reply to Richard Biener from comment #3)
> (In reply to Martin Sebor from comment #1)
> > The false positive is not due a shortcoming of the warning but rather due to
> > GCC not having a sufficiently sophisticated analysis of relationships of
> > pointers into the same objects. The same warning (and probably a numbers as
> > well) can be reproduced with a simpler example.
> >
> > $ cat pr94675.c && gcc -O2 -S -Wall -fdump-tree-vrp=/dev/stdout pr94675.c
> > unsigned char c, n;
> >
> > int f (void)
> > {
> > if (n <= 7) return 0;
> >
> > unsigned char *p = , *q = p + n;
> >
> > if (q - p <= 7) // not eliminated
> > return 0;
>
> Not sure why you write not eliminated - it is eliminated. I believe
> your testcase is bogus - why would the p[7] access never happen?
> Because p + n is invoking undefined behavior?
I may not have entirely accurately describe what happens in the small test case
but yes, p + n is undefined for values of n > 1, so the test (either of them)
can be assumed to be true and the dereference can be eliminated. The following
might be better:
unsigned char c, n;
int f (void)
{
unsigned char *p = , *q = p + n;
if (q - p <= 7) // not eliminated but could be
return 0;
return p[7]; // spurious -Warray-bounds
}
[Bug tree-optimization/94675] [9/10 regression] -Warray-bounds false positive with -O2 since r9-1948
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94675 --- Comment #10 from rguenther at suse dot de --- On Tue, 21 Apr 2020, law at redhat dot com wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94675 > > --- Comment #9 from Jeffrey A. Law --- > Yea, unrolling and the array-bounds warning do have bad interactions. Note I agree they don't belong in the SSA based VRP pass. Driving them from the DOM-based early VRP range analysis engine would be better. I still would like to see the SSA propagator VRP pass to go away (its jump threading is another blocker there).
[Bug tree-optimization/94675] [9/10 regression] -Warray-bounds false positive with -O2 since r9-1948
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94675 --- Comment #9 from Jeffrey A. Law --- Yea, unrolling and the array-bounds warning do have bad interactions. I suspect if we cleaned up this block that the backwards threader would likely kick in: # iftmp.2_18 = PHI <1(3), 1(4), 0(5)> _19 = (_Bool) iftmp.2_18; _2 = ~_19; _3 = (long int) _2; _4 = __builtin_expect (_3, 0); if (_4 == 0) goto ; [INV] else goto ; [INV] I'll play with that and see if there's something we can do cleanly early enough to matter.
[Bug tree-optimization/94675] [9/10 regression] -Warray-bounds false positive with -O2 since r9-1948
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94675 --- Comment #8 from rguenther at suse dot de --- On Tue, 21 Apr 2020, law at redhat dot com wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94675 > > Jeffrey A. Law changed: > >What|Removed |Added > > CC||law at redhat dot com > > --- Comment #7 from Jeffrey A. Law --- > Backwards jump threading (where we'd like to handle this) doesn't handle the > cast assignment (and assignments in general). I've hesitated adding this > capability because it'll duplicate a lot of the work done by Ranger. > > So the threadable path is left in the IL when VRP1 runs, thus generating the > false positive. I'd really like to see those warnings move out of VRP, or at > least deferred until VRP2 where the dead paths have been cleaned up. We've moved the warning to VRP1 only because at VRP2 time all the loop opts have been run and esp. unrolling is prone to "thread" unreachable cases.
[Bug tree-optimization/94675] [9/10 regression] -Warray-bounds false positive with -O2 since r9-1948
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94675 Jeffrey A. Law changed: What|Removed |Added CC||law at redhat dot com --- Comment #7 from Jeffrey A. Law --- Backwards jump threading (where we'd like to handle this) doesn't handle the cast assignment (and assignments in general). I've hesitated adding this capability because it'll duplicate a lot of the work done by Ranger. So the threadable path is left in the IL when VRP1 runs, thus generating the false positive. I'd really like to see those warnings move out of VRP, or at least deferred until VRP2 where the dead paths have been cleaned up.
[Bug tree-optimization/94675] [9/10 regression] -Warray-bounds false positive with -O2 since r9-1948
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94675 Jakub Jelinek changed: What|Removed |Added Summary|[9/10 regression] |[9/10 regression] |-Warray-bounds false|-Warray-bounds false |positive with -O2 |positive with -O2 since ||r9-1948 CC||jakub at gcc dot gnu.org --- Comment #6 from Jakub Jelinek --- Started with r9-1948-gd893b683f40884cd00b5beb392566ecc7b67f721 on the original testcase.
