On Jul 25, 2013, at 9:20 , Pavel Labath <[email protected]> wrote:
>> On Tue, Jul 23, 2013 at 4:29 AM, Jordan Rose <[email protected]> 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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
