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

[Sanjoy Das via llvm-dev]

Hi Sean,

Sean Silva 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.

But asan won't catch problems (insofar I understand how it works) if
the free'ed BasicBlock is used as a key in a DenseMap or something --
if another BasicBlock gets allocated to the same location we'll end up
returning bad cached results.

I think the Right Solution(TM) here is to create a new wrapper around
(Small)DenseMap that in debug mode uses a CallbackVH in a way that:

  - The map is "poisoned" if a key is deleted.
  - A poisoned map can only be cleared; inserting, removing or looking
    up elements in a poisoned map fails a assertions.

If needed, we can allow erasing deleted keys from poisoned maps, but
then we won't catch bugs like:

   M[k] = v;
   delete k;
   k1 = new Key();  // allocated to the same location as k1
   M.erase(k1);  // "successful" deletion.

However, this quite a bit of work, and I'm not sure if the payoff will
be worth it.

-- Sanjoy

 > Thoughts? For the moment I have put in a workaround (r274457) that makes
 > jump-threading invalidate LVI.
 >
 > -- Sean Silva