<div dir="ltr"><div>fwiw: there is no void erase(iterator) in SmallPtrSet at all, though there is in DenseMap.</div><div><br></div><div>(and yes, I agree we should change densemap's and we should just fix the unsafe code)</div><div><br></div><div>So there is no safe way to erase while iterating *at all* in smallptrset that i know of (maybe i missed something?)</div><div><br></div><div>I hit this while converting something to smallptrset the other day.</div><div>I marked it as "to fix" and my workaround was to insert the members i wasn't erasing into a set, then swap the old set with the new one.</div><div><br></div><div><br></div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 27, 2015 at 8:15 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
> On 2015 Feb 26, at 22:29, Sanjoy Das <<a href="mailto:sanjoy@playingwithpointers.com">sanjoy@playingwithpointers.com</a>> wrote:<br>
><br>
> Hi ruiu, dexonsmith, dberlin,<br>
><br>
> This patch is an attempt at making `DenseMapIterator`s "fail-fast".<br>
> Fail-fast iterators that have been invalidated due to insertion into<br>
> the host `DenseMap` deterministically trip an assert (in debug mode)<br>
> on access, instead of non-deterministically hitting memory corruption<br>
> issues.<br>
><br>
> (This exact same thing can be done for `SmallPtrSet`s, but that is a<br>
> later change.)<br>
><br>
> This change is fairly rough at the moment, but I want to get some<br>
> early feedback on the approach and style before I spend too much time<br>
> on this.<br>
><br>
> This change, in its current form, has already helped me fix two<br>
> existing bugs:<br>
><br>
> <a href="http://reviews.llvm.org/rL230718" target="_blank">http://reviews.llvm.org/rL230718</a><br>
> <a href="http://reviews.llvm.org/rL230719" target="_blank">http://reviews.llvm.org/rL230719</a><br>
><br>
> <a href="http://reviews.llvm.org/D7931" target="_blank">http://reviews.llvm.org/D7931</a><br>
><br>
<br>
</span>+chandlerc, since we had a discussion about SmallPtrSet a few months<br>
ago.<br>
<br>
I'm in favour of this.<br>
<br>
Morever, I'd go further.  We should bump the epoch on `DenseMap`s and<br>
`SmallPtrSet`s on `erase()` calls as well.<br>
<br>
This would break a lot of existing code, so it should be as a second<br>
step, but the code would be fixed by deleting the generally unsafe:<br>
<br>
    void erase(iterator);<br>
<br>
in favour of the safer:<br>
<br>
    iterator erase(iterator);<br>
<br>
and updating code that looks correct (but isn't):<br>
<br>
    for (auto I = M.begin(), E = M.end(); I != E;) {<br>
      auto Current = I++;<br>
      if (foo(*Current))<br>
        M.erase(Current); // Or: M.erase(*Current);<br>
    }<br>
<br>
to the safer:<br>
<br>
    for (auto I = M.begin(), E = M.end(); I != E;) {<br>
      if (foo(*I))<br>
        I = M.erase(I);<br>
      else<br>
        ++I;<br>
    }<br>
<br>
Note that the former version is *incorrect* in `SmallPtrSet`,<br>
since in small mode this will skip over an element in the<br>
iteration (since `SmallPtrSetImplBase::erase_imp()` swaps the<br>
last element with the erased one).<br>
<span class=""><br>
<br>
> Files:<br>
>  include/llvm/ADT/ADTModCountTracker.h<br>
>  include/llvm/ADT/DenseMap.h<br>
><br>
> EMAIL PREFERENCES<br>
>  <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
</span>> <D7931.20826.patch><br>
<br>
</blockquote></div><br></div>