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

[Robert Muth]

I started working on this - I'll post updates to this thread.

On Fri, Oct 19, 2012 at 1:21 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 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.