[cfe-commits] clang patch for bug 14021

[David Blaikie]

On Thu, Oct 18, 2012 at 2:07 PM, Jan Voung <jvoung at chromium.org> wrote:
>
> On Thu, Oct 18, 2012 at 1:20 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> 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)
>>
>
> Okay, making some progress manually reducing the test.  I'll upload another
> version when I get it even smaller.

Thanks muchly.

>
>>
>> > 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.
>>
>
> Oh, I just meant that I tweaked Robert's patch locally, to know when the
> assert *would have* fired without actually having clang stop, so that clang
> could continue and check that the rest of the test-case was still valid c++.
> That was just for the purpose of reducing the test case.

Oh, fair enough - those autoreducers do have a tendency to go off into
the weeds of invalid code.

While you've got that hack applied - have you tried running the entire
regression suite? You might find existing test cases cover this bug,
in which case no test would be required (though a nice reduction might
be handy anyway, if you end up with it).

Doug Gregor mentioned in code review of the commit that we might want
to look at other callers into this function to see if they're doing
similar things (we could temporarily add in the same flag
raising/lowering logic to check that those calls are robust too).

But Richard Smith has some completely other idea about how to fix this
issue, which might invalidate that sort of investigation.

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