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

[Rui Ueyama]

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

I agree. I'd split this patch.

Maybe I'd leave the std::map for sections and segments as is at least in
the first patch. They are unlikely to have lots of elements, converting
them to DenseMap is pretty likely to be marginal from the performance point
of view.


> The style of the DenseMapInfo structs is strange, too.  E.g.,
>
> > Index: lib/ReaderWriter/ELF/DefaultLayout.h
> > ===================================================================
> > --- lib/ReaderWriter/ELF/DefaultLayout.h
> > +++ lib/ReaderWriter/ELF/DefaultLayout.h
> > @@ -29,8 +29,6 @@
> >  #include "llvm/ADT/StringSwitch.h"
> >  #include "llvm/Support/Errc.h"
> >  #include "llvm/Support/Format.h"
> > -#include <map>
> > -#include <unordered_map>
> >
> >  namespace lld {
> >  namespace elf {
> > @@ -89,8 +87,10 @@
> >
> >    // The Key used for creating Sections
> >    // The sections are created using
> > -  // SectionName, contentPermissions
> > +  // SectionName, contentPermissions,path
> >    struct SectionKey {
> > +    SectionKey() : _name(""), _perm(DefinedAtom::perm___), _path("") {}
> > +
> >      SectionKey(StringRef name, DefinedAtom::ContentPermissions perm,
> >                 StringRef path)
> >          : _name(name), _perm(perm), _path(path) {}
> > @@ -101,62 +101,81 @@
> >      StringRef _path;
> >    };
> >
> > -  struct SectionKeyHash {
> > -    int64_t operator()(const SectionKey &k) const {
> > +  struct SectionKeyInfo {
> > +    static SectionKey getEmptyKey() { return SectionKey(); }
> > +    static SectionKey getTombstoneKey() { return SectionKey(); }
> > +    static unsigned getHashValue(const SectionKey &k) {
> >        return llvm::hash_combine(k._name, k._perm, k._path);
> >      }
> > -  };
> > -
> > -  struct SectionKeyEq {
> > -    bool operator()(const SectionKey &lhs, const SectionKey &rhs) const
> {
> > +    static bool isEqual(const SectionKey &lhs, const SectionKey &rhs) {
> >        return ((lhs._name == rhs._name) && (lhs._perm == rhs._perm) &&
> >                (lhs._path == rhs._path));
> >      }
> >    };
> >
>
> followed eventually by:
>
> +  typedef llvm::DenseMap<SectionKey, AtomSection<ELFT> *, SectionKeyInfo>
> +      SectionMapT;
>
>
> Why isn't `SectionKeyInfo` just:
>
>     namespace llvm { template <> DenseMapInfo<SectionKey> { /* ... */ }; }
>
> and `SectionMapT`:
>
>     typedef llvm::DenseMap<SectionKey, AtomSection<ELFT> *> SectionMapT;
>
> ?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150225/7421c313/attachment.html>