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) @@ -142,6 +142,10 @@ bool isInteresting(SymbolRef sym) const; bool isInteresting(const MemRegion *R) const; bool isInteresting(SVal V) const; + + unsigned getInterestingObjectSetDescriptor() const { + return interestingSymbols.size() + interestingRegions.size(); + } /// \brief This allows for addition of meta data to the diagnostic. /// Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/MallocChecker.cpp (revision 153010) +++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp (working copy) @@ -194,19 +194,14 @@ /// BugReport path. For example, showing the allocation site of the leaked /// region. class MallocBugVisitor : public BugReporterVisitor { - protected: - enum NotificationMode { - Normal, - Complete, - ReallocationFailed - }; - // The allocated region symbol tracked by the main analysis. SymbolRef Sym; - 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), FailedReallocSymbol(0) {} virtual ~MallocBugVisitor() {} void Profile(llvm::FoldingSetNodeID &ID) const { @@ -846,10 +841,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); } @@ -1310,45 +1301,50 @@ return 0; // Find out if this is an interesting point and what is the kind. - if (Mode == Normal) { - if (isAllocated(RS, RSPrev, S)) { - Msg = "Memory is allocated"; - StackHint = new StackHintGeneratorForSymbol(Sym, - "Returned allocated memory"); - } else if (isReleased(RS, RSPrev, S)) { - Msg = "Memory is released"; - StackHint = new StackHintGeneratorForSymbol(Sym, - "Returned released memory"); - } else if (isReallocFailedCheck(RS, RSPrev, S)) { - Mode = ReallocationFailed; - Msg = "Reallocation failed"; - StackHint = new StackHintGeneratorForReallocationFailed(Sym, - "Reallocation failed"); + if (isReallocFailedCheck(RS, RSPrev, S)) { + // Note to maintainers: This check MUST come before the special + // "realloc failed mode" case below. BugReporter currently guarantees + // that a visitor will always move from the end to the start of the path, + // but it's possible some node pairs will be visited more than once. + Msg = "Reallocation failed"; + StackHint = new StackHintGeneratorForReallocationFailed(Sym, + "Reallocation failed"); - if (SymbolRef sym = findFailedReallocSymbol(state, statePrev)) - BR.markInteresting(sym); - } + 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."); - // 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. + BR.markInteresting(sym); + FailedReallocSymbol = sym; + } + } else if (FailedReallocSymbol) { + // We are in a special mode if a reallocation failed later in the path. + // Generate a special diagnostic when we find the call to realloc. if (!isAllocated(RS, RSPrev, S) && !isReleased(RS, RSPrev, S)) return 0; - // 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"; + // 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"); + + // We're at the reallocation point. + Msg = "Attempt to reallocate memory"; + StackHint = new StackHintGeneratorForSymbol(Sym, + "Returned reallocated memory"); + FailedReallocSymbol = NULL; + } + } else if (isAllocated(RS, RSPrev, S)) { + Msg = "Memory is allocated"; StackHint = new StackHintGeneratorForSymbol(Sym, - "Returned reallocated memory"); - Mode = Normal; + "Returned allocated memory"); + } else if (isReleased(RS, RSPrev, S)) { + Msg = "Memory is released"; + StackHint = new StackHintGeneratorForSymbol(Sym, + "Returned released memory"); } if (!Msg) Index: lib/StaticAnalyzer/Core/BugReporter.cpp =================================================================== --- lib/StaticAnalyzer/Core/BugReporter.cpp (revision 152998) +++ lib/StaticAnalyzer/Core/BugReporter.cpp (working copy) @@ -406,6 +406,31 @@ } } +typedef llvm::SmallVector CustomPathPieces; +static void GenerateCustomPathPieces(PathDiagnosticBuilder &PDB, + const ExplodedNode *N, + const ExplodedNode *NextNode, + CustomPathPieces &pathPieces) { + BugReport *R = PDB.getBugReport(); + unsigned originalInterestDesc, finalInterestDesc; + + // While generating diagnostics, it's possible the visitors will decide + // new symbols and regions are interesting. If they do, we need to retry + // all visitors again at this node. + do { + llvm::DeleteContainerPointers(pathPieces); + originalInterestDesc = R->getInterestingObjectSetDescriptor(); + + for (BugReport::visitor_iterator I = R->visitor_begin(), + E = R->visitor_end(); I!=E; ++I) { + if (PathDiagnosticPiece *p = (*I)->VisitNode(N, NextNode, PDB, *R)) + pathPieces.push_back(p); + } + + finalInterestDesc = R->getInterestingObjectSetDescriptor(); + } while (finalInterestDesc != originalInterestDesc); +} + static void CompactPathDiagnostic(PathPieces &path, const SourceManager& SM); static void GenerateMinimalPathDiagnostic(PathDiagnostic& PD, @@ -711,13 +736,16 @@ 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) { - if (PathDiagnosticPiece *p = (*I)->VisitNode(N, NextNode, PDB, *R)) { - PD.getActivePath().push_front(p); - updateStackPiecesWithMessage(p, CallStack); - } + CustomPathPieces pathPieces; + GenerateCustomPathPieces(PDB, N, NextNode, pathPieces); + + // Process the final path pieces we came up with. + // We do this in reverse order so that custom visitors' notes appear + // after notes from the default set of visitors added by BugReporter. + for (CustomPathPieces::reverse_iterator I = pathPieces.rbegin(), + E = pathPieces.rend(); I != E; ++I) { + PD.getActivePath().push_front(*I); + updateStackPiecesWithMessage(*I, CallStack); } } } @@ -1181,19 +1209,23 @@ if (!NextNode) continue; - // Add pieces from custom visitors. - BugReport *R = PDB.getBugReport(); - for (BugReport::visitor_iterator I = R->visitor_begin(), - E = R->visitor_end(); I!=E; ++I) { - if (PathDiagnosticPiece *p = (*I)->VisitNode(N, NextNode, PDB, *R)) { - const PathDiagnosticLocation &Loc = p->getLocation(); - EB.addEdge(Loc, true); - PD.getActivePath().push_front(p); - updateStackPiecesWithMessage(p, CallStack); + // Add diagnostic pieces from custom visitors. + CustomPathPieces pathPieces; + GenerateCustomPathPieces(PDB, N, NextNode, pathPieces); - if (const Stmt *S = Loc.asStmt()) - EB.addExtendedContext(PDB.getEnclosingStmtLocation(S).asStmt()); - } + // Process the final path pieces we came up with. + // We do this in reverse order so that custom visitors' notes appear + // after notes from the default set of visitors added by BugReporter. + for (CustomPathPieces::reverse_iterator I = pathPieces.rbegin(), + E = pathPieces.rend(); I != E; ++I) { + PathDiagnosticPiece *p = *I; + const PathDiagnosticLocation &Loc = p->getLocation(); + EB.addEdge(Loc, true); + PD.getActivePath().push_front(p); + updateStackPiecesWithMessage(p, CallStack); + + if (const Stmt *S = Loc.asStmt()) + EB.addExtendedContext(PDB.getEnclosingStmtLocation(S).asStmt()); } } }