<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Apr 30, 2014 at 5:34 AM, Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.</blockquote>
<div><br></div><div>Well, all I've been trying to do here is to a) document and b) encode in the assert the actual current CFG invariant the analyzer relies on for correctness.</div><div>The problem is that if what is returned from ResolveCondition is *not* evaluated in the block, we have no guarantee that it was evaluated at all - but the correctness of all the path analyses relies on it being in the SVal cache of the state. Otherwise we will assume branches are reachable that are clearly unreachable because of which parts of the logical operator tree we *assumed* to be true (so we know the condition must be statically knowable in the current state).</div>
<div><br></div><div>I have a hard time putting that into words though, so any help for how to wordsmith this to be easier to understand would be highly appreciated...</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
(Especially if/when this whole block of code can go away!)<br></blockquote><div><br></div><div>I'm not sure what you mean about "this whole block of code can go away". The only part that will go away is the special case handling for temporary dtor terminators, as those will be handled in a completely different code path.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I'm concerned about the test change Alex noted but otherwise this looks good.<br>
<br>
================<br>
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1355<br>
@@ -1354,1 +1354,3 @@<br>
<br>
+const Stmt* GetRightmostLeaf(const Stmt* Condition) {<br>
+  while (Condition) {<br>
----------------<br>
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.)<br>
<div class=""><br>
================<br>
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1389<br>
@@ -1363,1 +1388,3 @@<br>
<br>
+  // FIXME: This is a workaround until we handle temporary destructor branches<br>
+  // correctly; currently, temporary destructor branches lead to blocks that<br>
----------------<br>
</div><div class="">Alex McCarthy wrote:<br>
> Consider linking to <a href="http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-April/036653.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-April/036653.html</a> for context<br>
</div>No, we try not to put links or PR numbers in the source code itself. Commit messages, sure.<br>
<br>
================<br>
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1410<br>
@@ -1377,13 +1409,3 @@<br>
       continue;<br>
-    if (CS->getStmt() != Condition)<br>
-      break;<br>
-    return Condition;<br>
-  }<br>
-<br>
-  assert(I != E);<br>
-<br>
-  while (Condition) {<br>
-    BO = dyn_cast<BinaryOperator>(Condition);<br>
-    if (!BO || !BO->isLogicalOp())<br>
-      return Condition;<br>
-    Condition = BO->getRHS()->IgnoreParens();<br>
+    const clang::Stmt* LastStmt = CS->getStmt();<br>
+    assert(LastStmt == Condition || LastStmt == GetRightmostLeaf(Condition));<br>
----------------<br>
Style nitpicks: no "clang::", asterisk next to the variable name.<br>
<div class=""><br>
================<br>
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1414<br>
@@ -1390,3 +1413,3 @@<br>
   }<br>
   llvm_unreachable("could not resolve condition");<br>
 }<br>
----------------<br>
</div><div class="">Alex McCarthy wrote:<br>
> What happens if we actually hit an llvm_unreachable? Does it die like an assert?<br>
><br>
> If not, maybe assert before this?<br>
</div>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.<br>
<div class=""><br>
================<br>
Comment at: test/Analysis/temporaries.cpp:193<br>
@@ -194,3 +192,3 @@<br>
<br>
-    FIXME: This shouldn't cause a warning.<br>
-    if (compute(i == 5 &&<br>
+    if (check(NoReturnDtor()) &&<br>
+        compute(i == 5 &&<br>
----------------<br>
</div><div class="">Alex McCarthy wrote:<br>
> Out of curiosity, why the change to this test case?<br>
</div>Seconded; all the existing tests are designed to tickle past bugs, and I'd rather not change any of the conditions.<br>
<br>
<a href="http://reviews.llvm.org/D3552" target="_blank">http://reviews.llvm.org/D3552</a><br>
<br>
<br>
</blockquote></div><br></div></div>