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.

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

diff --git a/lib/StaticAnalyzer/Core/ExprEngine.cpp b/lib/StaticAnalyzer/Core/ExprEngine.cpp
index fd77e710b8351c0883da7939ae09c96b8d2667ba..b582749a02a83b4e263c95a64c7862aa653baacd 100644 (file)
--- a/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/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/test/Analysis/temporaries.cpp b/test/Analysis/temporaries.cpp
index 3bb88c4d4eebaa103b98c58ba69f32cab06e6ce4..c57d984a1dc8458bd595e34eb9e14fddb2d5bacb 100644 (file)
--- a/test/Analysis/temporaries.cpp
+++ b/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
 }