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

[Shankar Easwaran]

I was not aware of that style. Thanks for pointing that out. I will 
change this and see if I run into any ..

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.,
>
>> 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;
>
> ?
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>


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