r186925 - Revert "[analyzer] Add very limited support for temporary destructors"

[Jordan Rose]

On Jul 25, 2013, at 9:20 , Pavel Labath <labath at google.com> wrote:

>> On Tue, Jul 23, 2013 at 4:29 AM, Jordan Rose <jordan_rose at apple.com> wrote:
>> I did get around to reverting this, Pavel. Apart from PR16664, I realized that neither the binary operators nor their subexpressions are still live when the analyzer goes back through the chain of logical expressions to see which destructors to run. (You can see how this is working using the debug.DumpCFG checker.) Without the values previously computed for the expression, the analyzer won't actually be consistent about which branches to take.
>> 
>> I don't have an immediate solution in mind. It's sort of non-trivial to teach the liveness analysis that an expression is going to be revisited later, but the only alternative I can think of is crawling backwards through the evaluated blocks to find out which branch we took, which is a horrible idea because it could include inlined calls. If you come up with anything, please send it up for review!
>> 
> 
> Hello,
> 
> I agree that the solution to this problem will be not be trivial and will take a while. So, reverting it was certainly the right decision. However, as Manuel said, this feature is pretty important for us, so I really want to get it in somehow. I looked through the code some more and came up with two possible ways of solving it:
> 
> - "teach the liveness analysis that an expression is going to be revisited later": might be a bit hackish, but shouldn't be too hard, I think. I guess I would need to add some logic to SymbolReaper::isLive to keep the values of conditional operators alive longer. Or one could make it completely general and make sure a value is not deleted until an expression which references it is reachable. (This should be easy to detect -- the terms in the conditional destructor logic still reference the original conditional expressions).
> 
> - simplify the CFG: I find the constructed CFGs quite strange. instead of merging all the branches and then "evaluating" the expressions a again to work out which objects to destroy, one could just design the graph in a way that embeds this information directly in the control-flow edges. E.g., instead of (excuse the poor ascii art):
>   cond
>    /  \
>   /    \
> A()  B()
>   \    /
>    \  /
>   cond
>    /  \
>   /    \
> ~A() ~B()
>   \    /
>    \  /
>     ...
> 
> One could just make it:
>   cond
>    /  \
>   /    \
> A()  B()
>  |       |
> ~A() ~B()
>   \    /
>    \  /
>     ...
> 
> This will be a bigger change: it requires to change the way CFGs are constructed, probably will require the changes to other CFG users if this breaks any of their invariants, and it will still probably require changing some bits of the analyzer. However, I think I actually prefer this option: it addresses the issue at hand and at the same time it makes the CFG more logical (and a bit simpler), which could have other benefits.
> 
> Still, before I start doing that, I would like to hear what you (or anybody else reading this) think. Do you foresee any problems with designing the CFG like this? Which option seems better for you? Do you have any tips on the best way to approach this (for any of the options)? Or a completely different approach altogether? :)

Yeah...about that. Unfortunately, the paths for "A() || B()" don't actually look like what you wrote above; it's something more like this:

    |
   A()
   / \
  /   \
  |  B()
  |   |
  |  ~B()
  \   /
   \ /
   ~A()
    |

...and that's just to evaluate the condition; we still have to branch after that. And notice how this gets worse the more you stack logical expressions, because you may or may not have to evaluate destructors based on other parts of the condition.

(A() || (B() && C() && D()) || E())
could require
~A()
~A(), ~B(), ~C(), ~D()
~A(), ~B(), ~E()
~A(), ~B(), ~C(), ~E()
~A(), ~B(), ~C(), ~D(), ~E()

So the current strategy, which works fine for libAnalysis, is just to run through all the conditions again, which is at least linear, rather than try to be clever about joining up blocks ahead of time. The discussion that led to this is in PR11645, which I didn't realize wasn't yet concluded.

The current liveness algorithm lives in LiveVariables.cpp (despite the name it includes liveness info for statements and expressions as well). In most cases, an expression is only live when it's about to be consumed, which is the problem here. We don't want logical operators to be alive forever, but we'd need them all to be alive until we get a chance to run all of the destructors for that full-expression.

I don't have any ideas yet on this, but I'll keep it in the back of my head for a while, and let you know if I think of anything.

Jordan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130725/9a16b0c1/attachment.html>