Move -Wmaybe-uninitialized to -Wextra

[Jeff Law]

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 easy to avoid belongs in a different class, say -Wextra. We can
> rename that to W1/W2/... if Wall is too charged with historical meaning.
> 
> One issue is that, especially in a large, long-lived code base, people
> want to know when a change (or a new compiler) introduces a new warning.
> However, they do not want to have to go through hundreds or
> pre-existing, already analysed warnings. The easiest way to do that is
> to make sure there are no warnings at all (-Werror). For speculative
> warnings, that's really not convenient. I don't think there is a good
> solution to this (marking with a pragma the known false positives may be
> hard when instantiating some class somewhere causes a warning in some
> almost unrelated place, and you want the marking to be narrow enough
> that it won't also disable other warnings), and it is independent on
> whether the warning is in -Wall or -Wextra. I just have a feeling that
> splitting the easy and the not-easy warnings may help developers handle
> them differently.
I understand this sentiment.  Believe me I do.  I've worked with folks
that have to deal with legacy codebases and warning stability from
release to release is very important to them.  We don't have a real good
solution for them (this class of user was the motivation behind the
-Wuninitialized changes I proposed many years ago).  The best right now
is to identify a set of warnings that are important to those
organizations and use those and only those with -Werror.

Jeff

ps.  Yes, I know I did a lot of snipping of the discussion.  I didn't
feel I had much to add to the other parts.  I hope that in doing so I
haven't taken anything out of context or conflated any of the issues.