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

[Shankar Easwaran]

On 2/25/2015 7:43 PM, Duncan P. N. Exon Smith wrote:
>> 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.)
The order has to be deterministic. Linker doesnot need it for sorting 
purposes et all.

Shankar Easwaran

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation