<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Feb 25, 2015 at 12:38 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
> On 2015 Feb 25, at 12:27, Rui Ueyama <<a href="mailto:ruiu@google.com">ruiu@google.com</a>> wrote:<br>
><br>
> 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.<br>
><br>
> 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.<br>
><br>
> 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.<br>
><br>
> Even<br>
><br>
> m[x] = m[y]<br>
><br>
> 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).<br>
><br>
> I checked the code quickly, and it looks safe to me, but please review your patch again with the above thing in your mind.<br>
><br>
><br>
> <a href="http://reviews.llvm.org/D7885" target="_blank">http://reviews.llvm.org/D7885</a><br>
><br>
> EMAIL PREFERENCES<br>
> <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
><br>
<br>
</div></div>I agree with Rui. Even the changes from unordered_map<> have<br>
different guarantees about reference-validity.<br>
<br>
IMO, the data structures should be changed one-at-a-time, so that a<br>
future git bisect will be more useful. Each one really should be<br>
considered carefully.<br></blockquote><div><br></div><div>I agree. I'd split this patch.</div><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The style of the DenseMapInfo structs is strange, too. E.g.,<br>
<br>
> Index: lib/ReaderWriter/ELF/DefaultLayout.h<br>
> ===================================================================<br>
> --- lib/ReaderWriter/ELF/DefaultLayout.h<br>
> +++ lib/ReaderWriter/ELF/DefaultLayout.h<br>
> @@ -29,8 +29,6 @@<br>
> #include "llvm/ADT/StringSwitch.h"<br>
> #include "llvm/Support/Errc.h"<br>
> #include "llvm/Support/Format.h"<br>
> -#include <map><br>
> -#include <unordered_map><br>
><br>
> namespace lld {<br>
> namespace elf {<br>
> @@ -89,8 +87,10 @@<br>
><br>
> // The Key used for creating Sections<br>
> // The sections are created using<br>
> - // SectionName, contentPermissions<br>
> + // SectionName, contentPermissions,path<br>
> struct SectionKey {<br>
> + SectionKey() : _name(""), _perm(DefinedAtom::perm___), _path("") {}<br>
> +<br>
> SectionKey(StringRef name, DefinedAtom::ContentPermissions perm,<br>
> StringRef path)<br>
> : _name(name), _perm(perm), _path(path) {}<br>
> @@ -101,62 +101,81 @@<br>
> StringRef _path;<br>
> };<br>
><br>
> - struct SectionKeyHash {<br>
> - int64_t operator()(const SectionKey &k) const {<br>
> + struct SectionKeyInfo {<br>
> + static SectionKey getEmptyKey() { return SectionKey(); }<br>
> + static SectionKey getTombstoneKey() { return SectionKey(); }<br>
> + static unsigned getHashValue(const SectionKey &k) {<br>
> return llvm::hash_combine(k._name, k._perm, k._path);<br>
> }<br>
> - };<br>
> -<br>
> - struct SectionKeyEq {<br>
> - bool operator()(const SectionKey &lhs, const SectionKey &rhs) const {<br>
> + static bool isEqual(const SectionKey &lhs, const SectionKey &rhs) {<br>
> return ((lhs._name == rhs._name) && (lhs._perm == rhs._perm) &&<br>
> (lhs._path == rhs._path));<br>
> }<br>
> };<br>
><br>
<br>
followed eventually by:<br>
<br>
+ typedef llvm::DenseMap<SectionKey, AtomSection<ELFT> *, SectionKeyInfo><br>
+ SectionMapT;<br>
<br>
<br>
Why isn't `SectionKeyInfo` just:<br>
<br>
namespace llvm { template <> DenseMapInfo<SectionKey> { /* ... */ }; }<br>
<br>
and `SectionMapT`:<br>
<br>
typedef llvm::DenseMap<SectionKey, AtomSection<ELFT> *> SectionMapT;<br>
<br>
?</blockquote></div><br></div></div>