[cfe-dev] new -Wuninitialized implementation in Clang

[Chris Lattner]

On Feb 4, 2011, at 8:56 AM, Ted Kremenek wrote:

> On Feb 4, 2011, at 8:12 AM, Nico Weber wrote:
> 
>> Suggestions:
>> * I found your above explanation of how the analysis works very
>> helpful. Maybe there could be a page with this explanation, it could
>> also list common false positives and recommended work-arounds.
> 
> Yes, documenting the expected behavior makes a lot of sense.

Yes, there should be a cogent discussion of this in compatibility.html, because it will keep coming up.

>> * If it warns, provide a way to explain the warning ("Variable could
>> be uninitialized on this path: declared here, else branch here, then
>> goto here, use here")
> I'm mixed on this one.  This could possibly be done with notes, but I somewhat feel that this should remain the purview of the static analyzer.  The whole point of this warning is to be simple and to catch obvious uses of uninitialized values.

I completely agree that the static analyzer is the true solution for handling interprocedural issues, aliasing, and other "hard" cases.  However, it could still be useful to show (not by default) the "path" on which the uninitialized variable occurs.

In my ideal implementation of this warning, the warning would be broken up into three flags: 

1. Unconditional uses of uninitialized variables.
2. Conditional uses of uninit variables, which occur after path merging where not-all predecessor paths are uninit.  -Wuninitialized-conditionally or something.
3. Verbose dumping of the path leading up to a warning.

The idea is that -Wuninitialized (which should be on by default) would turn on both #1 and #2, and that -Wno-uninitialized-conditionally (or whatever) would be useful for turning off the warnings in "path sensitive" cases where there may be many false positives.

#3 is an optional nicety that people could use to better understand a complex warning.  It should definitely be off by default, but could help for people who don't understand where the warning is coming from in a complex function.

Another refinement of this, it would be really nice to add an attribute((silence_uninitialized)) attribute, which would allow turning the warning off for a particular variable.  This could be used by proponents of "I know my code is safe and I don't want to initialize the variable".  It is good IMO to have an attribute here, because then that desire is explicit in the code.  This should be mentioned in the compatibility.html file.

> The reason that I am mixed is that if an uninitialized value warning requires a complicated explanation of what the code did to trigger the warning, I see two possibilities:
> 
> 1) The code is too complicated for the simple -Wuninitialized analysis to handle, probably because of control-dependencies.

The code in the clang warning should be engineered to be simple and fast, with clearly defined/documented limitations (no struct fields? no address of variables, no interprocedural).  People wanting more complex cases should use the analyzer.  Initializing variables or slapping the attribute onto one are perfectly reasonable solutions for dealing with false positives.

If Chrome is *only* getting ~20 warnings from this, it sounds like it is already quite reasonable.  Clang is generally "more noisy" by default than GCC, and this is a good thing IMO.  So long as there is a reasonable way to silence the warnings (=0 and attribute) I think we're fine.

-Chris