On 2/20/19, Jeff Law <l...@redhat.com> wrote: > 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.
Part of that could be because they're still using the old name of "-W" for it. Since the documentation says the name "-Wextra" is preferred, maybe it's time to deprecate the old "-W" alias to get people to spell it just one way; that would make it easier to count its usage. > > 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 >
