<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jul 11, 2016 at 7:51 PM, Sanjoy Das <span dir="ltr"><<a href="mailto:sanjoy@playingwithpointers.com" target="_blank">sanjoy@playingwithpointers.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Sean,<span class=""><br>
<br>
Sean Silva wrote:<br>
> While building test-suite with the new PM, I ran into problems with<br>
> AssertingVH being triggered which is obvious in retrospect:<br>
> <a href="https://llvm.org/bugs/show_bug.cgi?id=28400" rel="noreferrer" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=28400</a><br>
><br>
> Both cases I ran into revolve around LVI which holds AssertingVH.<br>
> Essentially, what happens is this:<br>
><br>
> 1. LVI holds an AssertingVH pointing at a BasicBlock<br>
> 2. Some other pass ends up deleting that BB (e.g. SimplifyCFG)<br>
> 3. BOOM<br>
><br>
> Notice that this ends up happening even though SimplifyCFG ultimately<br>
> invalidates LVI. But that only happens after the AssertingVH has been<br>
> triggered.<br>
><br>
> We would avoid this issue in principle by just switching those<br>
> AssertingVH to observing pointers, but then we would have dangling<br>
> pointers. I'm pretty sure that any analysis that keeps pointers to IR is<br>
> in fact ending up with dangling pointers like this.<br>
><br>
> I've noticed that AssumptionCache using CallbackVH that allow it to<br>
> update its data structures correctly in this case.<br>
><br>
> Just using regular pointers will end up with dangling pointers in this<br>
> scenario. This isn't a problem in practice since the analysis will<br>
> hopefully be invalidated and stop holding the dangling pointers, but it<br>
> just seems weird. Thankfully, ASan can generally catch problems if we do<br>
> try to access through any of these dangling pointers.<br>
<br></span>
But asan won't catch problems (insofar I understand how it works) if<br>
the free'ed BasicBlock is used as a key in a DenseMap or something --<br>
if another BasicBlock gets allocated to the same location we'll end up<br>
returning bad cached results.<br></blockquote><div><br></div><div>ASan's allocator is specifically hardened against reusing memory, which mitigates this, but I'm not sure by how much (but I think by a significant amount).</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I think the Right Solution(TM) here is to create a new wrapper around<br>
(Small)DenseMap that in debug mode uses a CallbackVH in a way that:<br>
<br>
 - The map is "poisoned" if a key is deleted.<br>
 - A poisoned map can only be cleared; inserting, removing or looking<br>
   up elements in a poisoned map fails a assertions.<br>
<br>
If needed, we can allow erasing deleted keys from poisoned maps, but<br>
then we won't catch bugs like:<br>
<br>
  M[k] = v;<br>
  delete k;<br>
  k1 = new Key();  // allocated to the same location as k1<br>
  M.erase(k1);  // "successful" deletion.<br>
<br>
However, this quite a bit of work, and I'm not sure if the payoff will<br>
be worth it.</blockquote><div><br></div><div>Hmm.. interesting idea but yeah that seems like a lot of work. From my basic testing, it seems like LVI is the only analysis that uses this pattern so I'm especially not sure it is worth it for a single analysis (unless we want to change more analyses to use this pattern).</div><div><br></div><div>There's a relatively simple workaround for now which is manually invalidating LVI at the end of passes that use it (JumpThreading and CorrelatedValuePropagation).</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="HOEnZb"><font color="#888888"><br>
<br>
-- Sanjoy</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
> Thoughts? For the moment I have put in a workaround (r274457) that makes<br>
> jump-threading invalidate LVI.<br>
><br>
> -- Sean Silva<br>
</div></div></blockquote></div><br></div></div>