[PATCH] D116281: [ELF] Move gotIndex/pltIndex/globalDynIndex to SymbolAux
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 4 13:04:13 PST 2022
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
More information about the llvm-commits
mailing list