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

[Jordan Rose]

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