From d2f642b56e87493edfc3b0dab359b5e32d5f8a5e Mon Sep 17 00:00:00 2001 From: Ted Kremenek Date: Mon, 14 Apr 2008 17:39:48 +0000 Subject: [PATCH] Hooked up the dead-store checker to the BugReporter interface. Now dead-store warnings are emitted as part of the warnings registered by GRSimpleVals. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@49658 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Analysis/LocalCheckers.h | 4 +- .../Analysis/PathSensitive/BugReporter.h | 29 +++-- .../Analysis/PathSensitive/GRExprEngine.h | 5 + lib/Analysis/BasicObjCFoundationChecks.cpp | 35 ++---- lib/Analysis/BugReporter.cpp | 70 ++++++----- lib/Analysis/CFRefCount.cpp | 8 +- lib/Analysis/DeadStores.cpp | 112 ++++++++++++++++-- lib/Analysis/GRSimpleVals.cpp | 65 ++++------ 8 files changed, 206 insertions(+), 122 deletions(-) diff --git a/include/clang/Analysis/LocalCheckers.h b/include/clang/Analysis/LocalCheckers.h index 8f963b71c4..6dcad7f376 100644 --- a/include/clang/Analysis/LocalCheckers.h +++ b/include/clang/Analysis/LocalCheckers.h @@ -23,6 +23,7 @@ class Diagnostic; class ASTContext; class PathDiagnosticClient; class GRTransferFuncs; +class BugType; void CheckDeadStores(CFG& cfg, ASTContext &Ctx, Diagnostic &Diags); @@ -30,7 +31,8 @@ void CheckUninitializedValues(CFG& cfg, ASTContext& Ctx, Diagnostic& Diags, bool FullUninitTaint=false); GRTransferFuncs* MakeGRSimpleValsTF(); -GRTransferFuncs* MakeCFRefCountTF(ASTContext& Ctx); +GRTransferFuncs* MakeCFRefCountTF(ASTContext& Ctx); +BugType* MakeDeadStoresChecker(); } // end namespace clang diff --git a/include/clang/Analysis/PathSensitive/BugReporter.h b/include/clang/Analysis/PathSensitive/BugReporter.h index e3596b8a12..792b2e2d9e 100644 --- a/include/clang/Analysis/PathSensitive/BugReporter.h +++ b/include/clang/Analysis/PathSensitive/BugReporter.h @@ -15,6 +15,7 @@ #ifndef LLVM_CLANG_ANALYSIS_BUGREPORTER #define LLVM_CLANG_ANALYSIS_BUGREPORTER +#include "clang/Basic/SourceLocation.h" #include "clang/Analysis/PathSensitive/ValueState.h" #include "clang/Analysis/PathSensitive/ExplodedGraph.h" #include "llvm/ADT/SmallPtrSet.h" @@ -29,6 +30,7 @@ class Diagnostic; class BugReporter; class GRExprEngine; class ValueState; +class Stmt; class BugType { public: @@ -42,13 +44,18 @@ public: }; class BugReport { - const BugType& Desc; + const BugType& Desc; + ExplodedNode *N; public: - BugReport(const BugType& D) : Desc(D) {} + BugReport(const BugType& D, ExplodedNode *n) : Desc(D), N(n) {} virtual ~BugReport(); const BugType& getBugType() const { return Desc; } + + ExplodedNode* getEndNode() const { return N; } + + Stmt* getStmt() const; const char* getName() const { return getBugType().getName(); } @@ -56,8 +63,9 @@ public: return getBugType().getDescription(); } - virtual PathDiagnosticPiece* getEndPath(ASTContext& Ctx, - ExplodedNode *N) const; + virtual PathDiagnosticPiece* getEndPath(ASTContext& Ctx) const; + + virtual FullSourceLoc getLocation(SourceManager& Mgr); virtual void getRanges(const SourceRange*& beg, const SourceRange*& end) const; @@ -68,10 +76,12 @@ public: ASTContext& Ctx); }; - class RangedBugReport : public BugReport { +class RangedBugReport : public BugReport { std::vector Ranges; public: - RangedBugReport(const BugType& D) : BugReport(D) {} + RangedBugReport(const BugType& D, ExplodedNode *n) + : BugReport(D, n) {} + virtual ~RangedBugReport(); void addRange(SourceRange R) { Ranges.push_back(R); } @@ -114,16 +124,15 @@ public: GRExprEngine& getEngine() { return Eng; } - void EmitPathWarning(BugReport& R, ExplodedNode* N); + void EmitPathWarning(BugReport& R); - void EmitWarning(BugReport& R, ExplodedNode* N); + void EmitWarning(BugReport& R); void clearCache() { CachedErrors.clear(); } bool IsCached(ExplodedNode* N); - void GeneratePathDiagnostic(PathDiagnostic& PD, BugReport& R, - ExplodedNode* N); + void GeneratePathDiagnostic(PathDiagnostic& PD, BugReport& R); }; } // end clang namespace diff --git a/include/clang/Analysis/PathSensitive/GRExprEngine.h b/include/clang/Analysis/PathSensitive/GRExprEngine.h index fc83dcb189..d64223a5e5 100644 --- a/include/clang/Analysis/PathSensitive/GRExprEngine.h +++ b/include/clang/Analysis/PathSensitive/GRExprEngine.h @@ -189,6 +189,11 @@ public: void ViewGraph(NodeTy** Beg, NodeTy** End); + /// getLiveness - Returned computed live-variables information for the + /// analyzed function. + const LiveVariables& getLiveness() const { return Liveness; } + LiveVariables& getLiveness() { return Liveness; } + /// getInitialState - Return the initial state used for the root vertex /// in the ExplodedGraph. ValueState* getInitialState(); diff --git a/lib/Analysis/BasicObjCFoundationChecks.cpp b/lib/Analysis/BasicObjCFoundationChecks.cpp index e5a31bf50d..717f5b8d80 100644 --- a/lib/Analysis/BasicObjCFoundationChecks.cpp +++ b/lib/Analysis/BasicObjCFoundationChecks.cpp @@ -68,7 +68,9 @@ public: SourceRange R; public: - Report(NilArg& Desc, ObjCMessageExpr* ME, unsigned Arg) : BugReport(Desc) { + Report(NilArg& Desc, ExplodedNode* N, + ObjCMessageExpr* ME, unsigned Arg) + : BugReport(Desc, N) { Expr* E = ME->getArg(Arg); R = E->getSourceRange(); @@ -90,22 +92,6 @@ public: B = &R; E = B+1; } - - virtual PathDiagnosticPiece* getEndPath(ASTContext& Ctx, - ExplodedNode *N) const { - - Stmt* S = cast(N->getLocation()).getStmt(); - - if (!S) - return NULL; - - FullSourceLoc L(S->getLocStart(), Ctx.getSourceManager()); - PathDiagnosticPiece* P = new PathDiagnosticPiece(L, s); - - P->addRange(R); - - return P; - } }; }; @@ -115,7 +101,7 @@ class VISIBILITY_HIDDEN BasicObjCFoundationChecks : public GRSimpleAPICheck { ASTContext &Ctx; ValueStateManager* VMgr; - typedef std::vector > ErrorsTy; + typedef std::vector ErrorsTy; ErrorsTy Errors; RVal GetRVal(ValueState* St, Expr* E) { return VMgr->GetRVal(St, E); } @@ -134,7 +120,7 @@ public: virtual ~BasicObjCFoundationChecks() { for (ErrorsTy::iterator I = Errors.begin(), E = Errors.end(); I!=E; ++I) - delete I->second; + delete *I; } virtual bool Audit(ExplodedNode* N); @@ -143,12 +129,12 @@ public: private: - void AddError(NodeTy* N, BugReport* R) { - Errors.push_back(std::make_pair(N, R)); + void AddError(BugReport* R) { + Errors.push_back(R); } void WarnNilArg(NodeTy* N, ObjCMessageExpr* ME, unsigned Arg) { - AddError(N, new NilArg::Report(Desc, ME, Arg)); + AddError(new NilArg::Report(Desc, N, ME, Arg)); } }; @@ -203,9 +189,8 @@ static inline bool isNil(RVal X) { void BasicObjCFoundationChecks::EmitWarnings(BugReporter& BR) { - for (ErrorsTy::iterator I=Errors.begin(), E=Errors.end(); I!=E; ++I) - - BR.EmitPathWarning(*I->second, I->first); + for (ErrorsTy::iterator I=Errors.begin(), E=Errors.end(); I!=E; ++I) + BR.EmitPathWarning(**I); } bool BasicObjCFoundationChecks::CheckNilArg(NodeTy* N, unsigned Arg) { diff --git a/lib/Analysis/BugReporter.cpp b/lib/Analysis/BugReporter.cpp index 148d2582dc..310fdc9acc 100644 --- a/lib/Analysis/BugReporter.cpp +++ b/lib/Analysis/BugReporter.cpp @@ -52,11 +52,13 @@ static inline Stmt* GetStmt(const CFGBlock* B) { return (*B)[0]; } - -PathDiagnosticPiece* -BugReport::getEndPath(ASTContext& Ctx, ExplodedNode *N) const { +Stmt* BugReport::getStmt() const { + return N ? GetStmt(N->getLocation()) : NULL; +} - Stmt* S = GetStmt(N->getLocation()); +PathDiagnosticPiece* BugReport::getEndPath(ASTContext& Ctx) const { + + Stmt* S = getStmt(); if (!S) return NULL; @@ -83,11 +85,24 @@ BugReport::getEndPath(ASTContext& Ctx, ExplodedNode *N) const { } void BugReport::getRanges(const SourceRange*& beg, - const SourceRange*& end) const { + const SourceRange*& end) const { beg = NULL; end = NULL; } +FullSourceLoc BugReport::getLocation(SourceManager& Mgr) { + + if (!N) + return FullSourceLoc(); + + Stmt* S = GetStmt(N->getLocation()); + + if (!S) + return FullSourceLoc(); + + return FullSourceLoc(S->getLocStart(), Mgr); +} + PathDiagnosticPiece* BugReport::VisitNode(ExplodedNode* N, ExplodedNode* PrevN, ExplodedGraph& G, @@ -96,10 +111,13 @@ PathDiagnosticPiece* BugReport::VisitNode(ExplodedNode* N, } void BugReporter::GeneratePathDiagnostic(PathDiagnostic& PD, - BugReport& R, - ExplodedNode* N) { + BugReport& R) { - if (PathDiagnosticPiece* Piece = R.getEndPath(Ctx,N)) + ExplodedNode* N = R.getEndNode(); + + assert (N && "Path diagnostic requires a ExplodedNode."); + + if (PathDiagnosticPiece* Piece = R.getEndPath(Ctx)) PD.push_back(Piece); else return; @@ -325,10 +343,12 @@ bool BugReporter::IsCached(ExplodedNode* N) { return false; } -void BugReporter::EmitPathWarning(BugReport& R, ExplodedNode* N) { +void BugReporter::EmitPathWarning(BugReport& R) { + + ExplodedNode* N = R.getEndNode(); - if (!PD) { - EmitWarning(R, N); + if (!PD || !N) { + EmitWarning(R); return; } @@ -336,15 +356,17 @@ void BugReporter::EmitPathWarning(BugReport& R, ExplodedNode* N) { return; PathDiagnostic D(R.getName()); - GeneratePathDiagnostic(D, R, N); + GeneratePathDiagnostic(D, R); if (!D.empty()) PD->HandlePathDiagnostic(D); } +void BugReporter::EmitWarning(BugReport& R) { -void BugReporter::EmitWarning(BugReport& R, ExplodedNode* N) { - if (IsCached(N)) + ExplodedNode* N = R.getEndNode(); + + if (N && IsCached(N)) return; std::ostringstream os; @@ -355,23 +377,9 @@ void BugReporter::EmitWarning(BugReport& R, ExplodedNode* N) { // FIXME: Add support for multiple ranges. - Stmt* S = GetStmt(N->getLocation()); - - if (!S) - return; - + FullSourceLoc L = R.getLocation(Ctx.getSourceManager()); + const SourceRange *Beg, *End; R.getRanges(Beg, End); - - if (Beg == End) { - SourceRange Range = S->getSourceRange(); - - Diag.Report(FullSourceLoc(S->getLocStart(), Ctx.getSourceManager()), - ErrorDiag, NULL, 0, &Range, 1); - - } - else - Diag.Report(FullSourceLoc(S->getLocStart(), Ctx.getSourceManager()), - ErrorDiag, NULL, 0, Beg, End - Beg); - + Diag.Report(L, ErrorDiag, NULL, 0, Beg, End - Beg); } diff --git a/lib/Analysis/CFRefCount.cpp b/lib/Analysis/CFRefCount.cpp index ff78fd749e..df479884ed 100644 --- a/lib/Analysis/CFRefCount.cpp +++ b/lib/Analysis/CFRefCount.cpp @@ -911,9 +911,9 @@ void UseAfterRelease::EmitWarnings(BugReporter& BR) { for (CFRefCount::use_after_iterator I = TF.use_after_begin(), E = TF.use_after_end(); I != E; ++I) { - RangedBugReport report(*this); + RangedBugReport report(*this, I->first); report.addRange(I->second->getSourceRange()); - BR.EmitPathWarning(report, I->first); + BR.EmitPathWarning(report); } } @@ -922,9 +922,9 @@ void BadRelease::EmitWarnings(BugReporter& BR) { for (CFRefCount::bad_release_iterator I = TF.bad_release_begin(), E = TF.bad_release_end(); I != E; ++I) { - RangedBugReport report(*this); + RangedBugReport report(*this, I->first); report.addRange(I->second->getSourceRange()); - BR.EmitPathWarning(report, I->first); + BR.EmitPathWarning(report); } } diff --git a/lib/Analysis/DeadStores.cpp b/lib/Analysis/DeadStores.cpp index 0848336e58..2e57757f5c 100644 --- a/lib/Analysis/DeadStores.cpp +++ b/lib/Analysis/DeadStores.cpp @@ -15,6 +15,8 @@ #include "clang/Analysis/LocalCheckers.h" #include "clang/Analysis/Analyses/LiveVariables.h" #include "clang/Analysis/Visitors/CFGRecStmtVisitor.h" +#include "clang/Analysis/PathSensitive/BugReporter.h" +#include "clang/Analysis/PathSensitive/GRExprEngine.h" #include "clang/Basic/Diagnostic.h" #include "clang/AST/ASTContext.h" #include "llvm/Support/Compiler.h" @@ -26,8 +28,11 @@ namespace { class VISIBILITY_HIDDEN DeadStoreObs : public LiveVariables::ObserverTy { ASTContext &Ctx; Diagnostic &Diags; + DiagnosticClient &Client; public: - DeadStoreObs(ASTContext &ctx,Diagnostic &diags) : Ctx(ctx), Diags(diags){} + DeadStoreObs(ASTContext &ctx, Diagnostic &diags, DiagnosticClient &client) + : Ctx(ctx), Diags(diags), Client(client) {} + virtual ~DeadStoreObs() {} virtual void ObserveStmt(Stmt* S, @@ -41,7 +46,8 @@ public: if (VarDecl* VD = dyn_cast(DR->getDecl())) if (VD->hasLocalStorage() && !Live(VD,AD)) { SourceRange R = B->getRHS()->getSourceRange(); - Diags.Report(Ctx.getFullLoc(DR->getSourceRange().getBegin()), + Diags.Report(&Client, + Ctx.getFullLoc(DR->getSourceRange().getBegin()), diag::warn_dead_store, 0, 0, &R, 1); } } @@ -63,7 +69,8 @@ public: if (!E->isConstantExpr(Ctx,NULL)) { // Flag a warning. SourceRange R = E->getSourceRange(); - Diags.Report(Ctx.getFullLoc(V->getLocation()), + Diags.Report(&Client, + Ctx.getFullLoc(V->getLocation()), diag::warn_dead_store, 0, 0, &R, 1); } } @@ -74,14 +81,103 @@ public: } // end anonymous namespace -namespace clang { +//===----------------------------------------------------------------------===// +// Driver function to invoke the Dead-Stores checker on a CFG. +//===----------------------------------------------------------------------===// -void CheckDeadStores(CFG& cfg, ASTContext &Ctx, Diagnostic &Diags) { - +void clang::CheckDeadStores(CFG& cfg, ASTContext &Ctx, Diagnostic &Diags) { LiveVariables L(cfg); L.runOnCFG(cfg); - DeadStoreObs A(Ctx, Diags); + DeadStoreObs A(Ctx, Diags, Diags.getClient()); L.runOnAllBlocks(cfg, &A); } -} // end namespace clang +//===----------------------------------------------------------------------===// +// BugReporter-based invocation of the Dead-Stores checker. +//===----------------------------------------------------------------------===// + +namespace { + +class VISIBILITY_HIDDEN DiagBugReport : public RangedBugReport { + std::list Strs; + FullSourceLoc L; +public: + DiagBugReport(const BugType& D, FullSourceLoc l) : + RangedBugReport(D, NULL), L(l) {} + + virtual ~DiagBugReport() {} + virtual FullSourceLoc getLocation(SourceManager&) { return L; } + + void addString(const std::string& s) { Strs.push_back(s); } + + typedef std::list::const_iterator str_iterator; + str_iterator str_begin() const { return Strs.begin(); } + str_iterator str_end() const { return Strs.end(); } +}; + +class VISIBILITY_HIDDEN DiagCollector : public DiagnosticClient { + std::list Reports; + const BugType& D; +public: + DiagCollector(BugType& d) : D(d) {} + + virtual ~DiagCollector() {} + + virtual void HandleDiagnostic(Diagnostic &Diags, + Diagnostic::Level DiagLevel, + FullSourceLoc Pos, + diag::kind ID, + const std::string *Strs, + unsigned NumStrs, + const SourceRange *Ranges, + unsigned NumRanges) { + + // FIXME: Use a map from diag::kind to BugType, instead of having just + // one BugType. + + Reports.push_back(DiagBugReport(D, Pos)); + DiagBugReport& R = Reports.back(); + + for ( ; NumRanges ; --NumRanges, ++Ranges) + R.addRange(*Ranges); + + for ( ; NumStrs ; --NumStrs, ++Strs) + R.addString(*Strs); + } + + // Iterators. + + typedef std::list::iterator iterator; + iterator begin() { return Reports.begin(); } + iterator end() { return Reports.end(); } +}; + +class VISIBILITY_HIDDEN DeadStoresChecker : public BugType { +public: + virtual const char* getName() const { + return "dead store"; + } + + virtual const char* getDescription() const { + return "Value stored to variable is never used."; + } + + virtual void EmitWarnings(BugReporter& BR) { + + // Run the dead store checker and collect the diagnostics. + DiagCollector C(*this); + DeadStoreObs A(BR.getContext(), BR.getDiagnostic(), C); + GRExprEngine& Eng = BR.getEngine(); + Eng.getLiveness().runOnAllBlocks(Eng.getCFG(), &A); + + // Emit the bug reports. + + for (DiagCollector::iterator I = C.begin(), E = C.end(); I != E; ++I) + BR.EmitWarning(*I); + } +}; +} // end anonymous namespace + +BugType* clang::MakeDeadStoresChecker() { + return new DeadStoresChecker(); +} diff --git a/lib/Analysis/GRSimpleVals.cpp b/lib/Analysis/GRSimpleVals.cpp index c350ab9752..b37ccf5f80 100644 --- a/lib/Analysis/GRSimpleVals.cpp +++ b/lib/Analysis/GRSimpleVals.cpp @@ -44,8 +44,8 @@ void GenericEmitWarnings(BugReporter& BR, const BugType& D, ITER I, ITER E) { for (; I != E; ++I) { - BugReport R(D); - BR.EmitPathWarning(R, GetNode(I)); + BugReport R(D, GetNode(I)); + BR.EmitPathWarning(R); } } @@ -159,20 +159,6 @@ public: class VISIBILITY_HIDDEN BadArg : public BugType { -protected: - - class Report : public BugReport { - const SourceRange R; - public: - Report(BugType& D, Expr* E) : BugReport(D), R(E->getSourceRange()) {} - virtual ~Report() {} - - virtual void getRanges(const SourceRange*& B, const SourceRange*& E) const { - B = &R; - E = B+1; - } - }; - public: virtual ~BadArg() {} @@ -192,10 +178,11 @@ public: E = Eng.undef_arg_end(); I!=E; ++I) { // Generate a report for this bug. - Report report(*this, I->second); + RangedBugReport report(*this, I->first); + report.addRange(I->second->getSourceRange()); // Emit the warning. - BR.EmitPathWarning(report, I->first); + BR.EmitPathWarning(report); } } @@ -218,35 +205,16 @@ public: E = Eng.msg_expr_undef_arg_end(); I!=E; ++I) { // Generate a report for this bug. - Report report(*this, I->second); + RangedBugReport report(*this, I->first); + report.addRange(I->second->getSourceRange()); // Emit the warning. - BR.EmitPathWarning(report, I->first); - } - + BR.EmitPathWarning(report); + } } }; class VISIBILITY_HIDDEN BadReceiver : public BugType { - - class Report : public BugReport { - SourceRange R; - public: - Report(BugType& D, ExplodedNode *N) : BugReport(D) { - Stmt *S = cast(N->getLocation()).getStmt(); - Expr* E = cast(S)->getReceiver(); - assert (E && "Receiver cannot be NULL"); - R = E->getSourceRange(); - } - - virtual ~Report() {} - - virtual void getRanges(const SourceRange*& B, const SourceRange*& E) const { - B = &R; - E = B+1; - } - }; - public: virtual const char* getName() const { return "bad receiver"; @@ -263,10 +231,16 @@ public: E = Eng.undef_receivers_end(); I!=E; ++I) { // Generate a report for this bug. - Report report(*this, *I); + RangedBugReport report(*this, *I); + + ExplodedNode* N = *I; + Stmt *S = cast(N->getLocation()).getStmt(); + Expr* E = cast(S)->getReceiver(); + assert (E && "Receiver cannot be NULL"); + report.addRange(E->getSourceRange()); // Emit the warning. - BR.EmitPathWarning(report, *I); + BR.EmitPathWarning(report); } } }; @@ -291,6 +265,8 @@ public: } // end anonymous namespace void GRSimpleVals::RegisterChecks(GRExprEngine& Eng) { + + // Path-sensitive checks. Eng.Register(new NullDeref()); Eng.Register(new UndefDeref()); Eng.Register(new UndefBranch()); @@ -302,6 +278,9 @@ void GRSimpleVals::RegisterChecks(GRExprEngine& Eng) { Eng.Register(new BadMsgExprArg()); Eng.Register(new BadReceiver()); + // Flow-sensitive checks. + Eng.Register(MakeDeadStoresChecker()); + // Add extra checkers. GRSimpleAPICheck* FoundationCheck = -- 2.40.0