On 2/1/19 1:31 PM, Marc Glisse wrote: > > I am not surprised, but I had to at least start the conversation. Would > you mind providing a patch that changes the definition of -Wall, since > the current one doesn't quite match reality anymore? Also, what do you > recommend people do when they hit false positives? So I'm not sure how I'd change the definition of -Wall.
> >> If we move it to Wextra it's going to see a lot less usage in real >> world codebases > > I am very tempted by the strawman: should we deprecate -Wextra since > nobody uses it? (Sorry) :-) I don't see a lot of use of -Wextra and I'm always disappointed when we have to relegate useful warnings to it precisely because it gets used so rarely. But as I've indicated earlier on this thread, it may make sense to split out uninitialized warnings on aggregates/addressables and relegate those to -Wextra until we can really beef up analysis in that space. > > Ideally serious projects would use (parts of) -Wextra, at least > occasionally, and with circumspection. But some warnings like > -Wmaybe-uninitialized are dangerous tools in the hands of quickfix > developers, and I am indeed trying to keep it out of their hands... Well, that's a different problem. If the developers are doing quick, dumb hacks to avoid a warning, then the developers need to be retrained. We shouldn't cater the warning set to make that class of developer happy or less dangerous. > >> and potentially lead to the re-introduction of a class of bugs that >> we've largely helped stomp out. > > That's very optimistic. -Wmaybe-uninitialized only detects a very small > proportion of uninitialized uses. Also, my experience is that people > have stomped out the warning, not the bugs. In some cases they even > introduced bugs to stomp out false warnings, or made it harder to detect > real bugs in the future, so the warning did more harm than good. I am > mostly working on large C++ header-only template-heavy scientific > libraries, it is quite possible that people who handle different types > of code bases have a different experience, and -Wmaybe-uninitialized may > have had a more positive impact on other projects. I see things differently. Specifically we've done a good job at making the analysis good for simple objects (anything that's an SSA_NAME). False positives happen, but are relatively rare and developers have through the decades addressed most of these problems (we've had this stuff in -Wall for 3 decades). Are there cases where developers have screwed things up in the process, certainly, but I suspect that's the exception rather than the rule. The simple fact is that if we throw an uninitialized warning on a scalar you *really* have to look at the code to figure out if it's a false positive or not. And you have to really think hard about how to initialize the value if that's in fact what you end up doing -- there's no guaranteed safe value and that's why I've resisted calls to have "initialize everything to XXX" flags through the years. On the addressable/aggregate side things are totally different. We miss tons of warnings, our ability to analyze things to avoid false positives is poor at best, etc. > >> It's also the case that maybe uninitialized vs is uninitialized is >> really just a function of CFG shape. Give me any "maybe uninitialized" >> case and I can turn it into a "is uninitialized" with simple block >> duplication of the forms done by jump threading, path isolation, >> superblock formation, etc. > > Hmm, you know those things better than me, so you are probably right, > but I am not seeing it. We omit "maybe" if, starting from the entry of > the function, and barring exceptions, we know the statement will always > be executed. If you take a false positive for maybe-uninitialized, i.e. > a statement in a dead branch, I don't see how block duplication can make > it so the statement is now always executed. The natural way to remove > "maybe" is through function cloning or outlining. Then you can create > functions that are never called, and any warning about those is a false > positive. Well, that's the problem. Right now the maybe vs always is based on if the value flows through a PHI. But I can avoid having the value flow through a PHI with simple block copying and light CFG manipulation. But in both cases there's still a control dependency path to get to the point where the uninitialized use happens. And you also have the issue of proving that a particular function even executes to begin with. That's why I think the distinction simply doesn't make any sense. We should just call them all maybe-uninitialized uses. Jeff
