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

[Jordan Rose]

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

Jordan

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