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

[Manuel Klimek]

On Wed, Apr 30, 2014 at 5:34 AM, Jordan Rose <jordan_rose at apple.com> wrote:

> 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.


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.
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).

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...

(Especially if/when this whole block of code can go away!)
>

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.


>
> I'm concerned about the test change Alex noted but otherwise this looks
> good.
>
> ================
> Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1355
> @@ -1354,1 +1354,3 @@
>
> +const Stmt* GetRightmostLeaf(const Stmt* Condition) {
> +  while (Condition) {
> ----------------
> 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.)
>
> ================
> Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1389
> @@ -1363,1 +1388,3 @@
>
> +  // FIXME: This is a workaround until we handle temporary destructor
> branches
> +  // correctly; currently, temporary destructor branches lead to blocks
> that
> ----------------
> Alex McCarthy wrote:
> > Consider linking to
> http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-April/036653.html for
> context
> No, we try not to put links or PR numbers in the source code itself.
> Commit messages, sure.
>
> ================
> Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1410
> @@ -1377,13 +1409,3 @@
>        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));
> ----------------
> Style nitpicks: no "clang::", asterisk next to the variable name.
>
> ================
> Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1414
> @@ -1390,3 +1413,3 @@
>    }
>    llvm_unreachable("could not resolve condition");
>  }
> ----------------
> Alex McCarthy wrote:
> > What happens if we actually hit an llvm_unreachable? Does it die like an
> assert?
> >
> > If not, maybe assert before this?
> 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.
>
> ================
> Comment at: test/Analysis/temporaries.cpp:193
> @@ -194,3 +192,3 @@
>
> -    FIXME: This shouldn't cause a warning.
> -    if (compute(i == 5 &&
> +    if (check(NoReturnDtor()) &&
> +        compute(i == 5 &&
> ----------------
> Alex McCarthy wrote:
> > Out of curiosity, why the change to this test case?
> Seconded; all the existing tests are designed to tickle past bugs, and I'd
> rather not change any of the conditions.
>
> http://reviews.llvm.org/D3552
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140430/d9e382c4/attachment.html>