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

[Duncan P. N. Exon Smith]

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

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

?