Re: Move -Wmaybe-uninitialized to -Wextra
On Mon, 2019-12-16 at 15:45 +0100, Martin Jambor wrote: > Hi Jeff, > > On Sat, Dec 07 2019, Jeff Law wrote: > > [...] > > The whole point behind the uninitialized warning is to capture cases > > where objects may not be properly initialized. For modern code the > > simple cases typically "just work". What is by far the most > > interesting cases are those with complex flow control, often > > interacting with inline functions, address-of stripping, etc. These > > are precisely the cases that humans aren't particularly good at > > catching and having the compiler analyze those paths and issue warnings > > that humans fix ultimately results in better quality code. > > Or "fixes" like: > > https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/expmed.c?r1=277864=277863=277864 :( > > if even we cannot deal with the false positive in our own compiler, how > can we expect our users to do so? Fair point. And I've certainly seen painful-to-fix fallout in my Fedora builds -- by far the most painful cases have been addressables where we're able to strip the address-of due to inlining. > > > Experience has shown that if you put something in -Wall, people will > > pay attention to it, and that is good for the long term quality of code > > bases. If the diagnostic is outside of -Wall, it's largely ignored. I > > think the pain of dealing with the Wuninitialized warts is smaller than > > the pain of allowing these errors to persist. > > I'm afraid I that -Wmaybe-uninitialized is getting out of hand. I bet > that some users regularly get these warnings coming from c++ header > "libraries" (like they sometimes come out our vec.h which recently > "broke" bootstrap) which they sometimes even cannot change and they then > conclude that our -Wall is "unusable" and stop paying attention to all > warnings. If it's coming from our headers, then we make a huge effort to fix the issues :-) jeff
Re: Move -Wmaybe-uninitialized to -Wextra
On 12/16/19 2:45 PM, Martin Jambor wrote: > On Sat, Dec 07 2019, Jeff Law wrote: >> [...] > I'm afraid I that -Wmaybe-uninitialized is getting out of hand. I bet > that some users regularly get these warnings coming from c++ header > "libraries" (like they sometimes come out our vec.h which recently > "broke" bootstrap) which they sometimes even cannot change and they then > conclude that our -Wall is "unusable" and stop paying attention to all > warnings. -Wmaybe-uninitialized that trigger in std::optional (and clones) (PR80635 [1]) are particularly annoying, and there's no sane workaround the user can apply. You'll find quite a number of those just by googling for it: https://www.google.com/search?q=std+optional+"-Wmaybe-uninitialized; We have a few of those in GDB, and because GDB uses -Wall + -Werror, GDB nowadays builds with -Wno-error=maybe-uninitialized so that we see the warnings but the build continues without error. People still occasionally get confused and waste time with those warnings, though. Here, just this week, point 5: https://sourceware.org/ml/gdb-patches/2019-12/msg00706.html FWIW, I've considered completely disabling -Wmaybe-uninitialized in GDB instead of downgrading it from -Werror to a warning with -Wno-error. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635 -- Pedro Alves
Re: Move -Wmaybe-uninitialized to -Wextra
Hi Jeff, On Sat, Dec 07 2019, Jeff Law wrote: > [...] > The whole point behind the uninitialized warning is to capture cases > where objects may not be properly initialized. For modern code the > simple cases typically "just work". What is by far the most > interesting cases are those with complex flow control, often > interacting with inline functions, address-of stripping, etc. These > are precisely the cases that humans aren't particularly good at > catching and having the compiler analyze those paths and issue warnings > that humans fix ultimately results in better quality code. Or "fixes" like: https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/expmed.c?r1=277864=277863=277864 if even we cannot deal with the false positive in our own compiler, how can we expect our users to do so? > > Experience has shown that if you put something in -Wall, people will > pay attention to it, and that is good for the long term quality of code > bases. If the diagnostic is outside of -Wall, it's largely ignored. I > think the pain of dealing with the Wuninitialized warts is smaller than > the pain of allowing these errors to persist. I'm afraid I that -Wmaybe-uninitialized is getting out of hand. I bet that some users regularly get these warnings coming from c++ header "libraries" (like they sometimes come out our vec.h which recently "broke" bootstrap) which they sometimes even cannot change and they then conclude that our -Wall is "unusable" and stop paying attention to all warnings. Martin
Re: Move -Wmaybe-uninitialized to -Wextra
Hi, On Tue, Nov 26 2019, Michael Witten wrote: > [...] > From what I've read, `-Wmaybe-uninitialized' is essentially > customized for `-O2', I don't think that is true. It can be perfectly useful -O1 and there are many nasty false positives at -O2 too. > [...] > * If `-Wmaybe-uninitialized' is enabled by the user, then it > implies `-O2' computations. > > That is to say, when the user specifies: > > -Og -Wmaybe-uninitialized > > or: > > -Og -Wextra > > then the user is explicitly telling the compiler to do all > the optimizations of `-O2', but ONLY for the purpose of > implementing `-Wmaybe-uninitialized' If you try to implement this proposal, you'll quickly find that there is no such thing as like "do all optimizations only for purposes of a warning." You either perform a transformation or not and subsequent passes, such as pass_late_warn_uninitialized, start work where the previous ones left off. Martin
Re: Move -Wmaybe-uninitialized to -Wextra
On Tue, 2019-11-26 at 19:33 +, Michael Witten wrote: > The problem with false positives is correlated with the degree of > optimization; a lot of people have reported problems at only the > `-Og' optimization level (even when the code in question is > embarrassingly correct). > > Therefore, the general solution is probably that the implem- > entation of `-Wmaybe-uninitialized' be customized for the given > level of optimization. > > However, I get the impression that this is easier said than done. > > From what I've read, `-Wmaybe-uninitialized' is essentially > customized for `-O2', which means that it will work pretty darn > well at that optimization level. So, in the interim, I propose a > simple approximation of the general solution: > > At an optimization level below `-O2' (or other than `-O2'): > > * `-Wmaybe-uninitialized' is moved to `-Wextra'. > > If `-Wall' has been specified, then gcc emits an > informational note stating that `-Wmaybe-uninitialized' (or > any other warning that has similar problems) has been > disabled. > > * Provide a command-line option to disable such a note. > > * The user may always enable `-Wmaybe-uninitialized' either > explicitly or as part of `-Wextra'. > > * If `-Wmaybe-uninitialized' is enabled by the user, then it > implies `-O2' computations. > > That is to say, when the user specifies: > > -Og -Wmaybe-uninitialized > > or: > > -Og -Wextra > > then the user is explicitly telling the compiler to do all > the optimizations of `-O2', but ONLY for the purpose of > implementing `-Wmaybe-uninitialized' (or whichever warning > requires those optimizations to function well); all those > optimizations are to be thrown out after they have been > used to good effect by `-Wmaybe-uninitialized'. The Code > generation, etc., shall be performed at the optimization > level the user specified (namely, `-Og' in this case). > > In other words, save the user from gcc's foibles, but let the > user pay for the extra computation if so desired! > > What do you think? I think this would be a terrible idea. Yes, it's absolutely true that changing options can get you different warning results. Yes it's absolutely true that there are false positives. But, no the warning is not customized to -O2. It's just that O2 enables more optimizations that are particularly good at weeding out false positives. The whole point behind the uninitialized warning is to capture cases where objects may not be properly initialized. For modern code the simple cases typically "just work". What is by far the most interesting cases are those with complex flow control, often interacting with inline functions, address-of stripping, etc. These are precisely the cases that humans aren't particularly good at catching and having the compiler analyze those paths and issue warnings that humans fix ultimately results in better quality code. Experience has shown that if you put something in -Wall, people will pay attention to it, and that is good for the long term quality of code bases. If the diagnostic is outside of -Wall, it's largely ignored. I think the pain of dealing with the Wuninitialized warts is smaller than the pain of allowing these errors to persist. jeff
Re: Move -Wmaybe-uninitialized to -Wextra
The problem with false positives is correlated with the degree of optimization; a lot of people have reported problems at only the `-Og' optimization level (even when the code in question is embarrassingly correct). Therefore, the general solution is probably that the implem- entation of `-Wmaybe-uninitialized' be customized for the given level of optimization. However, I get the impression that this is easier said than done. >From what I've read, `-Wmaybe-uninitialized' is essentially customized for `-O2', which means that it will work pretty darn well at that optimization level. So, in the interim, I propose a simple approximation of the general solution: At an optimization level below `-O2' (or other than `-O2'): * `-Wmaybe-uninitialized' is moved to `-Wextra'. If `-Wall' has been specified, then gcc emits an informational note stating that `-Wmaybe-uninitialized' (or any other warning that has similar problems) has been disabled. * Provide a command-line option to disable such a note. * The user may always enable `-Wmaybe-uninitialized' either explicitly or as part of `-Wextra'. * If `-Wmaybe-uninitialized' is enabled by the user, then it implies `-O2' computations. That is to say, when the user specifies: -Og -Wmaybe-uninitialized or: -Og -Wextra then the user is explicitly telling the compiler to do all the optimizations of `-O2', but ONLY for the purpose of implementing `-Wmaybe-uninitialized' (or whichever warning requires those optimizations to function well); all those optimizations are to be thrown out after they have been used to good effect by `-Wmaybe-uninitialized'. The Code generation, etc., shall be performed at the optimization level the user specified (namely, `-Og' in this case). In other words, save the user from gcc's foibles, but let the user pay for the extra computation if so desired! What do you think? Sincerely, Michael Witten PS All of this trouble indicates that C (and other languages) are just not expressive enough with regard to initialization. Initialization semantics are basically a matter of API contract specification; the programmer needs the tools to write it down. Surely, gcc could provide `__builtin_assume_initialized(x);' and parameter attributes to help inform the reader (i.e., to help inform the compiler) about the code; a function parameter attribute could specify whether the given argument can be considered initialized after a call, and maybe specify further constraints, such as whether the guarantee is made only when the function return value is nonzero (or a certain value), etc. We need the language to write our thoughts down!
Re: Move -Wmaybe-uninitialized to -Wextra
Hi, On Wed, 20 Feb 2019, Jeff Law wrote: > No, I'm saying the distinction between maybe and always uninitialized is > a false distinction. Code duplication can easily take something that > triggers a "maybe" warning and turn it into a "always" warning. The > distinction between them is just bogus. > > To take your example > > if (a) > b = 42 > ... > if (a) > f(b); > > Can be rewritten as > > if (a) { > b = 42 > [ copy of ...] > f (b); > } else { > ... > f (b); // xxx > } No, it cannot. It would introduce a call to f(b) in a path (at xxx) where it wasn't before. Any transformation that transforms maybe-uninit into must-uninit is either wrong or introduces more-obviously-dead-than-before code, like in this variant of yours that is correct: if (a) { b = 42 [ copy of ...] f (b); } else { ... if (a) // clearly false, as dominated by if (!a) f (b); // hence more obviously dead than in original code } > x4 = PHI (x0(D), x1, x2, x3) > use x4; > > That's a maybe uninitialized warning by way of the value flow of x0 > through the PHI into x4. So clearly the use of x4 when reached via edge using x0(D) must be dead (or we have a real uninit use) but we can't differ because there's only one use of x4 statically ... nevertheless it's unobviously-dead. > Which we then simplify into > > BB: > use x0(D) > goto join > > BB: > x4 = PHI (x1, x2, x3) > use x4 > goto join And this makes the uninit use of x0(D) more obviously dead because now the problematic and non-problematic uses are separated. So, the transformation introduced more-obviously-dead code, which it just as well could have removed/not generated, getting rid of the must-uninit warning. I guess what I'm saying is that whenever a transformation is about to introduce a must-uninit from a maybe-uninit that instead all paths leading to that must-uninit should be removed instead. > That changes the warning from a maybe-uninitialized into an always > uninitialized. Note that we still may or may not reach the use at > runtime. THe distinction between maybe and always is just bogus. I think as stated that's a bit extreme :) On user code before optimization the distinction is useful. The problem stems from us generating warnings somewhen in the middle of the optimization pipeline. As long as we continue doing that we will always have these different opinions about what is or isn't rightfully a maybe-uninit or not :-/ Ciao, Michael.
Re: Move -Wmaybe-uninitialized to -Wextra
On 2/20/19, Jeff Law 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
Re: Move -Wmaybe-uninitialized to -Wextra
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
Re: Move -Wmaybe-uninitialized to -Wextra
On 2/2/19 1:20 PM, Segher Boessenkool wrote: > On Fri, Feb 01, 2019 at 12:27:57PM -0700, Jeff Law wrote: >> On 2/1/19 7:01 AM, Marek Polacek wrote: >>> On Fri, Feb 01, 2019 at 07:19:25AM -0600, Segher Boessenkool wrote: On Fri, Feb 01, 2019 at 12:32:45PM +0100, Marc Glisse wrote: > My opinion is that -Wmaybe-uninitialized would serve its purpose better > as > part of -Wextra. +1 >>> >>> +1 from me too. >> I disagree strongly. If we move it to Wextra it's going to see a lot >> less usage in real world codebases and potentially lead to the >> re-introduction of a class of bugs that we've largely helped stomp out. > > The usual workaround, especially for programs that build with multiple > (i.e. older) versions of GCC, is to initialise any such variable (to an > either or not useful value) early. This doesn't fix the actual problem > usually (which is that your control flow is too complex). > >> 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. > > Are you saying that -Wmaybe-uninitialized should be included in > -Wuninitialized, since it has no extra false positives? That wasn't true > at all historically: -Wuninitialized has false positives on paths that > cannot be executed because of function preconditions, but > -Wmaybe-uninitialized used to warn for things that can be locally proven to > never execute, like > if (a) > b = 42; > ... > if (a) > f(b); No, I'm saying the distinction between maybe and always uninitialized is a false distinction. Code duplication can easily take something that triggers a "maybe" warning and turn it into a "always" warning. The distinction between them is just bogus. To take your example if (a) b = 42 ... if (a) f(b); Can be rewritten as if (a) { b = 42 [ copy of ...] f (b); } else { ... f (b); } But more importantly consider when the value flows by way of a PHI argument. We consider those "maybe" warnings (and it's not just the uninitialized warning analysis). But fairly simple code duplication can eliminate the flow via PHI and suddenly we go from a "maybe" to an "always" warning. Let's consider this... x4 = PHI (x0(D), x1, x2, x3) use x4; That's a maybe uninitialized warning by way of the value flow of x0 through the PHI into x4. But we can duplicate the block creating something like this: BB: x5 = PHI (x0(D)) use x5 goto join BB: x4 = PHI (x1, x2, x3) use x4 goto join Which we then simplify into BB: use x0(D) goto join BB: x4 = PHI (x1, x2, x3) use x4 goto join That changes the warning from a maybe-uninitialized into an always uninitialized. Note that we still may or may not reach the use at runtime. THe distinction between maybe and always is just bogus. Jeff
Re: Move -Wmaybe-uninitialized to -Wextra
On 2/3/19 3:02 AM, Marc Glisse wrote: > On Sat, 2 Feb 2019, Martin Sebor wrote: > >> 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.] > > Thank you for taking the time to write this down. > >> 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? > > I've been wanting to post this for years, but I only got motivated > enough recently after seeing negative effects of -Wmaybe-uninitialized > in two projects within a few days. I don't think the rate of false > positives has gone up significantly in gcc-9, they just randomly appear > or disappear from one release to the next. In some sense, for projects > that support multiple compiler versions, each new gcc release makes the > number of false positives (on at least one version) go up, but that > doesn't count as the warning getting worse. > > The discussion and/or the change don't need to happen now, but if I > didn't post this patch while motivated, it might be years before I > actually do it, so I ignored stage 4. I'll also note that I proposed changes to how we handled Wuninitialized many years ago which were designed to address the problem of instability in these warnings from release to release. We would have the ability to issue warnings from early in the pipeline -- before the optimizers really muck things up. This gives you much more stable warnings from release to release (at the expense of many more false positives). We'd also be able to use the results of that very early analysis to indicate to the user when the optimizers were able to ultimately prove the early uninitialized use was optimized away (or the path which lead to the flow of an uninitialized value was optimized away). I ultimately gave up, not because of technical issues, but because of the inability to get any kind of consensus on changing things in this space. It was an amazingly frustrating experience, but did certainly highlight that there is no one answer in this space. Everyone wants different defaults. > > I am mostly looking at what I see in practice. I regularly see false > positives of -Wstrict-overflow (less now that I neutered parts of it) > and -Wmaybe-uninitialized. And when I have strange crashes that later > turn out to be caused by uninitialized memory uses, it is very seldom > that -Wmaybe-uninitialized helps detect them. IIRC, last time, I had > slightly more success with -Wreturn-local-addr -O3 > -fkeep-inline-functions -fkeep-static-functions. Which is consistent with the belief that we simply don't do a good job at uninitialized warnings for addressables and aggregates. > >> "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. It's certainly undesirable and we go to nearly heroic lengths to avoid them. I'm really looking forward to the new range infrastructure as it can be exploited in the backwards threader to address a significant class of these false positives. I'd also really like to see the predicate analysis we currently use to prune uninit warnings be split out as a separate analysis. THe ability to prove a path is infeasible, even if we have to leave it in the CFG has uses elsewhere. I'd also like a way to mark infeasible paths found by the threader, but which the threader decides are not worth optimizing (usually because of the amount of code it would need to duplicate). Marking them (and somehow keeping the data accurate) would also eliminate classes of false positives from the uninitialized warnings. Finally, we also still have cases where iteration of threading + dom is needed to remove infeasible paths and avoid false positives. I'm not going to suggest we return to the that world, but we do need to continue to explore ways to thread deeper in a single iteration. >\ > 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
Re: Move -Wmaybe-uninitialized to -Wextra
On 2/20/19 8:11 AM, Martin Sebor wrote: > On 2/20/19 7:25 AM, Jeff Law wrote: >> On 2/20/19 5:36 AM, Pedro Alves wrote: >>> On 02/05/2019 05:07 PM, Jeff Law wrote: FWIW, I've been doing Fedora rawhide builds with gcc-9 since early Jan. Not everything builds with -Wall -Werror, but lots of packages do. I've only seen one maybe-uninit warning cause problems -- it was spurious and for a memory object. I didn't dig into it at all. >>> >>> Do you have insight into how many Fedora packages explicitly disable >>> maybe-uninit? I really don't know the answer to that, but I'd at least >>> keep that in mind as a possible reason for the warning seemingly >>> not causing trouble often. >> Unfortunately not. And my tester only saves failed log files, so I >> can't just grep the log files for it. > > Would it difficult to add a pass over the build logs to scan for > these sorts of things? E.g., count the number of instances of > each warning in builds that don't use -Werror? It's essentially the same as saving the successful logs :-) Everything is built with fedpkg which buries the log files. For a failed build the scripts dig them out of the filesystem and dump them to stdout. Doing the same for a successful build wouldn't be hard. Once that's in place we can scan for warnings or whatever else, even in a successful build. I'll try to get that added for the next mass test which would start Sunday after the next gcc-9 snapshot. jeff
Re: Move -Wmaybe-uninitialized to -Wextra
On 2/20/19 7:25 AM, Jeff Law wrote: On 2/20/19 5:36 AM, Pedro Alves wrote: On 02/05/2019 05:07 PM, Jeff Law wrote: FWIW, I've been doing Fedora rawhide builds with gcc-9 since early Jan. Not everything builds with -Wall -Werror, but lots of packages do. I've only seen one maybe-uninit warning cause problems -- it was spurious and for a memory object. I didn't dig into it at all. Do you have insight into how many Fedora packages explicitly disable maybe-uninit? I really don't know the answer to that, but I'd at least keep that in mind as a possible reason for the warning seemingly not causing trouble often. Unfortunately not. And my tester only saves failed log files, so I can't just grep the log files for it. Would it difficult to add a pass over the build logs to scan for these sorts of things? E.g., count the number of instances of each warning in builds that don't use -Werror? Martin GDB disables it (or rather, removes it from -Werror), and I wouldn't be surprised if other C++ programs do the same, when they start using std::optional more. Since boost::optional also triggers the same warnings, set may be even larger than C++11-or-later programs. Well, one of the proposals is to distinguish between scalars and memory/addressables. That should (in theory) make it possible to deal with this more sanely. jeff
Re: Move -Wmaybe-uninitialized to -Wextra
On 2/20/19 5:36 AM, Pedro Alves wrote: > On 02/05/2019 05:07 PM, Jeff Law wrote: >> FWIW, I've been doing Fedora rawhide builds with gcc-9 since early Jan. >> Not everything builds with -Wall -Werror, but lots of packages do. >> I've only seen one maybe-uninit warning cause problems -- it was >> spurious and for a memory object. I didn't dig into it at all. > > Do you have insight into how many Fedora packages explicitly disable > maybe-uninit? I really don't know the answer to that, but I'd at least > keep that in mind as a possible reason for the warning seemingly > not causing trouble often. Unfortunately not. And my tester only saves failed log files, so I can't just grep the log files for it. > > GDB disables it (or rather, removes it from -Werror), and I wouldn't be > surprised if other C++ programs do the same, when they start using > std::optional more. Since boost::optional also triggers the same warnings, > set may be even larger than C++11-or-later programs. Well, one of the proposals is to distinguish between scalars and memory/addressables. That should (in theory) make it possible to deal with this more sanely. jeff
Re: Move -Wmaybe-uninitialized to -Wextra
On 02/05/2019 05:07 PM, Jeff Law wrote: > FWIW, I've been doing Fedora rawhide builds with gcc-9 since early Jan. > Not everything builds with -Wall -Werror, but lots of packages do. > I've only seen one maybe-uninit warning cause problems -- it was > spurious and for a memory object. I didn't dig into it at all. Do you have insight into how many Fedora packages explicitly disable maybe-uninit? I really don't know the answer to that, but I'd at least keep that in mind as a possible reason for the warning seemingly not causing trouble often. GDB disables it (or rather, removes it from -Werror), and I wouldn't be surprised if other C++ programs do the same, when they start using std::optional more. Since boost::optional also triggers the same warnings, set may be even larger than C++11-or-later programs. I don't know how to search the Fedora codebase, but a github search for "-Wno-maybe-uninitialized" shows > 100k hits: https://github.com/search?q=-Wno-maybe-uninitialized=Code and a search for -Wno-error=maybe-uninitialized shows >10k: https://github.com/search?q=-Wno-error%3Dmaybe-uninitialized=Code That's a much higher hit rate than say -Wno-array-bounds or -Wno-format-overflow, or -Wno-stringop-overflow, which were some searches I tried. > > In contrast things like the missing NUL termination warnings, buffer > overflow warnings, NULL strings to *printf* warnings, etc all caught > numerous (dozens) of real bugs. I mention it because it's one of the > ways I know packages are building with -Wall -Werror :-) Thanks, Pedro Alves
Re: Move -Wmaybe-uninitialized to -Wextra
On 2/4/19 3:52 PM, Martin Jambor wrote: > Hi, > > On Mon, Feb 04 2019, Marc Glisse wrote: >> On Mon, 4 Feb 2019, Martin Sebor wrote: >>> > > ... > >>> 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.) >> >> Looking for "optional" and "-Wmaybe-uninitialized" shows >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78044 >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635 >> >> Google also gives >> https://www.boost.org/doc/libs/1_69_0/libs/optional/doc/html/boost_optional/tutorial/gotchas/false_positive_with__wmaybe_uninitialized.html >> https://sourceware.org/ml/gdb-patches/2017-05/msg00130.html >> etc >> >> And that's just for using a type called 'optional' (3 implementations of >> it). > > from my very quick reading of the first googled testcase, I assume the > instance of the optional class got SRAed and a warning was generated for > what originally was a class member, which indeed is not easy to > initialize on its own in order to avoid the warning. > > Would it perhaps make sense to split the -Wmaybe-uninitialized warning > into two, one for scalars that are scalars in the original code and one > for SRA-created scalars and move only the latter to -Wextra? I could support that. It fits in with the general sense that we're not handling aggregates and addressables as well as we could. JEff
Re: Move -Wmaybe-uninitialized to -Wextra
On 2/14/19 7:23 AM, Tom Tromey wrote: >> "Marc" == Marc Glisse writes: > >>> Lastly, in the case of uninitialized variables, the usual solution >>> of initializing them is trivial and always safe (some coding styles >>> even require it). > > Marc> Here it shows that we don't work with the same type of code at all. If > Marc> I am using a boost::optional, i.e. a class with a buffer and a boolean > Marc> that says if the buffer is initialized, how do I initialize the > Marc> (private) buffer? Or should boost itself zero out the buffer whenever > Marc> the boolean is set to false? > > This is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635 (I know you > know, but maybe others on the thread don't). > > I think in this specific case (std::optional and similar classes), GCC > should provide a way for the class to indicate that > -Wmaybe-uninitialized should not apply to the payload. > >>> 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. > > Marc> This data is going to be super hard to get. Most projects have been > Marc> compiling for years and tweaking their code to avoid some warnings. We > Marc> do not get to see the code that people originally write, we can only > Marc> see what they commit. > > gdb has gone through this over the years -- it turns on many warnings > and sometimes false positives show up. Most of the time there's a > comment, for -Wmaybe-uninitialized grep for "init.*gcc" in the source. > Unfortunately the comment isn't standardized; but I only get ~20 hits > for this in gdb, so it isn't really so bad in practice. Yea, in retrospect we should have had a consistent marker for GCC as well. I suspect a goodly number of those initializations that went in early in the process are no longer needed. Jeff
Re: Move -Wmaybe-uninitialized to -Wextra
> "Marc" == Marc Glisse writes: >> Lastly, in the case of uninitialized variables, the usual solution >> of initializing them is trivial and always safe (some coding styles >> even require it). Marc> Here it shows that we don't work with the same type of code at all. If Marc> I am using a boost::optional, i.e. a class with a buffer and a boolean Marc> that says if the buffer is initialized, how do I initialize the Marc> (private) buffer? Or should boost itself zero out the buffer whenever Marc> the boolean is set to false? This is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635 (I know you know, but maybe others on the thread don't). I think in this specific case (std::optional and similar classes), GCC should provide a way for the class to indicate that -Wmaybe-uninitialized should not apply to the payload. >> 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. Marc> This data is going to be super hard to get. Most projects have been Marc> compiling for years and tweaking their code to avoid some warnings. We Marc> do not get to see the code that people originally write, we can only Marc> see what they commit. gdb has gone through this over the years -- it turns on many warnings and sometimes false positives show up. Most of the time there's a comment, for -Wmaybe-uninitialized grep for "init.*gcc" in the source. Unfortunately the comment isn't standardized; but I only get ~20 hits for this in gdb, so it isn't really so bad in practice. Tom
Re: Move -Wmaybe-uninitialized to -Wextra
On 2/4/19 11:24 PM, Marc Glisse wrote: > On Mon, 4 Feb 2019, Martin Jambor wrote: > >>> Looking for "optional" and "-Wmaybe-uninitialized" shows >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78044 >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635 >>> >>> Google also gives >>> https://www.boost.org/doc/libs/1_69_0/libs/optional/doc/html/boost_optional/tutorial/gotchas/false_positive_with__wmaybe_uninitialized.html >>> >>> https://sourceware.org/ml/gdb-patches/2017-05/msg00130.html >>> etc >>> >>> And that's just for using a type called 'optional' (3 implementations of >>> it). >> >> from my very quick reading of the first googled testcase, I assume the >> instance of the optional class got SRAed and a warning was generated for >> what originally was a class member, which indeed is not easy to >> initialize on its own in order to avoid the warning. > > Hmm, SRA and -Wmaybe-uninitialized, I saw that recently in > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66459 > >> Would it perhaps make sense to split the -Wmaybe-uninitialized warning >> into two, one for scalars that are scalars in the original code and one >> for SRA-created scalars and move only the latter to -Wextra? > > If SRA is really the main source of false positives (I don't know about > that), maybe. But I am afraid we will also miss a significant proportion > of the already rare true positives that -Wmaybe-uninitialized currently > finds. I really have no idea if such a split would work great or badly > (sorry, I am not being very useful). I've had some discussions with Kees and others in this space. The general consensus is that for pure scalars that everyone does a pretty good job at generating good maybe-uninitialized warnings. LLVM and GCC take different approaches, both are valid and both have been useful at giving developers actionable warnings. However, the general consensus is that for aggregates or anything living in memory is that most compilers aren't doing a particularly good job across the board in this space. Not enough objects are being thoroughly analyzed and those that are analyzed give too many false positives, etc. On the Microsoft side, they've chosen to focus on improving DSE and just initializing everything in memory (presumably flag controlled). It's not clear where LLVM is going to go in this space. ISTM that most of the analysis to do a good job at DSE-ing away the redundant initializer code should play directly into doing a good job at maybe-uninit warnings for objects in memory. However, I don't really have the time to explore here. I would generally support some experimentation in the overall space, that might include allowing for different default settings for uninit warnings of pure scalars vs aggregates/addressables. FWIW, I've been doing Fedora rawhide builds with gcc-9 since early Jan. Not everything builds with -Wall -Werror, but lots of packages do. I've only seen one maybe-uninit warning cause problems -- it was spurious and for a memory object. I didn't dig into it at all. In contrast things like the missing NUL termination warnings, buffer overflow warnings, NULL strings to *printf* warnings, etc all caught numerous (dozens) of real bugs. I mention it because it's one of the ways I know packages are building with -Wall -Werror :-) Jeff
Re: Move -Wmaybe-uninitialized to -Wextra
On Mon, 4 Feb 2019, Martin Jambor wrote: Looking for "optional" and "-Wmaybe-uninitialized" shows https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78044 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635 Google also gives https://www.boost.org/doc/libs/1_69_0/libs/optional/doc/html/boost_optional/tutorial/gotchas/false_positive_with__wmaybe_uninitialized.html https://sourceware.org/ml/gdb-patches/2017-05/msg00130.html etc And that's just for using a type called 'optional' (3 implementations of it). from my very quick reading of the first googled testcase, I assume the instance of the optional class got SRAed and a warning was generated for what originally was a class member, which indeed is not easy to initialize on its own in order to avoid the warning. Hmm, SRA and -Wmaybe-uninitialized, I saw that recently in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66459 Would it perhaps make sense to split the -Wmaybe-uninitialized warning into two, one for scalars that are scalars in the original code and one for SRA-created scalars and move only the latter to -Wextra? If SRA is really the main source of false positives (I don't know about that), maybe. But I am afraid we will also miss a significant proportion of the already rare true positives that -Wmaybe-uninitialized currently finds. I really have no idea if such a split would work great or badly (sorry, I am not being very useful). -- Marc Glisse
Re: Move -Wmaybe-uninitialized to -Wextra
On 2/4/19, Martin Jambor wrote: > Hi, > > On Mon, Feb 04 2019, Marc Glisse wrote: >> On Mon, 4 Feb 2019, Martin Sebor wrote: >>> > > ... > >>> 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.) >> >> Looking for "optional" and "-Wmaybe-uninitialized" shows >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78044 >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635 >> >> Google also gives >> https://www.boost.org/doc/libs/1_69_0/libs/optional/doc/html/boost_optional/tutorial/gotchas/false_positive_with__wmaybe_uninitialized.html >> https://sourceware.org/ml/gdb-patches/2017-05/msg00130.html >> etc >> >> And that's just for using a type called 'optional' (3 implementations of >> it). > > from my very quick reading of the first googled testcase, I assume the > instance of the optional class got SRAed and a warning was generated for > what originally was a class member, which indeed is not easy to > initialize on its own in order to avoid the warning. > > Would it perhaps make sense to split the -Wmaybe-uninitialized warning > into two, one for scalars that are scalars in the original code and one > for SRA-created scalars and move only the latter to -Wextra? What would the new warning names be once split? > > Martin >
Re: Move -Wmaybe-uninitialized to -Wextra
On 2/4/19, Martin Sebor 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
Re: Move -Wmaybe-uninitialized to -Wextra
Hi, On Mon, Feb 04 2019, Marc Glisse wrote: > On Mon, 4 Feb 2019, Martin Sebor wrote: >> ... >> 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.) > > Looking for "optional" and "-Wmaybe-uninitialized" shows > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78044 > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635 > > Google also gives > https://www.boost.org/doc/libs/1_69_0/libs/optional/doc/html/boost_optional/tutorial/gotchas/false_positive_with__wmaybe_uninitialized.html > https://sourceware.org/ml/gdb-patches/2017-05/msg00130.html > etc > > And that's just for using a type called 'optional' (3 implementations of > it). from my very quick reading of the first googled testcase, I assume the instance of the optional class got SRAed and a warning was generated for what originally was a class member, which indeed is not easy to initialize on its own in order to avoid the warning. Would it perhaps make sense to split the -Wmaybe-uninitialized warning into two, one for scalars that are scalars in the original code and one for SRA-created scalars and move only the latter to -Wextra? Martin
Re: Move -Wmaybe-uninitialized to -Wextra
On Mon, 4 Feb 2019, Martin Sebor 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. And I'd rather suggest as default something that can be combined with -Werror (not that you should do it, just that you could), leaving the non-trivial warnings that require motivation to investigate properly to people who have at least the motivation to enable extra flags. We can only agree to disagree... 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.) Looking for "optional" and "-Wmaybe-uninitialized" shows https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78044 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635 Google also gives https://www.boost.org/doc/libs/1_69_0/libs/optional/doc/html/boost_optional/tutorial/gotchas/false_positive_with__wmaybe_uninitialized.html https://sourceware.org/ml/gdb-patches/2017-05/msg00130.html etc And that's just for using a type called 'optional' (3 implementations of it). 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). Well, warnings can't be completely arbitrary, they have to serve a purpose. Preventing some control flows where the use is not dominated by the initialization clearly enough could be one, but in my experience, on average, this does not lead to better programs for any metric besides the number of warnings. Again, experience can vary. 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. Yes. (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.) It seems to depend on the type of code base, unsurprisingly. 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. True. The main point is that there is a feeling that "gcc recommends using -Wall and fixing **all** those warnings **easily**" that does not exist (to the same level) with -Wextra. I guess that's not such a strong argument (you can probably feel that I've already given up). Coincidentally, yesterday, I had to debug a crash in one of my programs. No warning, whatever I tried. At first, -fsanitize=address didn't give anything either. Then I found out about ASAN_OPTIONS=detect_stack_use_after_return=1, which pointed out super precisely which local variable I was accessing from where, which was
Re: Move -Wmaybe-uninitialized to -Wextra
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. 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
Re: Move -Wmaybe-uninitialized to -Wextra
On Fri, Feb 1, 2019 at 8:28 PM Jeff Law wrote: > > On 2/1/19 7:01 AM, Marek Polacek wrote: > > On Fri, Feb 01, 2019 at 07:19:25AM -0600, Segher Boessenkool wrote: > >> Hi Marc, > >> > >> On Fri, Feb 01, 2019 at 12:32:45PM +0100, Marc Glisse wrote: > >>> -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. > >> > >> That is true for *every* warning; if not, it should be an error, not a > >> warning. > >> > >>> My opinion is that -Wmaybe-uninitialized would serve its purpose better as > >>> part of -Wextra. > >> > >> +1 > > > > +1 from me too. > I disagree strongly. If we move it to Wextra it's going to see a lot > less usage in real world codebases and potentially lead to the > re-introduction of a class of bugs that we've largely helped stomp out. > > 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. Agreed. Also note that we consider everything executed when function entry is executed as not "maybe" even though functions can be called conditionally. So there's the artificial function boundary issue when distinguishing maybe from must-be stuff as well. Richard. > > > > >>> 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...). > >> > >> Yes, using -Werror is usually a terrible idea. > Generally agreed in released versions of any code. -Werror *may* be > appropriate in development versions depending on the project's policies, > procedures and quality of codebase. > > > Jeff
Re: Move -Wmaybe-uninitialized to -Wextra
On Sat, Feb 2, 2019 at 9:20 PM Segher Boessenkool wrote: > > On Fri, Feb 01, 2019 at 12:27:57PM -0700, Jeff Law wrote: > > On 2/1/19 7:01 AM, Marek Polacek wrote: > > > On Fri, Feb 01, 2019 at 07:19:25AM -0600, Segher Boessenkool wrote: > > >> On Fri, Feb 01, 2019 at 12:32:45PM +0100, Marc Glisse wrote: > > >>> My opinion is that -Wmaybe-uninitialized would serve its purpose better > > >>> as > > >>> part of -Wextra. > > >> > > >> +1 > > > > > > +1 from me too. > > I disagree strongly. If we move it to Wextra it's going to see a lot > > less usage in real world codebases and potentially lead to the > > re-introduction of a class of bugs that we've largely helped stomp out. > > The usual workaround, especially for programs that build with multiple > (i.e. older) versions of GCC, is to initialise any such variable (to an > either or not useful value) early. This doesn't fix the actual problem > usually (which is that your control flow is too complex). > > > 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. > > Are you saying that -Wmaybe-uninitialized should be included in > -Wuninitialized, since it has no extra false positives? That wasn't true > at all historically: -Wuninitialized has false positives on paths that > cannot be executed because of function preconditions, but > -Wmaybe-uninitialized used to warn for things that can be locally proven to > never execute, like > if (a) > b = 42; > ... > if (a) > f(b); IMHO the main reason of most false positives is that we rely on SSA default-defs in PHIs which are spurious when definition and use are not in domination relation as in if (p) a = 1; if (p) ... = a; the uninit pass tries to catch those cases but that's obviously hard. > > >> Yes, using -Werror is usually a terrible idea. > > Generally agreed in released versions of any code. -Werror *may* be > > appropriate in development versions depending on the project's policies, > > procedures and quality of codebase. > > IMO it is more useful it is much more useful if you make your build system > less noisy so that problems are more obvious, instead of breaking the build > for no reason all the time (see PR89162, etc. etc.) > > > Segher
Re: Move -Wmaybe-uninitialized to -Wextra
On Sat, 2 Feb 2019, Martin Sebor wrote: 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.] Thank you for taking the time to write this down. 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? I've been wanting to post this for years, but I only got motivated enough recently after seeing negative effects of -Wmaybe-uninitialized in two projects within a few days. I don't think the rate of false positives has gone up significantly in gcc-9, they just randomly appear or disappear from one release to the next. In some sense, for projects that support multiple compiler versions, each new gcc release makes the number of false positives (on at least one version) go up, but that doesn't count as the warning getting worse. The discussion and/or the change don't need to happen now, but if I didn't post this patch while motivated, it might be years before I actually do it, so I ignored stage 4. 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. (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.) Indeed the definition of false positive is important. In some sense, no warning is ever a false positive, when its definition is "the compiler decided to warn". But that makes the warning useless. -Wmaybe-uninitialized does not say there is an uninitialized use, it says the control flow is too complicated or the compiler not clever enough (or there is indeed an uninitialized use for some input, and only this subcase is a bug). Another warning, -Wstrict-overflow (after it moved to the middle-end), was more of an optimization note than a true warning: "warning: assuming your code is valid when generating code", Duh! Was I supposed to make it invalid to quiet the warning? 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. 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? I am mostly looking at what I see in practice. I regularly see false positives of -Wstrict-overflow (less now that I neutered parts of it) and -Wmaybe-uninitialized. And when I have strange crashes that later turn out to be caused by uninitialized memory uses, it is very seldom that -Wmaybe-uninitialized helps detect them. IIRC, last time, I had slightly more success with -Wreturn-local-addr -O3 -fkeep-inline-functions -fkeep-static-functions. I think several of the new middle-end warnings you added are about string manipulation. I don't do any of that, which may explain why I am not seeing them. Also, we are not using gcc-9 much yet. 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
Re: Move -Wmaybe-uninitialized to -Wextra
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
Re: Move -Wmaybe-uninitialized to -Wextra
On Fri, Feb 01, 2019 at 12:27:57PM -0700, Jeff Law wrote: > On 2/1/19 7:01 AM, Marek Polacek wrote: > > On Fri, Feb 01, 2019 at 07:19:25AM -0600, Segher Boessenkool wrote: > >> On Fri, Feb 01, 2019 at 12:32:45PM +0100, Marc Glisse wrote: > >>> My opinion is that -Wmaybe-uninitialized would serve its purpose better > >>> as > >>> part of -Wextra. > >> > >> +1 > > > > +1 from me too. > I disagree strongly. If we move it to Wextra it's going to see a lot > less usage in real world codebases and potentially lead to the > re-introduction of a class of bugs that we've largely helped stomp out. The usual workaround, especially for programs that build with multiple (i.e. older) versions of GCC, is to initialise any such variable (to an either or not useful value) early. This doesn't fix the actual problem usually (which is that your control flow is too complex). > 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. Are you saying that -Wmaybe-uninitialized should be included in -Wuninitialized, since it has no extra false positives? That wasn't true at all historically: -Wuninitialized has false positives on paths that cannot be executed because of function preconditions, but -Wmaybe-uninitialized used to warn for things that can be locally proven to never execute, like if (a) b = 42; ... if (a) f(b); > >> Yes, using -Werror is usually a terrible idea. > Generally agreed in released versions of any code. -Werror *may* be > appropriate in development versions depending on the project's policies, > procedures and quality of codebase. IMO it is more useful it is much more useful if you make your build system less noisy so that problems are more obvious, instead of breaking the build for no reason all the time (see PR89162, etc. etc.) Segher
Re: Move -Wmaybe-uninitialized to -Wextra
On Fri, 1 Feb 2019, Jeff Law wrote: On 2/1/19 7:01 AM, Marek Polacek wrote: On Fri, Feb 01, 2019 at 07:19:25AM -0600, Segher Boessenkool wrote: Hi Marc, On Fri, Feb 01, 2019 at 12:32:45PM +0100, Marc Glisse wrote: -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. That is true for *every* warning; if not, it should be an error, not a warning. My opinion is that -Wmaybe-uninitialized would serve its purpose better as part of -Wextra. +1 +1 from me too. I disagree strongly. 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? 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) 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... 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. 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. There is a matter of statistics. In practice maybe-uninitialized has way more false positives than uninitialized, which makes it more problematic. But if you prefer to move both to -Wextra (this is the current default when front-ends don't override it), that's ok with me ;-) -- Marc Glisse
Re: Move -Wmaybe-uninitialized to -Wextra
On 2/1/19 7:01 AM, Marek Polacek wrote: > On Fri, Feb 01, 2019 at 07:19:25AM -0600, Segher Boessenkool wrote: >> Hi Marc, >> >> On Fri, Feb 01, 2019 at 12:32:45PM +0100, Marc Glisse wrote: >>> -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. >> >> That is true for *every* warning; if not, it should be an error, not a >> warning. >> >>> My opinion is that -Wmaybe-uninitialized would serve its purpose better as >>> part of -Wextra. >> >> +1 > > +1 from me too. I disagree strongly. If we move it to Wextra it's going to see a lot less usage in real world codebases and potentially lead to the re-introduction of a class of bugs that we've largely helped stomp out. 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. > >>> 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...). >> >> Yes, using -Werror is usually a terrible idea. Generally agreed in released versions of any code. -Werror *may* be appropriate in development versions depending on the project's policies, procedures and quality of codebase. Jeff
Re: Move -Wmaybe-uninitialized to -Wextra
On Fri, Feb 01, 2019 at 09:01:53AM -0500, Marek Polacek wrote: > On Fri, Feb 01, 2019 at 07:19:25AM -0600, Segher Boessenkool wrote: > > > 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. > > > > If it is not part of -Wall and not of -W, and not special purpose, then it > > might as well not exist. > > There are warnings that *do* make sense, but have issues e.g. with macro > expansion, so will be outside -Wall/-Wextra unless that's fixed. E.g. > -Wlogical-op, -Wduplicated-conds, or a warning I posted to some PR > called -Wsame-arguments I think, etc. Yes, we agree on that. I'm just saying such general-purpose warnings are not used much until they are part of -Wall or -W. Segher
Re: Move -Wmaybe-uninitialized to -Wextra
On Fri, Feb 01, 2019 at 07:19:25AM -0600, Segher Boessenkool wrote: > Hi Marc, > > On Fri, Feb 01, 2019 at 12:32:45PM +0100, Marc Glisse wrote: > > -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. > > That is true for *every* warning; if not, it should be an error, not a > warning. > > > My opinion is that -Wmaybe-uninitialized would serve its purpose better as > > part of -Wextra. > > +1 +1 from me too. > > 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...). > > Yes, using -Werror is usually a terrible idea. > > > 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. > > If it is not part of -Wall and not of -W, and not special purpose, then it > might as well not exist. There are warnings that *do* make sense, but have issues e.g. with macro expansion, so will be outside -Wall/-Wextra unless that's fixed. E.g. -Wlogical-op, -Wduplicated-conds, or a warning I posted to some PR called -Wsame-arguments I think, etc. Marek
Re: Move -Wmaybe-uninitialized to -Wextra
Hi Marc, On Fri, Feb 01, 2019 at 12:32:45PM +0100, Marc Glisse wrote: > -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. That is true for *every* warning; if not, it should be an error, not a warning. > My opinion is that -Wmaybe-uninitialized would serve its purpose better as > part of -Wextra. +1 > 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...). Yes, using -Werror is usually a terrible idea. > 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. If it is not part of -Wall and not of -W, and not special purpose, then it might as well not exist. Segher
Move -Wmaybe-uninitialized to -Wextra
Hello, first, I expect this to be controversial, so feel free to complain. 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. 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). 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. 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...). 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. --- 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 * 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 * c.opt (Wmaybe-uninitialized): Enable with Wextra. gcc/testsuite/ChangeLog: 2019-02-01 Marc Glisse * 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. *