[analyzer] PR8962 again. Ban ParenExprs (and friends) from block-level expressions (by calling IgnoreParens before adding expressions to blocks). Undo 132769 (LiveVariables' local IgnoreParens), since it's no longer necessary.

Also, have Environment stop looking through NoOp casts; it didn't match the behavior of LiveVariables. And once that's gone, the whole cast block of that switch is unnecessary.


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

5 files changed, 41 insertions, 42 deletions

diff --git a/lib/Analysis/AnalysisContext.cpp b/lib/Analysis/AnalysisContext.cpp
index ddc5e88703..678f02fd71 100644
--- a/lib/Analysis/AnalysisContext.cpp
+++ b/lib/Analysis/AnalysisContext.cpp
@@ -78,16 +78,16 @@ void AnalysisContext::registerForcedBlockExpression(const Stmt *stmt) {
   if (!forcedBlkExprs)
     forcedBlkExprs = new CFG::BuildOptions::ForcedBlkExprs();
   // Default construct an entry for 'stmt'.
-  if (const ParenExpr *pe = dyn_cast<ParenExpr>(stmt))
-    stmt = pe->IgnoreParens();
+  if (const Expr *e = dyn_cast<Expr>(stmt))
+    stmt = e->IgnoreParens();
   (void) (*forcedBlkExprs)[stmt];
 }
 
 const CFGBlock *
 AnalysisContext::getBlockForRegisteredExpression(const Stmt *stmt) {
   assert(forcedBlkExprs);
-  if (const ParenExpr *pe = dyn_cast<ParenExpr>(stmt))
-    stmt = pe->IgnoreParens();
+  if (const Expr *e = dyn_cast<Expr>(stmt))
+    stmt = e->IgnoreParens();
   CFG::BuildOptions::ForcedBlkExprs::const_iterator itr = 
     forcedBlkExprs->find(stmt);
   assert(itr != forcedBlkExprs->end());
diff --git a/lib/Analysis/CFG.cpp b/lib/Analysis/CFG.cpp
index 621adb3e51..3e540203ea 100644
--- a/lib/Analysis/CFG.cpp
+++ b/lib/Analysis/CFG.cpp
@@ -397,6 +397,8 @@ private:
     if (alwaysAdd(S))
       cachedEntry->second = B;
 
+    // All block-level expressions should have already been IgnoreParens()ed.
+    assert(!isa<Expr>(S) || cast<Expr>(S)->IgnoreParens() == S);
     B->appendStmt(const_cast<Stmt*>(S), cfg->getBumpVectorContext());
   }
   void appendInitializer(CFGBlock *B, CXXCtorInitializer *I) {
@@ -841,11 +843,14 @@ void CFGBuilder::prependAutomaticObjDtorsWithTerminator(CFGBlock* Blk,
 ///   blocks for ternary operators, &&, and ||.  We also process "," and
 ///   DeclStmts (which may contain nested control-flow).
 CFGBlock* CFGBuilder::Visit(Stmt * S, AddStmtChoice asc) {
-tryAgain:
   if (!S) {
     badCFG = true;
     return 0;
   }
+
+  if (Expr *E = dyn_cast<Expr>(S))
+    S = E->IgnoreParens();
+
   switch (S->getStmtClass()) {
     default:
       return VisitStmt(S, asc);
@@ -957,10 +962,6 @@ tryAgain:
     case Stmt::ObjCForCollectionStmtClass:
       return VisitObjCForCollectionStmt(cast<ObjCForCollectionStmt>(S));
 
-    case Stmt::ParenExprClass:
-      S = cast<ParenExpr>(S)->getSubExpr();
-      goto tryAgain;
-
     case Stmt::NullStmtClass:
       return Block;
 
@@ -3049,6 +3050,7 @@ static BlkExprMapTy* PopulateBlkExprMap(CFG& cfg) {
       if (!CS)
         continue;
       if (Expr* Exp = dyn_cast<Expr>(CS->getStmt())) {
+        assert((Exp->IgnoreParens() == Exp) && "No parens on block-level exps");
 
         if (BinaryOperator* B = dyn_cast<BinaryOperator>(Exp)) {
           // Assignment expressions that are not nested within another
@@ -3056,13 +3058,16 @@ static BlkExprMapTy* PopulateBlkExprMap(CFG& cfg) {
           // another expression.
           if (B->isAssignmentOp() && !SubExprAssignments.count(Exp))
             continue;
-        } else if (const StmtExpr* Terminator = dyn_cast<StmtExpr>(Exp)) {
+        } else if (const StmtExpr* SE = dyn_cast<StmtExpr>(Exp)) {
           // Special handling for statement expressions.  The last statement in
           // the statement expression is also a block-level expr.
-          const CompoundStmt* C = Terminator->getSubStmt();
+          const CompoundStmt* C = SE->getSubStmt();
           if (!C->body_empty()) {
+            const Stmt *Last = C->body_back();
+            if (const Expr *LastEx = dyn_cast<Expr>(Last))
+              Last = LastEx->IgnoreParens();
             unsigned x = M->size();
-            (*M)[C->body_back()] = x;
+            (*M)[Last] = x;
           }
         }
 
@@ -3076,8 +3081,8 @@ static BlkExprMapTy* PopulateBlkExprMap(CFG& cfg) {
     Stmt* S = (*I)->getTerminatorCondition();
 
     if (S && M->find(S) == M->end()) {
-        unsigned x = M->size();
-        (*M)[S] = x;
+      unsigned x = M->size();
+      (*M)[S] = x;
     }
   }
 
diff --git a/lib/Analysis/LiveVariables.cpp b/lib/Analysis/LiveVariables.cpp
index 0fe87e8719..7b36f85ab6 100644
--- a/lib/Analysis/LiveVariables.cpp
+++ b/lib/Analysis/LiveVariables.cpp
@@ -142,12 +142,8 @@ void TransferFuncs::Visit(Stmt *S) {
     if (AD.Observer)
       AD.Observer->ObserveStmt(S, currentBlock, AD, LiveState);
 
-    if (getCFG().isBlkExpr(S)) {
-      if (Expr *E = dyn_cast<Expr>(S)) 
-        LiveState(E->IgnoreParens(), AD) = Dead;
-      else
-        LiveState(S, AD) = Dead;
-    }
+    if (getCFG().isBlkExpr(S))
+      LiveState(S, AD) = Dead;
 
     StmtVisitor<TransferFuncs,void>::Visit(S);
   }
@@ -161,10 +157,7 @@ void TransferFuncs::Visit(Stmt *S) {
   }
   else {
     // For block-level expressions, mark that they are live.
-    if (Expr *E = dyn_cast<Expr>(S)) 
-      LiveState(E->IgnoreParens(), AD) = Alive;
-    else
-      LiveState(S, AD) = Alive;
+    LiveState(S, AD) = Alive;
   }
 }
   
@@ -181,9 +174,6 @@ void TransferFuncs::VisitTerminator(CFGBlock* B) {
     return;
 
   assert (getCFG().isBlkExpr(E));
-
-  if (const Expr *Ex = dyn_cast<Expr>(E))
-    E = Ex->IgnoreParens();
   LiveState(E, AD) = Alive;
 }
 
diff --git a/lib/StaticAnalyzer/Core/Environment.cpp b/lib/StaticAnalyzer/Core/Environment.cpp
index e9f5016b36..48f126bfd8 100644
--- a/lib/StaticAnalyzer/Core/Environment.cpp
+++ b/lib/StaticAnalyzer/Core/Environment.cpp
@@ -77,21 +77,6 @@ SVal Environment::getSVal(const Stmt *E, SValBuilder& svalBuilder,
       // For special C0xx nullptr case, make a null pointer SVal.
       case Stmt::CXXNullPtrLiteralExprClass:
         return svalBuilder.makeNull();
-      case Stmt::ImplicitCastExprClass:
-      case Stmt::CXXFunctionalCastExprClass:
-      case Stmt::CStyleCastExprClass: {
-        // We blast through no-op casts to get the descendant
-        // subexpression that has a value.
-        const CastExpr* C = cast<CastExpr>(E);
-        QualType CT = C->getType();
-        if (CT->isVoidType())
-          return UnknownVal();
-        if (C->getCastKind() == CK_NoOp) {
-          E = C->getSubExpr();
-          continue;
-        }
-        break;
-      }
       case Stmt::ExprWithCleanupsClass:
         E = cast<ExprWithCleanups>(E)->getSubExpr();
         continue;
diff --git a/test/Analysis/misc-ps.c b/test/Analysis/misc-ps.c
index 84992c6bb2..bef5b06181 100644
--- a/test/Analysis/misc-ps.c
+++ b/test/Analysis/misc-ps.c
@@ -62,3 +62,22 @@ int PR8962_d (int *t) {
   return *t; // no-warning
 }
 
+int PR8962_e (int *t) {
+  // Redundant casts can mess things up!
+  // Environment used to skip through NoOp casts, but LiveVariables didn't!
+  if (({ (t ? (int)(int)0L : (int)(int)1L); })) return 0;
+  return *t; // no-warning
+}
+
+int PR8962_f (int *t) {
+  // The StmtExpr isn't a block-level expression here,
+  // the __extension__ is. But the value should be attached to the StmtExpr
+  // anyway. Make sure the block-level check is /before/ IgnoreParens.
+  if ( __extension__({
+    _Bool r;
+    if (t) r = 0;
+    else r = 1;
+    r;
+  }) ) return 0;
+  return *t; // no-warning
+}