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

[Jordan Rose]

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.

If we do this under a flag, what behavior should we use elsewhere? The existing thing, of visiting all the conditions again?

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