run -Wnonnull later (PR 87489)

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

Ping:
   https://gcc.gnu.org/pipermail/gcc-patches/2021-February/564597.html

On 1/31/21 5:31 PM, Martin Sebor 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

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

Am 2021-02-01 um 01:31 schrieb Martin Sebor via Gcc-patches:
> 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.

Hi Martin,

I tested your patch and it showed also one more warning for this 
testcase with -O2 -Wnonnull:

class b {
public:
   long c();
};
class B {
public:
   B() : f() {}
   b *f;
};
long d, e;
class g : B {
public:
   void h() {
     long a = f->c();
     d = e = a;
   }
};
class j {
   j();
   g i;
};
j::j() { i.h(); }

I hope cvise didn't minimize too much, but at least in the original much 
larger code the warning looks reasonable too.

Franz

On 2/19/21 2:48 AM, Franz Sirl wrote:
> Am 2021-02-01 um 01:31 schrieb Martin Sebor via Gcc-patches:
>> 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.
> 
> Hi Martin,
> 
> I tested your patch and it showed also one more warning for this 
> testcase with -O2 -Wnonnull:
> 
> class b {
> public:
>    long c();
> };
> class B {
> public:
>    B() : f() {}
>    b *f;
> };
> long d, e;
> class g : B {
> public:
>    void h() {
>      long a = f->c();
>      d = e = a;
>    }
> };
> class j {
>    j();
>    g i;
> };
> j::j() { i.h(); }
> 
> I hope cvise didn't minimize too much, but at least in the original much 
> larger code the warning looks reasonable too.

Thanks.  Yes, the warning would be useful here since the f pointer
in the call to f->c() is null when i.h() is called from j's ctor.
The FRE3 pass clearly exposes this :

void j::j (struct j * const this)
{
   long int _9;

   <bb 2> [local count: 1073741824]:
   MEM[(struct B *)this_3(D)] ={v} {CLOBBER};
   MEM[(struct B *)this_3(D)].f = 0B;
   _9 = b::c (0B);
   ...

Because the warning runs early (after CCP2), the null pointer is
not detected.  To see it the warning code would have to work quite
hard to figure out that the two assignments below refer to the same
location (it would essentially have to do what FRE does):

   MEM[(struct B *)this_3(D)].f = 0B;
   _7 = MEM[(struct b * *)_1];
   _9 = b::c (_7);

It's probably too late to make this change for GCC 11 as Jeff has
already decided that it should be deferred until GCC 12, and even
then there's a concern that doing so might cause false positives.
I think it's worth revisiting the idea in GCC 12 to see if
the concern is founded.  Let me make a note of it in the bug.

Martin