On Thu, Oct 18, 2012 at 2:24 PM, David Blaikie <[email protected]> wrote:
> On Thu, Oct 18, 2012 at 2:07 PM, Jan Voung <[email protected]> wrote: > > > > 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. > > Thanks muchly. > > Okay here's a 100 line one. Now it only triggers a single call of TryUserDefinedConversion and asserts on the second iteration. > > >> > >> > 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. > > Oh, fair enough - those autoreducers do have a tendency to go off into > the weeds of invalid code. > > While you've got that hack applied - have you tried running the entire > regression suite? You might find existing test cases cover this bug, > in which case no test would be required (though a nice reduction might > be handy anyway, if you end up with it). > I tried running check-all with the hack applied, but it didn't trigger any new unexpected failures. Doug Gregor mentioned in code review of the commit that we might want > to look at other callers into this function to see if they're doing > similar things (we could temporarily add in the same flag > raising/lowering logic to check that those calls are robust too). > > But Richard Smith has some completely other idea about how to fix this > issue, which might invalidate that sort of investigation. > > > > > - 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 > >> > > >> > > > > > >
PR14021.ii
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
