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

Shankar Easwaran shankare at codeaurora.org
Wed Feb 25 16:13:51 PST 2015


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.

Shankar Easwaran

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




More information about the llvm-commits mailing list