On 1/31/21 5:31 PM, Martin Sebor via Gcc-patches wrote:
> The initial -Wnonnull implementation in the middle end took place
> too late in the pipeline (just before expansion), and as a result
> was prone to false positives (bug 78817). In an attempt to avoid
> the worst of those, the warning was moved to the ccp2 pass in
> r243874. However, as the test case in PR 87489 shows, this is
> in turn too early and causes other false positives as a result.
>
> A few experiments with running the warning later suggest that
> just before the mergephi2 pass is a good point to avoid this class
> of false positives without causing any regressions or introducing
> any new warnings either in Glibc or in Binutils/GDB.
>
> Since PR 87489 is a GCC 8-11 regression I propose to make this
> change on the trunk as well as on the release branches.
>
> Martin
>
> gcc-87489.diff
>
> PR middle-end/87489 - Spurious -Wnonnull warning on bitwise arithmetic and
> inlining
>
> gcc/ChangeLog:
>
> PR middle-end/87489
> * passes.def (pass_post_ipa_warn): Move later.
>
> gcc/testsuite/ChangeLog:
>
> PR middle-end/87489
> * gcc.dg/Wnonnull-6.c: New test.
> * gcc.dg/Wnonnull-7.c: New test.
So moving passes is generally not the right solution to most problems --
it usually turns into a game of wack-a-mole. So rather than looking
at pass reordering, let's look at the IL and see if there's reasonable
optimizations we could do that in turn would avoid the false positive.
I mentioned this in c#11 in the BZ.
What I missed last year was that we could improve the backwards threader.
If we look at the forwprop1 dump we have this leading up to the
problematical strlen call:
;; basic block 2, loop depth 0, maybe hot
;; prev block 0, next block 3, flags: (NEW, VISITED)
;; pred: ENTRY (FALLTHRU,EXECUTABLE)
j = 0;
if (i_13(D) != 0)
goto <bb 3>; [INV]
else
goto <bb 4>; [INV]
;; succ: 3 (TRUE_VALUE,EXECUTABLE)
;; 4 (FALSE_VALUE,EXECUTABLE)
;; basic block 3, loop depth 0, maybe hot
;; prev block 2, next block 4, flags: (NEW, VISITED)
;; pred: 2 (TRUE_VALUE,EXECUTABLE)
j.0_1 = j;
_2 = j.0_1 | 1;
j = _2;
;; succ: 4 (FALLTHRU,EXECUTABLE)
;; basic block 4, loop depth 0, maybe hot
;; prev block 3, next block 5, flags: (NEW, VISITED)
;; pred: 2 (FALSE_VALUE,EXECUTABLE)
;; 3 (FALLTHRU,EXECUTABLE)
j.1_3 = j;
if (j.1_3 != 0)
goto <bb 5>; [INV]
else
goto <bb 6>; [INV]
;; succ: 5 (TRUE_VALUE,EXECUTABLE)
;; 6 (FALSE_VALUE,EXECUTABLE)
;; basic block 5, loop depth 0, maybe hot
;; prev block 4, next block 6, flags: (NEW, VISITED)
;; pred: 4 (TRUE_VALUE,EXECUTABLE)
f (&j, 0);
;; succ: 6 (FALLTHRU,EXECUTABLE)
;; basic block 6, loop depth 0, maybe hot
;; prev block 5, next block 7, flags: (NEW, VISITED)
;; pred: 4 (FALSE_VALUE,EXECUTABLE)
;; 5 (FALLTHRU,EXECUTABLE)
j.2_4 = j;
_5 = j.2_4 & 1;
if (_5 != 0)
goto <bb 7>; [INV]
else
goto <bb 8>; [INV]
;; succ: 7 (TRUE_VALUE,EXECUTABLE)
;; 8 (FALSE_VALUE,EXECUTABLE)
Of particular interest is the fact that we always know where BB4 will
go. If we take 2->3->4 then 4 will always transfer to 5. If we take
2->4 then 4 will always transfer to 6.
The problem is the backwards threader is a bit dumb in that it only
allows a very limited number of RHS expressions. So when it sees j.1_3
= j in BB4 it gives up.
If we fix that and use walk_aliased_vdefs we can pretty easily find the
j = 0 statement in BB2 and thread 2->4->6. That in turn allows
subsequent optimizers to do a better job and the problematical call is gone.
While that is enough to fix the testcase and it helps clean up the code
in the original BZ a bit, we still get the warning. We may need to
handle variants of this kind of code:
;; basic block 4, loop depth 0, maybe hot
;; prev block 3, next block 5, flags: (NEW, VISITED)
;; pred: 2 (TRUE_VALUE,EXECUTABLE)
_1 = xl_xinfo.xinfo;
_2 = _1 | 16;
xl_xinfo.xinfo = _2;
xl_twophase.xid = twophase_xid_16(D);
_3 = xl_xinfo.xinfo;
if (_3 != 0)
goto <bb 5>; [INV]
else
goto <bb 6>; [INV]
;; succ: 5 (TRUE_VALUE,EXECUTABLE)
;; 6 (FALSE_VALUE,EXECUTABLE)
We can see pretty easily that _3 is never zero. The backwards threader
can't make that deduction, but I think I see how to add it pretty
easily. The combination of those two extensions *should* allow the
backwards threader to clean this up much earlier in the pipeline and
avoid the warning.
Anyways, still poking around, but my general sense is that changing pass
ordering can be avoided here.
jeff
