[cfe-commits] clang patch for bug 14021

[Robert Muth]

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.
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.

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

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