<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Oct 18, 2012 at 2:24 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">On Thu, Oct 18, 2012 at 2:07 PM, Jan Voung <<a href="mailto:jvoung@chromium.org">jvoung@chromium.org</a>> wrote:<br>

><br>
> On Thu, Oct 18, 2012 at 1:20 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> On Thu, Oct 18, 2012 at 1:15 PM, Jan Voung <<a href="mailto:jvoung@chromium.org">jvoung@chromium.org</a>> wrote:<br>
>> > Attached is a creduce'd test case which compiles without any clang<br>
>> > errors,<br>
>> > but would have broken the assertion that "lock_hack == 0" that was in<br>
>> > Robert's patch.<br>
>><br>
>> Better - and certainly not something that's totally unreasonable to<br>
>> commit, but it's helpful if the test case is as simple as possible so<br>
>> that, should it fail, there's not a lot of unrelated noise to<br>
>> investigate. Could you reduce this (by hand or automatically) further?<br>
>> I assume that's not the simplest it could possibly be. (though I'm<br>
>> happy to be corrected on that matter)<br>
>><br>
><br>
> Okay, making some progress manually reducing the test.  I'll upload another<br>
> version when I get it even smaller.<br>
<br>
</div>Thanks muchly.<br>
<div class="im"><br></div></blockquote><div><br></div><div>Okay here's a 100 line one.  Now it only triggers a single call of</div><div>TryUserDefinedConversion and asserts on the second iteration.</div><div> </div><div>
<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">
><br>
>><br>
>> > Well, I changed it to a print and grepped for the print so<br>
>> > that it wouldn't crash clang before clang had a chance to print other<br>
>> > errors.<br>
>><br>
>> I'm not quite sure what you mean by this. We don't intend to commit<br>
>> that hack & so an assert seems like it'd be fine... the simplest<br>
>> program that produces that assertion is what we want for this test<br>
>> case I think.<br>
>><br>
><br>
> Oh, I just meant that I tweaked Robert's patch locally, to know when the<br>
> assert *would have* fired without actually having clang stop, so that clang<br>
> could continue and check that the rest of the test-case was still valid c++.<br>
> That was just for the purpose of reducing the test case.<br>
<br>
</div>Oh, fair enough - those autoreducers do have a tendency to go off into<br>
the weeds of invalid code.<br>
<br>
While you've got that hack applied - have you tried running the entire<br>
regression suite? You might find existing test cases cover this bug,<br>
in which case no test would be required (though a nice reduction might<br>
be handy anyway, if you end up with it).<br></blockquote><div><br></div><div>I tried running check-all with the hack applied, but it didn't trigger any</div><div>new unexpected failures.</div><div><br></div><div><br>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Doug Gregor mentioned in code review of the commit that we might want<br>
to look at other callers into this function to see if they're doing<br>
similar things (we could temporarily add in the same flag<br>
raising/lowering logic to check that those calls are robust too).<br>
<br>
But Richard Smith has some completely other idea about how to fix this<br>
issue, which might invalidate that sort of investigation.<br>
<div class=""><div class="h5"><br>
><br>
> - Jan<br>
><br>
>><br>
>> ><br>
>> ><br>
>> ><br>
>> ><br>
>> ><br>
>> ><br>
>> > On Thu, Oct 18, 2012 at 9:59 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Thu, Oct 18, 2012 at 9:22 AM, Robert Muth <<a href="mailto:robertm@google.com">robertm@google.com</a>><br>
>> >> wrote:<br>
>> >> > On Tue, Oct 16, 2012 at 1:47 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> > wrote:<br>
>> >> >> On Tue, Oct 16, 2012 at 10:09 AM, Robert Muth <<a href="mailto:robertm@google.com">robertm@google.com</a>><br>
>> >> >> wrote:<br>
>> >> >>> On Tue, Oct 16, 2012 at 11:47 AM, David Blaikie<br>
>> >> >>> <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> >>> wrote:<br>
>> >> >>>> On Tue, Oct 16, 2012 at 8:12 AM, Robert Muth <<a href="mailto:robertm@google.com">robertm@google.com</a>><br>
>> >> >>>> wrote:<br>
>> >> >>>>> On Mon, Oct 15, 2012 at 10:57 PM, Rafael Espíndola<br>
>> >> >>>>> <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br>
>> >> >>>>>> On 15 October 2012 19:02, Robert Muth <<a href="mailto:robertm@google.com">robertm@google.com</a>><br>
>> >> >>>>>> wrote:<br>
>> >> >>>>>>> No small test, sadly.<br>
>> >> >>>>>>> I managed to cut a 10MB reproducer down to 2MB over 2h<br>
>> >> >>>>>>> but no further.<br>
>> >> >>>>>><br>
>> >> >>>>>> The testcase in the bug report is already a lot smaller than<br>
>> >> >>>>>> that.<br>
>> >> >>>>>> Running delta a bit more reduced it to the attached file, which<br>
>> >> >>>>>> I<br>
>> >> >>>>>> am<br>
>> >> >>>>>> sure can be reduced a bit more.<br>
>> >> >>>>><br>
>> >> >>>>> The reproducer in the bug has a lot of problems, e.g.<br>
>> >> >>>>> it is only a fragment - not valid C++ code.<br>
>> >> >>>>> It is also not a very stable reproducers meaning changing the<br>
>> >> >>>>> clang<br>
>> >> >>>>> command line slightly<br>
>> >> >>>>> will change clang's memory allocation enough that the bug does<br>
>> >> >>>>> not<br>
>> >> >>>>> get triggered<br>
>> >> >>>>> anymore and instead you get a compiler failure because the code<br>
>> >> >>>>> is<br>
>> >> >>>>> only a fragment.<br>
>> >> >>>>><br>
>> >> >>>>> This stability issue would likely effect any test for this<br>
>> >> >>>>> problem,<br>
>> >> >>>>> even valgrind based ones.<br>
>> >> >>>>><br>
>> >> >>>>> Having thought  about this a little more think the best way to<br>
>> >> >>>>> approach the problem of invalidated iterators<br>
>> >> >>>>> is not by adding tests each time we find one but by addressing it<br>
>> >> >>>>> at<br>
>> >> >>>>> a<br>
>> >> >>>>> higher level, e.g. compiling clang or llvm in a<br>
>> >> >>>>> special debug mode, linking in special debug versions of STL and<br>
>> >> >>>>> llvm/ADT/ that will cause crashes more reliably.<br>
>> >> >>>>> Apparently VS has a feature like that, c.f.<br>
>> >> >>>>><br>
>> >> >>>>><br>
>> >> >>>>> <a href="http://stackoverflow.com/questions/2062956/c-best-way-to-check-if-an-iterator-is-valid" target="_blank">http://stackoverflow.com/questions/2062956/c-best-way-to-check-if-an-iterator-is-valid</a><br>

>> >> >>>><br>
>> >> >>>> Fair point - Howard Hinnant started work on something like this<br>
>> >> >>>> for<br>
>> >> >>>> libc++ but it's not been finished yet (put on the back burner).<br>
>> >> >>>> MSVC's<br>
>> >> >>>> debug mode has really helped me on several occasions.<br>
>> >> >>>><br>
>> >> >>>> Takumi (who runs most/all of the MSVC bots) - do any of your bots<br>
>> >> >>>> already run with MSVC's iterator debugging turned up/on? Would<br>
>> >> >>>> that<br>
>> >> >>>> be<br>
>> >> >>>> a reasonable task? (& interestingly: does it catch this bug)<br>
>> >> >>><br>
>> >> >>> Without knowing much about how MSVC is accomplishing this,<br>
>> >> >>> it probably relies on extra book keeping inside STL.<br>
>> >> >><br>
>> >> >> That's correct, which means ITERATOR_DEBUG_LEVEL is ABI-changing (so<br>
>> >> >> you can't interoperate STL-using APIs built with differing IDLs, but<br>
>> >> >> they have some machinery that should usually give you a linker error<br>
>> >> >> if you try). Howard wanted to implement something that would not be<br>
>> >> >> ABI-changing and allow libraries with differing iterator debugging<br>
>> >> >> levels to interoperate in a best-effort manner. This meant using<br>
>> >> >> side-tables rather than in-structure bookkeeping.<br>
>> >> >><br>
>> >> >>> The container implicated here , DenseMap, is not an STL container,<br>
>> >> >>> though, so some extra<br>
>> >> >>> work would be required to make it fit the MSVC framework.<br>
>> >> >><br>
>> >> >> Ah, right - yeah, there are ways to plug into it but it's work. We<br>
>> >> >> might consider implementing our own standalone version for this<br>
>> >> >> instead.<br>
>> >> >><br>
>> >> >>>> Robert - while that would help stabilize the behavior, would it<br>
>> >> >>>> actually help catch this bug? Would it be possible for you to<br>
>> >> >>>> create<br>
>> >> >>>> a<br>
>> >> >>><br>
>> >> >>> I am pretty sure it would catch this bug *reliably*, because<br>
>> >> >>> the reproducer does add items to  DenseMap while iterating over it<br>
>> >> >>> which<br>
>> >> >>>  invalidates the iterators "de jure".<br>
>> >> >><br>
>> >> >> That's all I'm interested in.<br>
>> >> >><br>
>> >> >>> But this invalidation only becomes visible when it is accompanied<br>
>> >> >>> by a<br>
>> >> >>> resizing<br>
>> >> >>> of the DenseMap and that seems to depend on the concrete program<br>
>> >> >>> run,<br>
>> >> >>> e.g.<br>
>> >> >>> we can influence the resize trigger by changing the length of<br>
>> >> >>> strings<br>
>> >> >>> passed on clang command line.<br>
>> >> >><br>
>> >> >> The visibility I don't mind about right now - that's an<br>
>> >> >> infrastructure<br>
>> >> >> problem we'll need to fix & I wouldn't gate your fix on that.<br>
>> >> >><br>
>> >> >> What I'm interested in is a reproduction of invalid code according<br>
>> >> >> to<br>
>> >> >> DenseMap's contract, not its implementation. Basically a quick &<br>
>> >> >> dirty<br>
>> >> >> way to do this would be to raise some global flag around the loop in<br>
>> >> >> Sema::AddOverloadCandidate, then assert that that flag is not raised<br>
>> >> >> when things are added to the container in Sema::RequireCompleteType<br>
>> >> >> (I<br>
>> >> >> mention these two functions based on your analysis posted in the<br>
>> >> >> bug,<br>
>> >> >> not my own investigation). Then reduce to the simplest (hopefully<br>
>> >> >> valid) code that triggers the assert.<br>
>> >> >><br>
>> >> >> You don't need/we won't commit this assert/flag, but we'll know that<br>
>> >> >> this case reliably violates the DenseMap contract & if/when we add<br>
>> >> >> some iterator invalidation checking we'll have a repro ready to go.<br>
>> >> ><br>
>> >> ><br>
>> >> > David,<br>
>> >> ><br>
>> >> > I attached a set of llvm/clang patches to the bug that makes the<br>
>> >> > failure reliable.<br>
>> >><br>
>> >> Thanks - they look roughly like what I had in mind. (I won't nitpick<br>
>> >> them because they won't be committed - just a simple test hack)<br>
>> >><br>
>> >> > The patches  work with the existing reproducer (also attached).  A<br>
>> >> > colleague of<br>
>> >> > mine is preparing a "nicer" reproducer which he will add to the bug<br>
>> >> > once it is ready.<br>
>> >><br>
>> >> Sounds good.<br>
>> >><br>
>> >> > Since this activity is independent  of getting my fix into clang: are<br>
>> >> > there any other reservations<br>
>> >> > preventing the  fix from being committed?<br>
>> >><br>
>> >> I don't have any. This seems fairly reasonable to me. Committed as<br>
>> >> r166188.<br>
>> >><br>
>> >> Please provide a test case at your earliest convenience.<br>
>> >><br>
>> >> - David<br>
>> >><br>
>> >><br>
>> >><br>
>> >> ><br>
>> >> > Cheers,<br>
>> >> > Robert<br>
>> >> ><br>
>> >> ><br>
>> >> >><br>
>> >> >>>> reproduction that always violates the iterator invalidation<br>
>> >> >>>> contract<br>
>> >> >>>> but doesn't necessarily crash? I wouldn't mind having that in the<br>
>> >> >>>> test<br>
>> >> >>>> suite even in the absence of fancy iterator checkers being<br>
>> >> >>>> deployed<br>
>> >> >>>> immediately. Though I'm not immediately sure how you'd construct<br>
>> >> >>>> such<br>
>> >> >>>> a test case without those iterator checks. (maybe it could be<br>
>> >> >>>> ad-hoc'd<br>
>> >> >>><br>
>> >> >>> The best I could do so far was a 2MB reproducer which was not<br>
>> >> >>> reliable<br>
>> >> >>> and since the reproducer is so large it is hard to get to the<br>
>> >> >>> essence<br>
>> >> >>> of the problem.<br>
>> >> >>> I was hoping that one of the clang/c++ wizards would look at the<br>
>> >> >>> diagnosis in<br>
>> >> >>> <a href="http://llvm.org/bugs/show_bug.cgi?id=14021" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=14021</a><br>
>> >> >>> and go:<br>
>> >> >>> "yaeh, of course there is a scenario that when you try to pick the<br>
>> >> >>> right<br>
>> >> >>> constructors for an expression, additional constructors are being<br>
>> >> >>> added to the list of candidates because of some template<br>
>> >> >>> instantiation."<br>
>> >> >>><br>
>> >> >>>> up with a few well-placed assertions & temporary (non-committed)<br>
>> >> >>>> extra<br>
>> >> >>>> flags just so you can track that the dangerous loop is running<br>
>> >> >>>> during<br>
>> >> >>>> the modification)<br>
>> >> >>>><br>
>> >> >>>> - David<br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div>