<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><div>
<blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div class="gmail_extra"><div class="gmail_quote">On Tue, Jul 23, 2013 at 4:29 AM, Jordan Rose<span> </span><span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span><span> </span>wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div>I did get around to reverting this, Pavel. Apart from <a href="http://llvm.org/bugs/show_bug.cgi?id=16664" target="_blank">PR16664</a>, 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.</div>
<div><br></div><div>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!</div>
<span><font color="#888888"><div><br></div></font></span></div></blockquote></div></div></div></blockquote></div></div></div></div></blockquote></div></div></div></div></div></blockquote></div><br></div><div class="gmail_extra">
Hello,</div><div class="gmail_extra"><br></div><div class="gmail_extra">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:</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">- "<span style="font-family:Helvetica;font-size:12px">teach the liveness analysis that an expression is going to be revisited later": </span>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).</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">- 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):</div>
<div class="gmail_extra"> cond</div><div class="gmail_extra"> / \</div><div class="gmail_extra"> / \</div><div class="gmail_extra">A() B()</div><div class="gmail_extra"> \ /</div><div class="gmail_extra"> \ /</div>
<div class="gmail_extra"> cond</div><div class="gmail_extra"><div class="gmail_extra"> / \</div><div class="gmail_extra"> / \</div><div class="gmail_extra">~A() ~B()</div><div class="gmail_extra"> \ /</div><div class="gmail_extra">
\ /</div><div> ...</div><div><br></div><div>One could just make it:</div><div><div class="gmail_extra"> cond</div><div class="gmail_extra"> / \</div><div class="gmail_extra"> / \</div><div class="gmail_extra">
A() B()</div><div class="gmail_extra"> | |</div><div class="gmail_extra">~A() ~B()<br></div><div class="gmail_extra"><div class="gmail_extra"> \ /</div><div class="gmail_extra"> \ /</div><div> ...</div><div>
<br></div><div>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.</div>
<div><br></div><div>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? :)</div>
<div><br></div><div>cheers,</div><div>Pavel</div><div><br></div><div>PS: I still have one patch waiting for your review (<a href="http://llvm-reviews.chandlerc.com/D1133">http://llvm-reviews.chandlerc.com/D1133</a>) :D. Is that okay now, or do I need to change something else?</div>
</div></div></div></div>