From: Argyrios Kyrtzidis Date: Wed, 23 Feb 2011 00:16:01 +0000 (+0000) Subject: [analyzer] Refactor BugTypes and their ownership model. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=404fc3ad6bd844bf8ce70cbf9974ab297704a122;p=clang [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 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 1786fe610d..93d795831d 100644 --- 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 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::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 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 2793284e29..7b9bb03d8d 100644 --- 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 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::iterator iterator; - iterator begin() { return EQClasses.begin(); } - iterator end() { return EQClasses.end(); } - - typedef llvm::FoldingSet::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 c072d19a89..234fd896d4 100644 --- a/lib/StaticAnalyzer/Checkers/ExprEngine.cpp +++ b/lib/StaticAnalyzer/Checkers/ExprEngine.cpp @@ -3630,14 +3630,12 @@ void ExprEngine::ViewGraph(bool trim) { const_cast(*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(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(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 9a84045ebd..672982a3c0 100644 --- 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 bugTypes; for (BugTypesTy::iterator I=BugTypes.begin(), E=BugTypes.end(); I!=E; ++I) + bugTypes.push_back(*I); + for (llvm::SmallVector::iterator + I = bugTypes.begin(), E = bugTypes.end(); I != E; ++I) const_cast(*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(*I); - - typedef llvm::FoldingSet 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 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::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 & + entry = StrBugTypes.GetOrCreateValue(fullDesc); + BugType *BT = entry.getValue(); + if (!BT) { + BT = new BugType(name, category); + entry.setValue(BT); + } + return BT; +}