r339237 - [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls

[Kristof Umann via cfe-commits]

Author: szelethus
Date: Wed Aug  8 05:23:02 2018
New Revision: 339237

URL: http://llvm.org/viewvc/llvm-project?rev=339237&view=rev
Log:
[analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls

As of now, all constructor calls are ignored that are being called
by a constructor. The point of this was not to analyze the fields
of an object, so an uninitialized field wouldn't be reported
multiple times.

This however introduced false negatives when the two constructors
were in no relation to one another -- see the test file for a neat
example for this with singletons. This patch aims so fix this issue.

Differential Revision: https://reviews.llvm.org/D48436

Modified:
    cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
    cfe/trunk/test/Analysis/cxx-uninitialized-object.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp?rev=339237&r1=339236&r2=339237&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp Wed Aug  8 05:23:02 2018
@@ -225,12 +225,16 @@ static llvm::ImmutableListFactory<const
 
 /// Returns the object that was constructed by CtorDecl, or None if that isn't
 /// possible.
+// TODO: Refactor this function so that it returns the constructed object's
+// region.
 static Optional<nonloc::LazyCompoundVal>
 getObjectVal(const CXXConstructorDecl *CtorDecl, CheckerContext &Context);
 
-/// Checks whether the constructor under checking is called by another
-/// constructor.
-static bool isCalledByConstructor(const CheckerContext &Context);
+/// Checks whether the object constructed by \p Ctor will be analyzed later
+/// (e.g. if the object is a field of another object, in which case we'd check
+/// it multiple times).
+static bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor,
+                               CheckerContext &Context);
 
 /// Returns whether FD can be (transitively) dereferenced to a void pointer type
 /// (void*, void**, ...). The type of the region behind a void pointer isn't
@@ -273,7 +277,7 @@ void UninitializedObjectChecker::checkEn
     return;
 
   // This avoids essentially the same error being reported multiple times.
-  if (isCalledByConstructor(Context))
+  if (willObjectBeAnalyzedLater(CtorDecl, Context))
     return;
 
   Optional<nonloc::LazyCompoundVal> Object = getObjectVal(CtorDecl, Context);
@@ -433,8 +437,8 @@ bool FindUninitializedFields::isNonUnion
   }
 
   // Checking bases.
-  // FIXME: As of now, because of `isCalledByConstructor`, objects whose type
-  // is a descendant of another type will emit warnings for uninitalized
+  // FIXME: As of now, because of `willObjectBeAnalyzedLater`, objects whose
+  // type is a descendant of another type will emit warnings for uninitalized
   // inherited members.
   // This is not the only way to analyze bases of an object -- if we didn't
   // filter them out, and didn't analyze the bases, this checker would run for
@@ -661,18 +665,32 @@ getObjectVal(const CXXConstructorDecl *C
   return Object.getAs<nonloc::LazyCompoundVal>();
 }
 
-// TODO: We should also check that if the constructor was called by another
-// constructor, whether those two are in any relation to one another. In it's
-// current state, this introduces some false negatives.
-static bool isCalledByConstructor(const CheckerContext &Context) {
-  const LocationContext *LC = Context.getLocationContext()->getParent();
+static bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor,
+                               CheckerContext &Context) {
 
-  while (LC) {
-    if (isa<CXXConstructorDecl>(LC->getDecl()))
-      return true;
+  Optional<nonloc::LazyCompoundVal> CurrentObject = getObjectVal(Ctor, Context);
+  if (!CurrentObject)
+    return false;
+
+  const LocationContext *LC = Context.getLocationContext();
+  while ((LC = LC->getParent())) {
+
+    // If \p Ctor was called by another constructor.
+    const auto *OtherCtor = dyn_cast<CXXConstructorDecl>(LC->getDecl());
+    if (!OtherCtor)
+      continue;
 
-    LC = LC->getParent();
+    Optional<nonloc::LazyCompoundVal> OtherObject =
+        getObjectVal(OtherCtor, Context);
+    if (!OtherObject)
+      continue;
+
+    // If the CurrentObject is a subregion of OtherObject, it will be analyzed
+    // during the analysis of OtherObject.
+    if (CurrentObject->getRegion()->isSubRegionOf(OtherObject->getRegion()))
+      return true;
   }
+
   return false;
 }
 

Modified: cfe/trunk/test/Analysis/cxx-uninitialized-object.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/cxx-uninitialized-object.cpp?rev=339237&r1=339236&r2=339237&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/cxx-uninitialized-object.cpp (original)
+++ cfe/trunk/test/Analysis/cxx-uninitialized-object.cpp Wed Aug  8 05:23:02 2018
@@ -1040,13 +1040,12 @@ void assert(int b) {
 // While a singleton would make more sense as a static variable, that would zero
 // initialize all of its fields, hence the not too practical implementation.
 struct Singleton {
-  // TODO: we'd expect the note: {{uninitialized field 'this->i'}}
-  int i; // no-note
+  int i; // expected-note{{uninitialized field 'this->i'}}
+  int dontGetFilteredByNonPedanticMode = 0;
 
   Singleton() {
     assert(!isInstantiated);
-    // TODO: we'd expect the warning: {{1 uninitialized field}}
-    isInstantiated = true; // no-warning
+    isInstantiated = true; // expected-warning{{1 uninitialized field}}
   }
 
   ~Singleton() {