[cfe-commits] clang patch for bug 14021

[David Blaikie]

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.

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