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.<div>
<div><br></div><div><br><div><br></div><div><br></div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Oct 18, 2012 at 9:59 AM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">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>> wrote:<br>
>> On Tue, Oct 16, 2012 at 10:09 AM, Robert Muth <<a href="mailto:robertm@google.com">robertm@google.com</a>> wrote:<br>
>>> On Tue, Oct 16, 2012 at 11:47 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>>>> On Tue, Oct 16, 2012 at 8:12 AM, Robert Muth <<a href="mailto:robertm@google.com">robertm@google.com</a>> 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 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 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 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>
>>>>> <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). 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 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 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 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 resizing<br>
>>> of the DenseMap and that seems to depend on the concrete program run, 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>
</div></div>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>
<div class="im"><br>
> The patches  work with the existing reproducer (also attached).  A colleague of<br>
> mine is preparing a "nicer" reproducer which he will add to the bug<br>
> once it is ready.<br>
<br>
</div>Sounds good.<br>
<div class="im"><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>
</div>I don't have any. This seems fairly reasonable to me. Committed as r166188.<br>
<br>
Please provide a test case at your earliest convenience.<br>
<span class="HOEnZb"><font color="#888888"><br>
- David<br>
</font></span><div class="HOEnZb"><div class="h5"><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 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 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 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 right<br>
>>> constructors for an expression, additional constructors are being<br>
>>> added to the list of candidates because of some template instantiation."<br>
>>><br>
>>>> up with a few well-placed assertions & temporary (non-committed) extra<br>
>>>> flags just so you can track that the dangerous loop is running during<br>
>>>> the modification)<br>
>>>><br>
>>>> - David<br>
</div></div></blockquote></div><br></div>