[llvm-dev] Should analyses be able to hold AssertingVH to IR? (related to PR28400)

[Sean Silva via llvm-dev]

On Tue, Jul 5, 2016 at 6:56 PM, Sean Silva <chisophugis at gmail.com> wrote:

> While building test-suite with the new PM, I ran into problems with
> AssertingVH being triggered which is obvious in retrospect:
> https://llvm.org/bugs/show_bug.cgi?id=28400
>
> Both cases I ran into revolve around LVI which holds AssertingVH.
> Essentially, what happens is this:
>
> 1. LVI holds an AssertingVH pointing at a BasicBlock
> 2. Some other pass ends up deleting that BB (e.g. SimplifyCFG)
> 3. BOOM
>
> Notice that this ends up happening even though SimplifyCFG ultimately
> invalidates LVI. But that only happens after the AssertingVH has been
> triggered.
>
> We would avoid this issue in principle by just switching those AssertingVH
> to observing pointers, but then we would have dangling pointers. I'm pretty
> sure that any analysis that keeps pointers to IR is in fact ending up with
> dangling pointers like this.
>
> I've noticed that AssumptionCache using CallbackVH that allow it to update
> its data structures correctly in this case.
>
> Just using regular pointers will end up with dangling pointers in this
> scenario. This isn't a problem in practice since the analysis will
> hopefully be invalidated and stop holding the dangling pointers, but it
> just seems weird. Thankfully, ASan can generally catch problems if we do
> try to access through any of these dangling pointers.
>
> Thoughts? For the moment I have put in a workaround (r274457) that makes
> jump-threading invalidate LVI.
>

Is everybody happy with this workaround? Although I mentioned it a couple
times in this thread nobody seems to have explicitly commented on it.

This issue does manifest in almost any non-trivial pipeline containing
affected analyses (analyses that use AssertingVH to reference pieces of IR;
in particular, LazyValueInfo, ScalarEvolution, and CallGraph) and this
workaround seems simple enough for the moment.

One caveat I ran into is that if you ever manually
`require<scalar-evolution>` (as you would before entering a loop pass
manager with a loop pass that needs ScalarEvolution), you need to remember
to `invalidate<scalar-evolution>` after running the loop passes to make
sure it gets invalidated.

-- Sean Silva


>
> -- Sean Silva
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160808/d33423b8/attachment-0001.html>