[cfe-commits] clang patch for bug 14021

[Jan Voung]

On Thu, Oct 18, 2012 at 2:24 PM, David Blaikie <dblaikie at gmail.com> wrote:

> 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.
>
>
Okay here's a 100 line one.  Now it only triggers a single call of
TryUserDefinedConversion and asserts on the second iteration.


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

I tried running check-all with the hack applied, but it didn't trigger any
new unexpected failures.


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
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121018/5f26dbc8/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR14021.ii
Type: application/octet-stream
Size: 2261 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121018/5f26dbc8/attachment.obj>