From 7a58dd1046a8052619d173b769f32f2df3aafbe8 Mon Sep 17 00:00:00 2001 From: Fangrui Song Date: Wed, 28 Sep 2022 13:11:31 -0700 Subject: [PATCH] [ELF] Refactor Symbol initialization and overwriting Symbol::replace intends to overwrite a few fields (mostly Elf{32,64}_Sym fields), but the implementation copies all fields then restores some old fields. This is error-prone and wasteful. Add Symbol::overwrite to copy just the needed fields and add other overwrite member functions to copy the extra fields. --- lld/ELF/Driver.cpp | 16 +++++----- lld/ELF/InputFiles.cpp | 2 ++ lld/ELF/LTO.cpp | 4 +-- lld/ELF/LinkerScript.cpp | 4 +-- lld/ELF/Relocations.cpp | 12 +++---- lld/ELF/SymbolTable.cpp | 14 +++----- lld/ELF/Symbols.cpp | 18 +++++------ lld/ELF/Symbols.h | 83 +++++++++++++++++++++++++++--------------------- 8 files changed, 78 insertions(+), 75 deletions(-) diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp index 24a301b..fddc7af 100644 --- a/lld/ELF/Driver.cpp +++ b/lld/ELF/Driver.cpp @@ -1993,8 +1993,9 @@ static void replaceCommonSymbols() { auto *bss = make("COMMON", s->size, s->alignment); bss->file = s->file; inputSections.push_back(bss); - s->replace(Defined{s->file, StringRef(), s->binding, s->stOther, s->type, - /*value=*/0, s->size, bss}); + Defined(s->file, StringRef(), s->binding, s->stOther, s->type, + /*value=*/0, s->size, bss) + .overwrite(*s); } } } @@ -2010,11 +2011,9 @@ static void demoteSharedAndLazySymbols() { if (!(s && !cast(s->file)->isNeeded) && !sym->isLazy()) continue; - bool used = sym->used; uint8_t binding = sym->isLazy() ? sym->binding : uint8_t(STB_WEAK); - sym->replace( - Undefined{nullptr, sym->getName(), binding, sym->stOther, sym->type}); - sym->used = used; + Undefined(nullptr, sym->getName(), binding, sym->stOther, sym->type) + .overwrite(*sym); sym->versionId = VER_NDX_GLOBAL; } } @@ -2569,8 +2568,9 @@ void LinkerDriver::link(opt::InputArgList &args) { [](BitcodeFile *file) { file->postParse(); }); for (auto &it : ctx->nonPrevailingSyms) { Symbol &sym = *it.first; - sym.replace(Undefined{sym.file, sym.getName(), sym.binding, sym.stOther, - sym.type, it.second}); + Undefined(sym.file, sym.getName(), sym.binding, sym.stOther, sym.type, + it.second) + .overwrite(sym); cast(sym).nonPrevailing = true; } ctx->nonPrevailingSyms.clear(); diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp index fbfda40..24183e7 100644 --- a/lld/ELF/InputFiles.cpp +++ b/lld/ELF/InputFiles.cpp @@ -1129,6 +1129,8 @@ void ObjFile::initSectionsAndLocalSyms(bool ignoreComdats) { new (symbols[i]) Defined(this, name, STB_LOCAL, eSym.st_other, type, eSym.st_value, eSym.st_size, sec); symbols[i]->isUsedInRegularObj = true; + symbols[i]->auxIdx = -1; + symbols[i]->dynsymIndex = 0; } } diff --git a/lld/ELF/LTO.cpp b/lld/ELF/LTO.cpp index 60de5bb..d10c049 100644 --- a/lld/ELF/LTO.cpp +++ b/lld/ELF/LTO.cpp @@ -277,8 +277,8 @@ void BitcodeCompiler::add(BitcodeFile &f) { !(dr->section == nullptr && (!sym->file || sym->file->isElf())); if (r.Prevailing) - sym->replace( - Undefined{nullptr, StringRef(), STB_GLOBAL, STV_DEFAULT, sym->type}); + Undefined(nullptr, StringRef(), STB_GLOBAL, STV_DEFAULT, sym->type) + .overwrite(*sym); // We tell LTO to not apply interprocedural optimization for wrapped // (with --wrap) symbols because otherwise LTO would inline them while diff --git a/lld/ELF/LinkerScript.cpp b/lld/ELF/LinkerScript.cpp index 5a8c196..d59c2c0 100644 --- a/lld/ELF/LinkerScript.cpp +++ b/lld/ELF/LinkerScript.cpp @@ -238,7 +238,7 @@ void LinkerScript::addSymbol(SymbolAssignment *cmd) { Symbol *sym = symtab->insert(cmd->name); sym->mergeProperties(newSym); - sym->replace(newSym); + newSym.overwrite(*sym); sym->isUsedInRegularObj = true; cmd->sym = cast(sym); } @@ -256,7 +256,7 @@ static void declareSymbol(SymbolAssignment *cmd) { // We can't calculate final value right now. Symbol *sym = symtab->insert(cmd->name); sym->mergeProperties(newSym); - sym->replace(newSym); + newSym.overwrite(*sym); cmd->sym = cast(sym); cmd->provide = false; diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp index ecb0c48..6590d91 100644 --- a/lld/ELF/Relocations.cpp +++ b/lld/ELF/Relocations.cpp @@ -297,17 +297,15 @@ static SmallSet getSymbolsAt(SharedSymbol &ss) { static void replaceWithDefined(Symbol &sym, SectionBase &sec, uint64_t value, uint64_t size) { Symbol old = sym; + Defined(sym.file, StringRef(), sym.binding, sym.stOther, sym.type, value, + size, &sec) + .overwrite(sym); - sym.replace(Defined{sym.file, StringRef(), sym.binding, sym.stOther, - sym.type, value, size, &sec}); - - sym.auxIdx = old.auxIdx; - sym.verdefIndex = old.verdefIndex; sym.exportDynamic = true; sym.isUsedInRegularObj = true; // A copy relocated alias may need a GOT entry. - if (old.hasFlag(NEEDS_GOT)) - sym.setFlags(NEEDS_GOT); + sym.flags.store(old.flags.load(std::memory_order_relaxed) & NEEDS_GOT, + std::memory_order_relaxed); } // Reserve space in .bss or .bss.rel.ro for copy relocation. diff --git a/lld/ELF/SymbolTable.cpp b/lld/ELF/SymbolTable.cpp index cca1da0..e481a4e 100644 --- a/lld/ELF/SymbolTable.cpp +++ b/lld/ELF/SymbolTable.cpp @@ -87,18 +87,12 @@ Symbol *SymbolTable::insert(StringRef name) { Symbol *sym = reinterpret_cast(make()); symVector.push_back(sym); - // *sym was not initialized by a constructor. Fields that may get referenced - // when it is a placeholder must be initialized here. + // *sym was not initialized by a constructor. Initialize all Symbol fields. + memset(sym, 0, sizeof(Symbol)); sym->setName(name); - sym->symbolKind = Symbol::PlaceholderKind; sym->partition = 1; - sym->setVisibility(STV_DEFAULT); - sym->isUsedInRegularObj = false; - sym->exportDynamic = false; - sym->inDynamicList = false; - sym->referenced = false; - sym->traced = false; - sym->scriptDefined = false; + sym->auxIdx = -1; + sym->verdefIndex = -1; sym->versionId = VER_NDX_GLOBAL; if (pos != StringRef::npos) sym->hasVersionSuffix = true; diff --git a/lld/ELF/Symbols.cpp b/lld/ELF/Symbols.cpp index 741d8e7..a5e814e 100644 --- a/lld/ELF/Symbols.cpp +++ b/lld/ELF/Symbols.cpp @@ -415,7 +415,7 @@ void Symbol::resolveUndefined(const Undefined &other) { // existing undefined symbol for better error message later. if (isPlaceholder() || (isShared() && other.visibility() != STV_DEFAULT) || (isUndefined() && other.binding != STB_WEAK && other.discardedSecIdx)) { - replace(other); + other.overwrite(*this); return; } @@ -607,22 +607,22 @@ void Symbol::resolveCommon(const CommonSymbol &other) { // files were linked into a shared object first should not change the // regular rule that picks the largest st_size. uint64_t size = s->size; - replace(other); + other.overwrite(*this); if (size > cast(this)->size) cast(this)->size = size; } else { - replace(other); + other.overwrite(*this); } } void Symbol::resolveDefined(const Defined &other) { if (shouldReplace(other)) - replace(other); + other.overwrite(*this); } void Symbol::resolveLazy(const LazyObject &other) { if (isPlaceholder()) { - replace(other); + other.overwrite(*this); return; } @@ -631,7 +631,7 @@ void Symbol::resolveLazy(const LazyObject &other) { if (LLVM_UNLIKELY(isCommon()) && elf::config->fortranCommon && other.file->shouldExtractForCommon(getName())) { ctx->backwardReferences.erase(this); - replace(other); + other.overwrite(*this); other.extract(); return; } @@ -647,7 +647,7 @@ void Symbol::resolveLazy(const LazyObject &other) { // Symbols.h for the details. if (isWeak()) { uint8_t ty = type; - replace(other); + other.overwrite(*this); type = ty; binding = STB_WEAK; return; @@ -661,7 +661,7 @@ void Symbol::resolveLazy(const LazyObject &other) { void Symbol::resolveShared(const SharedSymbol &other) { if (isPlaceholder()) { - replace(other); + other.overwrite(*this); return; } if (isCommon()) { @@ -674,7 +674,7 @@ void Symbol::resolveShared(const SharedSymbol &other) { // An undefined symbol with non default visibility must be satisfied // in the same DSO. uint8_t bind = binding; - replace(other); + other.overwrite(*this); binding = bind; } else if (traced) printTraceSymbol(other, getName()); diff --git a/lld/ELF/Symbols.h b/lld/ELF/Symbols.h index 8bca6c8..2f6476a 100644 --- a/lld/ELF/Symbols.h +++ b/lld/ELF/Symbols.h @@ -39,6 +39,8 @@ class Undefined; class LazyObject; class InputFile; +void printTraceSymbol(const Symbol &sym, StringRef name); + enum { NEEDS_GOT = 1 << 0, NEEDS_PLT = 1 << 1, @@ -158,8 +160,6 @@ public: // True if the name contains '@'. uint8_t hasVersionSuffix : 1; - inline void replace(const Symbol &other); - // Symbol visibility. This is the computed minimum visibility of all // observed non-DSO symbols. uint8_t visibility() const { return stOther & 3; } @@ -272,6 +272,16 @@ protected: gotInIgot(false), folded(false), needsTocRestore(false), scriptDefined(false), dsoProtected(false) {} + void overwrite(Symbol &sym, Kind k) const { + if (sym.traced) + printTraceSymbol(*this, sym.getName()); + sym.file = file; + sym.type = type; + sym.binding = binding; + sym.stOther = (stOther & ~3) | sym.visibility(); + sym.symbolKind = k; + } + public: // True if this symbol is in the Iplt sub-section of the Plt and the Igot // sub-section of the .got.plt or .got. @@ -303,11 +313,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; @@ -348,6 +358,13 @@ public: size(size), section(section) { exportDynamic = config->exportDynamic; } + void overwrite(Symbol &sym) const { + Symbol::overwrite(sym, DefinedKind); + auto &s = static_cast(sym); + s.value = value; + s.size = size; + s.section = section; + } static bool classof(const Symbol *s) { return s->isDefined(); } @@ -385,6 +402,12 @@ public: alignment(alignment), size(size) { exportDynamic = config->exportDynamic; } + void overwrite(Symbol &sym) const { + Symbol::overwrite(sym, CommonKind); + auto &s = static_cast(sym); + s.alignment = alignment; + s.size = size; + } static bool classof(const Symbol *s) { return s->isCommon(); } @@ -398,6 +421,12 @@ public: uint8_t type, uint32_t discardedSecIdx = 0) : Symbol(UndefinedKind, file, name, binding, stOther, type), discardedSecIdx(discardedSecIdx) {} + void overwrite(Symbol &sym) const { + Symbol::overwrite(sym, UndefinedKind); + auto &s = static_cast(sym); + s.discardedSecIdx = discardedSecIdx; + s.nonPrevailing = nonPrevailing; + } static bool classof(const Symbol *s) { return s->kind() == UndefinedKind; } @@ -436,6 +465,14 @@ public: if (this->type == llvm::ELF::STT_GNU_IFUNC) this->type = llvm::ELF::STT_FUNC; } + void overwrite(Symbol &sym) const { + Symbol::overwrite(sym, SharedKind); + auto &s = static_cast(sym); + s.dsoProtected = dsoProtected; + s.value = value; + s.size = size; + s.alignment = alignment; + } uint64_t value; // st_value uint64_t size; // st_size @@ -456,6 +493,7 @@ public: LazyObject(InputFile &file) : Symbol(LazyObjectKind, &file, {}, llvm::ELF::STB_GLOBAL, llvm::ELF::STV_DEFAULT, llvm::ELF::STT_NOTYPE) {} + void overwrite(Symbol &sym) const { Symbol::overwrite(sym, LazyObjectKind); } static bool classof(const Symbol *s) { return s->kind() == LazyObjectKind; } }; @@ -514,8 +552,6 @@ union SymbolUnion { alignas(LazyObject) char e[sizeof(LazyObject)]; }; -void printTraceSymbol(const Symbol &sym, StringRef name); - size_t Symbol::getSymbolSize() const { switch (kind()) { case CommonKind: @@ -534,39 +570,12 @@ size_t Symbol::getSymbolSize() const { llvm_unreachable("unknown symbol kind"); } -// replace() replaces "this" object with a given symbol by memcpy'ing -// it over to "this". This function is called as a result of name -// resolution, e.g. to replace an undefind symbol with a defined symbol. -void Symbol::replace(const Symbol &other) { - 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; - setVisibility(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; - - // Print out a log message if --trace-symbol was specified. - // This is for debugging. - if (traced) - printTraceSymbol(*this, getName()); -} - template Defined *makeDefined(T &&...args) { - return new (reinterpret_cast( + auto *sym = new (reinterpret_cast( getSpecificAllocSingleton().Allocate())) Defined(std::forward(args)...); + sym->auxIdx = -1; + return sym; } void reportDuplicate(const Symbol &sym, const InputFile *newFile, -- 2.7.4