<br><div class="gmail_extra">On Thu, Oct 18, 2012 at 1:20 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">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 errors,<br>
> but would have broken the assertion that "lock_hack == 0" that was in<br>
> Robert's patch.<br>
<br>
</div>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>
<div class="im"><br></div></blockquote><div><br></div><div>Okay, making some progress manually reducing the test.  I'll upload another version when I get it even smaller.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
> 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>
</div>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>
<div class="HOEnZb"><div class="h5"><br></div></div></blockquote><div><br></div><div>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.</div>
<div><br></div><div>- Jan</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
><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>> wrote:<br>
>><br>
>> On Thu, Oct 18, 2012 at 9:22 AM, Robert Muth <<a href="mailto:robertm@google.com">robertm@google.com</a>> 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 <<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>> 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 that.<br>
>> >>>>>> Running delta a bit more reduced it to the attached file, which 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 clang<br>
>> >>>>> command line slightly<br>
>> >>>>> will change clang's memory allocation enough that the bug does not<br>
>> >>>>> get triggered<br>
>> >>>>> anymore and instead you get a compiler failure because the code is<br>
>> >>>>> only a fragment.<br>
>> >>>>><br>
>> >>>>> This stability issue would likely effect any test for this 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 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>
>> >>>>> <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 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 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 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 by a<br>
>> >>> resizing<br>
>> >>> of the DenseMap and that seems to depend on the concrete program run,<br>
>> >>> e.g.<br>
>> >>> we can influence the resize trigger by changing the length of strings<br>
>> >>> passed on clang command line.<br>
>> >><br>
>> >> The visibility I don't mind about right now - that's an 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 to<br>
>> >> DenseMap's contract, not its implementation. Basically a quick & 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 (I<br>
>> >> mention these two functions based on your analysis posted in the 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 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 deployed<br>
>> >>>> immediately. Though I'm not immediately sure how you'd construct 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 reliable<br>
>> >>> and since the reproducer is so large it is hard to get to the 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 during<br>
>> >>>> the modification)<br>
>> >>>><br>
>> >>>> - David<br>
><br>
><br>
</div></div></blockquote></div><br></div>