[analyzer][UninitializedObjectChecker] Uninit regions are only reported once

Especially with pointees, a lot of meaningless reports came from uninitialized
regions that were already reported. This is fixed by storing all reported fields
to the GDM.

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


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@347153 91177308-0d34-0410-b5e6-96231b3b80d8

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h

@@ -215,7 +215,11 @@ public:
                           const TypedValueRegion *const R,
                           const UninitObjCheckerOptions &Opts);
 
-  const UninitFieldMap &getUninitFields() { return UninitFields; }
+  /// Returns with the modified state and a map of (uninitialized region,
+  /// note message) pairs.
+  std::pair<ProgramStateRef, const UninitFieldMap &> getResults() {
+    return {State, UninitFields};
+  }
 
   /// Returns whether the analyzed region contains at least one initialized
   /// field. Note that this includes subfields as well, not just direct ones,
@@ -296,14 +300,16 @@ private:
   // TODO: Add a support for nonloc::LocAsInteger.
 
   /// Processes LocalChain and attempts to insert it into UninitFields. Returns
-  /// true on success.
+  /// true on success. Also adds the head of the list and \p PointeeR (if
+  /// supplied) to the GDM as already analyzed objects.
   ///
   /// Since this class analyzes regions with recursion, we'll only store
   /// references to temporary FieldNode objects created on the stack. This means
   /// that after analyzing a leaf of the directed tree described above, the
   /// elements LocalChain references will be destructed, so we can't store it
   /// directly.
-  bool addFieldToUninits(FieldChainInfo LocalChain);
+  bool addFieldToUninits(FieldChainInfo LocalChain,
+                         const MemRegion *PointeeR = nullptr);
 };
 
 /// Returns true if T is a primitive type. An object of a primitive type only

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp

@@ -28,9 +28,14 @@
 using namespace clang;
 using namespace clang::ento;
 
+/// We'll mark fields (and pointee of fields) that are confirmed to be
+/// uninitialized as already analyzed.
+REGISTER_SET_WITH_PROGRAMSTATE(AnalyzedRegions, const MemRegion *)
+
 namespace {
 
-class UninitializedObjectChecker : public Checker<check::EndFunction> {
+class UninitializedObjectChecker
+    : public Checker<check::EndFunction, check::DeadSymbols> {
   std::unique_ptr<BuiltinBug> BT_uninitField;
 
 public:
@@ -39,7 +44,9 @@ public:
 
   UninitializedObjectChecker()
       : BT_uninitField(new BuiltinBug(this, "Uninitialized fields")) {}
+
   void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const;
+  void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const;
 };
 
 /// A basic field type, that is not a pointer or a reference, it's dynamic and
@@ -140,14 +147,20 @@ void UninitializedObjectChecker::checkEndFunction(
 
   FindUninitializedFields F(Context.getState(), R, Opts);
 
-  const UninitFieldMap &UninitFields = F.getUninitFields();
+  std::pair<ProgramStateRef, const UninitFieldMap &> UninitInfo =
+      F.getResults();
 
-  if (UninitFields.empty())
+  ProgramStateRef UpdatedState = UninitInfo.first;
+  const UninitFieldMap &UninitFields = UninitInfo.second;
+
+  if (UninitFields.empty()) {
+    Context.addTransition(UpdatedState);
     return;
+  }
 
   // There are uninitialized fields in the record.
 
-  ExplodedNode *Node = Context.generateNonFatalErrorNode(Context.getState());
+  ExplodedNode *Node = Context.generateNonFatalErrorNode(UpdatedState);
   if (!Node)
     return;
 
@@ -188,6 +201,15 @@ void UninitializedObjectChecker::checkEndFunction(
   Context.emitReport(std::move(Report));
 }
 
+void UninitializedObjectChecker::checkDeadSymbols(SymbolReaper &SR,
+                                                  CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  for (const MemRegion *R : State->get<AnalyzedRegions>()) {
+    if (!SR.isLiveRegion(R))
+      State = State->remove<AnalyzedRegions>(R);
+  }
+}
+
 //===----------------------------------------------------------------------===//
 //                   Methods for FindUninitializedFields.
 //===----------------------------------------------------------------------===//
@@ -205,17 +227,34 @@ FindUninitializedFields::FindUninitializedFields(
     UninitFields.clear();
 }
 
-bool FindUninitializedFields::addFieldToUninits(FieldChainInfo Chain) {
+bool FindUninitializedFields::addFieldToUninits(FieldChainInfo Chain,
+                                                const MemRegion *PointeeR) {
+  const FieldRegion *FR = Chain.getUninitRegion();
+
+  assert((PointeeR || !isDereferencableType(FR->getDecl()->getType())) &&
+         "One must also pass the pointee region as a parameter for "
+         "dereferencable fields!");
+
+  if (State->contains<AnalyzedRegions>(FR))
+    return false;
+
+  if (PointeeR) {
+    if (State->contains<AnalyzedRegions>(PointeeR)) {
+      return false;
+    }
+    State = State->add<AnalyzedRegions>(PointeeR);
+  }
+
+  State = State->add<AnalyzedRegions>(FR);
+
   if (State->getStateManager().getContext().getSourceManager().isInSystemHeader(
-          Chain.getUninitRegion()->getDecl()->getLocation()))
+          FR->getDecl()->getLocation()))
     return false;
 
   UninitFieldMap::mapped_type NoteMsgBuf;
   llvm::raw_svector_ostream OS(NoteMsgBuf);
   Chain.printNoteMsg(OS);
-  return UninitFields
-      .insert(std::make_pair(Chain.getUninitRegion(), std::move(NoteMsgBuf)))
-      .second;
+  return UninitFields.insert({FR, std::move(NoteMsgBuf)}).second;
 }
 
 bool FindUninitializedFields::isNonUnionUninit(const TypedValueRegion *R,

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp

@@ -153,7 +153,7 @@ bool FindUninitializedFields::isDereferencableUninit(
 
   if (V.isUndef()) {
     return addFieldToUninits(
-        LocalChain.add(LocField(FR, /*IsDereferenced*/ false)));
+        LocalChain.add(LocField(FR, /*IsDereferenced*/ false)), FR);
   }
 
   if (!Opts.CheckPointeeInitialization) {
@@ -170,7 +170,7 @@ bool FindUninitializedFields::isDereferencableUninit(
   }
 
   if (DerefInfo->IsCyclic)
-    return addFieldToUninits(LocalChain.add(CyclicLocField(FR)));
+    return addFieldToUninits(LocalChain.add(CyclicLocField(FR)), FR);
 
   const TypedValueRegion *R = DerefInfo->R;
   const bool NeedsCastBack = DerefInfo->NeedsCastBack;
@@ -187,8 +187,9 @@ bool FindUninitializedFields::isDereferencableUninit(
   if (PointeeT->isUnionType()) {
     if (isUnionUninit(R)) {
       if (NeedsCastBack)
-        return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT)));
-      return addFieldToUninits(LocalChain.add(LocField(FR)));
+        return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT)),
+                                 R);
+      return addFieldToUninits(LocalChain.add(LocField(FR)), R);
     } else {
       IsAnyFieldInitialized = true;
       return false;
@@ -208,8 +209,8 @@ bool FindUninitializedFields::isDereferencableUninit(
 
   if (isPrimitiveUninit(PointeeV)) {
     if (NeedsCastBack)
-      return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT)));
-    return addFieldToUninits(LocalChain.add(LocField(FR)));
+      return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT)), R);
+    return addFieldToUninits(LocalChain.add(LocField(FR)), R);
   }
 
   IsAnyFieldInitialized = true;

test/Analysis/cxx-uninitialized-object-ptr-ref.cpp

@@ -865,3 +865,44 @@ void fReferenceTest5() {
   ReferenceTest4::RecordType c, d{37, 38};
   ReferenceTest4(d, c);
 }
+
+//===----------------------------------------------------------------------===//
+// Tests for objects containing multiple references to the same object.
+//===----------------------------------------------------------------------===//
+
+struct IntMultipleReferenceToSameObjectTest {
+  int *iptr; // expected-note{{uninitialized pointee 'this->iptr'}}
+  int &iref; // no-note, pointee of this->iref was already reported
+
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  IntMultipleReferenceToSameObjectTest(int *i) : iptr(i), iref(*i) {} // expected-warning{{1 uninitialized field}}
+};
+
+void fIntMultipleReferenceToSameObjectTest() {
+  int a;
+  IntMultipleReferenceToSameObjectTest Test(&a);
+}
+
+struct IntReferenceWrapper1 {
+  int &a; // expected-note{{uninitialized pointee 'this->a'}}
+
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  IntReferenceWrapper1(int &a) : a(a) {} // expected-warning{{1 uninitialized field}}
+};
+
+struct IntReferenceWrapper2 {
+  int &a; // no-note, pointee of this->a was already reported
+
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  IntReferenceWrapper2(int &a) : a(a) {} // no-warning
+};
+
+void fMultipleObjectsReferencingTheSameObjectTest() {
+  int a;
+
+  IntReferenceWrapper1 T1(a);
+  IntReferenceWrapper2 T2(a);
+}