On Thu, Oct 18, 2012 at 1:20 PM, David Blaikie <[email protected]> wrote:
> On Thu, Oct 18, 2012 at 1:15 PM, Jan Voung <[email protected]> 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 <[email protected]> > wrote: > >> > >> On Thu, Oct 18, 2012 at 9:22 AM, Robert Muth <[email protected]> > wrote: > >> > On Tue, Oct 16, 2012 at 1:47 PM, David Blaikie <[email protected]> > >> > wrote: > >> >> On Tue, Oct 16, 2012 at 10:09 AM, Robert Muth <[email protected]> > >> >> wrote: > >> >>> On Tue, Oct 16, 2012 at 11:47 AM, David Blaikie <[email protected] > > > >> >>> wrote: > >> >>>> On Tue, Oct 16, 2012 at 8:12 AM, Robert Muth <[email protected]> > >> >>>> wrote: > >> >>>>> On Mon, Oct 15, 2012 at 10:57 PM, Rafael Espíndola > >> >>>>> <[email protected]> wrote: > >> >>>>>> On 15 October 2012 19:02, Robert Muth <[email protected]> > 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 > > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
