From 3bc75ca0a636efdc93471c9b6bad43085a22bf3a Mon Sep 17 00:00:00 2001 From: Jordy Rose Date: Sat, 24 Mar 2012 03:03:29 +0000 Subject: [PATCH] [analyzer] Restart path diagnostic generation if any of the visitors change the report configuration while walking the path. This required adding a change count token to BugReport, but also allowed us to ditch ImmutableList as the BugReporterVisitor data type. Also, remove the hack from MallocChecker, now that visitors appear in the opposite order. This is not exactly a fix, but the common case -- custom diagnostics after generic ones -- is now the default behavior. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@153369 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Core/BugReporter/BugReporter.h | 28 ++++-- lib/StaticAnalyzer/Checkers/MallocChecker.cpp | 4 - lib/StaticAnalyzer/Core/BugReporter.cpp | 95 +++++++++++++------ 3 files changed, 84 insertions(+), 43 deletions(-) diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h index b607fe8b2f..57ffea99dd 100644 --- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h +++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h @@ -61,7 +61,8 @@ public: }; typedef const SourceRange *ranges_iterator; - typedef llvm::ImmutableList::iterator visitor_iterator; + typedef SmallVector VisitorList; + typedef VisitorList::iterator visitor_iterator; typedef SmallVector ExtraTextList; protected: @@ -90,25 +91,30 @@ protected: /// to include when constructing the final path diagnostic. Regions interestingRegions; - // Not the most efficient data structure, but we use an ImmutableList for the - // Callbacks because it is safe to make additions to list during iteration. - llvm::ImmutableList::Factory F; - llvm::ImmutableList Callbacks; + /// A set of custom visitors which generate "event" diagnostics at + /// interesting points in the path. + VisitorList Callbacks; + + /// Used for ensuring the visitors are only added once. llvm::FoldingSet CallbacksSet; + /// Used for clients to tell if the report's configuration has changed + /// since the last time they checked. + unsigned ConfigurationChangeToken; + public: BugReport(BugType& bt, StringRef desc, const ExplodedNode *errornode) : BT(bt), Description(desc), ErrorNode(errornode), - Callbacks(F.getEmptyList()) {} + ConfigurationChangeToken(0) {} BugReport(BugType& bt, StringRef shortDesc, StringRef desc, const ExplodedNode *errornode) : BT(bt), ShortDescription(shortDesc), Description(desc), - ErrorNode(errornode), Callbacks(F.getEmptyList()) {} + ErrorNode(errornode), ConfigurationChangeToken(0) {} BugReport(BugType& bt, StringRef desc, PathDiagnosticLocation l) : BT(bt), Description(desc), Location(l), ErrorNode(0), - Callbacks(F.getEmptyList()) {} + ConfigurationChangeToken(0) {} /// \brief Create a BugReport with a custom uniqueing location. /// @@ -120,7 +126,7 @@ public: BugReport(BugType& bt, StringRef desc, const ExplodedNode *errornode, PathDiagnosticLocation LocationToUnique) : BT(bt), Description(desc), UniqueingLocation(LocationToUnique), - ErrorNode(errornode), Callbacks(F.getEmptyList()) {} + ErrorNode(errornode), ConfigurationChangeToken(0) {} virtual ~BugReport(); @@ -142,6 +148,10 @@ public: bool isInteresting(SymbolRef sym) const; bool isInteresting(const MemRegion *R) const; bool isInteresting(SVal V) const; + + unsigned getConfigurationChangeToken() const { + return ConfigurationChangeToken; + } /// \brief This allows for addition of meta data to the diagnostic. /// diff --git a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 8f4502ca31..f5218746be 100644 --- a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -875,10 +875,6 @@ void MallocChecker::reportLeak(SymbolRef Sym, ExplodedNode *N, BugReport *R = new BugReport(*BT_Leak, os.str(), N, LocUsedForUniqueing); R->markInteresting(Sym); - // FIXME: This is a hack to make sure the MallocBugVisitor gets to look at - // the ExplodedNode chain first, in order to mark any failed realloc symbols - // as interesting for ConditionBRVisitor. - R->addVisitor(new ConditionBRVisitor()); R->addVisitor(new MallocBugVisitor(Sym)); C.EmitReport(R); } diff --git a/lib/StaticAnalyzer/Core/BugReporter.cpp b/lib/StaticAnalyzer/Core/BugReporter.cpp index 67dfcf51d2..643469d7e5 100644 --- a/lib/StaticAnalyzer/Core/BugReporter.cpp +++ b/lib/StaticAnalyzer/Core/BugReporter.cpp @@ -410,7 +410,8 @@ static void CompactPathDiagnostic(PathPieces &path, const SourceManager& SM); static void GenerateMinimalPathDiagnostic(PathDiagnostic& PD, PathDiagnosticBuilder &PDB, - const ExplodedNode *N) { + const ExplodedNode *N, + ArrayRef visitors) { SourceManager& SMgr = PDB.getSourceManager(); const LocationContext *LC = PDB.LC; @@ -712,8 +713,9 @@ static void GenerateMinimalPathDiagnostic(PathDiagnostic& PD, if (NextNode) { // Add diagnostic pieces from custom visitors. BugReport *R = PDB.getBugReport(); - for (BugReport::visitor_iterator I = R->visitor_begin(), - E = R->visitor_end(); I!=E; ++I) { + for (ArrayRef::iterator I = visitors.begin(), + E = visitors.end(); + I != E; ++I) { if (PathDiagnosticPiece *p = (*I)->VisitNode(N, NextNode, PDB, *R)) { PD.getActivePath().push_front(p); updateStackPiecesWithMessage(p, CallStack); @@ -1051,7 +1053,8 @@ void EdgeBuilder::addContext(const Stmt *S) { static void GenerateExtensivePathDiagnostic(PathDiagnostic& PD, PathDiagnosticBuilder &PDB, - const ExplodedNode *N) { + const ExplodedNode *N, + ArrayRef visitors) { EdgeBuilder EB(PD, PDB); const SourceManager& SM = PDB.getSourceManager(); StackDiagVector CallStack; @@ -1183,8 +1186,9 @@ static void GenerateExtensivePathDiagnostic(PathDiagnostic& PD, // Add pieces from custom visitors. BugReport *R = PDB.getBugReport(); - for (BugReport::visitor_iterator I = R->visitor_begin(), - E = R->visitor_end(); I!=E; ++I) { + for (ArrayRef::iterator I = visitors.begin(), + E = visitors.end(); + I != E; ++I) { if (PathDiagnosticPiece *p = (*I)->VisitNode(N, NextNode, PDB, *R)) { const PathDiagnosticLocation &Loc = p->getLocation(); EB.addEdge(Loc, true); @@ -1227,7 +1231,8 @@ void BugReport::addVisitor(BugReporterVisitor* visitor) { } CallbacksSet.InsertNode(visitor, InsertPos); - Callbacks = F.add(visitor, Callbacks); + Callbacks.push_back(visitor); + ++ConfigurationChangeToken; } BugReport::~BugReport() { @@ -1261,7 +1266,10 @@ void BugReport::Profile(llvm::FoldingSetNodeID& hash) const { void BugReport::markInteresting(SymbolRef sym) { if (!sym) return; - interestingSymbols.insert(sym); + + // If the symbol wasn't already in our set, note a configuration change. + if (interestingSymbols.insert(sym).second) + ++ConfigurationChangeToken; if (const SymbolMetadata *meta = dyn_cast(sym)) interestingRegions.insert(meta->getRegion()); @@ -1270,8 +1278,11 @@ void BugReport::markInteresting(SymbolRef sym) { void BugReport::markInteresting(const MemRegion *R) { if (!R) return; + + // If the base region wasn't already in our set, note a configuration change. R = R->getBaseRegion(); - interestingRegions.insert(R); + if (interestingRegions.insert(R).second) + ++ConfigurationChangeToken; if (const SymbolicRegion *SR = dyn_cast(R)) interestingSymbols.insert(SR->getSymbol()); @@ -1696,33 +1707,57 @@ void GRBugReporter::GeneratePathDiagnostic(PathDiagnostic& PD, R->addVisitor(new NilReceiverBRVisitor()); R->addVisitor(new ConditionBRVisitor()); - // Generate the very last diagnostic piece - the piece is visible before - // the trace is expanded. - PathDiagnosticPiece *LastPiece = 0; - for (BugReport::visitor_iterator I = R->visitor_begin(), - E = R->visitor_end(); I!=E; ++I) { - if (PathDiagnosticPiece *Piece = (*I)->getEndPath(PDB, N, *R)) { - assert (!LastPiece && - "There can only be one final piece in a diagnostic."); - LastPiece = Piece; + BugReport::VisitorList visitors; + unsigned originalReportConfigToken, finalReportConfigToken; + + // While generating diagnostics, it's possible the visitors will decide + // new symbols and regions are interesting, or add other visitors based on + // the information they find. If they do, we need to regenerate the path + // based on our new report configuration. + do { + // Get a clean copy of all the visitors. + for (BugReport::visitor_iterator I = R->visitor_begin(), + E = R->visitor_end(); I != E; ++I) + visitors.push_back((*I)->clone()); + + // Clear out the active path from any previous work. + PD.getActivePath().clear(); + originalReportConfigToken = R->getConfigurationChangeToken(); + + // Generate the very last diagnostic piece - the piece is visible before + // the trace is expanded. + PathDiagnosticPiece *LastPiece = 0; + for (BugReport::visitor_iterator I = visitors.begin(), E = visitors.end(); + I != E; ++I) { + if (PathDiagnosticPiece *Piece = (*I)->getEndPath(PDB, N, *R)) { + assert (!LastPiece && + "There can only be one final piece in a diagnostic."); + LastPiece = Piece; + } } - } - if (!LastPiece) - LastPiece = BugReporterVisitor::getDefaultEndPath(PDB, N, *R); - if (LastPiece) - PD.getActivePath().push_back(LastPiece); - else - return; + if (!LastPiece) + LastPiece = BugReporterVisitor::getDefaultEndPath(PDB, N, *R); + if (LastPiece) + PD.getActivePath().push_back(LastPiece); + else + return; - switch (PDB.getGenerationScheme()) { + switch (PDB.getGenerationScheme()) { case PathDiagnosticConsumer::Extensive: - GenerateExtensivePathDiagnostic(PD, PDB, N); + GenerateExtensivePathDiagnostic(PD, PDB, N, visitors); break; case PathDiagnosticConsumer::Minimal: - GenerateMinimalPathDiagnostic(PD, PDB, N); + GenerateMinimalPathDiagnostic(PD, PDB, N, visitors); break; - } - + } + + // Clean up the visitors we used. + llvm::DeleteContainerPointers(visitors); + + // Did anything change while generating this path? + finalReportConfigToken = R->getConfigurationChangeToken(); + } while(finalReportConfigToken != originalReportConfigToken); + // Finally, prune the diagnostic path of uninteresting stuff. bool hasSomethingInteresting = RemoveUneededCalls(PD.getMutablePieces()); assert(hasSomethingInteresting); -- 2.40.0