[PATCH] [ELF] Symbol::replace: copy fewer members

lld-copy-fewer-members.patch

From 7592fc59796cb41a4e7fe0d171b11d4198718a5b Mon Sep 17 00:00:00 2001
From: Fangrui Song
Date: Sat, 5 Feb 2022 16:48:23 -0800
Subject: [PATCH] [ELF] Symbol::replace: copy fewer members

Symbol::replace copies the whole struct and then restores some members.
This patch changes it to copy the needed members.

---
 lld/ELF/InputFiles.cpp  |  2 +
 lld/ELF/Relocations.cpp |  4 +-
 lld/ELF/SymbolTable.cpp | 16 ++++----
 lld/ELF/Symbols.h       | 82 +++++++++++++++++++++++++----------------
 4 files changed, 64 insertions(+), 40 deletions(-)

diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index ebe4f99e84d0..a15157cbda25 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -1064,6 +1064,8 @@ void ObjFile<ELFT>::initializeSymbols(const object::ELFFile<ELFT> &obj) {
     else
       new (symbols[i]) Defined(this, name, STB_LOCAL, eSym.st_other, type,
                                eSym.st_value, eSym.st_size, sec);
+    symbols[i]->hasVersionSuffix = false;
+    symbols[i]->init();
   }
 
   // Some entries have been filled by LazyObjFile.
diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index 1994a2b35199..6a636e47c540 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -306,6 +306,7 @@ static void replaceWithDefined(Symbol &sym, SectionBase &sec, uint64_t value,
   sym.verdefIndex = old.verdefIndex;
   sym.exportDynamic = true;
   sym.isUsedInRegularObj = true;
+  sym.needsCopy = false;
   // A copy relocated alias may need a GOT entry.
   sym.needsGot = old.needsGot;
 }
@@ -1577,14 +1578,13 @@ static bool handleNonPreemptibleIfunc(Symbol &sym) {
   if (!(sym.needsGot || sym.needsPlt || sym.hasDirectReloc))
     return true;
 
-  sym.isInIplt = true;
-
   // Create an Iplt and the associated IRELATIVE relocation pointing to the
   // original section/value pairs. For non-GOT non-PLT relocation case below, we
   // may alter section/value, so create a copy of the symbol to make
   // section/value fixed.
   auto *directSym = makeDefined(cast<Defined>(sym));
   directSym->allocateAux();
+  sym.isInIplt = directSym->isInIplt = true;
   addPltEntry(*in.iplt, *in.igotPlt, *in.relaIplt, target->iRelativeRel,
               *directSym);
   sym.allocateAux();
diff --git a/lld/ELF/SymbolTable.cpp b/lld/ELF/SymbolTable.cpp
index 58f8bc8cab40..85b888a2758d 100644
--- a/lld/ELF/SymbolTable.cpp
+++ b/lld/ELF/SymbolTable.cpp
@@ -83,23 +83,25 @@ Symbol *SymbolTable::insert(StringRef name) {
   // *sym was not initialized by a constructor. Fields that may get referenced
   // when it is a placeholder must be initialized here.
   sym->setName(name);
+  sym->file = nullptr;
+  sym->type = 0;
+  sym->binding = 0;
+  sym->stOther = 0;
   sym->symbolKind = Symbol::PlaceholderKind;
-  sym->partition = 1;
   sym->visibility = STV_DEFAULT;
   sym->isUsedInRegularObj = false;
   sym->exportDynamic = false;
-  sym->inDynamicList = false;
-  sym->referenced = false;
   sym->traced = false;
-  sym->scriptDefined = false;
-  sym->versionId = VER_NDX_GLOBAL;
-  if (pos != StringRef::npos)
-    sym->hasVersionSuffix = true;
+  sym->hasVersionSuffix = pos != StringRef::npos;
+  sym->init();
+
   return sym;
 }
 
 Symbol *SymbolTable::addSymbol(const Symbol &newSym) {
   Symbol *sym = insert(newSym.getName());
+  if (sym->isPlaceholder())
+    sym->init();
   sym->resolve(newSym);
   return sym;
 }
diff --git a/lld/ELF/Symbols.h b/lld/ELF/Symbols.h
index 271b0fadae23..c4fb29728dac 100644
--- a/lld/ELF/Symbols.h
+++ b/lld/ELF/Symbols.h
@@ -91,7 +91,7 @@ public:
   uint8_t symbolKind;
 
   // The partition whose dynamic symbol table contains this symbol's definition.
-  uint8_t partition = 1;
+  uint8_t partition;
 
   // Symbol visibility. This is the computed minimum visibility of all
   // observed non-DSO symbols.
@@ -240,17 +240,38 @@ protected:
          uint8_t stOther, uint8_t type)
       : file(file), nameData(name.data()), nameSize(name.size()),
         binding(binding), type(type), stOther(stOther), symbolKind(k),
-        visibility(stOther & 3), isPreemptible(false),
+        visibility(stOther & 3),
         isUsedInRegularObj(!file || file->kind() == InputFile::ObjKind),
-        used(false), exportDynamic(false), inDynamicList(false),
-        referenced(false), traced(false), hasVersionSuffix(false),
-        isInIplt(false), gotInIgot(false), folded(false),
-        needsTocRestore(false), scriptDefined(false), needsCopy(false),
-        needsGot(false), needsPlt(false), needsTlsDesc(false),
-        needsTlsGd(false), needsTlsGdToIe(false), needsGotDtprel(false),
-        needsTlsIe(false), hasDirectReloc(false) {}
+        exportDynamic(false) {}
 
 public:
+  void init() {
+    partition = 1;
+    isPreemptible = false;
+    used = false;
+    inDynamicList = false;
+    referenced = false;
+    isInIplt = false;
+    gotInIgot = false;
+    folded = false;
+    needsTocRestore = false;
+    scriptDefined = false;
+    needsCopy = false;
+    needsGot = false;
+    needsPlt = false;
+    needsTlsDesc = false;
+    needsTlsGd = false;
+    needsTlsGdToIe = false;
+    needsGotDtprel = false;
+    needsTlsIe = false;
+    hasDirectReloc = false;
+
+    auxIdx = -1;
+    dynsymIndex = 0;
+    verdefIndex = -1;
+    versionId = llvm::ELF::VER_NDX_GLOBAL;
+  }
+
   // True if this symbol is in the Iplt sub-section of the Plt and the Igot
   // sub-section of the .got.plt or .got.
   uint8_t isInIplt : 1;
@@ -289,11 +310,11 @@ public:
 
   // A symAux index used to access GOT/PLT entry indexes. This is allocated in
   // postScanRelocations().
-  uint32_t auxIdx = -1;
-  uint32_t dynsymIndex = 0;
+  uint32_t auxIdx;
+  uint32_t dynsymIndex;
 
   // This field is a index to the symbol's version definition.
-  uint16_t verdefIndex = -1;
+  uint16_t verdefIndex;
 
   // Version definition index.
   uint16_t versionId;
@@ -574,24 +595,21 @@ void Symbol::replace(const Symbol &other) {
     error("TLS attribute mismatch: " + toString(*this) + "\n>>> defined in " +
           toString(other.file) + "\n>>> defined in " + toString(file));
 
-  Symbol old = *this;
-  memcpy(this, &other, other.getSymbolSize());
-
-  // old may be a placeholder. The referenced fields must be initialized in
-  // SymbolTable::insert.
-  nameData = old.nameData;
-  nameSize = old.nameSize;
-  partition = old.partition;
-  visibility = old.visibility;
-  isPreemptible = old.isPreemptible;
-  isUsedInRegularObj = old.isUsedInRegularObj;
-  exportDynamic = old.exportDynamic;
-  inDynamicList = old.inDynamicList;
-  referenced = old.referenced;
-  traced = old.traced;
-  hasVersionSuffix = old.hasVersionSuffix;
-  scriptDefined = old.scriptDefined;
-  versionId = old.versionId;
+  // Copy members initialized by the ctor of other.
+  //
+  // Many members may have been set and need to be retained: nameData, nameSize,
+  // partition, visibility, isPreemptible, isUsedInRegularObj, exportDynamic,
+  // inDynamicList, referenced, traced, hasVersionSuffix, scriptDefined,
+  // versionId.
+  //
+  // The rest members have the default values. Whether they are copied does not
+  // matter.
+  file = other.file;
+  binding = other.binding;
+  type = other.type;
+  stOther = other.stOther;
+  symbolKind = other.symbolKind;
+  memcpy(this + 1, &other + 1, other.getSymbolSize() - sizeof(Symbol));
 
   // Print out a log message if --trace-symbol was specified.
   // This is for debugging.
@@ -600,9 +618,11 @@ void Symbol::replace(const Symbol &other) {
 }
 
 template <typename... T> Defined *makeDefined(T &&...args) {
-  return new (reinterpret_cast<Defined *>(
+  auto *sym = new (reinterpret_cast<Defined *>(
       getSpecificAllocSingleton<SymbolUnion>().Allocate()))
       Defined(std::forward<T>(args)...);
+  sym->init();
+  return sym;
 }
 
 void maybeWarnUnorderableSymbol(const Symbol *sym);
-- 
2.35

Linking chrome:

% hyperfine --warmup 2 --min-runs 20 "numactl -C 20-27 "{/tmp/c/0,/tmp/c/1}" -flavor gnu @response.txt --threads=8 -o lld"
Benchmark 1: numactl -C 20-27 /tmp/c/0 -flavor gnu @response.txt --threads=8 -o lld
  Time (mean ± σ):      5.452 s ±  0.057 s    [User: 5.555 s, System: 2.439 s]
  Range (min … max):    5.378 s …  5.619 s    20 runs
 
Benchmark 2: numactl -C 20-27 /tmp/c/1 -flavor gnu @response.txt --threads=8 -o lld
 ⠸ Current estimate: 5.459 s
  Time (mean ± σ):      5.457 s ±  0.037 s    [User: 5.576 s, System: 2.422 s]
  Range (min … max):    5.406 s …  5.569 s    20 runs
 
Summary
  'numactl -C 20-27 /tmp/c/0 -flavor gnu @response.txt --threads=8 -o lld' ran
    1.00 ± 0.01 times faster than 'numactl -C 20-27 /tmp/c/1 -flavor gnu @response.txt --threads=8 -o lld'

not significant. The code size a bit smaller.