[PATCH] D116281: [ELF] Move gotIndex/pltIndex/globalDynIndex to SymbolAux

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I've made some suggestions in inline comments. This type of change does tend to make the implementation slightly harder to read and more fragile, maybe worth getting some data on how much effect this has on linking performance? If it is negligble and there are no plans to follow it up with more changes that depend on it, is it worth doing?

No fundamental objections though. Would be good to hear other opinions too?



================
Comment at: lld/ELF/Relocations.cpp:1557
     d.section = in.iplt;
-    d.value = sym.pltIndex * target->ipltEntrySize;
+    d.value = symAux.back().pltIdx * target->ipltEntrySize;
     d.size = 0;
----------------
Would it be better to use `sym.getPltIdx()` here? It is slightly less efficient, but is more consistent with the rest of the code. 


================
Comment at: lld/ELF/Relocations.cpp:2185
               if (needEntry) {
+                sym->auxIdx = symAux.size();
+                symAux.emplace_back();
----------------
These two lines look like they could be replaced with `sym->allocIdx()`


================
Comment at: lld/ELF/Symbols.cpp:69
     elf::whyExtract;
+SmallVector<SymbolAux, 0> elf::symAux;
 
----------------
To reduce the number of dynamic allocations it may be worth using a vector and reserving 1% of the total number of global symbols in one allocation, using your data from clang?


================
Comment at: lld/ELF/Symbols.h:92
 public:
+  uint32_t auxIdx = -1;
   uint32_t dynsymIndex = 0;
----------------
Will be worth a comment as `auxIdx` isn't as obvous as `gotIndex`, `pltIndex`. 


================
Comment at: lld/ELF/SyntheticSections.cpp:655
 bool GotSection::addDynTlsEntry(Symbol &sym) {
-  if (sym.globalDynIndex != -1U)
-    return false;
-  sym.globalDynIndex = numEntries;
+  symAux.back().tlsGdIdx = numEntries;
   // Global Dynamic TLS entries take two GOT slots.
----------------
Using `symAux.back()` directly here makes me a bit nervous as the function body of the callee is a long way from the caller. I'd be more comfortable with using `symAux[sym.auxIdx]`.

Maybe an opportunity for `sym.setPltIndex(numEntries)` that could abstract this away in the same way that the `getPltIndex()` does.

If that doesn't appeal, perhaps something like `assert(sym.auxIdx == symAuxIdx.size() - 1)` to make sure that this doesn't go out of synch.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116281