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