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

Wed Apr 30 01:22:33 PDT 2014

On Tue, Apr 29, 2014 at 8:05 PM, Alex McCarthy <alexmc at google.com> wrote:

> Jordan/Ted, could you comment on the approach described below? Manuel,
> please correct any mistakes in my write up :)
>
> Manuel did some excellent debugging over the last couple of days, and this
> morning he and I sold ourselves on an approach to fix the temporary
> destructor issue described in http://llvm.org/bugs/show_bug.cgi?id=18159by following Jordan's recommendation to track temporary construction.
> Instead of changing the liveliness algorithms, we'll write patches that
> make the following changes:
>
> 1. Add new test cases to test/Analysis/temporaries.cpp that change boolean
> values during condition evaluation (e.g. if (!a || !b || !(a = false) ||
> NoReturnDtr()) )
> 2. Add a flag to wrap the new temporary tracking behavior introduced
> below, off by default
> 3. Track temporary creation in ProgramState whenever we process
> a CXXBindTemporaryExpr, similar to how we handle static variable
> initialization (sample patch attached, nowhere near ready to commit)
> 4. Modify VisitBinaryOperatorForTemporaryDtors (lib/Analysis/CFG.cpp:3512)
> and VisitConditionalOperatorForTemporaryDtors (lib/Analysis/CFG.cpp:3624)
> to set the temporary cleanup block's terminator to be the
> CXXBindTemporaryExpr instead of a conditional only if the new flag is
> turned on (exact implementation might differ slightly, maybe by using a
> different expr/stmt class)
> 5. Add a new helper to call a temporary object's destructor if the
> temporary object is tracked in
>

I think "call the destructor" is somewhat confusing - I think the idea is
to generate the exploded state that visits the block that calls the
desctructor iff the constructor was executed (true-branch), and visits the
false-branch otherwise. This can be basically done equivalently to how we
handle known Condition SVals in processBranch.

> ProgramState, similar to CoreEngine::HandleBranch
> (lib/StaticAnalyzer/Core/CoreEngine.cpp:453) and ExprEngine::processBranch
> (lib/StaticAnalyzer/Core/ExprEngine.cpp:1400). Alternative: modify
> processBranch to add special case logic for temporary destructor branching.
> 6. Modify HandleBlockExit (lib/StaticAnalyzer/Core/CoreEngine.cpp:345) to
> handle blocks with a CXXBindTemporaryExpr terminator by calling the new
> helper described above
> 7. Fix the assert caused by blocks with no statements and terminator
> conditions that fires in ResolveCondition
> (lib/StaticAnalyzer/Core/ExprEngine.cpp:1383). Manuel's working on
> understanding this better, and it might have a separate resolution.
>

I think with the method described above, this assert will not fire any more
(in fact, I think we'll add a new assert ;)

> 8. Do some large-scale testing, then set both the new temporary tracking
> behavior flag and the include-temporary-dtors flag to true by default if
> things look good.
>
> What do you think?
>
> -Alex
>
>
> On Tue, Apr 29, 2014 at 8:51 AM, Manuel Klimek <klimek at google.com> wrote:
>
>> 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/20140430/28ed630a/attachment.html>