[cfe-commits] clang patch for bug 14021

Thu Oct 18 13:15:04 PDT 2012

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

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