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

[Manuel Klimek]

On Thu, May 1, 2014 at 6:55 PM, Jordan Rose <jordan_rose at apple.com> wrote:

>
> On Apr 30, 2014, at 6:10 , Manuel Klimek <klimek at google.com> wrote:
>
> > I have two more question about how the static analyzer works:
> >
> > 1. how are evaluated conditions cached / invalidated?
> > If I have a block
> > [B42]
> > ...
> > T: if [B23.8]
> >
> > From reading processBranch, it looks like the correctness of the
> analysis of the terminator here relies on the result of B23.8 being
> available as an SVal if it was fully known at B23.8. If the result of this
> evaluation is guaranteed to be cached in the state, I'm not sure why doing
> the same for temporary destructor edges wouldn't work (apart from that it
> would be harder to implement, as we'd need to make sure the
> ResolveCondition returns the same part of the condition from the temp dtor
> terminator as for the terminator condition of the branch that created it,
> which is something we currently don't model).
> > If it's not guaranteed to be cached, I think we'd have a different
> problem, because the way temporary destructors are introduced when inside
> and if (condition && temp()), the evaluation of the condition && temp()
> part gets moved a few blocks away from the terminator that decides which
> if-branch to take. I have not been able to come up with an example that's
> problematic, though.
>
> I said this in the other e-mail, but an expression's value is intended to
> be live from the time it is computed to the time it is consumed. We also
> don't really have things set up for expressions to be consumed more than
> once.
>
> "Caching" really is the wrong word. The Environment stores everything that
> has been evaluated until the liveness analysis says it is no longer needed.
>
>
> >
> > 2. how does inlining work, and what are the actual invariants of what
> has the be inside the same block
> > Take the following from the dtor.cpp test:
> > struct CheckCustomDestructor {
> >   bool bool_;
> >   CheckCustomDestructor():bool_(true) {}
> >   ~CheckCustomDestructor();
> >   operator bool() const { return bool_; }
> > };
> > bool testUnnamedCustomDestructor() {
> >   if (CheckCustomDestructor c = CheckCustomDestructor())
> >     return true;
> >   return false;
> > }
> >
> > This is a regression test for a "return of garbage" warning from the
> bool operator. I tried to change the CFG in VisitExprWithCleanup to always
> insert a new block before the temporary dtor decision blocks (as that way
> the CFG looks more uniform regarding how terminator edges are handled).
> Funnily enough this breaks the above test (and a few other ones in
> similarly strange ways). The only difference in the CFG I see is that the
> last block is split into two blocks, just before the call to the temporary
> destructor. I have no idea why that would ever affect the copied bool
> inside 'c', which we later get warned on. In general, it seems like I don't
> have a good understanding of when putting two statements in different
> blocks will change how the analyzer sees them.
>
> Inlining essentially jumps to a different CFG's entry block, and then
> jumps back when the function is different, with some fix-up before and
> after. It doesn't affect liveness at all. I wouldn't think that the change
> you describe would actually affect anything here, but the "garbage
> collection" that cleans up the Environment, Store, and ConstraintManager
> does run when entering a new CFG block, among other times. ("On exit from a
> function", and the conditions listed in shouldRemoveDeadBindings in
> ExprEngine.cpp.)
>

Btw, I think this was a red herring and I discovered the bug to be an
ordering problem of the "assignment" statement and the temp dtor statement
in the CFG. Apparently the order doesn't matter as long as we're in the
same block, but starts to matter when we jump blocks in between.

See http://reviews.llvm.org/D3603 for an attempted fix.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140505/fa4c9552/attachment.html>