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
