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. 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. Since this activity is independent of getting my fix into clang: are there any other reservations preventing the fix from being committed? 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
