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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Symbols.cpp:69
     elf::whyExtract;
+SmallVector<SymbolAux, 0> elf::symAux;
 
----------------
peter.smith wrote:
> 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?
When linking chrome, `symtab->symbols().size() == 1806181 && symAux.size() == 19898`. symAux roughly takes 1%. 

When linking clang (release+asserts, all targets), `symtab->symbols().size() == 137246 && symAux.size() == 5184`.

symAux may be too small to bother with `reserve`.


================
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.
----------------
peter.smith wrote:
> 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.  
Thanks! Using `assert(sym.auxIdx == symAuxIdx.size() - 1)` for now.

`set*Index` is not appealing to me as that introduces some called-only-once functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116281