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

[Pavel Labath]

>
>   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<http://llvm.org/bugs/show_bug.cgi?id=16664>,
>>> 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? :)

cheers,
Pavel

PS: I still have one patch waiting for your review (
http://llvm-reviews.chandlerc.com/D1133) :D. Is that okay now, or do I need
to change something else?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130725/b4f2b71e/attachment.html>