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 <dblai...@gmail.com> wrote: > On Thu, Oct 18, 2012 at 9:22 AM, Robert Muth <robe...@google.com> wrote: > > On Tue, Oct 16, 2012 at 1:47 PM, David Blaikie <dblai...@gmail.com> > wrote: > >> On Tue, Oct 16, 2012 at 10:09 AM, Robert Muth <robe...@google.com> > wrote: > >>> On Tue, Oct 16, 2012 at 11:47 AM, David Blaikie <dblai...@gmail.com> > wrote: > >>>> On Tue, Oct 16, 2012 at 8:12 AM, Robert Muth <robe...@google.com> > wrote: > >>>>> On Mon, Oct 15, 2012 at 10:57 PM, Rafael Espíndola > >>>>> <rafael.espind...@gmail.com> wrote: > >>>>>> On 15 October 2012 19:02, Robert Muth <robe...@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 >
PR14021.ii
Description: Binary data
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits