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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:223-225
+  ExplodedNode *Node = Context.generateNonFatalErrorNode(Context.getState());
+  if (!Node)
+    return;
----------------
Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > NoQ wrote:
> > > > I suspect that a fatal error is better here. We don't want the user to receive duplicate report from other checkers that catch uninitialized values; just one report is enough.
> > > I think that would be a bad idea. For example, this checker shouts so loudly while checking the LLVM project, it would practically halt the analysis of the code, reducing the coverage, which means that checkers other then uninit value checkers would "suffer" from it.
> > > 
> > > However, I also think that having multiple uninit reports for the same object might be good, especially with this checker, as it would be very easy to see where the problem originated from.
> > > 
> > > What do you think?
> > Well, i guess that's the reason to not use the checker on LLVM. Regardless of fatal/nonfatal warnings, enabling this checker on LLVM regularly would be a bad idea because it's unlikely that anybody will be able to fix all the false positives to make it usable. And for other projects that don't demonstrate many false positives, this shouldn't be a problem.
> > 
> > In order to indicate where the problem originates from, we have our bug reporter visitors that try their best to add such info directly to the report. In particular, @george.karpenkov's recent `NoStoreFuncVisitor` highlights functions in which a variable was //not// initialized but was probably expected to be. Not sure if it highlights constructors in its current shape, but that's definitely a better way to give this piece of information to the user because it doesn't make the user look for a different report to understand the current report.
> LLVM is a special project in the fact that almost every part of it is so performance critical, that leaving many fields uninit matters. However, I would believe that in most projects, only a smaller portion of the code would be like that.
> 
> Suppose that we have a project that also defines a set of ADTs, like an `std::list`-like container. If that container had a field that would be left uninit after a ctor call, analysis on every execution path would be halted which would use an object like that.
> 
> My point is, as long as there is no way to tell the analyzer (or the checker) to ignore certain constructor calls, I think it would be best not to generate a fatal error.
> 
> >Regardless of fatal/nonfatal warnings, enabling this checker on LLVM regularly would be a bad idea because it's unlikely that anybody will be able to fix all the false positives to make it usable. And for other projects that don't demonstrate many false positives, this shouldn't be a problem.
> I wouldn't necessarily call them false positives. This checker doesn't look for bugs, and all reports I checked were correct in the fact that those fields really were left uninit. They just don't cause any trouble (just yet!).
I think of this check as a tool to support a specific programming model where every field needs to be initialized by the constructor. This programming model might be followed by some parts of the projects while a 3rd party library in the same project or some other files might not follow this model. Right now there is no easy way to turn off some checks for a set of files, and there is no way to turn off a set of checks for some headers. For this reason, I think it is not a good idea to make these errors fatal, as 3rd party headers might reduce the coverage of the analysis on a project in a way that the user cannot control.

If we are afraid of having multiple reports from this check we could turn off the check for that particular path, for example, we could have a bool stored in the GDM for each path whether this check is already reported an error or not and we can check that before emitting warnings. 


https://reviews.llvm.org/D45532