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

[Manuel Klimek]

On Mon, May 5, 2014 at 7:32 PM, Manuel Klimek <klimek at google.com> wrote:

> On Mon, May 5, 2014 at 6:37 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>
>>
>> On May 5, 2014, at 6:02 , Manuel Klimek <klimek at google.com> wrote:
>>
>> On Thu, May 1, 2014 at 7:16 PM, Jordan Rose <jordan_rose at apple.com>wrote:
>>
>>>
>>> On Apr 29, 2014, at 11:05 , 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 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?
>>>
>>>
>>> It sounds about right to me, but (4) is interesting. Ted and Anna and I
>>> have talked about similar things before. It's a good solution for the
>>> analyzer, but it might make the rest of the compiler unhappy. IIRC, all of
>>> the existing compiler flow-sensitive analyses *do* include temporary
>>> destructors right now. They probably aren't doing anything so special with
>>> them, but changing the CFG so that CXXBindTemporaryExpr can be a terminator
>>> is something that potentially affects all clients.
>>>
>>
>> Given that the terminator expression with temp dtors inside logical
>> binary ops is basically always not evaluated, thus not in the Environment,
>> I don't expect much fallout from clang's warnings.
>> In the end, I hope we have tests for those warnings that we can just run,
>> and fix the warnings that break? :)
>>
>>
>> Well, Clang's warnings don't evaluate things in the same way as the
>> analyzer. The few warnings that *do* track any sort of evaluation (like
>> -Wsometimes-uninitialized, IIRC?) can probably handle the current CFG, and
>> probably wouldn't be able to handle the new one.
>>
>
> I think the DeadCode one does (that might be the same you mention, I'll
> take a look). I'd be surprised if any check would be correct with the
> current CFG and temporary destructors in logical operators if it matters
> for the check (and if there are no logical ops, I'd assume all temp dtors
> are unconditionally called anyway, so there'll not be any terminators?)
>
>
>> I have a hard time thinking of which warnings the presence of temporary
>> destructors would affect, which unfortunately probably means we don't have
>> many tests. But on the other hand, that means we probably won't affect
>> anything important.
>>
>
> Yea, I concur that I'd hope people have put a test on anything they really
> care about :) but that's also why the proposal is to use an
> experimental-flag first, and have it run on at least our internal codebase,
> which will
>
already validate a ton of warnings...
>

Btw, from taking a quick look, I see 2 compiler-based warnings that pull
the statement out of a CFGTerminator (I think all others cannot be affected
by the change by design, right?):
ReachableCode.cpp and Consumed.cpp.
I think we'll need to take a good look at both anyway, at least
Consumed.cpp already seems to have problems when we encounter terminators
for temp dtor decisions.

Cheers,
/Manuel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140506/354fbdc0/attachment.html>