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.

llvm-svn: 71121
This commit is contained in:
Ted Kremenek 2009-05-06 21:39:49 +00:00
parent 908a66dc9b
commit bb8d546208
3 changed files with 157 additions and 98 deletions

View File

@ -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<GRState>* N,
const ExplodedNode<GRState>* 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<GRState> *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<GRState>* 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<GRState>* N,
const ExplodedNode<GRState>* PrevN,
BugReporterContext& BR);
/*
virtual PathDiagnosticPiece* VisitNode(const ExplodedNode<GRState>* N,
const ExplodedNode<GRState>* PrevN,
const ExplodedGraph<GRState>& 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<BugReporterVisitor*> Callbacks;
public:
BugReporterContext(GRBugReporter& br) : BR(br) {}
virtual ~BugReporterContext();
void addVisitor(BugReporterVisitor* visitor) {
if (visitor) Callbacks.push_back(visitor);
}
typedef std::vector<BugReporterVisitor*>::iterator visitor_iterator;
visitor_iterator visitor_begin() { return Callbacks.begin(); }
visitor_iterator visitor_end() { return Callbacks.end(); }
GRBugReporter& getBugReporter() { return BR; }
ExplodedGraph<GRState>& 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<std::string> Strs;

View File

@ -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<GRState> *ReportGraph;
class VISIBILITY_HIDDEN PathDiagnosticBuilder : public BugReporterContext {
BugReport *R;
const Decl& CodeDecl;
PathDiagnosticClient *PDC;
llvm::OwningPtr<ParentMap> PM;
NodeMapClosure NMC;
public:
PathDiagnosticBuilder(GRBugReporter &br, ExplodedGraph<GRState> *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<GRState>* N);
@ -140,29 +145,17 @@ public:
const ExplodedNode<GRState>* 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<GRState>& 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<GRState>* 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<ObjCMethodDecl>(CodeDecl) ? "method" : "function") << '.';
<< (isa<ObjCMethodDecl>(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<Expr>(S) && P.isConsumedExpr(cast<Expr>(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<GRState> *N) {
ASTContext& Ctx = PDB.getContext();
SourceManager& SMgr = PDB.getSourceManager();
const ExplodedNode<GRState>* 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<PostStmt>(&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<GRState>* 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<GRState>* N,
const ExplodedNode<GRState>* PrevN,
const ExplodedGraph<GRState>& G,
BugReporter& BR,
NodeResolver &NR) {
BugReporterContext &BRC) {
return NULL;
}
@ -1203,7 +1195,7 @@ PathDiagnosticPiece* BugReport::VisitNode(const ExplodedNode<GRState>* 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<const ExplodedNode<GRState>*> Nodes;
@ -1507,19 +1499,18 @@ void GRBugReporter::GeneratePathDiagnostic(PathDiagnostic& PD,
llvm::OwningPtr<NodeBackMap> BackMap(GPair.first.second);
const ExplodedNode<GRState> *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);

View File

@ -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<GRState>* N);
std::pair<const char**,const char**> getExtraDescriptiveText();
PathDiagnosticPiece* VisitNode(const ExplodedNode<GRState>* N,
const ExplodedNode<GRState>* PrevN,
const ExplodedGraph<GRState>& G,
BugReporter& BR,
NodeResolver& NR);
BugReporterContext& BRC);
};
class VISIBILITY_HIDDEN CFRefLeakReport : public CFRefReport {
@ -1995,7 +1993,7 @@ namespace {
ExplodedNode<GRState> *n, SymbolRef sym,
GRExprEngine& Eng);
PathDiagnosticPiece* getEndPath(BugReporter& BR,
PathDiagnosticPiece* getEndPath(BugReporterContext& BRC,
const ExplodedNode<GRState>* N);
SourceLocation getLocation() const { return AllocSite; }
@ -2096,12 +2094,10 @@ static inline bool contains(const llvm::SmallVectorImpl<ArgEffect>& V,
PathDiagnosticPiece* CFRefReport::VisitNode(const ExplodedNode<GRState>* N,
const ExplodedNode<GRState>* PrevN,
const ExplodedGraph<GRState>& G,
BugReporter& BR,
NodeResolver& NR) {
BugReporterContext& BRC) {
// Check if the type state has changed.
GRStateManager &StMgr = cast<GRBugReporter>(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<GRState>* 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<GRState>* N,
// program point
llvm::SmallVector<ArgEffect, 2> 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<PostStmt>(N->getLocation()).getStmt();
@ -2313,7 +2310,7 @@ PathDiagnosticPiece* CFRefReport::VisitNode(const ExplodedNode<GRState>* N,
return 0; // We have nothing to say!
Stmt* S = cast<PostStmt>(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<GRState>* N,
}
PathDiagnosticPiece*
CFRefReport::getEndPath(BugReporter& br, const ExplodedNode<GRState>* EndN) {
// Tell the BugReporter to report cases when the tracked symbol is
CFRefReport::getEndPath(BugReporterContext& BRC,
const ExplodedNode<GRState>* EndN) {
// Tell the BugReporterContext to report cases when the tracked symbol is
// assigned to different variables, etc.
GRBugReporter& BR = cast<GRBugReporter>(br);
cast<GRBugReporter>(BR).addNotableSymbol(Sym);
return RangedBugReport::getEndPath(BR, EndN);
BRC.addNotableSymbol(Sym);
return RangedBugReport::getEndPath(BRC, EndN);
}
PathDiagnosticPiece*
CFRefLeakReport::getEndPath(BugReporter& br, const ExplodedNode<GRState>* EndN){
CFRefLeakReport::getEndPath(BugReporterContext& BRC,
const ExplodedNode<GRState>* EndN){
GRBugReporter& BR = cast<GRBugReporter>(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<GRBugReporter>(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<GRState>* 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<PostStmt>(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<GRState>* 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<GRState>* 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<ObjCMethodDecl>(BR.getGraph().getCodeDecl());
ObjCMethodDecl& MD = cast<ObjCMethodDecl>(BRC.getCodeDecl());
os << " is returned from a method whose name ('"
<< MD.getSelector().getAsString()
<< "') does not contain 'copy' or otherwise starts with"