* [Bug middle-end/111669] bogus -Wnonnull in conditionally executed code
2023-10-03 4:34 [Bug middle-end/111669] New: bogus -Wnonnull in conditionally executed code zfigura at codeweavers dot com
@ 2023-10-04 1:44 ` xry111 at gcc dot gnu.org
2023-10-04 1:46 ` xry111 at gcc dot gnu.org
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: xry111 at gcc dot gnu.org @ 2023-10-04 1:44 UTC (permalink / raw)
To: gcc-bugs
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".
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Bug middle-end/111669] bogus -Wnonnull in conditionally executed code
2023-10-03 4:34 [Bug middle-end/111669] New: bogus -Wnonnull in conditionally executed code zfigura at codeweavers dot com
2023-10-04 1:44 ` [Bug middle-end/111669] " xry111 at gcc dot gnu.org
@ 2023-10-04 1:46 ` xry111 at gcc dot gnu.org
2023-10-04 15:44 ` zfigura at codeweavers dot com
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: xry111 at gcc dot gnu.org @ 2023-10-04 1:46 UTC (permalink / raw)
To: gcc-bugs
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Bug middle-end/111669] bogus -Wnonnull in conditionally executed code
2023-10-03 4:34 [Bug middle-end/111669] New: bogus -Wnonnull in conditionally executed code zfigura at codeweavers dot com
2023-10-04 1:44 ` [Bug middle-end/111669] " xry111 at gcc dot gnu.org
2023-10-04 1:46 ` xry111 at gcc dot gnu.org
@ 2023-10-04 15:44 ` zfigura at codeweavers dot com
2023-10-05 5:42 ` xry111 at gcc dot gnu.org
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: zfigura at codeweavers dot com @ 2023-10-04 15:44 UTC (permalink / raw)
To: gcc-bugs
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Bug middle-end/111669] bogus -Wnonnull in conditionally executed code
2023-10-03 4:34 [Bug middle-end/111669] New: bogus -Wnonnull in conditionally executed code zfigura at codeweavers dot com
` (2 preceding siblings ...)
2023-10-04 15:44 ` zfigura at codeweavers dot com
@ 2023-10-05 5:42 ` xry111 at gcc dot gnu.org
2023-10-05 5:48 ` xry111 at gcc dot gnu.org
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: xry111 at gcc dot gnu.org @ 2023-10-05 5:42 UTC (permalink / raw)
To: gcc-bugs
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Bug middle-end/111669] bogus -Wnonnull in conditionally executed code
2023-10-03 4:34 [Bug middle-end/111669] New: bogus -Wnonnull in conditionally executed code zfigura at codeweavers dot com
` (3 preceding siblings ...)
2023-10-05 5:42 ` xry111 at gcc dot gnu.org
@ 2023-10-05 5:48 ` xry111 at gcc dot gnu.org
2023-10-05 6:05 ` zfigura at codeweavers dot com
2023-10-05 6:14 ` xry111 at gcc dot gnu.org
6 siblings, 0 replies; 8+ messages in thread
From: xry111 at gcc dot gnu.org @ 2023-10-05 5:48 UTC (permalink / raw)
To: gcc-bugs
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;
}
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Bug middle-end/111669] bogus -Wnonnull in conditionally executed code
2023-10-03 4:34 [Bug middle-end/111669] New: bogus -Wnonnull in conditionally executed code zfigura at codeweavers dot com
` (4 preceding siblings ...)
2023-10-05 5:48 ` xry111 at gcc dot gnu.org
@ 2023-10-05 6:05 ` zfigura at codeweavers dot com
2023-10-05 6:14 ` xry111 at gcc dot gnu.org
6 siblings, 0 replies; 8+ messages in thread
From: zfigura at codeweavers dot com @ 2023-10-05 6:05 UTC (permalink / raw)
To: gcc-bugs
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Bug middle-end/111669] bogus -Wnonnull in conditionally executed code
2023-10-03 4:34 [Bug middle-end/111669] New: bogus -Wnonnull in conditionally executed code zfigura at codeweavers dot com
` (5 preceding siblings ...)
2023-10-05 6:05 ` zfigura at codeweavers dot com
@ 2023-10-05 6:14 ` xry111 at gcc dot gnu.org
6 siblings, 0 replies; 8+ messages in thread
From: xry111 at gcc dot gnu.org @ 2023-10-05 6:14 UTC (permalink / raw)
To: gcc-bugs
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.
^ permalink raw reply [flat|nested] 8+ messages in thread