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

[Alex McCarthy]

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=18159 by
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 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.
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/20140429/504ecf67/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Current-WIP-destructor-tracking-work.patch
Type: application/octet-stream
Size: 6506 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140429/504ecf67/attachment.obj>