On 2/1/19 4:32 AM, Marc Glisse wrote:
Hello,
first, I expect this to be controversial, so feel free to complain.
I don't feel too strongly about whether -Wmaybe-uninitialized should
be in -Wall or in -Wextra, and I might even be somewhat more inclined
to expect to find it in the latter. But since you sound like you are
gearing up for proposing other changes in the same vein down the line
where I might have stronger opinions, I should comment.
[It's a bit of a long-winded response because I've been thinking about
this topic a lot lately.]
In general, I think a discussion of warning groupings is worth having
(even needed) at the right time, but stage 4 doesn't strike me as
the most opportune occasion.
Specifically for -Wmaybe-uninitialized, the option has been in -Wall
for many releases, and no major changes to it have been made recently
that I know. So what I'm missing in this proposal is: why now? What
has changed to make this a pressing issue now? Has its rate of false
positives gone up significantly? If so, by how much and why?
The description of -Wall says "This enables all the warnings about
constructions that some users consider questionable, and that are easy
to avoid (or modify to prevent the warning), even in conjunction with
macros."
And the description of -Wmaybe-uninitialized "For an automatic variable,
if there exists a path from the function entry to a use of the variable
that is initialized, but there exist some other paths for which the
variable is not initialized, the compiler emits a warning if it cannot
prove the uninitialized paths are not executed at run time. These
warnings are made optional because GCC is not smart enough to see all
the reasons why the code might be correct in spite of appearing to have
an error."
-Wmaybe-uninitialized generates false positives, we can tweak the
compiler to reduce them, but there will always be some, that's in the
nature of this warning.
Please be clear about what you mean by false positives. Is it that
the warning triggers contrary to the documentation ("a path exists
where the variable is uninitialized along with one where it is"),
or that the path to the use of the variable does exist but we
(though not GCC) can tell from the context that it cannot be reached?
(The latter wouldn't qualify as a false positive as the term is defined
in literature; i.e., the warning works as designed, we just don't like
or agree with it.)
In practice, false positives (and negatives) of both kinds, whether
they fit the formal definition or the informal one, are the nature
of virtually all non-trivial static diagnostics, certainly all those
that depend on control or data flow analysis. Some are due to bugs
or limitations in the implementation of the warning. Others are
inherent in the technology. Is this warning more prone to one kind
than others? If so, is it because it's implemented poorly, or that
its implementation hasn't kept up with the improvements to
the optimizers, or has the infrastructure it depends on become
ill-suited for it to avoid some of the false positives (as formally
defined), or is it something else?
These false positives are not easy to avoid, as required to be part of
-Wall. Avoiding them, when it is possible at all, requires not just a
syntactic tweak, like adding parentheses, but a semantic change that can
make the code worse. Initializing something that does not need it is
extra code (increases code size and running time). It also prevents
better tools from detecting true uninitialized uses, either static
analyzers or runtime checkers (sanitizer, valgrind).
I don't find the argument very compelling that diagnosing potential
bugs should be avoided because the fix (or the suppression in
the case of a false positive) could be wrong. The risk exists with
all diagnostics.
I also take issue with the suggestion that dynamic analysis is
necessarily a superior mechanism for detecting bugs. They each have
their strengths and weaknesses. They are not an either-or proposition
but rather complementary solutions.
Lastly, in the case of uninitialized variables, the usual solution
of initializing them is trivial and always safe (some coding styles
even require it). Initializing scalars has a negligible performance
impact in practice, and, if it's thought to be necessary, can easily
be done conditionally (as in, based on some macro) so that
the uninitialized accesses can still be detected by dynamic analysis.
This message concentrates on the negatives, but that doesn't mean I
consider -Wmaybe-uninitialized as useless. It can find true
uninitialized uses. And even some false positives can point at places
where we can help the compiler generate better code (say with a default
__builtin_unreachable case in a switch). I did in the past contribute
patches to make it warn more often, and I might do so again in the future.
This paragraph makes me think you equate false positives from this
warning with those from -Wuninitialized. They are (obviously) not
the same. "May be unitialized" doesn't preclude the possibility that
the variable is not used uninitialized. I'm not just nit-picking
here either. It's important to use the same vocabulary.
My opinion is that -Wmaybe-uninitialized would serve its purpose better
as part of -Wextra. People tend to use -Wall with -Werror (either
explicitly or implicitly by modifying the code until all warnings are
gone). What I see currently in projects where I participate is that
-Wmaybe-uninitialized is making things worse. People don't investigate
deeply the cause of the warning, they just go for whatever "quick-fix"
makes the compiler shut up. Quite often, this involves extra code that
is less readable and performs worse, while it didn't even "fix" what
caused the warning, it just randomly ended up with code where the
compiler doesn't warn (possibly because the function got bigger, which
changed inlining decisions...).
I agree that we need to be mindful of warnings leading to unnecessary
and perhaps inappropriate code changes (as the author of a few warnings
with this potential it has been on mind a lot).
At the same time, not taking the time to properly analyze diagnostics
and come up with the appropriate solutions seems like a problem that's
orthogonal to the warnings themselves. Disabling warnings in hopes of
solving what's really a problem with the development process or
a culture of a subset of projects doesn't seem like an optimal solution.
If the warning is not enabled by default so much but only when people
are ready to investigate any warning thoroughly, the quickfix mentality
is less likely to be present. People using -Wmaybe-uninitialized need to
be willing to ignore false positives, or disable them with pragmas.
Note that similar arguments may apply to some other warnings that
somehow made their way into -Wall when they shouldn't have, but for now
I am only proposing to move -Wmaybe-uninitialized. Some people tend to
consider that if a warning is not part of -Wall, it might as well not
exist. Obviously I disagree with that.
The description of -Wall is vague enough that arguments about which
warnings should or shouldn't be enabled by it are unavoidably subject
to individual biases and unlikely to lead to a consensus, either among
ourselves or, even less likely, among users. A shared definition of
a false positive should be one of the very first steps to coming closer
to a consensus. Real world (as opposed to anecdotal) data on the rates
of actual rates of false positives and negatives vs true positives would
be also most helpful, as would some consensus of the severity of
the bugs the true positives expose, as well as some objective measure
of the ease of suppression. There probably are others but these would
be a start.
But one size never fits all, so even if we do manage to come to
a consensus among ourselves, it will not make everyone happy(*).
I wonder of we need a better solution than just two (or even four,
f you count enabled-by-default and disabled- by-default) more or
less arbitrarily selected buckets of warnings.
Martin
[*] There are nearly 30 open bugs in Bugzilla about options people
think should or shouldn't be enabled by -Wall or -Weextra, including
bug 89072 that request to enable -Wall and -Werror by default.
-------
Now the actual patch. Surprisingly, the middle-end puts both
Wuninitialized and Wmaybe-uninitialized in Wextra, it is the C-family of
front-ends that puts them in Wall. It also makes Wuninitialized enable
Wmaybe-uninitialized, which is backwards (it made sense historically),
Wuninitialized has much fewer false positives, and if we are willing to
be warned about possibly uninitialized uses, we certainly also want
warnings about uninitialized uses that are certain. So I am switching
the enabling relation between those 2, and enabling only Wuninitialized
at Wall.
If the patch gets in, this will of course require a mention in the
release notes.
I changed a set of tests based on a mix of grep and seeing what failed
make check. The exact list may not be optimal.
gcc/ChangeLog:
2019-02-01 Marc Glisse <marc.gli...@inria.fr>
* common.opt (Wuninitialized): Enable with Wmaybe-uninitialized.
(Wmaybe-uninitialized): Enable with Wextra.
* doc/invoke.texi: Update implications between Wuninitialized,
Wmaybe-uninitialized, Wall and Wextra.
gcc/c-family/ChangeLog:
2019-02-01 Marc Glisse <marc.gli...@inria.fr>
* c.opt (Wmaybe-uninitialized): Enable with Wextra.
gcc/testsuite/ChangeLog:
2019-02-01 Marc Glisse <marc.gli...@inria.fr>
* c-c++-common/pr69543-1.c: Use -Wmaybe-uninitialized.
* c-c++-common/pr69543-2.c: Likewise.
* c-c++-common/pr69543-3.c: Likewise.
* c-c++-common/pr69543-4.c: Likewise.
* c-c++-common/uninit-17.c: Likewise.
* g++.dg/pr48484.C: Likewise.
* g++.dg/uninit-pred-1_b.C: Likewise.
* g++.dg/uninit-pred-2_b.C: Likewise.
* g++.dg/uninit-pred-3_b.C: Likewise.
* g++.dg/warn/Wuninitialized-5.C: Likewise.
* g++.dg/warn/Wuninitialized-6.C: Likewise.
* g++.dg/warn/Wuninitialized-9.C: Likewise.
* gcc.dg/gomp/pr72781.c: Likewise.
* gcc.dg/pr18501.c: Likewise.
* gcc.dg/pr39666-2.c: Likewise.
* gcc.dg/pr45083.c: Likewise.
* gcc.dg/pr57149.c: Likewise.
* gcc.dg/pr59924.c: Likewise.
* gcc.dg/pr69543.c: Likewise.
* gcc.dg/uninit-11-O0.c: Likewise.
* gcc.dg/uninit-11.c: Likewise.
* gcc.dg/uninit-16.c: Likewise.
* gcc.dg/uninit-17.c: Likewise.
* gcc.dg/uninit-18.c: Likewise.
* gcc.dg/uninit-19.c: Likewise.
* gcc.dg/uninit-6-O0.c: Likewise.
* gcc.dg/uninit-pr19430-2.c: Likewise.
* gcc.dg/uninit-pr19430-O0.c: Likewise.
* gcc.dg/uninit-pr19430.c: Likewise.
* gcc.dg/uninit-pr20644-O0.c: Likewise.
* gcc.dg/uninit-pred-2_a.c: Likewise.
* gcc.dg/uninit-pred-2_b.c: Likewise.
* gcc.dg/uninit-pred-2_c.c: Likewise.
* gcc.dg/uninit-pred-3_a.c: Likewise.
* gcc.dg/uninit-pred-3_b.c: Likewise.
* gcc.dg/uninit-pred-3_c.c: Likewise.
* gcc.dg/uninit-pred-3_d.c: Likewise.
* gcc.dg/uninit-pred-3_e.c: Likewise.
* gcc.dg/uninit-pred-4_a.c: Likewise.
* gcc.dg/uninit-pred-4_b.c: Likewise.
* gcc.dg/uninit-pred-5_a.c: Likewise.
* gcc.dg/uninit-pred-5_b.c: Likewise.
* gcc.dg/uninit-pred-6_a.c: Likewise.
* gcc.dg/uninit-pred-6_b.c: Likewise.
* gcc.dg/uninit-pred-6_c.c: Likewise.
* gcc.dg/uninit-pred-6_d.c: Likewise.
* gcc.dg/uninit-pred-6_e.c: Likewise.
* gcc.dg/uninit-pred-7_a.c: Likewise.
* gcc.dg/uninit-pred-7_b.c: Likewise.
* gcc.dg/uninit-pred-7_c.c: Likewise.
* gcc.dg/uninit-pred-7_d.c: Likewise.
* gcc.dg/uninit-pred-8_a.c: Likewise.
* gcc.dg/uninit-pred-8_b.c: Likewise.
* gcc.dg/uninit-pred-8_c.c: Likewise.
* gcc.dg/uninit-pred-8_d.c: Likewise.
* gcc.dg/uninit-pred-9_a.c: Likewise.
* gcc.dg/uninit-pred-9_b.c: Likewise.
* gcc.dg/uninit-suppress_2.c: Likewise.
* gfortran.dg/pr25923.f90: Likewise.
* gfortran.dg/pr39666-2.f90: Likewise.
