<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Feb 25, 2015 at 3:17 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> On 2015 Feb 25, at 14:19, Shankar Easwaran <<a href="mailto:shankare@codeaurora.org">shankare@codeaurora.org</a>> wrote:<br>
<span class="">><br>
> On 2/25/2015 2:38 PM, Duncan P. N. Exon Smith wrote:<br>
>>> On 2015 Feb 25, at 12:27, Rui Ueyama <<a href="mailto:ruiu@google.com">ruiu@google.com</a>> wrote:<br>
>>><br>
>>> I usually don't apply someone else's patch to test Chromium build, but this should not affect the Windows port anyway, because this only changes the ELF port.<br>
>>><br>
>>> But this change may be riskier than you might think because of the subtle difference of semantics between std::map and llvm::DenseMap regarding references returned by operator[] and iterators. For std::map, it is guaranteed that inserting a new element does not invalidate existing references to a std::map. It doesn't invalidate iterators unless you remove an element and an iterator is pointing to the element.<br>
>>><br>
>>> These properties are not guaranteed by DenseMap. So, if you add a new item to a map while you are iterating over elements of the map, it may crash. It actually crashes only when the hash table is resized to add a new element for LHS, so it could produce a nasty flaky bug. For example the bug in the LayoutPass was there for more than 1 year until I fixed that.<br>
>>><br>
>>> Even<br>
>>><br>
>>> m[x] = m[y]<br>
>>><br>
>>> is not guaranteed to be safe for a DenseMap m, because a reference returned by m[y] can be invalidated by m[x]. This particular one is the bug in LayoutPass.cpp that Reid mentioned above (fix is r213969).<br>
>>><br>
>>> I checked the code quickly, and it looks safe to me, but please review your patch again with the above thing in your mind.<br>
>>><br>
>>><br>
>>> <a href="http://reviews.llvm.org/D7885" target="_blank">http://reviews.llvm.org/D7885</a><br>
>>><br>
>>> EMAIL PREFERENCES<br>
>>>  <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
>>><br>
>> I agree with Rui.  Even the changes from unordered_map<> have<br>
>> different guarantees about reference-validity.<br>
>><br>
>> IMO, the data structures should be changed one-at-a-time, so that a<br>
>> future git bisect will be more useful.  Each one really should be<br>
>> considered carefully.<br>
>><br>
>> The style of the DenseMapInfo structs is strange, too.  E.g.,<br>
</span>> I will change the data structures one at a time<br>
<br>
Great.<br>
<br>
> and commit. I would have all data structures consistent as its easy for me/us to maintain going forward,<br>
<br>
Note that we do use std::unordered_map and std::map in LLVM when<br>
they're useful or we need the stable address guarantee.  Each map<br>
you change, you need to be sure no users were relying on that<br>
property.<br>
<br>
Be especially careful of std::map, since it also guarantees a sorted<br>
order.  Be sure that, when you change from it, you've checked the<br>
validity of the new iteration order (either for sorted order, if<br>
a user cares, or just for determinism).<br>
<br>
> and as I said its easier to fix a bug/debug when dealing with llvm types with what Sanjoy talked about.<br>
<br>
I'm not really convinced it's that much easier, and the custom LLVM<br>
types have more gotchas than the types in the STL (particularly<br>
since people just assume they provide the same guarantees).<br></blockquote><div><br></div><div>I agree with that point. There are many uses of std::map, for example, in LLVM. The LLVM container types are not always better than the STL. You don't have to replace all occurrences of the STL classes with the LLVM containers if the STL already works well there.</div></div><br></div></div>