[cfe-commits] clang patch for bug 14021

Thu Oct 18 14:07:15 PDT 2012

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.

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

- 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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121018/86424e8b/attachment.html>