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

[klimek]

Summary

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; no tests
break - please let me know if I'm missing obvious test cases here, or this
doesn't make sense for some other reason.

[alexmc]

Awesome, thanks for this fix, Manuel!

lib/StaticAnalyzer/Core/ExprEngine.cpp

1389

Consider linking to http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-April/036653.html for context

1392

funcion -> function

1414

What happens if we actually hit an llvm_unreachable? Does it die like an assert?

If not, maybe assert before this?

test/Analysis/temporaries.cpp

193

Out of curiosity, why the change to this test case?

[jordan_rose]

What this is basically saying is that we will never be in a situation where the condition (or the rightmost branch in the condition) was not evaluated in the current CFG block. That seems a bit questionable in general but reasonable given the way the CFG is currently constructed. (Especially if/when this whole block of code can go away!)

I'm concerned about the test change Alex noted but otherwise this looks good.

lib/StaticAnalyzer/Core/ExprEngine.cpp

1355

Please follow the new LLVM style with lowercase function names. This can also be static. (And the asterisks go on the right of the space.)

1389

No, we try not to put links or PR numbers in the source code itself. Commit messages, sure.

1410

Style nitpicks: no "clang::", asterisk next to the variable name.

1414

It's like an assert when assertions are enabled, but acts as an optimization hint in NDEBUG builds. So it's slightly better than an assert.

test/Analysis/temporaries.cpp

193

Seconded; all the existing tests are designed to tickle past bugs, and I'd rather not change any of the conditions.

[klimek]

Review comment changes.

lib/StaticAnalyzer/Core/ExprEngine.cpp

1355

Done.

1392

Done.

1410

Done.

test/Analysis/temporaries.cpp

193

Good catch - that was a leftover from debugging, I didn't intend to change it.
Changed the test back (it still breaks, so I added the wrong expectation and a FIXME).

lib/StaticAnalyzer/Core/ExprEngine.cpp

static SVal RecoverCastedSymbol(ProgramStateManager& StateMgr,
 
   if (!bitsInit || !T->isIntegralOrEnumerationType() ||
       Ctx.getTypeSize(T) > bits)
     return UnknownVal();
 
   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))
     Condition = Ex->IgnoreParens();
 
   const BinaryOperator *BO = dyn_cast<BinaryOperator>(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 funcion 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.
   // We need to distinguish between those two cases here.
 
   // The invariants are still shifting, but it is possible that the
   // last element in a CFGBlock is not a CFGStmt.  Look for the last
   // CFGStmt as the value of the condition.
   CFGBlock::const_reverse_iterator I = B->rbegin(), E = B->rend();
   for (; I != E; ++I) {
     CFGElement Elem = *I;
     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 clang::Stmt* LastStmt = CS->getStmt();
+    assert(LastStmt == Condition || LastStmt == GetRightmostLeaf(Condition));
+    return LastStmt;
   }
   llvm_unreachable("could not resolve condition");
 }
 
 void ExprEngine::processBranch(const Stmt *Condition, const Stmt *Term,
                                NodeBuilderContext& BldCtx,
                                ExplodedNode *Pred,
                                ExplodedNodeSet &Dst,
                                const CFGBlock *DstT,
                                const CFGBlock *DstF) {

test/Analysis/temporaries.cpp

 namespace destructors {
   void testPR16664andPR18159Crash() {
     struct Dtor {
       ~Dtor();
     };
     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
   struct NoReturnDtor {
     ~NoReturnDtor() __attribute__((noreturn));
   };
 
   void noReturnTemp(int *x) {
void testConsistencyTernary(int i) {
     if (i != 5)
       return;
 
     (i == 5 && (i == 4 || check(NoReturnDtor()) || i == 5)) ? 1 : 0;
 
     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;
 
     if (compute(i == 5 &&
                 (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 &&
+    if (check(NoReturnDtor()) &&
+        compute(i == 5 &&
                 (i == 4 || i == 4 ||
                  compute(i == 5 && (i == 4 || check(NoReturnDtor()))))) ||
         i != 4) {
       clang_analyzer_eval(true); // no warning, unreachable code
     }
-  }*/
+  }
 
   // PR16664 and PR18159
   void testConsistencyNestedSimple(bool value) {
     if (value) {
       if (!value || check(NoReturnDtor())) {
         clang_analyzer_eval(true); // no warning, unreachable code
       }
     }
#ifdef TEMPORARY_DTORS
   void testConsistencyNestedWarning(bool value) {
     if (value) {
       if (!value || value || check(NoReturnDtor())) {
         clang_analyzer_eval(true); // expected-warning{{TRUE}}
       }
     }
   }
 
+  void testBinaryOperatorShortcut(bool value) {
+    if (value) {
+      if (false && false && check(NoReturnDtor()) && true) {
+        clang_analyzer_eval(true);
+      }
+    }
+  }
+
 #endif // TEMPORARY_DTORS
 }
 
 void testStaticMaterializeTemporaryExpr() {
   static const Trivial &ref = getTrivial();
   clang_analyzer_eval(ref.value == 42); // expected-warning{{TRUE}}
 
   static const Trivial &directRef = Trivial(42);