[cfe-commits] r62552 - in /cfe/trunk: include/clang/AST/ParentMap.h lib/AST/ParentMap.cpp lib/Analysis/CheckDeadStores.cpp test/Analysis/dead-stores.c

[Ted Kremenek]

Author: kremenek
Date: Mon Jan 19 18:47:45 2009
New Revision: 62552

URL: http://llvm.org/viewvc/llvm-project?rev=62552&view=rev
Log:
Dead stores checker: Fix <rdar://problem/6506065> by being more selective when say that a store is dead even though the computed value is used in the enclosing expression.

Modified:
    cfe/trunk/include/clang/AST/ParentMap.h
    cfe/trunk/lib/AST/ParentMap.cpp
    cfe/trunk/lib/Analysis/CheckDeadStores.cpp
    cfe/trunk/test/Analysis/dead-stores.c

Modified: cfe/trunk/include/clang/AST/ParentMap.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ParentMap.h?rev=62552&r1=62551&r2=62552&view=diff

==============================================================================
--- cfe/trunk/include/clang/AST/ParentMap.h (original)
+++ cfe/trunk/include/clang/AST/ParentMap.h Mon Jan 19 18:47:45 2009
@@ -28,8 +28,6 @@
   bool hasParent(Stmt* S) const {
     return !getParent(S);
   }
-  
-  bool isSubExpr(Stmt *S) const;
 };
   
 } // end clang namespace

Modified: cfe/trunk/lib/AST/ParentMap.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ParentMap.cpp?rev=62552&r1=62551&r2=62552&view=diff

==============================================================================
--- cfe/trunk/lib/AST/ParentMap.cpp (original)
+++ cfe/trunk/lib/AST/ParentMap.cpp Mon Jan 19 18:47:45 2009
@@ -45,11 +45,3 @@
   MapTy::iterator I = M->find(S);
   return I == M->end() ? 0 : I->second;
 }
-
-bool ParentMap::isSubExpr(Stmt* S) const {
-  if (!isa<Expr>(S))
-    return false;
-  
-  Stmt* P = getParent(S);
-  return P ? !isa<CompoundStmt>(P) : false;
-}

Modified: cfe/trunk/lib/Analysis/CheckDeadStores.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CheckDeadStores.cpp?rev=62552&r1=62551&r2=62552&view=diff

==============================================================================
--- cfe/trunk/lib/Analysis/CheckDeadStores.cpp (original)
+++ cfe/trunk/lib/Analysis/CheckDeadStores.cpp Mon Jan 19 18:47:45 2009
@@ -39,6 +39,9 @@
   
   virtual ~DeadStoreObs() {}
   
+  bool isConsumedExpr(Expr* E) const;
+  
+  
   void Report(VarDecl* V, DeadStoreKind dsk, SourceLocation L, SourceRange R) {
 
     std::string name = V->getNameAsString();
@@ -145,10 +148,9 @@
               return;
             
           // Otherwise, issue a warning.
-          DeadStoreKind dsk = 
-            Parents.isSubExpr(B)
-            ? Enclosing 
-            : (isIncrement(VD,B) ? DeadIncrement : Standard);
+          DeadStoreKind dsk = isConsumedExpr(B)
+                              ? Enclosing 
+                              : (isIncrement(VD,B) ? DeadIncrement : Standard);
           
           CheckVarDecl(VD, DR, B->getRHS(), dsk, AD, Live);
         }              
@@ -161,7 +163,7 @@
       //  about preincrements to dead variables when the preincrement occurs
       //  as a subexpression.  This can lead to false negatives, e.g. "(++x);"
       //  A generalized dead code checker should find such issues.
-      if (U->isPrefix() && Parents.isSubExpr(U))
+      if (U->isPrefix() && isConsumedExpr(U))
         return;
 
       Expr *Ex = U->getSubExpr()->IgnoreParenCasts();
@@ -203,6 +205,43 @@
   
 } // end anonymous namespace
 
+bool DeadStoreObs::isConsumedExpr(Expr* E) const {
+  Stmt *P = Parents.getParent(E);
+  Stmt *DirectChild = E;
+  
+  // Ignore parents that are parentheses or casts.
+  while (P && (isa<ParenExpr>(E) || isa<CastExpr>(E))) {
+    DirectChild = P;
+    P = Parents.getParent(P);
+  }
+  
+  if (!P)
+    return false;
+  
+  switch (P->getStmtClass()) {
+    default:
+      return isa<Expr>(P);
+    case Stmt::BinaryOperatorClass: {
+      BinaryOperator *BE = cast<BinaryOperator>(P);
+      return BE->getOpcode()==BinaryOperator::Comma && DirectChild==BE->getLHS();
+    }
+    case Stmt::ForStmtClass:
+      return DirectChild == cast<ForStmt>(P)->getCond();
+    case Stmt::WhileStmtClass:
+      return DirectChild == cast<WhileStmt>(P)->getCond();      
+    case Stmt::DoStmtClass:
+      return DirectChild == cast<DoStmt>(P)->getCond();
+    case Stmt::IfStmtClass:
+      return DirectChild == cast<IfStmt>(P)->getCond();
+    case Stmt::IndirectGotoStmtClass:
+      return DirectChild == cast<IndirectGotoStmt>(P)->getTarget();
+    case Stmt::SwitchStmtClass:
+      return DirectChild == cast<SwitchStmt>(P)->getCond();
+    case Stmt::ReturnStmtClass:
+      return true;
+  }
+}
+
 //===----------------------------------------------------------------------===//
 // Driver function to invoke the Dead-Stores checker on a CFG.
 //===----------------------------------------------------------------------===//

Modified: cfe/trunk/test/Analysis/dead-stores.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/dead-stores.c?rev=62552&r1=62551&r2=62552&view=diff

==============================================================================
--- cfe/trunk/test/Analysis/dead-stores.c (original)
+++ cfe/trunk/test/Analysis/dead-stores.c Mon Jan 19 18:47:45 2009
@@ -79,10 +79,9 @@
 
 int f11b() {
   int x = 4;
-  return ++x; // no-warning
+  return ((((++x)))); // no-warning
 }
 
-
 int f12a(int y) {
   int x = y;  // expected-warning{{never read}}
   return 1;
@@ -132,3 +131,19 @@
   int x = 1;
   x = x; // no-warning
 }
+
+// <rdar://problem/6506065>
+// The values of dead stores are only "consumed" in an enclosing expression
+// what that value is actually used.  In other words, don't say "Although the value stored to 'x' is used...".
+int f18() {
+   int x = 0; // no-warning
+   if (1)
+      x = 10;  // expected-warning{{Value stored to 'x' is never read}}
+   while (1)
+      x = 10;  // expected-warning{{Value stored to 'x' is never read}}
+   do
+      x = 10;   // expected-warning{{Value stored to 'x' is never read}}
+   while (1);
+
+   return (x = 10); // expected-warning{{Although the value stored to 'x' is used in the enclosing expression, the value is never actually read from 'x'}}
+}