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

[Szelethus]

Summary

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.

[Szelethus]

Polite ping :)

[NoQ]

Hmm, so we're reporting the same region twice in the same bug report through different field chains, right? Why is a state trait necessary here? Maybe just de-duplicate them locally before throwing the report?

[NoQ]

Hmm, i guess it's necessary to de-duplicate across multiple reports as well.

[NoQ]

Or, wait, hmm, no, we can still de-duplicate the reports with a global set stored in the checker object. Because we would probably like to de-duplicate across different paths as well.

[Szelethus]

In D51531#1234280, @NoQ wrote:

we can still de-duplicate the reports with a global set stored in the checker object.

I tried it, but it crashes randomly. I added a mutable std::set<const MemRegion *> to the checker class, used the same logic, but seems to me that you can't just erase elements like this. Is there any other checker doing the same thing?

[Szelethus]

Ping ^-^

[Szelethus]

Another possibility could be to gather CXXConstructorDecl, and emit one warning per ctor, but it would be waaay to drastic. Wouldn't a global set be too?

[Szelethus]

Ping, @NoQ, can you go a little bit more in depth about what you'd prefer to see here?

[Szelethus]

Ping^5 :)

[NoQ]

Ugh, i was sure i scanned through all of them, sorry! Pls let me recall what's going on><

[Szelethus]

As I understood it, your objection was that we could collect already reported regions in a global set stored in the checker objects, but I suspect it's impossible to implement (very difficult to see when a pointer is dangling etc).

[NoQ]

MemRegions have lifetime of ExprEngine, i.e. a single analysis of top-level function. You'll need to clean them up in checkEndAnalysis().

[NoQ]

I think RetainCountChecker is the only checker that maintains such maps and does such cleanup: https://github.com/llvm-mirror/clang/blob/efe41bf98/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h#L288

I didn't know it does that until today.

[Szelethus]

Sorry for the late reply.

I'm not actually super confident about this idea, I don't think it would add much value, compared to how complicated it would make things. This patch reduces report by a significant amount when pointer chasing is enabled, but a global set might not make things that much cleaner -- if the same report would be made on different execution paths, it would be uniqued with the help of the uniqueing location. If it it wouldn't be, I guess it shouldn't be.

Of course, I'm always open to a discussion!

[Szelethus]

Oh, and the reason why I think it would add a lot of complication: since only ExprEngine is avaible in the checkEndAnalysis callback, which, from what I can see, doesn't have a handly isDead method, so I'm not even sure how I'd implement it.

[Szelethus]

Ping, @NoQ, do you insist on a global set?

[NoQ]

Mm, ok, i admit that i don't know what specific de-duplication do we want to have and what are the usability implications of it.

If we want de-duplication for the same region reported in different spots of the same path, we need a state trait.

If we want de-duplication for the same region globally across the whole analysis, we need a checker-wide set of regions, like what you're doing.

If we want de-duplication for the same region globally across the whole translation unit, we need a checker-wide set of AST nodes (eg., field chains) that describe regions in terms that outlive ExprEngine.

If we want de-duplication for different regions within the same constructor call site, we need a uniqueing location at the call site, like what you're doing.

If we want de-duplication for different regions within the same constructor itself, we don't need a uniqueing location.

I guess you should just do whatever you want here?

In D51531#1286110, @Szelethus wrote:

Oh, and the reason why I think it would add a lot of complication: since only ExprEngine is avaible in the checkEndAnalysis callback, which, from what I can see, doesn't have a handly isDead method, so I'm not even sure how I'd implement it.

Mm, what do you need isDead for? Everything is dead at the end of the analysis, you need to clean up everything, otherwise you get use-after-free(?)

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp

169

Hmm, does LocUsedForUniqueing remain uninitialized sometimes? I guess it's on the top frame. What does it do in this case? Why do we need a uniqueing location in other cases? I guess it only *increases* the amount of reports, right? I.e., constructors called from different call sites produce different reports(?)

[Szelethus]

In D51531#1296256, @NoQ wrote:

In D51531#1286110, @Szelethus wrote:

Oh, and the reason why I think it would add a lot of complication: since only ExprEngine is avaible in the checkEndAnalysis callback, which, from what I can see, doesn't have a handly isDead method, so I'm not even sure how I'd implement it.

Mm, what do you need isDead for? Everything is dead at the end of the analysis, you need to clean up everything, otherwise you get use-after-free(?)

Oh, right. Silly me :)

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h

 
 public:
   /// Constructs the FindUninitializedField object, searches for and stores
   /// uninitialized fields in R.
   FindUninitializedFields(ProgramStateRef State,
                           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,
   /// and will return false if an uninitialized pointee is found with
   /// CheckPointeeInitialization enabled.
   bool isAnyFieldInitialized() { return IsAnyFieldInitialized; }
 
 private:
private:
   bool isPrimitiveUninit(const SVal &V);
 
   // Note that we don't have a method for arrays -- the elements of an array are
   // often left uninitialized intentionally even when it is of a C++ record
   // type, so we'll assume that an array is always initialized.
   // 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
 /// needs to be analyzed as much as checking whether their value is undefined.
 inline bool isPrimitiveType(const QualType &T) {
   return T->isBuiltinType() || T->isEnumeralType() ||
          T->isMemberPointerType() || T->isBlockPointerType() ||
          T->isFunctionType();

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp

 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h"
 
 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:
   // The fields of this struct will be initialized when registering the checker.
   UninitObjCheckerOptions Opts;
 
   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
 /// static type is the same.
 class RegularField final : public FieldNode {
 public:
   RegularField(const FieldRegion *FR) : FieldNode(FR) {}
 
if (willObjectBeAnalyzedLater(CtorDecl, Context))
     return;
 
   const TypedValueRegion *R = getConstructedRegion(CtorDecl, Context);
   if (!R)
     return;
 
   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;
 
   PathDiagnosticLocation LocUsedForUniqueing;
   const Stmt *CallSite = Context.getStackFrame()->getCallSite();
   if (CallSite)
     LocUsedForUniqueing = PathDiagnosticLocation::createBegin(
         CallSite, Context.getSourceManager(), Node->getLocationContext());
 
   // For Plist consumers that don't support notes just yet, we'll convert notes
   // to warnings.
   if (Opts.ShouldConvertNotesToWarnings) {
     for (const auto &Pair : UninitFields) {
 
void UninitializedObjectChecker::checkEndFunction(
   for (const auto &Pair : UninitFields) {
     Report->addNote(Pair.second,
                     PathDiagnosticLocation::create(Pair.first->getDecl(),
                                                    Context.getSourceManager()));
   }
   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.
 //===----------------------------------------------------------------------===//
 
 FindUninitializedFields::FindUninitializedFields(
     ProgramStateRef State, const TypedValueRegion *const R,
     const UninitObjCheckerOptions &Opts)
     : State(State), ObjectR(R), Opts(Opts) {
 
   isNonUnionUninit(ObjectR, FieldChainInfo(ChainFactory));
 
   // In non-pedantic mode, if ObjectR doesn't contain a single initialized
   // field, we'll assume that Object was intentionally left uninitialized.
   if (!Opts.IsPedantic && !isAnyFieldInitialized())
     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,
                                                FieldChainInfo LocalChain) {
   assert(R->getValueType()->isRecordType() &&
          !R->getValueType()->isUnionType() &&
          "This method only checks non-union record objects!");
 

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp

bool FindUninitializedFields::isDereferencableUninit(
 
   if (V.isUnknown() || V.getAs<loc::ConcreteInt>()) {
     IsAnyFieldInitialized = true;
     return false;
   }
 
   if (V.isUndef()) {
     return addFieldToUninits(
-        LocalChain.add(LocField(FR, /*IsDereferenced*/ false)));
+        LocalChain.add(LocField(FR, /*IsDereferenced*/ false)), FR);
   }
 
   if (!Opts.CheckPointeeInitialization) {
     IsAnyFieldInitialized = true;
     return false;
   }
 
   // At this point the pointer itself is initialized and points to a valid
   // location, we'll now check the pointee.
   llvm::Optional<DereferenceInfo> DerefInfo = dereference(State, FR);
   if (!DerefInfo) {
     IsAnyFieldInitialized = true;
     return false;
   }
 
   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;
 
   QualType DynT = R->getLocationType();
   QualType PointeeT = DynT->getPointeeType();
 
   if (PointeeT->isStructureOrClassType()) {
     if (NeedsCastBack)
       return isNonUnionUninit(R, LocalChain.add(NeedsCastLocField(FR, DynT)));
     return isNonUnionUninit(R, LocalChain.add(LocField(FR)));
   }
 
   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;
     }
   }
 
   if (PointeeT->isArrayType()) {
     IsAnyFieldInitialized = true;
     return false;
   }
 
   assert((isPrimitiveType(PointeeT) || isDereferencableType(PointeeT)) &&
          "At this point FR must either have a primitive dynamic type, or it "
          "must be a null, undefined, unknown or concrete pointer!");
 
   SVal PointeeV = State->getSVal(R);
 
   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;
   return false;
 }
 
 //===----------------------------------------------------------------------===//
 //                           Utility functions.

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

ReferenceTest4(RecordType &lref, RecordType &rref)
       : lref(lref), rref(static_cast<RecordType &&>(rref)) { // expected-warning{{2 uninitialized fields}}
   }
 };
 
 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);
+}