[cfe-commits] clang patch for bug 14021

[David Blaikie]

On Thu, Oct 18, 2012 at 1:15 PM, Jan Voung <jvoung at chromium.org> wrote:
> Attached is a creduce'd test case which compiles without any clang errors,
> but would have broken the assertion that "lock_hack == 0" that was in
> Robert's patch.

Better - and certainly not something that's totally unreasonable to
commit, but it's helpful if the test case is as simple as possible so
that, should it fail, there's not a lot of unrelated noise to
investigate. Could you reduce this (by hand or automatically) further?
I assume that's not the simplest it could possibly be. (though I'm
happy to be corrected on that matter)

> Well, I changed it to a print and grepped for the print so
> that it wouldn't crash clang before clang had a chance to print other
> errors.

I'm not quite sure what you mean by this. We don't intend to commit
that hack & so an assert seems like it'd be fine... the simplest
program that produces that assertion is what we want for this test
case I think.

>
>
>
>
>
>
> On Thu, Oct 18, 2012 at 9:59 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> 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
>
>