Bug 104986 - [12/13/14/15 Regression] bogus writing 1 byte into a region of size 0 with -fwrapv and -O2 -fpeel-loops since r12-4698-gf6d012338bf87f42
Summary: [12/13/14/15 Regression] bogus writing 1 byte into a region of size 0 with -f...
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 12.0
: P2 normal
Target Milestone: 12.5
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic, missed-optimization, needs-bisection
Depends on:
Blocks:
 
Reported: 2022-03-19 17:07 UTC by Andres Freund
Modified: 2024-06-20 09:02 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work: 13.1.0
Known to fail:
Last reconfirmed: 2022-03-21 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Freund 2022-03-19 17:07:46 UTC
Hi,

recently started seeing bogus warnings using gcc 12 to build postgres. I reduced the problem using cvise with some manual cleanups / improvements afterwards - certainly doesn't quite make sense anymore, but afaics shows a problem.

Originally I hit this with -O3, but found that -O2 -fpeel-loops is sufficient to trigger the problem.

repro: https://godbolt.org/z/ejK9h6von

code:
struct inet_struct {
  char family;
  char ipaddr[16];
};

void
inetnot(struct inet_struct *dst1, struct inet_struct *dst2, struct inet_struct *src) {
  int nb = src->family ? 4 : 6;
  char *psrc = src->ipaddr;
  char *pdst = dst1 ? dst1->ipaddr : dst2->ipaddr;
  while (nb-- > 0)
    pdst[nb] = psrc[nb];
}


gcc-12 -fwrapv -O2 -fpeel-loops -c network2.i

network2.i: In function ‘inetnot’:
network2.i:12:14: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
   12 |     pdst[nb] = psrc[nb];
      |     ~~~~~~~~~^~~~~~~~~~
network2.i:3:8: note: at offset -1 into destination object ‘ipaddr’ of size 16
    3 |   char ipaddr[16];
      |        ^~~~~~
network2.i:3:8: note: at offset -1 into destination object ‘ipaddr’ of size 16


which afaics is bogus, because the loop terminates before reaching offset -1, the condition is > 0, not >= 0. So the post decrement can't lead to -1 being reached.

version: gcc version 12.0.1 20220314 (experimental) [master r12-7638-g823b3b79cd2] (Debian 12-20220313-1) 

Regards,

Andres
Comment 1 Martin Liška 2022-03-21 09:31:54 UTC
Started with r12-4698-gf6d012338bf87f42.
Comment 2 Aldy Hernandez 2022-03-23 09:45:03 UTC
[Siddhesh, I don't know if this is something you're looking at.  If not, feel free to unsubscribe.]

This may have first been visible with r12-4698-gf6d012338bf87f42, but it isn't threader related as it can be reproduced with -fno-thread-jumps.

I can actually reproduce it without VRP[12] (but not without evrp).

+ ./cc1 a.c -fdump-tree-all-details -quiet -I/tmp -O2 -fpeel-loops -fwrapv -fno-thread-jumps -fdisable-tr
ee-vrp2 -fdisable-tree-vrp1
cc1: note: disable pass tree-vrp2 for functions in the range of [0, 4294967295]
cc1: note: disable pass tree-vrp1 for functions in the range of [0, 4294967295]
a.c: In function ‘inetnot’:
a.c:12:14: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
etc
etc

Disabling evrp causes the problem to go away, but all these middle end warnings are very sensitive to better ranges, so who knows.

For that matter, the only transformation evrp is doing is correct:

@@ -74,7 +90,7 @@
   <bb 9> :
   # nb_8 = PHI <iftmp.0_9(7), nb_23(8)>
   nb_23 = nb_8 + -1;
-  if (nb_8 > 0)
+  if (nb_8 != 0)
     goto <bb 8>; [INV]
   else
     goto <bb 10>; [INV]

...which is merely changing the conditional in the decreasing loop from > 0 into != 0.

It looks like a problem elsewhere.
Comment 3 Richard Biener 2022-03-23 13:05:44 UTC
This is peeling leaving us with unreachable code we warn on and somehow
while figuring prephitmp_30 + -6 is -1 we don't figure nb_58 is zero on
the path to bb9.

I think I've seen this backward-forward dependency issue with ranger before
in another missed optimization PR.
Comment 4 Aldy Hernandez 2022-03-23 14:45:14 UTC
(In reply to Richard Biener from comment #3)
> This is peeling leaving us with unreachable code we warn on and somehow
> while figuring prephitmp_30 + -6 is -1 we don't figure nb_58 is zero on
> the path to bb9.
> 
> I think I've seen this backward-forward dependency issue with ranger before
> in another missed optimization PR.

This is not ranger, but the legacy conditional folding in vr-values.  It's the test_for_singularity() code that turns x<=0 where x is [0,6] into x!=0.
Comment 5 Andrew Macleod 2022-03-23 14:46:43 UTC
(In reply to Richard Biener from comment #3)
> This is peeling leaving us with unreachable code we warn on and somehow
> while figuring prephitmp_30 + -6 is -1 we don't figure nb_58 is zero on
> the path to bb9.
> 
> I think I've seen this backward-forward dependency issue with ranger before
> in another missed optimization PR.

(In reply to Richard Biener from comment #3)
> This is peeling leaving us with unreachable code we warn on and somehow
> while figuring prephitmp_30 + -6 is -1 we don't figure nb_58 is zero on
> the path to bb9.
> 
> I think I've seen this backward-forward dependency issue with ranger before
> in another missed optimization PR.

DOM3 is the pass which converted prephitmp_30 + -6 to -1, and left nb_58 as is.

Ranger appears to see it just fine. Looking at VRP2, 

Folding statement: nb_68 = prephitmp_30 + -5;
Queued stmt for removal.  Folds to: 0
<...>
Folding statement: if (nb_68 != 0)
gimple_simplified to if (0 != 0)
Folded into: if (0 != 0)

strlen runs just before vrp2, and if I put a VRP pass immediately before strlen1, then the warning goes away.

Looking at the strlen1 output, it only ever asks ranger about 6 names:

334      range_of_expr(_36) at stmt _52 = iftmp.1_17 + _36;
         TRUE : (334) range_of_expr (_36) sizetype [3, 5]
maybe_invalidate called for *_52 = _60;
maybe_invalidate returns 1
342      range_of_expr(_69) at stmt _71 = iftmp.1_7 + _69;
         TRUE : (342) range_of_expr (_69) sizetype [3, 5]
maybe_invalidate called for *_71 = _72;
maybe_invalidate returns 1
348      range_of_expr(_6) at stmt _31 = iftmp.1_76 + _6;
         TRUE : (348) range_of_expr (_6) sizetype [2, 4]
maybe_invalidate called for *_31 = _32;
maybe_invalidate returns 1
367      range_of_expr(_37) at stmt _39 = iftmp.1_76 + _37;
         TRUE : (367) range_of_expr (_37) sizetype [1, 3]
maybe_invalidate called for *_39 = _40;
  statement may clobber object _31 0 bytes in size
  
maybe_invalidate returns 1
375      range_of_expr(_45) at stmt _47 = iftmp.1_76 + _45;
         TRUE : (375) range_of_expr (_45) sizetype [0, 2]
maybe_invalidate called for *_47 = _48;
  statement may clobber object _39 0 bytes in size
maybe_invalidate returns 1
383      range_of_expr(_53) at stmt _55 = iftmp.1_76 + _53;
         TRUE : (383) range_of_expr (_53) sizetype [1, 1]

Is there a reason it runs before vrp2?
Comment 6 Aldy Hernandez 2022-03-23 14:49:24 UTC
(In reply to Aldy Hernandez from comment #4)
> (In reply to Richard Biener from comment #3)
> > This is peeling leaving us with unreachable code we warn on and somehow
> > while figuring prephitmp_30 + -6 is -1 we don't figure nb_58 is zero on
> > the path to bb9.
> > 
> > I think I've seen this backward-forward dependency issue with ranger before
> > in another missed optimization PR.
> 
> This is not ranger, but the legacy conditional folding in vr-values.  It's
> the test_for_singularity() code that turns x<=0 where x is [0,6] into x!=0.

Ooops, sorry I totally missed Richi's comment on prephitmp.  I see Andrew answered though :).
Comment 7 Andrew Macleod 2022-03-23 14:58:53 UTC
and I forgot to show to dom3 output which did the transformation:

j.c.195t.dom3:Match-and-simplified (sizetype) nb_66 to 18446744073709551615
j.c.195t.dom3:Optimizing statement _2 = (sizetype) nb_66;
j.c.195t.dom3:  Replaced 'nb_66' with constant '-1'
j.c.195t.dom3:  nb_66 = -1;

but for whatever reason it could not reduce nb_58 to anything better than [-1, 0]

nb_58 = prephitmp_30 + -5;
Intersecting
  int [-1, 0]
and
  int [-1, 4]
to
  int [-1, 0]
Comment 8 Richard Biener 2022-03-23 15:05:05 UTC
(In reply to Andrew Macleod from comment #5)
> Looking at the strlen1 output, it only ever asks ranger about 6 names:
> 
> 334      range_of_expr(_36) at stmt _52 = iftmp.1_17 + _36;
>          TRUE : (334) range_of_expr (_36) sizetype [3, 5]
> maybe_invalidate called for *_52 = _60;
> maybe_invalidate returns 1
> 342      range_of_expr(_69) at stmt _71 = iftmp.1_7 + _69;
>          TRUE : (342) range_of_expr (_69) sizetype [3, 5]
> maybe_invalidate called for *_71 = _72;
> maybe_invalidate returns 1
> 348      range_of_expr(_6) at stmt _31 = iftmp.1_76 + _6;
>          TRUE : (348) range_of_expr (_6) sizetype [2, 4]
> maybe_invalidate called for *_31 = _32;
> maybe_invalidate returns 1
> 367      range_of_expr(_37) at stmt _39 = iftmp.1_76 + _37;
>          TRUE : (367) range_of_expr (_37) sizetype [1, 3]
> maybe_invalidate called for *_39 = _40;
>   statement may clobber object _31 0 bytes in size
>   
> maybe_invalidate returns 1
> 375      range_of_expr(_45) at stmt _47 = iftmp.1_76 + _45;
>          TRUE : (375) range_of_expr (_45) sizetype [0, 2]
> maybe_invalidate called for *_47 = _48;
>   statement may clobber object _39 0 bytes in size
> maybe_invalidate returns 1
> 383      range_of_expr(_53) at stmt _55 = iftmp.1_76 + _53;
>          TRUE : (383) range_of_expr (_53) sizetype [1, 1]

So maybe strlen can invoke ranger on GIMPLE_CONDs to mark unreachable
code on the fly?  IIRC domwalk has some ability to skip not executable
edges so if the walk makes sure to properly initialize that for
outgoing edges it might work in that it would refrain from optimizing/diagnosing
in unreachable regions.

> Is there a reason it runs before vrp2?

Likely historical or the idea that VRP can do something with its results,
like propagate constants (but there's now a CCP pass after it).
Comment 9 Jakub Jelinek 2022-04-19 05:20:00 UTC
The strlen pass itself for its original purpose doesn't use the ranger at all, it is just the warning code that uses the strlen infrastructure and is invoked during the pass that does.
Comment 10 Jakub Jelinek 2022-05-06 08:33:03 UTC
GCC 12.1 is being released, retargeting bugs to GCC 12.2.
Comment 11 Richard Biener 2022-08-19 08:26:03 UTC
GCC 12.2 is being released, retargeting bugs to GCC 12.3.
Comment 12 Richard Biener 2023-05-08 12:24:11 UTC
GCC 12.3 is being released, retargeting bugs to GCC 12.4.
Comment 13 Andrew Pinski 2023-09-02 20:02:51 UTC
Looks like SCEV can figure out the bounds more correctly in GCC 13+ which fixes the warning and the missed optimization.

GCC 12.3.0:
```
Estimating # of iterations of loop 1
Matching expression match.pd:2119, generic-match.cc:693
Matching expression match.pd:2122, generic-match.cc:753
Matching expression match.pd:2129, generic-match.cc:776
Analyzing # of iterations of loop 1
  exit condition [prephitmp_30, + , -1] != 0
  bounds on difference of bases: -5 ... 1
Applying pattern match.pd:182, generic-match.cc:30997
Applying pattern match.pd:5698, generic-match.cc:11425
Matching expression match.pd:2119, generic-match.cc:693
Matching expression match.pd:2122, generic-match.cc:753
Matching expression match.pd:2129, generic-match.cc:776
Applying pattern match.pd:354, generic-match.cc:5780
  result:
    # of iterations (unsigned int) prephitmp_30, bounded by 4294967295
Statement (exit)if (nb_24 != 0)
 is executed at most (unsigned int) prephitmp_30 (bounded by 4294967295) + 1 times in loop 1.
```

While GCC 13+:
```
Estimating # of iterations of loop 1
Matching expression match.pd:2404, generic-match.cc:727
Matching expression match.pd:2407, generic-match.cc:787
Matching expression match.pd:2414, generic-match.cc:810
Analyzing # of iterations of loop 1
  exit condition [prephitmp_30, + , -1] != 0
  bounds on difference of bases: -5 ... 0
Applying pattern match.pd:182, generic-match.cc:33250
Applying pattern match.pd:6155, generic-match.cc:21253
Matching expression match.pd:2404, generic-match.cc:727
Matching expression match.pd:2407, generic-match.cc:787
Matching expression match.pd:2414, generic-match.cc:810
Applying pattern match.pd:365, generic-match.cc:21998
  result:
    # of iterations (unsigned int) prephitmp_30, bounded by 5
Statement (exit)if (nb_24 != 0)
 is executed at most (unsigned int) prephitmp_30 (bounded by 5) + 1 times in loop 1.
```
Comment 14 Richard Biener 2024-06-20 09:02:43 UTC
GCC 12.4 is being released, retargeting bugs to GCC 12.5.