[cfe-commits] clang patch for bug 14021

[David Blaikie]

On Thu, Oct 18, 2012 at 9:22 AM, Robert Muth <robertm at google.com> wrote:
> On Tue, Oct 16, 2012 at 1:47 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> On Tue, Oct 16, 2012 at 10:09 AM, Robert Muth <robertm at google.com> wrote:
>>> On Tue, Oct 16, 2012 at 11:47 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>>> On Tue, Oct 16, 2012 at 8:12 AM, Robert Muth <robertm at google.com> wrote:
>>>>> On Mon, Oct 15, 2012 at 10:57 PM, Rafael Espíndola
>>>>> <rafael.espindola at gmail.com> wrote:
>>>>>> On 15 October 2012 19:02, Robert Muth <robertm at google.com> wrote:
>>>>>>> No small test, sadly.
>>>>>>> I managed to cut a 10MB reproducer down to 2MB over 2h
>>>>>>> but no further.
>>>>>>
>>>>>> The testcase in the bug report is already a lot smaller than that.
>>>>>> Running delta a bit more reduced it to the attached file, which I am
>>>>>> sure can be reduced a bit more.
>>>>>
>>>>> The reproducer in the bug has a lot of problems, e.g.
>>>>> it is only a fragment - not valid C++ code.
>>>>> It is also not a very stable reproducers meaning changing the clang
>>>>> command line slightly
>>>>> will change clang's memory allocation enough that the bug does not get triggered
>>>>> anymore and instead you get a compiler failure because the code is
>>>>> only a fragment.
>>>>>
>>>>> This stability issue would likely effect any test for this problem,
>>>>> even valgrind based ones.
>>>>>
>>>>> Having thought  about this a little more think the best way to
>>>>> approach the problem of invalidated iterators
>>>>> is not by adding tests each time we find one but by addressing it at a
>>>>> higher level, e.g. compiling clang or llvm in a
>>>>> special debug mode, linking in special debug versions of STL and
>>>>> llvm/ADT/ that will cause crashes more reliably.
>>>>> Apparently VS has a feature like that, c.f.
>>>>> http://stackoverflow.com/questions/2062956/c-best-way-to-check-if-an-iterator-is-valid
>>>>
>>>> Fair point - Howard Hinnant started work on something like this for
>>>> libc++ but it's not been finished yet (put on the back burner). MSVC's
>>>> debug mode has really helped me on several occasions.
>>>>
>>>> Takumi (who runs most/all of the MSVC bots) - do any of your bots
>>>> already run with MSVC's iterator debugging turned up/on? Would that be
>>>> a reasonable task? (& interestingly: does it catch this bug)
>>>
>>> Without knowing much about how MSVC is accomplishing this,
>>> it probably relies on extra book keeping inside STL.
>>
>> That's correct, which means ITERATOR_DEBUG_LEVEL is ABI-changing (so
>> you can't interoperate STL-using APIs built with differing IDLs, but
>> they have some machinery that should usually give you a linker error
>> if you try). Howard wanted to implement something that would not be
>> ABI-changing and allow libraries with differing iterator debugging
>> levels to interoperate in a best-effort manner. This meant using
>> side-tables rather than in-structure bookkeeping.
>>
>>> The container implicated here , DenseMap, is not an STL container,
>>> though, so some extra
>>> work would be required to make it fit the MSVC framework.
>>
>> Ah, right - yeah, there are ways to plug into it but it's work. We
>> might consider implementing our own standalone version for this
>> instead.
>>
>>>> Robert - while that would help stabilize the behavior, would it
>>>> actually help catch this bug? Would it be possible for you to create a
>>>
>>> I am pretty sure it would catch this bug *reliably*, because
>>> the reproducer does add items to  DenseMap while iterating over it which
>>>  invalidates the iterators "de jure".
>>
>> That's all I'm interested in.
>>
>>> But this invalidation only becomes visible when it is accompanied by a resizing
>>> of the DenseMap and that seems to depend on the concrete program run, e.g.
>>> we can influence the resize trigger by changing the length of strings
>>> passed on clang command line.
>>
>> The visibility I don't mind about right now - that's an infrastructure
>> problem we'll need to fix & I wouldn't gate your fix on that.
>>
>> What I'm interested in is a reproduction of invalid code according to
>> DenseMap's contract, not its implementation. Basically a quick & dirty
>> way to do this would be to raise some global flag around the loop in
>> Sema::AddOverloadCandidate, then assert that that flag is not raised
>> when things are added to the container in Sema::RequireCompleteType (I
>> mention these two functions based on your analysis posted in the bug,
>> not my own investigation). Then reduce to the simplest (hopefully
>> valid) code that triggers the assert.
>>
>> You don't need/we won't commit this assert/flag, but we'll know that
>> this case reliably violates the DenseMap contract & if/when we add
>> some iterator invalidation checking we'll have a repro ready to go.
>
>
> David,
>
> I attached a set of llvm/clang patches to the bug that makes the
> failure reliable.

Thanks - they look roughly like what I had in mind. (I won't nitpick
them because they won't be committed - just a simple test hack)

> The patches  work with the existing reproducer (also attached).  A colleague of
> mine is preparing a "nicer" reproducer which he will add to the bug
> once it is ready.

Sounds good.

> Since this activity is independent  of getting my fix into clang: are
> there any other reservations
> preventing the  fix from being committed?

I don't have any. This seems fairly reasonable to me. Committed as r166188.

Please provide a test case at your earliest convenience.

- David



>
> Cheers,
> Robert
>
>
>>
>>>> reproduction that always violates the iterator invalidation contract
>>>> but doesn't necessarily crash? I wouldn't mind having that in the test
>>>> suite even in the absence of fancy iterator checkers being deployed
>>>> immediately. Though I'm not immediately sure how you'd construct such
>>>> a test case without those iterator checks. (maybe it could be ad-hoc'd
>>>
>>> The best I could do so far was a 2MB reproducer which was not reliable
>>> and since the reproducer is so large it is hard to get to the essence
>>> of the problem.
>>> I was hoping that one of the clang/c++ wizards would look at the diagnosis in
>>> http://llvm.org/bugs/show_bug.cgi?id=14021
>>> and go:
>>> "yaeh, of course there is a scenario that when you try to pick the right
>>> constructors for an expression, additional constructors are being
>>> added to the list of candidates because of some template instantiation."
>>>
>>>> up with a few well-placed assertions & temporary (non-committed) extra
>>>> flags just so you can track that the dangerous loop is running during
>>>> the modification)
>>>>
>>>> - David