[cfe-commits] r166188 - /cfe/trunk/lib/Sema/SemaInit.cpp

[David Blaikie]

On Fri, Oct 19, 2012 at 10:10 AM, Matthieu Monrocq
<matthieu.monrocq at gmail.com> wrote:
>
>
> On Thu, Oct 18, 2012 at 11:59 PM, Robert Muth <robertm at google.com> wrote:
>>
>> On Thu, Oct 18, 2012 at 4:48 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> > On Thu, Oct 18, 2012 at 1:45 PM, Douglas Gregor <dgregor at apple.com>
>> > wrote:
>> >>
>> >> On Oct 18, 2012, at 9:57 AM, David Blaikie <dblaikie at gmail.com> 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.