[PATCH] RFC: fail-fast iterators for DenseMap

Daniel Berlin dberlin at dberlin.org
Sun Mar 1 00:18:09 PST 2015


fwiw: there is no void erase(iterator) in SmallPtrSet at all, though there
is in DenseMap.

(and yes, I agree we should change densemap's and we should just fix the
unsafe code)

So there is no safe way to erase while iterating *at all* in smallptrset
that i know of (maybe i missed something?)

I hit this while converting something to smallptrset the other day.
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.





On Fri, Feb 27, 2015 at 8:15 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2015 Feb 26, at 22:29, Sanjoy Das <sanjoy at playingwithpointers.com>
> wrote:
> >
> > Hi ruiu, dexonsmith, dberlin,
> >
> > This patch is an attempt at making `DenseMapIterator`s "fail-fast".
> > Fail-fast iterators that have been invalidated due to insertion into
> > the host `DenseMap` deterministically trip an assert (in debug mode)
> > on access, instead of non-deterministically hitting memory corruption
> > issues.
> >
> > (This exact same thing can be done for `SmallPtrSet`s, but that is a
> > later change.)
> >
> > This change is fairly rough at the moment, but I want to get some
> > early feedback on the approach and style before I spend too much time
> > on this.
> >
> > This change, in its current form, has already helped me fix two
> > existing bugs:
> >
> > http://reviews.llvm.org/rL230718
> > http://reviews.llvm.org/rL230719
> >
> > http://reviews.llvm.org/D7931
> >
>
> +chandlerc, since we had a discussion about SmallPtrSet a few months
> ago.
>
> I'm in favour of this.
>
> Morever, I'd go further.  We should bump the epoch on `DenseMap`s and
> `SmallPtrSet`s on `erase()` calls as well.
>
> This would break a lot of existing code, so it should be as a second
> step, but the code would be fixed by deleting the generally unsafe:
>
>     void erase(iterator);
>
> in favour of the safer:
>
>     iterator erase(iterator);
>
> and updating code that looks correct (but isn't):
>
>     for (auto I = M.begin(), E = M.end(); I != E;) {
>       auto Current = I++;
>       if (foo(*Current))
>         M.erase(Current); // Or: M.erase(*Current);
>     }
>
> to the safer:
>
>     for (auto I = M.begin(), E = M.end(); I != E;) {
>       if (foo(*I))
>         I = M.erase(I);
>       else
>         ++I;
>     }
>
> Note that the former version is *incorrect* in `SmallPtrSet`,
> since in small mode this will skip over an element in the
> iteration (since `SmallPtrSetImplBase::erase_imp()` swaps the
> last element with the erased one).
>
>
> > Files:
> >  include/llvm/ADT/ADTModCountTracker.h
> >  include/llvm/ADT/DenseMap.h
> >
> > EMAIL PREFERENCES
> >  http://reviews.llvm.org/settings/panel/emailpreferences/
> > <D7931.20826.patch>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150301/1838e443/attachment.html>


More information about the llvm-commits mailing list