From 9480a8d28700c7a327fe6e1eda8ff84266c8031c Mon Sep 17 00:00:00 2001
From: Kristof Umann <dkszelethus@gmail.com>
Date: Wed, 8 Aug 2018 12:23:02 +0000
Subject: [PATCH] [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


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@339237 91177308-0d34-0410-b5e6-96231b3b80d8
---
 .../Checkers/UninitializedObjectChecker.cpp   | 48 +++++++++++++------
 test/Analysis/cxx-uninitialized-object.cpp    |  7 ++-
 2 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp b/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
index 2383ecff22..4e6ccd9da3 100644
--- a/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
@@ -225,12 +225,16 @@ static llvm::ImmutableListFactory<const FieldRegion *> Factory;
 
 /// 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::checkEndFunction(
     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::isNonUnionUninit(const TypedValueRegion *R,
   }
 
   // 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 *CtorDecl, CheckerContext &Context) {
   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;
 }
 
diff --git a/test/Analysis/cxx-uninitialized-object.cpp b/test/Analysis/cxx-uninitialized-object.cpp
index 0c5c1c246c..4fc455fea8 100644
--- a/test/Analysis/cxx-uninitialized-object.cpp
+++ b/test/Analysis/cxx-uninitialized-object.cpp
@@ -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() {
-- 
2.50.1