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

[Manuel Klimek]

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

Cheers,
/Manuel


>
> Jordan
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140505/3cfb79ed/attachment.html>