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

[David Blaikie]

On Mon, Oct 29, 2012 at 7:15 AM, Robert Muth <robertm at google.com> wrote:
> Here is an update:
>
> The good news:
> I have CL that instruments DenseMap to check for iterator invalidation,
> no other file needs to be touched to turn it on.
>
> The bad news:
> it will not catch the bug which was the subject of this CL.
> The problem is that the typical DenseMap usage  patterns is some like this:
> * look up a key in the DenseMap.
> * this returns a (primary) iterator
> * use "iterator->second" to immediately look up what the "mappess"
> * this results in two secondary iterators denoting the range of "mappees"
>
> It is those secondary iterators which are being invalidated.
> Unfortunately, they are not real iterators but just "pointers",
> so there is no easy way to hook them.
> I will keep looking into this but this will likely take more time.

While I really appreciate your putting the work in to help add general
iterator (in)validation to the DenseMap type, my interest in the short
term was just having a trivial, commit-able test case (even if it only
fires while using those uncommitted hacks for checking the safety of
this particular use case). I see the last version posted was 100 lines
- that's pretty close, but I would expect it could be much smaller
still. I haven't had a chance to look at it myself, but can do so if
necessary/desirable.

As for the attempt to improve DenseMap validation: it sounds like what
you're saying is code is relying on the validity of DenseMap values
over DenseMap insertions & since the uses of the value aren't part of
the DenseMap itself there's no way to check them or their lifetime
against the use of the DenseMap itself.

That seems like a fundamental limitation - did you have some idea
about how you might approach it? because nothing comes to my mind,
really, so I wouldn't worry about it too much. Perhaps we can build in
some kind of primitives for ubisan one day so that these kinds of
relationships could be expressed.

- David

>
>
> On Fri, Oct 19, 2012 at 1:26 PM, Robert Muth <robertm at google.com> wrote:
>> 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.