[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: george.karpenkov, NoQ, xazax.hun, rnkovacs.
Herald added subscribers: cfe-commits, mikhail.ramalho, a.sidorin, szepet, whisperity.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D48436

Files:
  lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
  test/Analysis/cxx-uninitialized-object.cpp


Index: test/Analysis/cxx-uninitialized-object.cpp
===================================================================
--- test/Analysis/cxx-uninitialized-object.cpp
+++ test/Analysis/cxx-uninitialized-object.cpp
@@ -1035,13 +1035,12 @@
 // 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() {
Index: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
@@ -212,9 +212,11 @@
 Optional<nonloc::LazyCompoundVal>
 getObjectVal(const CXXConstructorDecl *CtorDecl, CheckerContext &Context);
 
-/// Checks whether the constructor under checking is called by another
-/// constructor.
-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).
+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
@@ -255,7 +257,7 @@
     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);
@@ -419,8 +421,8 @@
   }
 
   // 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,15 +663,29 @@
   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.
-bool isCalledByConstructor(const CheckerContext &Context) {
+bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor,
+                               CheckerContext &Context) {
   const LocationContext *LC = Context.getLocationContext()->getParent();
 
+  Optional<nonloc::LazyCompoundVal> CurrentObject =
+      getObjectVal(Ctor, Context);
+  if (!CurrentObject)
+    return false;
+
   while (LC) {
-    if (isa<CXXConstructorDecl>(LC->getDecl()))
-      return true;
+    // If this constructor was called by another constructor.
+    if (const auto *OtherCtor = dyn_cast<CXXConstructorDecl>(LC->getDecl())) {
+
+      Optional<nonloc::LazyCompoundVal> OtherObject =
+          getObjectVal(OtherCtor, Context);
+      if (!OtherObject)
+        continue;
+
+      // If the CurrentObject is a field of OtherObject, it will be analyzed
+      // during the analysis of OtherObject.
+      if (CurrentObject->getRegion()->isSubRegionOf(OtherObject->getRegion()))
+        return true;
+    }
 
     LC = LC->getParent();
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D48436.152306.patch
Type: text/x-patch
Size: 4039 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180621/3ad02af9/attachment-0001.bin>