[analyzer] Refactor BugTypes and their ownership model.

-In general, don't have the BugReporter deleting BugTypes, BugTypes will eventually become owned by checkers
 and outlive the BugReporter. In the meantime, there will be some leaks since some checkers assume that
 the BugTypes they create will be destroyed by the BugReporter.
-Have BugReporter::EmitBasicReport create BugTypes that are reused if the same name & category strings
 are passed to EmitBasicReport. These BugTypes are owned and destroyed by the BugReporter.
 This allows bugs reported through EmitBasicReport to be coalesced.
-Remove the llvm::FoldingSet<BugReportEquivClass> from BugType and move it into the BugReporter.
 For uniquing BugReportEquivClass also use the BugType* so that we can iterate over all of them using only one set.

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

diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
index 1786fe610d6b0c30238df4d4b1893d5aebd9eae0..93d795831d3c1544f9f8921a1535b5789c0d3c50 100644 (file)
--- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
+++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
@@ -72,6 +72,7 @@ protected:
   friend class BugReportEquivClass;
 
   virtual void Profile(llvm::FoldingSetNodeID& hash) const {
+    hash.AddPointer(&BT);
     hash.AddInteger(getLocation().getRawEncoding());
     hash.AddString(Description);
   }
@@ -277,6 +278,8 @@ private:
 
   void FlushReport(BugReportEquivClass& EQ);
 
+  llvm::FoldingSet<BugReportEquivClass> EQClasses;
+
 protected:
   BugReporter(BugReporterData& d, Kind k) : BugTypes(F.getEmptySet()), kind(k),
                                             D(d) {}
@@ -302,6 +305,10 @@ public:
   iterator begin() { return BugTypes.begin(); }
   iterator end() { return BugTypes.end(); }
 
+  typedef llvm::FoldingSet<BugReportEquivClass>::iterator EQClasses_iterator;
+  EQClasses_iterator EQClasses_begin() { return EQClasses.begin(); }
+  EQClasses_iterator EQClasses_end() { return EQClasses.end(); }
+
   ASTContext& getContext() { return D.getASTContext(); }
 
   SourceManager& getSourceManager() { return D.getSourceManager(); }
@@ -344,6 +351,13 @@ public:
   }
 
   static bool classof(const BugReporter* R) { return true; }
+
+private:
+  llvm::StringMap<BugType *> StrBugTypes;
+
+  /// \brief Returns a BugType that is associated with the given name and
+  /// category.
+  BugType *getBugTypeForName(llvm::StringRef name, llvm::StringRef category);
 };
 
 // FIXME: Get rid of GRBugReporter.  It's the wrong abstraction.

diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
index 2793284e298f44e06314265586f88574fe828547..7b9bb03d8d05a567fa52886b8696a03bfc6093b9 100644 (file)
--- a/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
+++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
@@ -29,8 +29,6 @@ class BugType {
 private:
   const std::string Name;
   const std::string Category;
-  llvm::FoldingSet<BugReportEquivClass> EQClasses;
-  friend class BugReporter;
   bool SuppressonSink;
 public:
   BugType(llvm::StringRef name, llvm::StringRef cat)
@@ -48,14 +46,6 @@ public:
   void setSuppressOnSink(bool x) { SuppressonSink = x; }
 
   virtual void FlushReports(BugReporter& BR);
-
-  typedef llvm::FoldingSet<BugReportEquivClass>::iterator iterator;
-  iterator begin() { return EQClasses.begin(); }
-  iterator end() { return EQClasses.end(); }
-
-  typedef llvm::FoldingSet<BugReportEquivClass>::const_iterator const_iterator;
-  const_iterator begin() const { return EQClasses.begin(); }
-  const_iterator end() const { return EQClasses.end(); }
 };
 
 class BuiltinBug : public BugType {

diff --git a/lib/StaticAnalyzer/Checkers/ExprEngine.cpp b/lib/StaticAnalyzer/Checkers/ExprEngine.cpp
index c072d19a894977e7f76ed8cf8bdab5d5981ebdb2..234fd896d41b9d963009a91f0556dd02c44981db 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/ExprEngine.cpp
+++ b/lib/StaticAnalyzer/Checkers/ExprEngine.cpp
@@ -3630,14 +3630,12 @@ void ExprEngine::ViewGraph(bool trim) {
       const_cast<BugType*>(*I)->FlushReports(BR);
 
     // Iterate through the reports and get their nodes.
-    for (BugReporter::iterator I=BR.begin(), E=BR.end(); I!=E; ++I) {
-      for (BugType::const_iterator I2=(*I)->begin(), E2=(*I)->end();
-           I2!=E2; ++I2) {
-        const BugReportEquivClass& EQ = *I2;
-        const BugReport &R = **EQ.begin();
-        ExplodedNode *N = const_cast<ExplodedNode*>(R.getErrorNode());
-        if (N) Src.push_back(N);
-      }
+    for (BugReporter::EQClasses_iterator
+           EI = BR.EQClasses_begin(), EE = BR.EQClasses_end(); EI != EE; ++EI) {
+      BugReportEquivClass& EQ = *EI;
+      const BugReport &R = **EQ.begin();
+      ExplodedNode *N = const_cast<ExplodedNode*>(R.getErrorNode());
+      if (N) Src.push_back(N);
     }
 
     ViewGraph(&Src[0], &Src[0]+Src.size());

diff --git a/lib/StaticAnalyzer/Core/BugReporter.cpp b/lib/StaticAnalyzer/Core/BugReporter.cpp
index 9a84045ebd976dbef0ffa9cc37ce30bc8cb5c853..672982a3c0254600577b195e0d5b021398fadd9f 100644 (file)
--- a/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ b/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -1202,16 +1202,8 @@ static void GenerateExtensivePathDiagnostic(PathDiagnostic& PD,
 //===----------------------------------------------------------------------===//
 // Methods for BugType and subclasses.
 //===----------------------------------------------------------------------===//
-BugType::~BugType() {
-  // Free up the equivalence class objects.  Observe that we get a pointer to
-  // the object first before incrementing the iterator, as destroying the
-  // node before doing so means we will read from freed memory.
-  for (iterator I = begin(), E = end(); I !=E; ) {
-    BugReportEquivClass *EQ = &*I;
-    ++I;
-    delete EQ;
-  }
-}
+BugType::~BugType() { }
+
 void BugType::FlushReports(BugReporter &BR) {}
 
 //===----------------------------------------------------------------------===//
@@ -1315,28 +1307,30 @@ void BugReporter::FlushReports() {
     return;
 
   // First flush the warnings for each BugType.  This may end up creating new
-  // warnings and new BugTypes.  Because ImmutableSet is a functional data
-  // structure, we do not need to worry about the iterators being invalidated.
+  // warnings and new BugTypes.
+  // FIXME: Only NSErrorChecker needs BugType's FlushReports.
+  // Turn NSErrorChecker into a proper checker and remove this.
+  llvm::SmallVector<const BugType*, 16> bugTypes;
   for (BugTypesTy::iterator I=BugTypes.begin(), E=BugTypes.end(); I!=E; ++I)
+    bugTypes.push_back(*I);
+  for (llvm::SmallVector<const BugType*, 16>::iterator
+         I = bugTypes.begin(), E = bugTypes.end(); I != E; ++I)
     const_cast<BugType*>(*I)->FlushReports(*this);
 
-  // Iterate through BugTypes a second time.  BugTypes may have been updated
-  // with new BugType objects and new warnings.
-  for (BugTypesTy::iterator I=BugTypes.begin(), E=BugTypes.end(); I!=E; ++I) {
-    BugType *BT = const_cast<BugType*>(*I);
-
-    typedef llvm::FoldingSet<BugReportEquivClass> SetTy;
-    SetTy& EQClasses = BT->EQClasses;
-
-    for (SetTy::iterator EI=EQClasses.begin(), EE=EQClasses.end(); EI!=EE;++EI){
-      BugReportEquivClass& EQ = *EI;
-      FlushReport(EQ);
-    }
-
-    // Delete the BugType object.
-    delete BT;
+  typedef llvm::FoldingSet<BugReportEquivClass> SetTy;
+  for (SetTy::iterator EI=EQClasses.begin(), EE=EQClasses.end(); EI!=EE;++EI){
+    BugReportEquivClass& EQ = *EI;
+    FlushReport(EQ);
   }
 
+  // BugReporter owns and deletes only BugTypes created implicitly through
+  // EmitBasicReport.
+  // FIXME: There are leaks from checkers that assume that the BugTypes they
+  // create will be destroyed by the BugReporter.
+  for (llvm::StringMap<BugType*>::iterator
+         I = StrBugTypes.begin(), E = StrBugTypes.end(); I != E; ++I)
+    delete I->second;
+
   // Remove all references to the BugType objects.
   BugTypes = F.getEmptySet();
 }
@@ -1632,11 +1626,11 @@ void BugReporter::EmitReport(BugReport* R) {
   BugType& BT = R->getBugType();
   Register(&BT);
   void *InsertPos;
-  BugReportEquivClass* EQ = BT.EQClasses.FindNodeOrInsertPos(ID, InsertPos);
+  BugReportEquivClass* EQ = EQClasses.FindNodeOrInsertPos(ID, InsertPos);
 
   if (!EQ) {
     EQ = new BugReportEquivClass(R);
-    BT.EQClasses.InsertNode(EQ, InsertPos);
+    EQClasses.InsertNode(EQ, InsertPos);
   }
   else
     EQ->AddReport(R);
@@ -1887,10 +1881,24 @@ void BugReporter::EmitBasicReport(llvm::StringRef name,
                                   llvm::StringRef str, SourceLocation Loc,
                                   SourceRange* RBeg, unsigned NumRanges) {
 
-  // 'BT' will be owned by BugReporter as soon as we call 'EmitReport'.
-  BugType *BT = new BugType(name, category);
+  // 'BT' is owned by BugReporter.
+  BugType *BT = getBugTypeForName(name, category);
   FullSourceLoc L = getContext().getFullLoc(Loc);
   RangedBugReport *R = new DiagBugReport(*BT, str, L);
   for ( ; NumRanges > 0 ; --NumRanges, ++RBeg) R->addRange(*RBeg);
   EmitReport(R);
 }
+
+BugType *BugReporter::getBugTypeForName(llvm::StringRef name,
+                                        llvm::StringRef category) {
+  llvm::SmallString<136> fullDesc;
+  llvm::raw_svector_ostream(fullDesc) << name << ":" << category;
+  llvm::StringMapEntry<BugType *> &
+      entry = StrBugTypes.GetOrCreateValue(fullDesc);
+  BugType *BT = entry.getValue();
+  if (!BT) {
+    BT = new BugType(name, category);
+    entry.setValue(BT);
+  }
+  return BT;
+}