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
