[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:258-260
+    Report->addNote(NoteBuf,
+                    PathDiagnosticLocation::create(FieldChain.getEndOfChain(),
+                                                   Context.getSourceManager()));
----------------
Szelethus wrote:
> NoQ wrote:
> > Aha, ok, got it, so you're putting the warning at the end of the constructor and squeezing all uninitialized fields into a single warning by presenting them as notes.
> > 
> > Because for now notes aren't supported everywhere (and aren't always supported really well), i guess it'd be necessary to at least provide an option to make note-less reports before the checker is enabled by default everywhere. In this case notes contain critical pieces of information and as such cannot be really discarded.
> > 
> > I'm not sure that notes are providing a better user experience here than simply saying that "field x.y->z is never initialized". With notes, the user will have to scroll around (at least in scan-build) to find which field is uninitialized.
> > 
> > Notes will probably also not decrease the number of reports too much because in most cases there will be just one uninitialized field. Though i agree that when there is more than one report, the user will be likely to deal with all fields at once rather than separately.
> > 
> > So it's a bit wonky.
> > 
> > We might want to enable one mode or another in the checker depending on the analyzer output flag. Probably in the driver (`RenderAnalyzerOptions()`).
> > 
> > It'd be a good idea to land the checker into alpha first and fix this in a follow-up patch because this review is already overweight.
> > [...]i guess it'd be necessary to at least provide an option to make note-less reports before the checker is enabled by default everywhere. In this case notes contain critical pieces of information and as such cannot be really discarded.
> This can already be achieved with `-analyzer-config notes-as-events=true`. There no good reason however not to make an additional flag that would emit warnings instead of notes.
> > I'm not sure that notes are providing a better user experience here than simply saying that "field x.y->z is never initialized". With notes, the user will have to scroll around (at least in scan-build) to find which field is uninitialized.
> This checker had a previous implementation that emitted a warning for each uninitialized field, instead of squeezing them in a single warning per constructor call. One of the major reasons behind rewriting it was that it emitted so many of them that it was difficult to differentiate which warning belonged to which constructor call. Also, in the case of the LLVM/Clang project, the warnings from this checker alone would be in the thousands.
> > Notes will probably also not decrease the number of reports too much because in most cases there will be just one uninitialized field.
> While this is true to some extent, it's not uncommon that a single constructor call leaves 7 uninitialized -- in the LLVM/Clang project I found one that had over 30!
> Since its practically impossible to determine whether this was a performance enhancement or just poor programming, the few cases where it is intentional, an object would emit many more warnings would make  make majority of the findings (where an object truly had only 1 or 2 fields uninit) a lot harder to spot in my opinion.
> 
> In general, I think one warning per constructor call makes the best user experience, but a temporary solution should be implemented until there's better support for notes.
> 
> I agree, this should be a topic of a follow-up patch.
> 
> This can already be achieved with `-analyzer-config notes-as-events=true`.

Yep. But the resulting user experience seems pretty bad to me. It was a good quick proof-of-concept trick to test things, but the fact that notes aren't path pieces is way too apparent in case of this checker. So i guess this flag was a bad idea.

> Also, in the case of the LLVM/Clang project, the warnings from this checker alone would be in the thousands.

This makes me a bit worried, i wonder what's the reason why does the checker shout so loudly on a codebase that doesn't seem to have actual uninitialized value bugs all over the place.

Are any of these duplicates found in the same constructor for the same field in different translation units? Suppose we discard the duplicates (if any) and warn exactly once (across the whole LLVM) for every uninitialized field (which is probably more than once per constructor). Then I wonder:
1. How many uninitialize fields would we be able to find this way?
2. How many of such fields were intentionally left uninitialized?
3. How many were found by deep inspection of the heap through field chains involving pointer dereference "links"?


https://reviews.llvm.org/D45532