[PATCH] D133003: [WIP][ELF] Parallelize relocation scanning

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Relocations.cpp:299-300
                                uint64_t size) {
-  Symbol old = sym;
-
+  Symbol old{Symbol::PlaceholderKind, nullptr, StringRef(), 0, 0, 0};
+  memcpy(&old, &sym, sizeof(old));
   sym.replace(Defined{sym.file, StringRef(), sym.binding, sym.stOther,
----------------
ikudrin wrote:
> Why not define a copy constructor?
Good idea. Adopted


================
Comment at: lld/ELF/Symbols.h:289
 
-  // True if this symbol needs a canonical PLT entry, or (during
-  // postScanRelocations) a copy relocation.
-  uint8_t needsCopy : 1;
+  uint8_t needsTlsGdToIe : 1;
 
----------------
ikudrin wrote:
> MaskRay wrote:
> > ikudrin wrote:
> > > Why is not `needsTlsGdToIe` moved under `atomic` like `needsTlsGd` and alike?
> > All the 8 bits of `std::atomic<uint8_t>` have been used. We need one not in atomic if we want to keep the size of `SymbolUnion` unchanged.
> Does that mean that some flags in the atomic do not really need to be handled as such, or that this flag is left outside despite it can be potentially updated concurrently, but there is no space for it in `flags`? In any case, that is worth documenting, at least.
I have replaced `Symbol::visibility` with `Symbol::stOther` and atomic<uint16_t> is fine now, but I suspect 16-bit atomic operations are not efficient on common architectures.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133003/new/

https://reviews.llvm.org/D133003