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

[David Blaikie]

On Thu, Oct 18, 2012 at 1:50 PM, Douglas Gregor <dgregor at apple.com> wrote:
>
> On Oct 18, 2012, at 1: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.
>
> That seems unlikely to help; this is only a problem when the underlying StoredDeclsList has to reallocate.

The hack attached to the bug is to raise a flag on the DenseMap at the
start of the loop & assert within DenseMap if that flag is raised
while any mutation operation is done - so it should fire even without
having to coax a reallocation into the operation. Not something to
commit, but useful for reducing the test case to represent the
problem, even if it doesn't actually fail. (one day it'd be nice to
add some debug checked iterators in - and on that day the test case
will make more sense (test case still hasn't been committed, working
with Robert & folks to get something of a reasonable size/reduction to
commit - see the review thread for more detail/discussion))

- David