Fix: <rdar://problem/6740387>. Sending nil to an object that returns a struct

should only be an error if that value is consumed. This fix was largely
accomplished by moving 'isConsumedExpr' back to ParentMap.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@68195 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/AST/ParentMap.h b/include/clang/AST/ParentMap.h
index 94890e3feb5c1949991fc044eebb339fa0d507bb..199a360ca0dcbaa41a472034dba35bac741ffcd1 100644 (file)
--- a/include/clang/AST/ParentMap.h
+++ b/include/clang/AST/ParentMap.h
@@ -16,6 +16,7 @@
 
 namespace clang {
 class Stmt;
+class Expr;
   
 class ParentMap {
   void* Impl;
@@ -32,6 +33,12 @@ public:
   bool hasParent(Stmt* S) const {
     return getParent(S) != 0;
   }
+  
+  bool isConsumedExpr(Expr *E) const;
+  
+  bool isConsumedExpr(const Expr *E) const {
+    return isConsumedExpr(const_cast<Expr*>(E));
+  }
 };
   
 } // end clang namespace

diff --git a/include/clang/Analysis/PathSensitive/GRExprEngine.h b/include/clang/Analysis/PathSensitive/GRExprEngine.h
index 1dee5d267adf6666a0b5f2494c3bef314ba85008..f53a9abaad8cc5b76c0737ee9ec1cc012217294b 100644 (file)
--- a/include/clang/Analysis/PathSensitive/GRExprEngine.h
+++ b/include/clang/Analysis/PathSensitive/GRExprEngine.h
@@ -98,7 +98,7 @@ protected:
   //   to lazily evaluate such logic.  The downside is that it eagerly
   //   bifurcates paths.
   const bool EagerlyAssume;
-  
+
 public:
   typedef llvm::SmallPtrSet<NodeTy*,2> ErrorNodes;  
   typedef llvm::DenseMap<NodeTy*, Expr*> UndefArgsTy;

diff --git a/lib/AST/ParentMap.cpp b/lib/AST/ParentMap.cpp
index 54472ecb963a98ddfcc16033855808f689ba0f2b..877b43b6de9234b3fc3cbd076b1259c11e75efee 100644 (file)
--- a/lib/AST/ParentMap.cpp
+++ b/lib/AST/ParentMap.cpp
@@ -45,3 +45,43 @@ Stmt* ParentMap::getParent(Stmt* S) const {
   MapTy::iterator I = M->find(S);
   return I == M->end() ? 0 : I->second;
 }
+
+bool ParentMap::isConsumedExpr(Expr* E) const {
+  Stmt *P = getParent(E);
+  Stmt *DirectChild = E;
+  
+  // Ignore parents that are parentheses or casts.
+  while (P && (isa<ParenExpr>(E) || isa<CastExpr>(E))) {
+    DirectChild = P;
+    P = getParent(P);
+  }
+  
+  if (!P)
+    return false;
+  
+  switch (P->getStmtClass()) {
+    default:
+      return isa<Expr>(P);
+    case Stmt::DeclStmtClass:
+      return true;
+    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;
+  }
+}
+

diff --git a/lib/Analysis/CheckDeadStores.cpp b/lib/Analysis/CheckDeadStores.cpp
index 504de3c97dce0c75fcaa20c5ecfd91c4b0ef2557..d1b3be511a8a41605aa48bc9b74cca4da621567f 100644 (file)
--- a/lib/Analysis/CheckDeadStores.cpp
+++ b/lib/Analysis/CheckDeadStores.cpp
@@ -38,10 +38,7 @@ public:
     : Ctx(ctx), BR(br), Parents(parents) {}
   
   virtual ~DeadStoreObs() {}
-  
-  bool isConsumedExpr(Expr* E) const;
-  
-  
+
   void Report(VarDecl* V, DeadStoreKind dsk, SourceLocation L, SourceRange R) {
 
     std::string name = V->getNameAsString();
@@ -148,7 +145,7 @@ public:
               return;
             
           // Otherwise, issue a warning.
-          DeadStoreKind dsk = isConsumedExpr(B)
+          DeadStoreKind dsk = Parents.isConsumedExpr(B)
                               ? Enclosing 
                               : (isIncrement(VD,B) ? DeadIncrement : Standard);
           
@@ -163,7 +160,7 @@ public:
       //  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() && isConsumedExpr(U))
+      if (U->isPrefix() && Parents.isConsumedExpr(U))
         return;
 
       Expr *Ex = U->getSubExpr()->IgnoreParenCasts();
@@ -218,43 +215,6 @@ public:
   
 } // 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.
 //===----------------------------------------------------------------------===//

diff --git a/lib/Analysis/GRExprEngine.cpp b/lib/Analysis/GRExprEngine.cpp
index 2b5808f562e9c3faa96729f297b06f88bfab314a..0e3d3ad2d4e3974803adf041887720b39e8eb4de 100644 (file)
--- a/lib/Analysis/GRExprEngine.cpp
+++ b/lib/Analysis/GRExprEngine.cpp
@@ -13,9 +13,9 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/AST/ParentMap.h"
 #include "clang/Analysis/PathSensitive/GRExprEngine.h"
 #include "clang/Analysis/PathSensitive/GRExprEngineBuilders.h"
-
 #include "clang/Analysis/PathSensitive/BugReporter.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/PrettyStackTrace.h"
@@ -1690,7 +1690,8 @@ void GRExprEngine::VisitObjCMessageExprDispatchHelper(ObjCMessageExpr* ME,
     
     if (isFeasibleNull) {
       // Check if the receiver was nil and the return value a struct.
-      if (ME->getType()->isRecordType()) {
+      if (ME->getType()->isRecordType() &&
+          BR.getParentMap().isConsumedExpr(ME)) {
         // The [0 ...] expressions will return garbage.  Flag either an
         // explicit or implicit error.  Because of the structure of this
         // function we currently do not bifurfacte the state graph at

diff --git a/test/Analysis/rdar-6600344-nil-receiver-undefined-struct-ret.m b/test/Analysis/rdar-6600344-nil-receiver-undefined-struct-ret.m
index d1f0802abf8e64bcacd9efb03720816bce9d0ff8..5d1fa37c46a845b7ecb22d2e63dd397631b456c2 100644 (file)
--- a/test/Analysis/rdar-6600344-nil-receiver-undefined-struct-ret.m
+++ b/test/Analysis/rdar-6600344-nil-receiver-undefined-struct-ret.m
@@ -17,3 +17,9 @@ void createFoo() {
   Bar f = [obj foo]; // expected-warning{{The receiver in the message expression is 'nil' and results in the returned value (of type 'Bar') to be garbage or otherwise undefined.}}
 }
 
+void createFoo2() {
+  MyClass *obj = 0;  
+  [obj foo]; // no-warning
+  Bar f = [obj foo]; // expected-warning{{The receiver in the message expression is 'nil' and results in the returned value (of type 'Bar') to be garbage or otherwise undefined.}}
+}
+