From: Ted Kremenek Date: Fri, 20 Jun 2008 21:45:25 +0000 (+0000) Subject: Modified the dead stores checker to... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=1a654b60ef40e84f3943cdb581795c4d4dae1e45;p=clang Modified the dead stores checker to... 1) Check if a dead store appears as a subexpression. For such cases, we emit a verbose diagnostic so that users aren't confused. This addresses: checker gives misleading report for dead store in loop 2) Don't emit a dead store warning when assigning a null value to a pointer. This is a common form of defensive programming. We may wish to make this an option to the the checker one day. This addresses the feature request in the following email: http://lists.cs.uiuc.edu/pipermail/cfe-dev/2008-June/001978.html git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@52555 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Analysis/LocalCheckers.h b/include/clang/Analysis/LocalCheckers.h index 530bf90e7e..2369196b0b 100644 --- a/include/clang/Analysis/LocalCheckers.h +++ b/include/clang/Analysis/LocalCheckers.h @@ -25,8 +25,10 @@ class PathDiagnosticClient; class GRTransferFuncs; class BugType; class LangOptions; +class ParentMap; -void CheckDeadStores(CFG& cfg, ASTContext &Ctx, Diagnostic &Diags); +void CheckDeadStores(CFG& cfg, ASTContext &Ctx, ParentMap& Parents, + Diagnostic &Diags); void CheckUninitializedValues(CFG& cfg, ASTContext& Ctx, Diagnostic& Diags, bool FullUninitTaint=false); diff --git a/include/clang/Analysis/PathSensitive/BugReporter.h b/include/clang/Analysis/PathSensitive/BugReporter.h index c9cabf2073..cf9de720a2 100644 --- a/include/clang/Analysis/PathSensitive/BugReporter.h +++ b/include/clang/Analysis/PathSensitive/BugReporter.h @@ -34,6 +34,7 @@ class GRExprEngine; class ValueState; class Stmt; class BugReport; +class ParentMap; class BugType { public: @@ -156,6 +157,8 @@ public: CFG& getCFG() { return getGraph().getCFG(); } + ParentMap& getParentMap(); + void EmitWarning(BugReport& R); void GeneratePathDiagnostic(PathDiagnostic& PD, BugReport& R); diff --git a/include/clang/Analysis/PathSensitive/GRCoreEngine.h b/include/clang/Analysis/PathSensitive/GRCoreEngine.h index 5cb4fd7d52..19028ee875 100644 --- a/include/clang/Analysis/PathSensitive/GRCoreEngine.h +++ b/include/clang/Analysis/PathSensitive/GRCoreEngine.h @@ -47,19 +47,10 @@ protected: friend class GRIndirectGotoNodeBuilderImpl; friend class GRSwitchNodeBuilderImpl; friend class GREndPathNodeBuilderImpl; - - typedef llvm::DenseMap ParentMapTy; /// G - The simulation graph. Each node is a (location,state) pair. llvm::OwningPtr G; - - /// ParentMap - A lazily populated map from a Stmt* to its parent Stmt*. - void* ParentMap; - - /// CurrentBlkExpr - The current Block-level expression being processed. - /// This is used when lazily populating ParentMap. - Stmt* CurrentBlkExpr; - + /// WList - A set of queued nodes that need to be processed by the /// worklist algorithm. It is up to the implementation of WList to decide /// the order that nodes are processed. diff --git a/include/clang/Analysis/PathSensitive/GRExprEngine.h b/include/clang/Analysis/PathSensitive/GRExprEngine.h index 4ea03adc4f..c9d1676c2d 100644 --- a/include/clang/Analysis/PathSensitive/GRExprEngine.h +++ b/include/clang/Analysis/PathSensitive/GRExprEngine.h @@ -28,6 +28,7 @@ namespace clang { class BugType; class PathDiagnosticClient; class Diagnostic; + class ParentMap; class GRExprEngine { @@ -51,6 +52,9 @@ protected: /// G - the simulation graph. GraphTy& G; + /// Parents - a lazily created map from Stmt* to parents. + ParentMap* Parents; + /// Liveness - live-variables information the ValueDecl* and block-level /// Expr* in the CFG. Used to prune out dead state. LiveVariables Liveness; @@ -213,6 +217,10 @@ public: GraphTy& getGraph() { return G; } const GraphTy& getGraph() const { return G; } + + /// getParentMap - Return a map from Stmt* to parents for the function/method + /// body being analyzed. This map is lazily constructed as needed. + ParentMap& getParentMap(); typedef BugTypeSet::iterator bug_type_iterator; typedef BugTypeSet::const_iterator const_bug_type_iterator; diff --git a/lib/Analysis/BugReporter.cpp b/lib/Analysis/BugReporter.cpp index 240ecd19fe..26dba4e7e8 100644 --- a/lib/Analysis/BugReporter.cpp +++ b/lib/Analysis/BugReporter.cpp @@ -39,6 +39,10 @@ ValueStateManager& BugReporter::getStateManager() { return Eng.getStateManager(); } +ParentMap& BugReporter::getParentMap() { + return Eng.getParentMap(); +} + static inline Stmt* GetStmt(const ProgramPoint& P) { if (const PostStmt* PS = dyn_cast(&P)) { return PS->getStmt(); diff --git a/lib/Analysis/DeadStores.cpp b/lib/Analysis/DeadStores.cpp index 0f08b233b5..7d912b046e 100644 --- a/lib/Analysis/DeadStores.cpp +++ b/lib/Analysis/DeadStores.cpp @@ -19,28 +19,49 @@ #include "clang/Analysis/PathSensitive/GRExprEngine.h" #include "clang/Basic/Diagnostic.h" #include "clang/AST/ASTContext.h" +#include "clang/AST/ParentMap.h" #include "llvm/Support/Compiler.h" using namespace clang; namespace { - + class VISIBILITY_HIDDEN DeadStoreObs : public LiveVariables::ObserverTy { ASTContext &Ctx; Diagnostic &Diags; DiagnosticClient &Client; + ParentMap& Parents; + public: - DeadStoreObs(ASTContext &ctx, Diagnostic &diags, DiagnosticClient &client) - : Ctx(ctx), Diags(diags), Client(client) {} + DeadStoreObs(ASTContext &ctx, Diagnostic &diags, DiagnosticClient &client, + ParentMap& parents) + : Ctx(ctx), Diags(diags), Client(client), Parents(parents) {} virtual ~DeadStoreObs() {} - unsigned GetDiag(VarDecl* VD) { - std::string msg = "value stored to '" + std::string(VD->getName()) + - "' is never used"; + unsigned GetDiag(VarDecl* VD, bool inEnclosing = false) { + std::string name(VD->getName()); + + std::string msg = inEnclosing + ? "Although the value stored to '" + name + + "' is used in the enclosing expression, the value is never actually read" + " from '" + name + "'" + : "Value stored to '" + name + "' is never read"; - return Diags.getCustomDiagID(Diagnostic::Warning, msg.c_str()); - + return Diags.getCustomDiagID(Diagnostic::Warning, msg.c_str()); + } + + void CheckVarDecl(VarDecl* VD, Expr* Ex, Expr* Val, + bool hasEnclosing, + const LiveVariables::AnalysisDataTy& AD, + const LiveVariables::ValTy& Live) { + + if (VD->hasLocalStorage() && !Live(VD, AD)) { + SourceRange R = Val->getSourceRange(); + Diags.Report(&Client, + Ctx.getFullLoc(Ex->getSourceRange().getBegin()), + GetDiag(VD, hasEnclosing), 0, 0, &R, 1); + } } void CheckDeclRef(DeclRefExpr* DR, Expr* Val, @@ -48,12 +69,7 @@ public: const LiveVariables::ValTy& Live) { if (VarDecl* VD = dyn_cast(DR->getDecl())) - if (VD->hasLocalStorage() && !Live(VD, AD)) { - SourceRange R = Val->getSourceRange(); - Diags.Report(&Client, - Ctx.getFullLoc(DR->getSourceRange().getBegin()), - GetDiag(VD), 0, 0, &R, 1); - } + CheckVarDecl(VD, DR, Val, false, AD, Live); } virtual void ObserveStmt(Stmt* S, @@ -68,7 +84,22 @@ public: if (!B->isAssignmentOp()) return; // Skip non-assignments. if (DeclRefExpr* DR = dyn_cast(B->getLHS())) - CheckDeclRef(DR, B->getRHS(), AD, Live); + if (VarDecl *VD = dyn_cast(DR->getDecl())) { + + // Special case: check for assigning null to a pointer. This + // is a common form of defensive programming. + // FIXME: Make this optional? + + Expr* Val = B->getRHS(); + llvm::APSInt Result(Ctx.getTypeSize(Val->getType())); + + if (VD->getType()->isPointerType() && + Val->IgnoreParenCasts()->isIntegerConstantExpr(Result, Ctx, 0)) + if (Result == 0) + return; + + CheckVarDecl(VD, DR, Val, Parents.isSubExpr(B), AD, Live); + } } else if (UnaryOperator* U = dyn_cast(S)) { if (!U->isIncrementOp()) @@ -116,10 +147,11 @@ public: // Driver function to invoke the Dead-Stores checker on a CFG. //===----------------------------------------------------------------------===// -void clang::CheckDeadStores(CFG& cfg, ASTContext &Ctx, Diagnostic &Diags) { +void clang::CheckDeadStores(CFG& cfg, ASTContext &Ctx, + ParentMap& Parents, Diagnostic &Diags) { LiveVariables L(cfg); L.runOnCFG(cfg); - DeadStoreObs A(Ctx, Diags, Diags.getClient()); + DeadStoreObs A(Ctx, Diags, Diags.getClient(), Parents); L.runOnAllBlocks(cfg, &A); } @@ -197,9 +229,10 @@ public: // 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); + DeadStoreObs A(BR.getContext(), BR.getDiagnostic(), C, BR.getParentMap()); + + GRExprEngine& Eng = BR.getEngine(); + Eng.getLiveness().runOnAllBlocks(BR.getCFG(), &A); // Emit the bug reports. diff --git a/lib/Analysis/GRCoreEngine.cpp b/lib/Analysis/GRCoreEngine.cpp index 7c2ca0c13f..a4bb7fa631 100644 --- a/lib/Analysis/GRCoreEngine.cpp +++ b/lib/Analysis/GRCoreEngine.cpp @@ -253,19 +253,6 @@ void GRCoreEngineImpl::HandlePostStmt(const PostStmt& L, CFGBlock* B, } } -typedef llvm::DenseMap ParentMapTy; -/// PopulateParentMap - Recurse the AST starting at 'Parent' and add the -/// mappings between child and parent to ParentMap. -static void PopulateParentMap(Stmt* Parent, ParentMapTy& M) { - for (Stmt::child_iterator I=Parent->child_begin(), - E=Parent->child_end(); I!=E; ++I) { - - assert (M.find(*I) == M.end()); - M[*I] = Parent; - PopulateParentMap(*I, M); - } -} - /// GenerateNode - Utility method to generate nodes, hook up successors, /// and add nodes to the worklist. void GRCoreEngineImpl::GenerateNode(const ProgramPoint& Loc, void* State, diff --git a/lib/Analysis/GRExprEngine.cpp b/lib/Analysis/GRExprEngine.cpp index e8c1ec57a9..9abfe6f79f 100644 --- a/lib/Analysis/GRExprEngine.cpp +++ b/lib/Analysis/GRExprEngine.cpp @@ -13,6 +13,7 @@ // //===----------------------------------------------------------------------===// +#include "clang/AST/ParentMap.h" #include "clang/Analysis/PathSensitive/GRExprEngine.h" #include "clang/Analysis/PathSensitive/BugReporter.h" #include "clang/Basic/SourceManager.h" @@ -41,6 +42,7 @@ static inline Selector GetNullarySelector(const char* name, ASTContext& Ctx) { GRExprEngine::GRExprEngine(CFG& cfg, Decl& CD, ASTContext& Ctx) : CoreEngine(cfg, CD, Ctx, *this), G(CoreEngine.getGraph()), + Parents(0), Liveness(G.getCFG()), Builder(NULL), StateMgr(G.getContext(), G.getAllocator()), @@ -56,7 +58,7 @@ GRExprEngine::GRExprEngine(CFG& cfg, Decl& CD, ASTContext& Ctx) Liveness.runOnAllBlocks(G.getCFG(), NULL, true); } -GRExprEngine::~GRExprEngine() { +GRExprEngine::~GRExprEngine() { for (BugTypeSet::iterator I = BugTypes.begin(), E = BugTypes.end(); I!=E; ++I) delete *I; @@ -69,6 +71,17 @@ GRExprEngine::~GRExprEngine() { delete *I; delete [] NSExceptionInstanceRaiseSelectors; + + delete Parents; +} + +ParentMap& GRExprEngine::getParentMap() { + if (!Parents) { + Stmt* Body = getGraph().getCodeDecl().getCodeBody(); + Parents = new ParentMap(Body); + } + + return *Parents; } //===----------------------------------------------------------------------===// diff --git a/test/Analysis/dead-stores.c b/test/Analysis/dead-stores.c index 21427c6910..972410ac98 100644 --- a/test/Analysis/dead-stores.c +++ b/test/Analysis/dead-stores.c @@ -3,12 +3,12 @@ void f1() { int k, y; int abc=1; - long idx=abc+3*5; // expected-warning {{never used}} + long idx=abc+3*5; // expected-warning {{never read}} } void f2(void *b) { char *c = (char*)b; // no-warning - char *d = b+1; // expected-warning {{never used}} + char *d = b+1; // expected-warning {{never read}} printf("%s", c); } @@ -27,19 +27,32 @@ void f4(int k) { if (k) f1(); - k = 2; // expected-warning {{never used}} + k = 2; // expected-warning {{never read}} } void f5() { int x = 4; // no-warning - int *p = &x; // expected-warning{{never used}} + int *p = &x; // expected-warning{{never read}} } int f6() { int x = 4; - ++x; // expected-warning{{never used}} + ++x; // expected-warning{{never read}} return 1; } + +int f7(int *p) { + // This is allowed for defensive programming. + p = 0; // no-warning + return 1; +} + +int f8(int *p) { + if (p = baz()) // expected-warning{{Although the value}} + return 1; + return 0; +} +