<div dir="ltr"><div class="gmail_quote"><div>On the code review thread, Sanjoy mentioned:</div><div><br></div><div>(I hope the formatting isn't too broken)</div><div><div>>>! In D29006#653587, @sanjoy wrote:</div><div>> Hi Chandler,</div><div>> </div><div>> I'm not sure this is a good idea.</div><div>> </div><div>> If this is the *only* path forward then I'll be okay going ahead with</div><div>> it, but I think `AssertingVH` (not unlike most other similar things)</div><div>> is useful and provides a superset of functionality provided by</div><div>> sanitizers.</div><div><br></div><div>Note that the biggest advantage IMO is the ability to error immediately on the deletion as that points to the cause of the bug. Anything we build here will instead point to the point of use which is a bit unfortunate. ASan's heap-use-after-free is actually better at this because it stores the stack trace for the deletion and shows it. I don't really want to try to rebuild that functionality.</div><div><br></div><div>> </div><div>>> For example, a debug-build-only</div><div>>> WeakVH and an explicit assert that it hasn't gone null when it is *used*</div><div>>> so that it can still *exist* even when the value goes away.</div><div>> </div><div>> But we don't *have* such a thing today, and for that reason this</div><div>> change is a strict decrease in debug-ability.  If you wanted to add</div><div>> something like that, and replace existing `AssertingVH` uses with it</div><div>> then I may feel better about this; however `AssertingVH` is stronger</div><div>> than what you suggest since `AssertingVH` catches cases where we've</div><div>> keyed a hash table off `AssertingVH` s.  To be on part with that</div><div>> functionality, we'd also need to write data structures like `DenseMap`</div><div>> that can be "poisoned" by a `CallbackVH` key.</div></div><div><br></div><div>As I mention above, I actually think AssertingVH is stronger than ASan because it triggers the error on deletion.</div><div><br></div><div>ASan will use a quarantine to work quite hard to avoid accidental re-use of an entry for a newly allocated pointer. So most bugs with hash tables I would still expect something like ASan to uncover. The only things it won't find are when getting a fresh address masks the bug *and* you don't ever re-use the stale key in any way. That is a hole in the ASan based approach, although historically I have not had this be a frequent failure mode. But that's just me, and I've honestly not worked on SCEV or LVI enough to have any intuition about those passes.</div><div><br></div><div>Anyways, I have built the replacement. It's somewhat awful IMO, but it will at least cover the case above. It will be somewhat harder to debug than an ASan failure as it won't give a backtrace for the deletion, but you can set a breakpoint to get that in a debugger.</div><div><br></div><div>Patch adding a "poisoning" VH: <a href="https://reviews.llvm.org/D29061">https://reviews.llvm.org/D29061</a></div><div>Updated patch using it: <a href="https://reviews.llvm.org/D29006">https://reviews.llvm.org/D29006</a></div><div><br></div><div>Thoughts?</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000" class="gmail_msg">
  </div></blockquote></div></div>