Index: include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h =================================================================== --- include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h (revision 152998) +++ include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h (working copy) @@ -61,7 +61,8 @@ }; 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 @@ /// 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 @@ 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 @@ 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. /// Index: include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h =================================================================== --- include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h (revision 152998) +++ include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h (working copy) @@ -32,6 +32,13 @@ public: virtual ~BugReporterVisitor(); + /// \brief Returns a copy of this BugReporter. + /// + /// Custom BugReporterVisitors should not override this method directly. + /// Instead, they should inherit from BugReporterVisitorImpl and provide + /// a protected copy constructor. + virtual BugReporterVisitor *clone() const = 0; + /// \brief Return a diagnostic piece which should be associated with the /// given node. /// @@ -61,7 +68,16 @@ }; -class FindLastStoreBRVisitor : public BugReporterVisitor { +template +class BugReporterVisitorImpl : public BugReporterVisitor { + virtual BugReporterVisitor *clone() const { + return new DERIVED(*(DERIVED *)this); + } +}; + +class FindLastStoreBRVisitor + : public BugReporterVisitorImpl +{ const MemRegion *R; SVal V; bool satisfied; @@ -94,7 +110,9 @@ BugReport &BR); }; -class TrackConstraintBRVisitor : public BugReporterVisitor { +class TrackConstraintBRVisitor + : public BugReporterVisitorImpl +{ DefinedSVal Constraint; const bool Assumption; bool isSatisfied; @@ -111,7 +129,9 @@ BugReport &BR); }; -class NilReceiverBRVisitor : public BugReporterVisitor { +class NilReceiverBRVisitor + : public BugReporterVisitorImpl +{ public: void Profile(llvm::FoldingSetNodeID &ID) const { static int x = 0; @@ -125,7 +145,7 @@ }; /// Visitor that tries to report interesting diagnostics from conditions. -class ConditionBRVisitor : public BugReporterVisitor { +class ConditionBRVisitor : public BugReporterVisitorImpl { public: void Profile(llvm::FoldingSetNodeID &ID) const { static int x = 0; Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/MallocChecker.cpp (revision 153010) +++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp (working copy) @@ -193,20 +193,26 @@ /// The bug visitor which allows us to print extra diagnostics along the /// BugReport path. For example, showing the allocation site of the leaked /// region. - class MallocBugVisitor : public BugReporterVisitor { + class MallocBugVisitor : public BugReporterVisitorImpl { protected: enum NotificationMode { Normal, - Complete, ReallocationFailed }; // The allocated region symbol tracked by the main analysis. SymbolRef Sym; + + // The mode we are in, i.e. what kind of diagnostics will be emitted. NotificationMode Mode; + // A symbol from when the primary region should have been reallocated. + SymbolRef FailedReallocSymbol; + public: - MallocBugVisitor(SymbolRef S) : Sym(S), Mode(Normal) {} + MallocBugVisitor(SymbolRef S) + : Sym(S), Mode(Normal), FailedReallocSymbol(0) {} + virtual ~MallocBugVisitor() {} void Profile(llvm::FoldingSetNodeID &ID) const { @@ -846,10 +852,6 @@ BugReport *R = new BugReport(*BT_Leak, "Memory is never released; potential memory leak", 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); } @@ -1325,30 +1327,31 @@ StackHint = new StackHintGeneratorForReallocationFailed(Sym, "Reallocation failed"); - if (SymbolRef sym = findFailedReallocSymbol(state, statePrev)) + if (SymbolRef sym = findFailedReallocSymbol(state, statePrev)) { + // Is it possible to fail two reallocs WITHOUT testing in between? + assert((!FailedReallocSymbol || FailedReallocSymbol == sym) && + "We only support one failed realloc at a time."); BR.markInteresting(sym); + FailedReallocSymbol = sym; + } } // We are in a special mode if a reallocation failed later in the path. } else if (Mode == ReallocationFailed) { - // Generate a special diagnostic for the first realloc we find. - if (!isAllocated(RS, RSPrev, S) && !isReleased(RS, RSPrev, S)) - return 0; + // Is this is the first appearance of the reallocated symbol? + if (!statePrev->get(FailedReallocSymbol)) { + // If we ever hit this assert, that means BugReporter has decided to skip + // node pairs or visit them out of order. + assert(state->get(FailedReallocSymbol) && + "Missed the reallocation point"); - // Check that the name of the function is realloc. - const CallExpr *CE = dyn_cast(S); - if (!CE) - return 0; - const FunctionDecl *funDecl = CE->getDirectCallee(); - if (!funDecl) - return 0; - StringRef FunName = funDecl->getName(); - if (!(FunName.equals("realloc") || FunName.equals("reallocf"))) - return 0; - Msg = "Attempt to reallocate memory"; - StackHint = new StackHintGeneratorForSymbol(Sym, - "Returned reallocated memory"); - Mode = Normal; + // We're at the reallocation point. + Msg = "Attempt to reallocate memory"; + StackHint = new StackHintGeneratorForSymbol(Sym, + "Returned reallocated memory"); + FailedReallocSymbol = NULL; + Mode = Normal; + } } if (!Msg) Index: lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp (revision 152998) +++ lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp (working copy) @@ -131,7 +131,8 @@ /// The bug visitor which allows us to print extra diagnostics along the /// BugReport path. For example, showing the allocation site of the leaked /// region. - class SecKeychainBugVisitor : public BugReporterVisitor { + class SecKeychainBugVisitor + : public BugReporterVisitorImpl { protected: // The allocated region symbol tracked by the main analysis. SymbolRef Sym; Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp (revision 153007) +++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp (working copy) @@ -1705,7 +1705,7 @@ // Bug Reports. // //===---------===// - class CFRefReportVisitor : public BugReporterVisitor { + class CFRefReportVisitor : public BugReporterVisitorImpl { protected: SymbolRef Sym; const SummaryLogTy &SummaryLog; @@ -1740,6 +1740,15 @@ PathDiagnosticPiece *getEndPath(BugReporterContext &BRC, const ExplodedNode *N, BugReport &BR); + + virtual BugReporterVisitor *clone() const { + // The curiously-recurring template pattern only works for one level of + // subclassing. Rather than make a new template base for + // CFRefReportVisitor, we simply override clone() to do the right thing. + // This could be trouble someday if BugReporterVisitorImpl is ever + // used for something else besides a convenient implementation of clone(). + return new CFRefLeakReportVisitor(*this); + } }; class CFRefReport : public BugReport { Index: lib/StaticAnalyzer/Core/BugReporter.cpp =================================================================== --- lib/StaticAnalyzer/Core/BugReporter.cpp (revision 152998) +++ lib/StaticAnalyzer/Core/BugReporter.cpp (working copy) @@ -410,7 +410,8 @@ 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 @@ 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 @@ 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 @@ // 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 @@ } CallbacksSet.InsertNode(visitor, InsertPos); - Callbacks = F.add(visitor, Callbacks); + Callbacks.push_back(visitor); + ++ConfigurationChangeToken; } BugReport::~BugReport() { @@ -1261,8 +1266,11 @@ 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(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 @@ 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);