[PATCH] [ELF] Use llvm ADT's instead of std.

[Duncan P. N. Exon Smith]

> On 2015 Feb 25, at 16:13, Shankar Easwaran <shankare at codeaurora.org> wrote:
> 
> On 2/25/2015 5:17 PM, Duncan P. N. Exon Smith wrote:
>>> On 2015 Feb 25, at 14:19, Shankar Easwaran <shankare at codeaurora.org> wrote:
>>> 
>>> On 2/25/2015 2:38 PM, Duncan P. N. Exon Smith wrote:
>>>>> On 2015 Feb 25, at 12:27, Rui Ueyama <ruiu at google.com> wrote:
>>>>> 
>>>>> 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.
>>>>> 
>>>>> 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.
>>>>> 
>>>>> 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.
>>>>> 
>>>>> Even
>>>>> 
>>>>> m[x] = m[y]
>>>>> 
>>>>> 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).
>>>>> 
>>>>> I checked the code quickly, and it looks safe to me, but please review your patch again with the above thing in your mind.
>>>>> 
>>>>> 
>>>>> http://reviews.llvm.org/D7885
>>>>> 
>>>>> EMAIL PREFERENCES
>>>>>  http://reviews.llvm.org/settings/panel/emailpreferences/
>>>>> 
>>>> I agree with Rui.  Even the changes from unordered_map<> have
>>>> different guarantees about reference-validity.
>>>> 
>>>> IMO, the data structures should be changed one-at-a-time, so that a
>>>> future git bisect will be more useful.  Each one really should be
>>>> considered carefully.
>>>> 
>>>> The style of the DenseMapInfo structs is strange, too.  E.g.,
>>> I will change the data structures one at a time
>> Great.
>> 
>>> and commit. I would have all data structures consistent as its easy for me/us to maintain going forward,
>> Note that we do use std::unordered_map and std::map in LLVM when
>> they're useful or we need the stable address guarantee.  Each map
>> you change, you need to be sure no users were relying on that
>> property.
> Agree.
>> Be especially careful of std::map, since it also guarantees a sorted
>> order.  Be sure that, when you change from it, you've checked the
>> validity of the new iteration order (either for sorted order, if
>> a user cares, or just for determinism).
> I replaced with llvm/ADT/MapVector which allows me to replace std::map.

(Only if determinism is the sole requirement; not if it needs to be sorted.)