On 2/4/19, Martin Sebor <mse...@gmail.com> wrote: >>> 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. >> >> Yes, and I argue that these warnings belong in a different "level" of >> warnings than the trivial warnings. > > Introducing more levels sounds fine to me. I wouldn't want to see > a more permissive default; my preference would be the opposite, > leaving it to projects to adjust.
For reference, besides -Wuninitialized and -Wmaybe-uninitialized, clang also has its uninitialized warnings split further into -Wsometimes-uninitialized and -Wconditional-uninitialized. -Wsometimes-uninitialized relies on the integration of their static analyzer into their FE and gets stuff that gcc puts under -Wjump-misses-init and -Wswitch-unreachable instead. -Wconditional-uninitialized is their super-aggressive variant and warns about just about everything. It intentionally includes false positives as described in the comments on this bug: https://bugs.llvm.org/show_bug.cgi?id=38856 If gcc wants to introduce more levels to -Wuninitialized, I'd start by considering the additional flags clang supports. > >> >>> Lastly, in the case of uninitialized variables, the usual solution >>> of initializing them is trivial and always safe (some coding styles >>> even require it). >> >> Here it shows that we don't work with the same type of code at all. If I >> am using a boost::optional, i.e. a class with a buffer and a boolean >> that says if the buffer is initialized, how do I initialize the >> (private) buffer? Or should boost itself zero out the buffer whenever >> the boolean is set to false? The variables can easily be hidden behind >> dozens of levels of abstraction that make them hard to initialize, and >> there may be nothing meaningful to initialize them with. Uninitialized >> also includes clobbered (out-of-scope for instance) in gcc, where it >> isn't clear what you are supposed to initialize to quiet the warning. > > You're right that this is hard to imagine without first hand experience > with the problem. If this is a common pattern with the warning in C++ > class templates in general, a representative test case would help get > a better appreciation of the problem and might also give us an idea > of a better solution. (If there is one in Bugzilla please point me > at it.) > >>>> 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. >> >> Uh? No, I don't know what gave you this impression. > > It's common to view as a false positive (or "bad" as you write below) > every instance of a warning designed to detect or prevent bugs that > doesn't indicate one (as opposed to warnings designed to help write > better/clearer code like -Wparentheses). > >>> "May be unitialized" doesn't preclude the possibility that the >>> variable is not used uninitialized. >> >> But if the variable can never be used uninitialized and gcc is simply >> unable to prove it, the warning is bad. It may not be a "false positive" >> depending on your definition, but it is undesirable. > > Ideally each instance of every warning designed to find bugs would > point out one. But 100% accuracy or a zero rate of the undesirable > instances is not attainable in general, and reducing their rate at > the expense of the helpful ones also isn't the best tradeoff either. > The challenge is striking the right balance between their ratios. > (Only if that can't done then it might be time to consider > disabling/demoting the warning. I don't have the impression > we are at that point with -Wmaybe-uninitialized but I haven't > done any research.) > >>>> 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. >> >> I agree with that. But if I complain about the mindless quickfixes, the >> reply is that customers ask for a library that does not warn, and the >> developers do not have infinite time, so they do the minimum to avoid >> warnings. This is strongly linked to the warning levels. >> >> One could recommend that customers include the library with -isystem to >> disable the warnings inside the library, bug that's fragile (though it >> has gotten quite good), and most importantly it also disables some >> warnings that are relevant to the user. > > Right, that wouldn't be much better than disabling the warning on > the command line (or us removing it from -Wall and perhaps also > from -Wextra). > > So since mindless quick fixes aren't the way to go and assuming we > agree that warnings have value even with some noise, is demoting > them to lower levels because they're not always used properly > the best solution? No predetermined system of warning levels is > going to make everyone happy. Different projects have different > constraints and tolerances for noise, or even resources to fix > real bugs -- GCC with over 400 wrong-code bugs in Open status > being a case in point, so a level that works for one, like > a library, may be overly pedantic for an application. > > More warning levels might help. Warning profiles (say one for > system libraries or freestanding code like the kernel, another > for C++ class libraries, yet another for applications) might > be another approach. Others might be worth brainstorming about. > >>>> 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. >> >> True. To me, one of the key documented properties of -Wall is being >> "easy to avoid". -Wall -Werror should be a usable combination. Anything >> not easy to avoid belongs in a different class, say -Wextra. We can >> rename that to W1/W2/... if Wall is too charged with historical meaning. > > I would have expected dealing with -Wmaybe-uninitialized to always > be easy. Your comment about std::optional suggests there are cases > when it isn't. Can we look at some of those cases and see if there > is something we can do to make it easier? > >>> 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. >> >> This data is going to be super hard to get. Most projects have been >> compiling for years and tweaking their code to avoid some warnings. We >> do not get to see the code that people originally write, we can only see >> what they commit. > > It's not perfect but we should be able to get a rough idea based > on bug reports in Bugzilla. Many distros rebuild the world before > a GCC release and new instances of build-breaking warnings tend to > get filed there. Meta-bugs can be helpful as the first step. > A finer-grained classification by search terms (e.g., "bogus" > "spurious", etc.) helps narrow it down even more. > > Martin >
