I started working on this - I'll post updates to this thread.
On Fri, Oct 19, 2012 at 1:21 PM, David Blaikie <[email protected]> wrote: > On Fri, Oct 19, 2012 at 10:10 AM, Matthieu Monrocq > <[email protected]> wrote: >> >> >> On Thu, Oct 18, 2012 at 11:59 PM, Robert Muth <[email protected]> wrote: >>> >>> On Thu, Oct 18, 2012 at 4:48 PM, David Blaikie <[email protected]> wrote: >>> > On Thu, Oct 18, 2012 at 1:45 PM, Douglas Gregor <[email protected]> >>> > wrote: >>> >> >>> >> On Oct 18, 2012, at 9:57 AM, David Blaikie <[email protected]> wrote: >>> >> >>> >>> Author: dblaikie >>> >>> Date: Thu Oct 18 11:57:32 2012 >>> >>> New Revision: 166188 >>> >>> >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=166188&view=rev >>> >>> Log: >>> >>> PR14021: Copy lookup results to ensure safe iteration. >>> >>> >>> >>> Within the body of the loop the underlying map may be modified via >>> >>> >>> >>> Sema::AddOverloadCandidate >>> >>> -> Sema::CompareReferenceRelationship >>> >>> -> Sema::RequireCompleteType >>> >>> >>> >>> to avoid the use of invalid iterators the sequence is copied first. >>> >> >>> >> Did you audit other uses of LookupConstructors to ensure that this is >>> >> the only ticking time bomb in this area? >>> > >>> > [+Robert Muth] >>> > >>> > I've not performed any such audit, no. With the hack debug check >>> > inserted we could just run the whole test suite to see if anything >>> > pops up. >>> >>> I did not perform any such audit either. I feel those are only of limited >>> use >>> and the time would be better spend addressing the root of the issue. >>> If there is interest I could try add some diagnostic code to the DenseMap >>> which would only be enabled in Debug mode. >>> We kicked around some ideas over lunch and here was one suggestion >>> that could work: >>> add an atomic timestamp to each DenseMap. >>> increment the time step whenever iterators are invalidated (e.g. >>> insertions), >>> Each iterator also holds a copy of the timestamp form when it was created >>> so we can compare it against the parent container timestamp when\ever >>> the iterator is used. >>> I have not thought this through all this much, so it may not work in the >>> end. >>> Any feedback would be welcome. >> >> >> >> I remember using such a trick. It covers the requiring of invalidating >> iterators quite well. Also, since it requires embedding a pointer to the >> original container within the iterator it also makes it possible whenever >> comparing two iterators whether they originated from the same container >> (comparing iterators from different containers does not make sense). >> >> However it seriously increases the footprint of the iterator (triples its >> size) and makes it impossible to mix Debug and Release libraries (because >> the types end up being different). > > I think we already have some members that are enable/disabled based on > DEBUG (given the -Wunused-member warning we have, it's sort of > encouraged otherwise you break the release build when you don't use > your debug variables) so that ship has probably already sailed. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
