Bug 87489 - [8/9/10/11 Regression] Spurious -Wnonnull warning
Summary: [8/9/10/11 Regression] Spurious -Wnonnull warning
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 8.2.0
: P2 normal
Target Milestone: 11.0
Assignee: Not yet assigned to anyone
URL:
Keywords: deferred, diagnostic
Depends on:
Blocks: Wnonnull
  Show dependency treegraph
 
Reported: 2018-10-02 17:07 UTC by Andres Freund
Modified: 2020-06-03 16:48 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 10.1.0, 11.0, 8.4.0, 9.3.0
Last reconfirmed: 2020-06-03 00:00:00


Attachments
Repro (432 bytes, text/plain)
2018-10-02 17:07 UTC, Andres Freund
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Freund 2018-10-02 17:07:39 UTC
Created attachment 44775 [details]
Repro

Hi,

With the attached, obviously heavily condensed, testcase I get a spurious -Wnonnull warning.  It's quite true that the argument to strlen is NULL, but the code isn't reachable if so.   As the warning isn't phrased as "may be", that doesn't seem quite right.

$ gcc-8 -Wnonnull -O2 -c xact.stripped.i
In function ‘XactLogCommitRecord.constprop’,
    inlined from ‘RecordTransactionCommit’ at xact.stripped.i:51:3:
xact.stripped.i:44:50: warning: argument 1 null where non-null expected [-Wnonnull]
    XLogRegisterData((const char *) twophase_gid, strlen(twophase_gid) + 1);
                                                  ^~~~~~~~~~~~~~~~~~~~
xact.stripped.i: In function ‘RecordTransactionCommit’:
xact.stripped.i:19:15: note: in a call to function ‘strlen’ declared here
 extern size_t strlen (const char *__s)
               ^~~~~~


While the concrete problem in postgres only triggers for gcc-8 at -O3, the simplified testcase triggers for 7, 8, and a recent-ish trunk, and even with -O1.

Without the first XLogRegisterData() call the warning does not appear.

- Andres
Comment 1 Jonathan Wakely 2018-10-02 17:38:41 UTC
The first XLogRegisterData could change the value of xl_xinfo.xid to be non-zero, in which case the second XLogRegisterData call would happen despite the null string.
Comment 2 Andres Freund 2018-10-02 17:43:35 UTC
Maybe (In reply to Jonathan Wakely from comment #1)
> The first XLogRegisterData could change the value of xl_xinfo.xid to be
> non-zero, in which case the second XLogRegisterData call would happen
> despite the null string.

Well, that'd then still only be a ok for a "may be" warning, not one that say "argument 1 null where" (i.e. without a may be). Also, XLogRegisterData here takes a const char * argument, so it really shouldn't modify the argument?
Comment 3 Manuel López-Ibáñez 2018-10-02 20:35:50 UTC
(In reply to Jonathan Wakely from comment #1)
> The first XLogRegisterData could change the value of xl_xinfo.xid to be
> non-zero, in which case the second XLogRegisterData call would happen
> despite the null string.

This is not what is happening, though. At 083t.fixup_cfg3, GCC does propagate the null pointer to the call of strlen(). This gets cleaned up latter so that the whole code boils down to:

void
RecordTransactionCommit(void)
{
  return;
}

but not before the ipa warn pass sees the strlen(0). When the first XLogRegisterData() call is replace by return, the code is simple enough that the CCP pass before the warning pass is able to simply the whole function to a return as above, so no warning.

What is interesting is that VRP info already shows that this cannot be zero:

  # RANGE [0, 9223372036854775805] NONZERO 9223372036854775807
  # USE = anything 
  _6 = strlenD.895 (0B);

so the warning could be enhanced to look at this info if available (it is available even at -O1). Or the pass moved later in the pipeline (after fre3).
Comment 4 Martin Sebor 2018-10-02 22:05:25 UTC
The original -Wnonnull implementation was done in the front end, making it nearly useless (bug 17308).  To improve it, the enhancement committed in r243661 did the checking very late (in calls.c).  The initial commit triggered a flurry of warnings during bootstrap (bug 78817) due to various limitations in GCC's optimization passes, which ultimately led to moving the implementation to a much earlier stage where it doesn't benefit from some of the useful optimizations that are done later.
Comment 5 Andrew Church 2018-10-30 04:34:56 UTC
Simpler testcase (based on the testcase in bug 87041):

extern int strcmp(const char *a, const char *b) __attribute__((nonnull(1, 2)));
int foo(void) {
    const char * const s = 0;
    if (s)
        return strcmp(s, "");
    else
        return 2;
}

foo.c: In function 'foo':
foo.c:5:16: warning: null argument where non-null required (argument 1) [-Wnonnull]
         return strcmp(s, "");
                ^~~~~~
Comment 6 Martin Sebor 2018-10-30 17:15:40 UTC
(In reply to Andrew Church from comment #5)
> Simpler testcase (based on the testcase in bug 87041):

This is a case of the warning being issued by the front end well before dead code elimination has had a chance to run.  The fix is conceptually simple: remove the warning from the front-end and enhance the middle-end implementation to handle the cases it doesn't handle, such as:

  int foo (int i, const char *s)
  {
    return strcmp (i ? s : 0, "");
  }

But the middle-end implementation of -Wnonnull only runs with optimization enabled (just after CCP) so the change would have the effect of a large number of  false negatives at -O0.  I'm not sure handling the constant test case in comment #5 is worth it.
Comment 7 Andrew Church 2018-10-31 01:44:35 UTC
Would it be reasonable to have the FE warning trigger only on a literal null value and not on variables whose values are known to be null?  I don't know the history behind -Wnonnull warning at two separate points, but if the FE warning is intended to cover cases in which (for example) argument order is inadvertently switched to pass an otherwise-reasonable null to a nonnull parameter, then not warning on variables doesn't seem like it would make much of a difference.  And I would not expect the compiler to catch data flow bugs ("variable declared as constant null" -> "constant null variable passed to nonnull function parameter") at -O0 anyway.
Comment 8 Martin Sebor 2018-10-31 02:35:06 UTC
It might be reasonable but it isn't possible to distinguish one from the other.  The value of s has been folded into a constant by the time the warning is issued, and its origin is lost.  In both cases the warning code sees something like

  <integer_cst 0x7fffefb9d948 type <pointer_type 0x7fffefb95930> constant 0>

It's the same consequence of r254930 as bug 87041.
Comment 9 Jakub Jelinek 2019-02-05 10:45:38 UTC
(In reply to Manuel López-Ibáñez from comment #3)
> What is interesting is that VRP info already shows that this cannot be zero:
> 
>   # RANGE [0, 9223372036854775805] NONZERO 9223372036854775807

No, bits set in NONZERO bitmask are "may be nonzero", not must be nonzero.  Bits clear in that bitmask are "must be zero".  So NONZERO 9223372036854775807 just says that the most significant bit is clear, nothing else.
(In reply to Andres Freund from comment #2)

> Maybe (In reply to Jonathan Wakely from comment #1)
> > The first XLogRegisterData could change the value of xl_xinfo.xid to be
> > non-zero, in which case the second XLogRegisterData call would happen
> > despite the null string.
> 
> Well, that'd then still only be a ok for a "may be" warning, not one that
> say "argument 1 null where" (i.e. without a may be). Also, XLogRegisterData
> here takes a const char * argument, so it really shouldn't modify the
> argument?

For most of the warnings, we don't have "may be" and "is" variants, and while we do have it for -W*uninitialized, the boundary isn't that clear.  The warning on the #c0 is IMHO desirable, whenever that strlen is called, it will be with NULL argument, and the compiler can't prove (because the other call that takes the address of the info stuff) the call will not be called with the NULL argument ever.  For -W*uninitialized, we call "is" if the uninitialized use happens always if the function is called, no exception is thrown or calls on the path to the statement never return, and "may be" otherwise.  Even the "is" are "may be", it all depends on that the function is called at all and that the code is actually reached.
Comment 10 Jakub Jelinek 2019-02-22 15:19:34 UTC
GCC 8.3 has been released.
Comment 11 Jeffrey A. Law 2020-01-29 22:49:47 UTC
A few notes.

After FRE3 would be the first point where things are optimized enough to avoid the warning.  Though in general pass order juggling is not the way to solve these kinds of problems.

To fix this without pass juggling we'd have to improve something between inlining and CCP2 inclusive.  Inlining is necessary to expose the constants.  I don't any good pass between those points to optimize this mess:

;;   basic block 2, loop depth 0
;;    pred:       ENTRY
  xl_xinfo.xinfo = 0;
  _3 = xl_xinfo.xinfo;
  if (_3 != 0)
    goto <bb 3>; [33.00%]
  else
    goto <bb 4>; [67.00%]
;;    succ:       3
;;                4

;;   basic block 3, loop depth 0
;;    pred:       2
  XLogRegisterData (&xl_xinfo.xinfo, 4);
;;    succ:       4

;;   basic block 4, loop depth 0
;;    pred:       2
;;                3
  _4 = xl_xinfo.xinfo;
  _5 = _4 & 16;
  if (_5 != 0)
    goto <bb 5>; [33.00%]
  else
    goto <bb 6>; [67.00%]
;;    succ:       5
;;                6

;;   basic block 5, loop depth 0
;;    pred:       4
  _6 = strlen (0B);
  _7 = (unsigned int) _6;
  _8 = _7 + 1;
  _9 = (int) _8;
  XLogRegisterData (0B, _9);
;;    succ:       6


To optimize things enough to avoid the false positive we'd have to realize that _3 is zero, which in turn would allow removal of the conditional at the end of bb2 and all of bb3.  Then we'd have to discover that _4 is zero, which is only possible *after* removing bb3.  Once we discover that _4 is zero, then we'd be able to optimize away the conditional at the end of bb4 and avoid the false positive.

Now the first step could be bolted onto CCP.  It's not terribly hard.  But the secondary effects we need (specifically removing bb3 and discovery that _4 is always zero) are quite difficult to capture at this point, particularly since they occur as post-pass cleanups.

So I don't see improving CCP as a viable path to addressing this problem.  What would probably work better would be a FRE pass between CCP and the IPA warnings.  But that's pretty expensive.  SImilarly running DOM + threading between CCP and the IPA warnings would be sufficient to find and eliminate the dead code.

As noted we could move this warning late, but that brings its own set of problems.

Realistically this is going to have to defer to gcc-11 and likely beyond.