Fix crash when resolving branch conditions for temporary destructor condition blocks.

Document and simplify ResolveCondition.

1. Introduce a temporary special case for temporary desctructors when resolving
the branch condition - in an upcoming patch, alexmc will change temporary
destructor conditions to not run through this logic, in which case we can remove
this (marked as FIXME); this currently fixes a crash.

2. Simplify ResolveCondition; while documenting the function, I noticed that it
always returns the last statement - either that statement is the condition
itself (in which case the condition was returned anyway), or the rightmost
leaf is returned; for correctness, the rightmost leaf must be evaluated anyway
(which the CFG does in the last statement), thus we can just return the last
statement in that case, too. Added an assert to verify the invariant.

llvm-svn: 207957

2 files changed, 52 insertions, 25 deletions

diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index fd77e710b83..b582749a02a 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1352,6 +1352,31 @@ static SVal RecoverCastedSymbol(ProgramStateManager& StateMgr,
   return state->getSVal(Ex, LCtx);
 }
 
+const Stmt *getRightmostLeaf(const Stmt *Condition) {
+  while (Condition) {
+    const BinaryOperator *BO = dyn_cast<BinaryOperator>(Condition);
+    if (!BO || !BO->isLogicalOp()) {
+      return Condition;
+    }
+    Condition = BO->getRHS()->IgnoreParens();
+  }
+  return nullptr;
+}
+
+// Returns the condition the branch at the end of 'B' depends on and whose value
+// has been evaluated within 'B'.
+// In most cases, the terminator condition of 'B' will be evaluated fully in
+// the last statement of 'B'; in those cases, the resolved condition is the
+// given 'Condition'.
+// If the condition of the branch is a logical binary operator tree, the CFG is
+// optimized: in that case, we know that the expression formed by all but the
+// rightmost leaf of the logical binary operator tree must be true, and thus
+// the branch condition is at this point equivalent to the truth value of that
+// rightmost leaf; the CFG block thus only evaluates this rightmost leaf
+// expression in its final statement. As the full condition in that case was
+// not evaluated, and is thus not in the SVal cache, we need to use that leaf
+// expression to evaluate the truth value of the condition in the current state
+// space.
 static const Stmt *ResolveCondition(const Stmt *Condition,
                                     const CFGBlock *B) {
   if (const Expr *Ex = dyn_cast<Expr>(Condition))
@@ -1361,6 +1386,12 @@ static const Stmt *ResolveCondition(const Stmt *Condition,
   if (!BO || !BO->isLogicalOp())
     return Condition;
 
+  // FIXME: This is a workaround until we handle temporary destructor branches
+  // correctly; currently, temporary destructor branches lead to blocks that
+  // only have a terminator (and no statements). These blocks violate the
+  // invariant this function assumes.
+  if (B->getTerminator().isTemporaryDtorsBranch()) return Condition;
+
   // For logical operations, we still have the case where some branches
   // use the traditional "merge" approach and others sink the branch
   // directly into the basic blocks representing the logical operation.
@@ -1375,18 +1406,9 @@ static const Stmt *ResolveCondition(const Stmt *Condition,
     Optional<CFGStmt> CS = Elem.getAs<CFGStmt>();
     if (!CS)
       continue;
-    if (CS->getStmt() != Condition)
-      break;
-    return Condition;
-  }
-
-  assert(I != E);
-
-  while (Condition) {
-    BO = dyn_cast<BinaryOperator>(Condition);
-    if (!BO || !BO->isLogicalOp())
-      return Condition;
-    Condition = BO->getRHS()->IgnoreParens();
+    const Stmt *LastStmt = CS->getStmt();
+    assert(LastStmt == Condition || LastStmt == getRightmostLeaf(Condition));
+    return LastStmt;
   }
   llvm_unreachable("could not resolve condition");
 }
diff --git a/clang/test/Analysis/temporaries.cpp b/clang/test/Analysis/temporaries.cpp
index 3bb88c4d4ee..c57d984a1dc 100644
--- a/clang/test/Analysis/temporaries.cpp
+++ b/clang/test/Analysis/temporaries.cpp
@@ -118,13 +118,11 @@ namespace destructors {
     extern bool coin();
     extern bool check(const Dtor &);
 
-#ifndef TEMPORARY_DTORS
-    // FIXME: Don't assert here when tmp dtors are enabled.
+    // Regression test: we used to assert here when tmp dtors are enabled.
     // PR16664 and PR18159
     if (coin() && (coin() || coin() || check(Dtor()))) {
       Dtor();
     }
-#endif
   }
 
 #ifdef TEMPORARY_DTORS
@@ -170,17 +168,16 @@ namespace destructors {
     clang_analyzer_eval(true); // no warning, unreachable code
   }
 
-/*
+  // Regression test: we used to assert here.
   // PR16664 and PR18159
-  FIXME: Don't assert here.
   void testConsistencyNested(int i) {
     extern bool compute(bool);
-  
+
     if (i == 5 && (i == 4 || i == 5 || check(NoReturnDtor())))
-      clang_analyzer_eval(true); // expected TRUE
-  
+      clang_analyzer_eval(true);  // expected-warning{{TRUE}}
+
     if (i == 5 && (i == 4 || i == 5 || check(NoReturnDtor())))
-      clang_analyzer_eval(true);  // expected TRUE
+      clang_analyzer_eval(true);  // expected-warning{{TRUE}}
 
     if (i != 5)
       return;
@@ -189,17 +186,17 @@ namespace destructors {
                 (i == 4 || compute(true) ||
                  compute(i == 5 && (i == 4 || check(NoReturnDtor()))))) ||
         i != 4) {
-      clang_analyzer_eval(true); // expected TRUE
+      clang_analyzer_eval(true);  // expected-warning{{TRUE}}
     }
 
-    FIXME: This shouldn't cause a warning.
     if (compute(i == 5 &&
                 (i == 4 || i == 4 ||
                  compute(i == 5 && (i == 4 || check(NoReturnDtor()))))) ||
         i != 4) {
-      clang_analyzer_eval(true); // no warning, unreachable code
+      // FIXME: This shouldn't cause a warning.
+      clang_analyzer_eval(true);  // expected-warning{{TRUE}}
     }
-  }*/
+  }
 
   // PR16664 and PR18159
   void testConsistencyNestedSimple(bool value) {
@@ -229,6 +226,14 @@ namespace destructors {
     }
   }
 
+  void testBinaryOperatorShortcut(bool value) {
+    if (value) {
+      if (false && false && check(NoReturnDtor()) && true) {
+        clang_analyzer_eval(true);
+      }
+    }
+  }
+
 #endif // TEMPORARY_DTORS
 }