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

Rui Ueyama ruiu at google.com
Wed Feb 25 15:52:30 PST 2015


On Wed, Feb 25, 2015 at 3:17 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> 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.
>
> 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).
>

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150225/deb114ff/attachment.html>


More information about the llvm-commits mailing list