[Bug middle-end/111669] New: bogus -Wnonnull in conditionally executed code

[Bug middle-end/111669] New: bogus -Wnonnull in conditionally executed code

[zfigura at codeweavers dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111669

            Bug ID: 111669
           Summary: bogus -Wnonnull in conditionally executed code
           Product: gcc
           Version: 13.2.0
            Status: UNCONFIRMED
          Keywords: diagnostic
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: zfigura at codeweavers dot com
  Target Milestone: ---

Created attachment 56032
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56032&action=edit
reduced testcase, compile with -O2 -Werror=nonnull

Sorry about the rather useless title, but I can't really figure out what
actually triggers this error. It seems to depend on some arcane combination of
optimizations.

I was able to reduce this down to a pretty minimal test case, attached here. It
may be possible to reduce it further but I couldn't easily find a way.

The actual code that triggers this is here [1]. lstrcpyA() and lstrcatA() are
trivial wrappers around strcpy/strcat. The NULL comes from get_search_path() at
line 192. The offending strcpy/strcat will never be reached if
GetWindowsDirectoryA() returns nonzero, which it always should. However, gcc
triggers a -Wnonnull warning anyway.

Interestingly, the original Wine code only triggers the warning with
-march=bdver2 (or other values of -march), but the reduced testcase triggers it
with no -march flags.

[1]
https://source.winehq.org/git/wine.git/blob/HEAD:/dlls/krnl386.exe16/file.c#l639

[Bug middle-end/111669] bogus -Wnonnull in conditionally executed code

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111669

Xi Ruoyao <xry111 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |xry111 at gcc dot gnu.org

--- Comment #1 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
The warning given for the reduced test case is correct because it does not make
sense.  It should be just rewritten as

int GetSystemDirectory16(char *path, int count)
{
    if (GetWindowsDirectoryA())
        __builtin_unreachable();

    return 0;
}

because in the case if GetWindowsDirectoryA() returns non-zero, the code always
invoke undefined behavior.

I'm not sure about the original file.c, but AFAIK GCC developers are keeping to
say "hey, don't use -Werror".

[Bug middle-end/111669] bogus -Wnonnull in conditionally executed code

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111669

--- Comment #2 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(In reply to Xi Ruoyao from comment #1)
> The warning given for the reduced test case is correct because it does not
> make sense.  It should be just rewritten as

I mean, the code does not make sense.

And the warning is given exactly because GCC is optimizing the strcpy call to
unreachable.

[Bug middle-end/111669] bogus -Wnonnull in conditionally executed code

[zfigura at codeweavers dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111669

--- Comment #3 from Zeb Figura <zfigura at codeweavers dot com> ---
(In reply to Xi Ruoyao from comment #2)
> (In reply to Xi Ruoyao from comment #1)
> > The warning given for the reduced test case is correct because it does not
> > make sense.  It should be just rewritten as
> 
> I mean, the code does not make sense.
> 
> And the warning is given exactly because GCC is optimizing the strcpy call
> to unreachable.

If GetWindowsDirectoryA() was idempotent, and GetSystemDirectory16() had no
other users, that might be true, but as it is I don't think so.

The pattern both of those functions call is, much like snprintf(), you pass a
buffer and a size, and if the size is 0 then they'll return the size that would
have been written if there was a large enough buffer. In that case the buffer
can be NULL. In trying to reduce the test case down to the minimal possible
complexity I obscured that fact, but regardless I don't think the reduced
testcase is nonsensical.

[Bug middle-end/111669] bogus -Wnonnull in conditionally executed code

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111669

--- Comment #4 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(In reply to Zeb Figura from comment #3)
> (In reply to Xi Ruoyao from comment #2)
> > (In reply to Xi Ruoyao from comment #1)
> > > The warning given for the reduced test case is correct because it does not
> > > make sense.  It should be just rewritten as
> > 
> > I mean, the code does not make sense.
> > 
> > And the warning is given exactly because GCC is optimizing the strcpy call
> > to unreachable.
> 
> If GetWindowsDirectoryA() was idempotent, and GetSystemDirectory16() had no
> other users, that might be true, but as it is I don't think so.
> 
> The pattern both of those functions call is, much like snprintf(), you pass
> a buffer and a size, and if the size is 0 then they'll return the size that
> would have been written if there was a large enough buffer. In that case the
> buffer can be NULL. In trying to reduce the test case down to the minimal
> possible complexity I obscured that fact, but regardless I don't think the
> reduced testcase is nonsensical.

Then that's why -Wnonnull is a warning, not an error.

Using -Werror is just telling the compiler "you can reject valid code", anyway.

[Bug middle-end/111669] bogus -Wnonnull in conditionally executed code

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111669

--- Comment #5 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
And you can tell the compiler some fact about the semantics of the Windoge API
functions if you really need -Werror=nonnull (though I cannot see any reason
you must use -Werror here):

int GetSystemDirectory16(char *path, int count)
{
    if (GetWindowsDirectoryA()) {
        /* For blah blah blah... reason
           path will never be NULL if GetWindowsDirectoryA returns true */
        if (!path)
            __builtin_unreachable();

        __builtin_strcpy(path, "foo");
    }

    return 0;
}

[Bug middle-end/111669] bogus -Wnonnull in conditionally executed code

[zfigura at codeweavers dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111669

--- Comment #6 from Zeb Figura <zfigura at codeweavers dot com> ---
It is my impression that gcc is interested in avoiding false positives for its
warnings. This isn't to say that there aren't some number of false positives in
existence, but it is my impression that gcc is interested in reducing that
number.

It is also my impression that -Wnonnull is not *supposed* to emit warnings for
cases where, from the compiler's point of view, NULL might be passed, but some
high-level invariant prevents this. Compare -Wmaybe-uninitialized, where the
documentation clearly specifies otherwise.

If both of these impressions are incorrect, this bug report can be closed as
WONTFIX.


(In reply to Xi Ruoyao from comment #5)
> And you can tell the compiler some fact about the semantics of the Windoge
> API functions if you really need -Werror=nonnull (though I cannot see any
> reason you must use -Werror here):

If it makes a difference, please feel free to pretend I said -Wnonnull, rather
than -Werror=nonnull. It was merely a debugging aid, meant to help me try to
narrow down the conditions causing this error.

[Bug middle-end/111669] bogus -Wnonnull in conditionally executed code

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111669

Xi Ruoyao <xry111 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement

--- Comment #7 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(In reply to Zeb Figura from comment #6)
> It is my impression that gcc is interested in avoiding false positives for
> its warnings.

Correct, but we are also interested in avoiding false negatives.  Without extra
information provided by something like __builtin_unreachable, any change
decreasing false positives will increase false negatives (unless the false
positive is completely stupid: for the simplified test case I think the false
positive not completely stupid, but maybe it is completely stupid for your
original program).

> It is also my impression that -Wnonnull is not *supposed* to emit warnings
> for cases where, from the compiler's point of view, NULL might be passed,
> but some high-level invariant prevents this. Compare -Wmaybe-uninitialized,
> where the documentation clearly specifies otherwise.

Maybe we can separate -Wnonnull into -Wmaybe-nonnull and -Wnonnull, or just
make -Wnonnull not to emit warnings for conditional paths and tell users
expecting a nonnull warning in conditional paths to use the analyzer (it's very
supposed to warn even in conditional paths) instead.

> If both of these impressions are incorrect, this bug report can be closed as
> WONTFIX.

I'll keep it open but make it an enhancement.