From 8966bc1c8ce271c09936c0eaf6c841aef4a0af1b Mon Sep 17 00:00:00 2001 From: Ted Kremenek Date: Wed, 6 May 2009 21:39:49 +0000 Subject: [PATCH] Refactor BugReporter interface to have a new 'BugReporterContext' and 'BugReporterVisitor'. This simplifies callbacks from BugReporter to BugReports (via VisitNode). It also lays the foundation for arbitrary visitor "call backs" that can be registered to a BugReporterContext as a PathDiagnostic is constructed. These call backs can help operate as separate "experts" that can work on constructed pieces of a PathDiagnostic for which they possess special knowledge. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@71121 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Analysis/PathSensitive/BugReporter.h | 83 +++++++++++- lib/Analysis/BugReporter.cpp | 119 ++++++++---------- lib/Analysis/CFRefCount.cpp | 53 ++++---- 3 files changed, 157 insertions(+), 98 deletions(-) diff --git a/include/clang/Analysis/PathSensitive/BugReporter.h b/include/clang/Analysis/PathSensitive/BugReporter.h index f941a7a570..99cbfd844b 100644 --- a/include/clang/Analysis/PathSensitive/BugReporter.h +++ b/include/clang/Analysis/PathSensitive/BugReporter.h @@ -34,6 +34,7 @@ class PathDiagnosticClient; class ASTContext; class Diagnostic; class BugReporter; +class BugReporterContext; class GRExprEngine; class GRState; class Stmt; @@ -43,9 +44,19 @@ class ParentMap; //===----------------------------------------------------------------------===// // Interface for individual bug reports. //===----------------------------------------------------------------------===// + +class BugReporterVisitor { +public: + virtual ~BugReporterVisitor(); + virtual PathDiagnosticPiece* VisitNode(const ExplodedNode* N, + const ExplodedNode* PrevN, + BugReporterContext& BR) = 0; + + virtual bool isOwnedByReporterContext() { return true; } +}; // FIXME: Combine this with RangedBugReport and remove RangedBugReport. -class BugReport { +class BugReport : public BugReporterVisitor { protected: BugType& BT; std::string ShortDescription; @@ -76,8 +87,9 @@ public: const ExplodedNode *n) : BT(bt), ShortDescription(shortDesc), Description(desc), EndNode(n) {} - virtual ~BugReport(); + + virtual bool isOwnedByReporterContext() { return false; } const BugType& getBugType() const { return BT; } BugType& getBugType() { return BT; } @@ -87,6 +99,8 @@ public: // FIXME: Do we need this? Maybe getLocation() should return a ProgramPoint // object. + // FIXME: If we do need it, we can probably just make it private to + // BugReporter. Stmt* getStmt(BugReporter& BR) const; const std::string& getDescription() const { return Description; } @@ -101,7 +115,7 @@ public: } // FIXME: Perhaps move this into a subclass. - virtual PathDiagnosticPiece* getEndPath(BugReporter& BR, + virtual PathDiagnosticPiece* getEndPath(BugReporterContext& BR, const ExplodedNode* N); /// getLocation - Return the "definitive" location of the reported bug. @@ -114,12 +128,17 @@ public: virtual void getRanges(BugReporter& BR,const SourceRange*& beg, const SourceRange*& end); - // FIXME: Perhaps this should be moved into a subclass? + virtual PathDiagnosticPiece* VisitNode(const ExplodedNode* N, + const ExplodedNode* PrevN, + BugReporterContext& BR); + + /* virtual PathDiagnosticPiece* VisitNode(const ExplodedNode* N, const ExplodedNode* PrevN, const ExplodedGraph& G, - BugReporter& BR, + BugReporterContext& BR, NodeResolver& NR); + */ }; //===----------------------------------------------------------------------===// @@ -332,7 +351,7 @@ public: static bool classof(const BugReporter* R) { return true; } }; - + // FIXME: Get rid of GRBugReporter. It's the wrong abstraction. class GRBugReporter : public BugReporter { GRExprEngine& Eng; @@ -371,6 +390,58 @@ public: return R->getKind() == GRBugReporterKind; } }; + +class BugReporterContext { + GRBugReporter &BR; + std::vector Callbacks; +public: + BugReporterContext(GRBugReporter& br) : BR(br) {} + virtual ~BugReporterContext(); + + void addVisitor(BugReporterVisitor* visitor) { + if (visitor) Callbacks.push_back(visitor); + } + + typedef std::vector::iterator visitor_iterator; + visitor_iterator visitor_begin() { return Callbacks.begin(); } + visitor_iterator visitor_end() { return Callbacks.end(); } + + GRBugReporter& getBugReporter() { return BR; } + + ExplodedGraph& getGraph() { return BR.getGraph(); } + + void addNotableSymbol(SymbolRef Sym) { + // FIXME: For now forward to GRBugReporter. + BR.addNotableSymbol(Sym); + } + + bool isNotable(SymbolRef Sym) const { + // FIXME: For now forward to GRBugReporter. + return BR.isNotable(Sym); + } + + GRStateManager& getStateManager() { + return BR.getStateManager(); + } + + ASTContext& getASTContext() { + return BR.getContext(); + } + + SourceManager& getSourceManager() { + return BR.getSourceManager(); + } + + const Decl& getCodeDecl() { + return getStateManager().getCodeDecl(); + } + + const CFG& getCFG() { + return *BR.getCFG(); + } + + virtual BugReport::NodeResolver& getNodeResolver() = 0; +}; class DiagBugReport : public RangedBugReport { std::list Strs; diff --git a/lib/Analysis/BugReporter.cpp b/lib/Analysis/BugReporter.cpp index 77a83fef8c..36dc6b75ee 100644 --- a/lib/Analysis/BugReporter.cpp +++ b/lib/Analysis/BugReporter.cpp @@ -30,6 +30,12 @@ using namespace clang; +BugReporterVisitor::~BugReporterVisitor() {} +BugReporterContext::~BugReporterContext() { + for (visitor_iterator I = visitor_begin(), E = visitor_end(); I != E; ++I) + if ((*I)->isOwnedByReporterContext()) delete *I; +} + //===----------------------------------------------------------------------===// // Helper routines for walking the ExplodedGraph and fetching statements. //===----------------------------------------------------------------------===// @@ -118,21 +124,20 @@ public: } }; -class VISIBILITY_HIDDEN PathDiagnosticBuilder { - GRBugReporter &BR; - SourceManager &SMgr; - ExplodedGraph *ReportGraph; +class VISIBILITY_HIDDEN PathDiagnosticBuilder : public BugReporterContext { BugReport *R; - const Decl& CodeDecl; PathDiagnosticClient *PDC; llvm::OwningPtr PM; NodeMapClosure NMC; public: - PathDiagnosticBuilder(GRBugReporter &br, ExplodedGraph *reportGraph, + PathDiagnosticBuilder(GRBugReporter &br, BugReport *r, NodeBackMap *Backmap, - const Decl& codedecl, PathDiagnosticClient *pdc) - : BR(br), SMgr(BR.getSourceManager()), ReportGraph(reportGraph), R(r), - CodeDecl(codedecl), PDC(pdc), NMC(Backmap) {} + PathDiagnosticClient *pdc) + : BugReporterContext(br), + R(r), PDC(pdc), NMC(Backmap) + { + addVisitor(R); + } PathDiagnosticLocation ExecutionContinues(const ExplodedNode* N); @@ -140,29 +145,17 @@ public: const ExplodedNode* N); ParentMap& getParentMap() { - if (PM.get() == 0) PM.reset(new ParentMap(CodeDecl.getBody(getContext()))); + if (PM.get() == 0) + PM.reset(new ParentMap(getCodeDecl().getBody(getASTContext()))); return *PM.get(); } const Stmt *getParent(const Stmt *S) { return getParentMap().getParent(S); } - - const CFG& getCFG() { - return *BR.getCFG(); - } - - const Decl& getCodeDecl() { - return BR.getStateManager().getCodeDecl(); - } - - ExplodedGraph& getGraph() { return *ReportGraph; } - NodeMapClosure& getNodeMapClosure() { return NMC; } - ASTContext& getContext() { return BR.getContext(); } - SourceManager& getSourceManager() { return SMgr; } + + virtual NodeMapClosure& getNodeResolver() { return NMC; } BugReport& getReport() { return *R; } - GRBugReporter& getBugReporter() { return BR; } - GRStateManager& getStateManager() { return BR.getStateManager(); } PathDiagnosticLocation getEnclosingStmtLocation(const Stmt *S); @@ -187,9 +180,10 @@ public: PathDiagnosticLocation PathDiagnosticBuilder::ExecutionContinues(const ExplodedNode* N) { if (Stmt *S = GetNextStmt(N)) - return PathDiagnosticLocation(S, SMgr); + return PathDiagnosticLocation(S, getSourceManager()); - return FullSourceLoc(CodeDecl.getBodyRBrace(getContext()), SMgr); + return FullSourceLoc(getCodeDecl().getBodyRBrace(getASTContext()), + getSourceManager()); } PathDiagnosticLocation @@ -204,10 +198,11 @@ PathDiagnosticBuilder::ExecutionContinues(llvm::raw_string_ostream& os, if (Loc.asStmt()) os << "Execution continues on line " - << SMgr.getInstantiationLineNumber(Loc.asLocation()) << '.'; + << getSourceManager().getInstantiationLineNumber(Loc.asLocation()) + << '.'; else os << "Execution jumps to the end of the " - << (isa(CodeDecl) ? "method" : "function") << '.'; + << (isa(getCodeDecl()) ? "method" : "function") << '.'; return Loc; } @@ -216,6 +211,7 @@ PathDiagnosticLocation PathDiagnosticBuilder::getEnclosingStmtLocation(const Stmt *S) { assert(S && "Null Stmt* passed to getEnclosingStmtLocation"); ParentMap &P = getParentMap(); + SourceManager &SMgr = getSourceManager(); while (isa(S) && P.isConsumedExpr(cast(S))) { const Stmt *Parent = P.getParent(S); @@ -459,7 +455,7 @@ static void CompactPathDiagnostic(PathDiagnostic &PD, const SourceManager& SM); static void GenerateMinimalPathDiagnostic(PathDiagnostic& PD, PathDiagnosticBuilder &PDB, const ExplodedNode *N) { - ASTContext& Ctx = PDB.getContext(); + SourceManager& SMgr = PDB.getSourceManager(); const ExplodedNode* NextNode = N->pred_empty() ? NULL : *(N->pred_begin()); @@ -541,7 +537,7 @@ static void GenerateMinimalPathDiagnostic(PathDiagnostic& PD, } if (GetRawInt) - os << LHS->EvaluateAsInt(Ctx); + os << LHS->EvaluateAsInt(PDB.getASTContext()); os << ":' at line " << End.asLocation().getInstantiationLineNumber(); @@ -714,12 +710,11 @@ static void GenerateMinimalPathDiagnostic(PathDiagnostic& PD, } } - if (PathDiagnosticPiece* p = - PDB.getReport().VisitNode(N, NextNode, PDB.getGraph(), - PDB.getBugReporter(), - PDB.getNodeMapClosure())) { - PD.push_front(p); - } + for (BugReporterContext::visitor_iterator I = PDB.visitor_begin(), + E = PDB.visitor_end(); I!=E; ++I) { + if (PathDiagnosticPiece* p = (*I)->VisitNode(N, NextNode, PDB)) + PD.push_front(p); + } if (const PostStmt* PS = dyn_cast(&P)) { // Scan the region bindings, and see if a "notable" symbol has a new @@ -834,7 +829,7 @@ public: // statement (if it doesn't already exist). // FIXME: Should handle CXXTryStmt if analyser starts supporting C++. if (const CompoundStmt *CS = - PDB.getCodeDecl().getCompoundBody(PDB.getContext())) + PDB.getCodeDecl().getCompoundBody(PDB.getASTContext())) if (!CS->body_empty()) { SourceLocation Loc = (*CS->body_begin())->getLocStart(); rawAddEdge(PathDiagnosticLocation(Loc, PDB.getSourceManager())); @@ -1105,17 +1100,16 @@ static void GenerateExtensivePathDiagnostic(PathDiagnostic& PD, continue; } - PathDiagnosticPiece* p = - PDB.getReport().VisitNode(N, NextNode, PDB.getGraph(), - PDB.getBugReporter(), PDB.getNodeMapClosure()); - - if (p) { - const PathDiagnosticLocation &Loc = p->getLocation(); - EB.addEdge(Loc, true); - PD.push_front(p); - if (const Stmt *S = Loc.asStmt()) - EB.addExtendedContext(PDB.getEnclosingStmtLocation(S).asStmt()); - } + for (BugReporterContext::visitor_iterator I = PDB.visitor_begin(), + E = PDB.visitor_end(); I!=E; ++I) { + if (PathDiagnosticPiece* p = (*I)->VisitNode(N, NextNode, PDB)) { + const PathDiagnosticLocation &Loc = p->getLocation(); + EB.addEdge(Loc, true); + PD.push_front(p); + if (const Stmt *S = Loc.asStmt()) + EB.addExtendedContext(PDB.getEnclosingStmtLocation(S).asStmt()); + } + } } } @@ -1144,19 +1138,19 @@ Stmt* BugReport::getStmt(BugReporter& BR) const { } PathDiagnosticPiece* -BugReport::getEndPath(BugReporter& BR, +BugReport::getEndPath(BugReporterContext& BRC, const ExplodedNode* EndPathNode) { - Stmt* S = getStmt(BR); + Stmt* S = getStmt(BRC.getBugReporter()); if (!S) return NULL; - FullSourceLoc L(S->getLocStart(), BR.getContext().getSourceManager()); + FullSourceLoc L(S->getLocStart(), BRC.getSourceManager()); PathDiagnosticPiece* P = new PathDiagnosticEventPiece(L, getDescription()); const SourceRange *Beg, *End; - getRanges(BR, Beg, End); + getRanges(BRC.getBugReporter(), Beg, End); for (; Beg != End; ++Beg) P->addRange(*Beg); @@ -1192,9 +1186,7 @@ SourceLocation BugReport::getLocation() const { PathDiagnosticPiece* BugReport::VisitNode(const ExplodedNode* N, const ExplodedNode* PrevN, - const ExplodedGraph& G, - BugReporter& BR, - NodeResolver &NR) { + BugReporterContext &BRC) { return NULL; } @@ -1203,7 +1195,7 @@ PathDiagnosticPiece* BugReport::VisitNode(const ExplodedNode* N, //===----------------------------------------------------------------------===// BugReportEquivClass::~BugReportEquivClass() { - for (iterator I=begin(), E=end(); I!=E; ++I) delete *I; + for (iterator I=begin(), E=end(); I!=E; ++I) delete *I; } GRBugReporter::~GRBugReporter() { FlushReports(); } @@ -1477,7 +1469,7 @@ static void CompactPathDiagnostic(PathDiagnostic &PD, const SourceManager& SM) { } void GRBugReporter::GeneratePathDiagnostic(PathDiagnostic& PD, - BugReportEquivClass& EQ) { + BugReportEquivClass& EQ) { std::vector*> Nodes; @@ -1507,19 +1499,18 @@ void GRBugReporter::GeneratePathDiagnostic(PathDiagnostic& PD, llvm::OwningPtr BackMap(GPair.first.second); const ExplodedNode *N = GPair.second.first; - // Start building the path diagnostic... - if (PathDiagnosticPiece* Piece = R->getEndPath(*this, N)) + // Start building the path diagnostic... + PathDiagnosticBuilder PDB(*this, R, BackMap.get(), getPathDiagnosticClient()); + + if (PathDiagnosticPiece* Piece = R->getEndPath(PDB, N)) PD.push_back(Piece); else return; - PathDiagnosticBuilder PDB(*this, ReportGraph.get(), R, BackMap.get(), - getStateManager().getCodeDecl(), - getPathDiagnosticClient()); switch (PDB.getGenerationScheme()) { case PathDiagnosticClient::Extensive: - GenerateExtensivePathDiagnostic(PD,PDB, N); + GenerateExtensivePathDiagnostic(PD, PDB, N); break; case PathDiagnosticClient::Minimal: GenerateMinimalPathDiagnostic(PD, PDB, N); diff --git a/lib/Analysis/CFRefCount.cpp b/lib/Analysis/CFRefCount.cpp index 4c517fd537..7cf69e13a2 100644 --- a/lib/Analysis/CFRefCount.cpp +++ b/lib/Analysis/CFRefCount.cpp @@ -22,7 +22,7 @@ #include "clang/Analysis/PathDiagnostic.h" #include "clang/Analysis/PathSensitive/BugReporter.h" #include "clang/Analysis/PathSensitive/SymbolManager.h" -#include "clang/AST/DeclObjC.h" +#include "clang/AST/DeclObjC.h" #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/FoldingSet.h" #include "llvm/ADT/ImmutableMap.h" @@ -1975,16 +1975,14 @@ namespace { SymbolRef getSymbol() const { return Sym; } - PathDiagnosticPiece* getEndPath(BugReporter& BR, + PathDiagnosticPiece* getEndPath(BugReporterContext& BRC, const ExplodedNode* N); std::pair getExtraDescriptiveText(); PathDiagnosticPiece* VisitNode(const ExplodedNode* N, const ExplodedNode* PrevN, - const ExplodedGraph& G, - BugReporter& BR, - NodeResolver& NR); + BugReporterContext& BRC); }; class VISIBILITY_HIDDEN CFRefLeakReport : public CFRefReport { @@ -1995,7 +1993,7 @@ namespace { ExplodedNode *n, SymbolRef sym, GRExprEngine& Eng); - PathDiagnosticPiece* getEndPath(BugReporter& BR, + PathDiagnosticPiece* getEndPath(BugReporterContext& BRC, const ExplodedNode* N); SourceLocation getLocation() const { return AllocSite; } @@ -2096,12 +2094,10 @@ static inline bool contains(const llvm::SmallVectorImpl& V, PathDiagnosticPiece* CFRefReport::VisitNode(const ExplodedNode* N, const ExplodedNode* PrevN, - const ExplodedGraph& G, - BugReporter& BR, - NodeResolver& NR) { + BugReporterContext& BRC) { - // Check if the type state has changed. - GRStateManager &StMgr = cast(BR).getStateManager(); + // Check if the type state has changed. + GRStateManager &StMgr = BRC.getStateManager(); GRStateRef PrevSt(PrevN->getState(), StMgr); GRStateRef CurrSt(N->getState(), StMgr); @@ -2156,7 +2152,7 @@ PathDiagnosticPiece* CFRefReport::VisitNode(const ExplodedNode* N, os << "+0 retain count (non-owning reference)."; } - PathDiagnosticLocation Pos(S, BR.getContext().getSourceManager()); + PathDiagnosticLocation Pos(S, BRC.getSourceManager()); return new PathDiagnosticEventPiece(Pos, os.str()); } @@ -2164,7 +2160,8 @@ PathDiagnosticPiece* CFRefReport::VisitNode(const ExplodedNode* N, // program point llvm::SmallVector AEffects; - if (const RetainSummary *Summ = TF.getSummaryOfNode(NR.getOriginalNode(N))) { + if (const RetainSummary *Summ = + TF.getSummaryOfNode(BRC.getNodeResolver().getOriginalNode(N))) { // We only have summaries attached to nodes after evaluating CallExpr and // ObjCMessageExprs. Stmt* S = cast(N->getLocation()).getStmt(); @@ -2313,7 +2310,7 @@ PathDiagnosticPiece* CFRefReport::VisitNode(const ExplodedNode* N, return 0; // We have nothing to say! Stmt* S = cast(N->getLocation()).getStmt(); - PathDiagnosticLocation Pos(S, BR.getContext().getSourceManager()); + PathDiagnosticLocation Pos(S, BRC.getSourceManager()); PathDiagnosticPiece* P = new PathDiagnosticEventPiece(Pos, os.str()); // Add the range by scanning the children of the statement for any bindings @@ -2388,21 +2385,21 @@ GetAllocationSite(GRStateManager& StateMgr, const ExplodedNode* N, } PathDiagnosticPiece* -CFRefReport::getEndPath(BugReporter& br, const ExplodedNode* EndN) { - // Tell the BugReporter to report cases when the tracked symbol is +CFRefReport::getEndPath(BugReporterContext& BRC, + const ExplodedNode* EndN) { + // Tell the BugReporterContext to report cases when the tracked symbol is // assigned to different variables, etc. - GRBugReporter& BR = cast(br); - cast(BR).addNotableSymbol(Sym); - return RangedBugReport::getEndPath(BR, EndN); + BRC.addNotableSymbol(Sym); + return RangedBugReport::getEndPath(BRC, EndN); } PathDiagnosticPiece* -CFRefLeakReport::getEndPath(BugReporter& br, const ExplodedNode* EndN){ +CFRefLeakReport::getEndPath(BugReporterContext& BRC, + const ExplodedNode* EndN){ - GRBugReporter& BR = cast(br); - // Tell the BugReporter to report cases when the tracked symbol is + // Tell the BugReporterContext to report cases when the tracked symbol is // assigned to different variables, etc. - cast(BR).addNotableSymbol(Sym); + BRC.addNotableSymbol(Sym); // We are reporting a leak. Walk up the graph to get to the first node where // the symbol appeared, and also get the first VarDecl that tracked object @@ -2411,13 +2408,13 @@ CFRefLeakReport::getEndPath(BugReporter& br, const ExplodedNode* EndN){ const MemRegion* FirstBinding = 0; llvm::tie(AllocNode, FirstBinding) = - GetAllocationSite(BR.getStateManager(), EndN, Sym); + GetAllocationSite(BRC.getStateManager(), EndN, Sym); // Get the allocate site. assert(AllocNode); Stmt* FirstStmt = cast(AllocNode->getLocation()).getStmt(); - SourceManager& SMgr = BR.getContext().getSourceManager(); + SourceManager& SMgr = BRC.getSourceManager(); unsigned AllocLine =SMgr.getInstantiationLineNumber(FirstStmt->getLocStart()); // Compute an actual location for the leak. Sometimes a leak doesn't @@ -2444,8 +2441,8 @@ CFRefLeakReport::getEndPath(BugReporter& br, const ExplodedNode* EndN){ } if (!L.isValid()) { - const Decl &D = BR.getStateManager().getCodeDecl(); - L = PathDiagnosticLocation(D.getBodyRBrace(BR.getContext()), SMgr); + const Decl &D = BRC.getCodeDecl(); + L = PathDiagnosticLocation(D.getBodyRBrace(BRC.getASTContext()), SMgr); } std::string sbuf; @@ -2463,7 +2460,7 @@ CFRefLeakReport::getEndPath(BugReporter& br, const ExplodedNode* EndN){ // FIXME: Per comments in rdar://6320065, "create" only applies to CF // ojbects. Only "copy", "alloc", "retain" and "new" transfer ownership // to the caller for NS objects. - ObjCMethodDecl& MD = cast(BR.getGraph().getCodeDecl()); + ObjCMethodDecl& MD = cast(BRC.getCodeDecl()); os << " is returned from a method whose name ('" << MD.getSelector().getAsString() << "') does not contain 'copy' or otherwise starts with" -- 2.40.0