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:

   <rdar://problem/5968508> 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 530bf90e7e4ee91642132cf23464c9967540f2fc..2369196b0b26062353176723f39dc4e43f1cee84 100644 (file)
--- 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 c9cabf207352b14c0961e14746eb43c1fae53451..cf9de720a280622360c722a4eef0d32a680b9a72 100644 (file)
--- 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 5cb4fd7d5209c85755075861573aa3ec5ad7d2ba..19028ee8751abc8f240de3ef77755f5759345fc9 100644 (file)
--- 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<Stmt*,Stmt*> ParentMapTy;
     
   /// G - The simulation graph.  Each node is a (location,state) pair.
   llvm::OwningPtr<ExplodedGraphImpl> 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 4ea03adc4f8eeaf7bb2ef57998ef178b112970f9..c9d1676c2d115255cc0a107eb7d7ed37a65716c4 100644 (file)
--- 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 240ecd19fed3db0e0fa9ee62e13c9527c8cd4586..26dba4e7e870b888863e551816bd3f670892bc53 100644 (file)
--- 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<PostStmt>(&P)) {
     return PS->getStmt();

diff --git a/lib/Analysis/DeadStores.cpp b/lib/Analysis/DeadStores.cpp
index 0f08b233b5467896b189d5b3b67a5b3cd4178c7b..7d912b046ebd4c54231eb698256946cacbc05706 100644 (file)
--- 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<VarDecl>(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<DeclRefExpr>(B->getLHS()))
-        CheckDeclRef(DR, B->getRHS(), AD, Live);
+        if (VarDecl *VD = dyn_cast<VarDecl>(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<UnaryOperator>(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 7c2ca0c13f698d94a2c4601fdb8eff713c15554e..a4bb7fa63117ccda2c13d917b46f928a3a31f96e 100644 (file)
--- 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<Stmt*,Stmt*> 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 e8c1ec57a986f373acd8dec3b7f2223a32b094b5..9abfe6f79f38a0b1bdcd15c32c7d67888cf516f0 100644 (file)
--- 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 21427c69106b32f2e3bf13966b8446a924120f4b..972410ac98c658d4e923a3de06c31ac19821550b 100644 (file)
--- 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;
+}
+