[cfe-dev] [PATCH] Clang Static Analyzer support for temporary destructors

[Manuel Klimek]

FYI, I figured ResolveCondition out (at least I think so). I'll send you a
patch adding some code comments, where you can correct my understanding.
Alex and I also now believe we have a plan how to fix the overall problem.


On Tue, Apr 29, 2014 at 2:16 PM, Manuel Klimek <klimek at google.com> wrote:

> On Tue, Apr 29, 2014 at 3:27 AM, Jordan Rose <jordan_rose at apple.com>wrote:
>
>>
>> On Apr 28, 2014, at 9:35 , Manuel Klimek <klimek at google.com> wrote:
>>
>> Hi Jordan,
>>
>> I have looked into the remaining problems today; let me quickly summarize
>> so we make sure we're on the same page.
>> There are basically 2 problems left:
>> 1. The invariant
>> As you mentioned on the rollback CL, the current layout of the CFG
>> regarding temporary destructors violates the invariants assumed by other
>> parts of the analyzer: namely, a CFG block with a terminator must not be
>> empty.
>> After looking into it, I fully agree that we need a principled solution
>> here; the question is: do we want to keep the invariant, and fix the CFG
>> with temporary destructors to match the invariant, or do we want to say the
>> invariant doesn't hold, and we should fix the rest of the code to not
>> assume that invariant (and work with the current layout)? (I don't even
>> know whether the former is possible theoretically).
>> Any hints on what your gut tells you the right solutions is would be
>> helpful, and minimize dead-ends ;)
>> How would you want the CFG to look here?
>>
>>
>> I'm not actually sure why we care that a CFG block with a terminator is
>> not empty. It might be because we expect the terminator condition to have
>> been computed within the block, and that it's trying to catch mistakes in
>> constructing the CFG. The possible things that could be relying on this:
>>
>> - maybe we ask for the first CFGElement unilaterally if the block isn't
>> the entrance or exit block
>> - worse, maybe we assume the block *is* the entrance or exit block if
>> it's "empty" (since the terminator isn't included in the element count)
>> - other things I haven't thought of
>>
>> But really I don't think this is an important invariant, and if the right
>> solution to (2) involves removing it, then so be it.
>>
>
> Ok, I've tried to look into what exactly is happening in
> ExprEngine.cpp:1355, ResolveCondition.
> I have not yet found out what ResolveCondition should actually do :)
> It seems like if it's called with a logical binary operator, it's always
> called with the TerminatorCondition of the given CFGBlock.
> Most of the time, it will just return that condition (namely if the
> condition is the last statement in the block).
> If the condition is not the last statement in the block, it will drill
> down through the condition into the innermost RHS of logical binary
> operators, and then return that.
>
> Can you explain what's going on here? (That's the point where the analyzer
> currently assumes the block is not empty, assert(I != E) triggers).
>
> Thanks!
> /Manuel
>
>
>>
>>
>>
>> 2. Taking all the correct branches
>> So far I'm not sure I have seen a case where we're not running into (1),
>> but I'm not sure (the case where the temporary with the no-return dtor is
>> the last in a sequence of binary logical ops seems to be (1) at least).
>> The correct solution seems to be to add tracking lifetime of temporaries
>> in conditionals correctly. alexmc is working on a patch here.
>>
>>
>> I agree; it's the particular value of "correctly" that makes the problem
>> difficult. ;-) Very glad Alex has been able to spend time on this.
>>
>> Jordan
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140429/ccd8d25b/attachment.html>