[PATCH] RFC: fail-fast iterators for DenseMap

Duncan P. N. Exon Smith dexonsmith at apple.com
Sun Mar 1 08:23:08 PST 2015


> On 2015 Mar 1, at 00:18, Daniel Berlin <dberlin at dberlin.org> wrote:
> 
> 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?)

Right.  There is no safe way to erase while iterating right now.

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

Ugly, but safe :).

> 
> 
> 
> 
> 
> 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>
> 
> 





More information about the llvm-commits mailing list