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

[Duncan P. N. Exon Smith]

> 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.

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).

> and as I said its easier to fix a bug/debug when dealing with llvm types with what Sanjoy talked about.

I'm not really convinced it's that much easier, and the custom LLVM
types have more gotchas than the types in the STL (particularly
since people just assume they provide the same guarantees).