Make replaceSymbol a member function of Symbol.

[ruiu]

Summary

This is a mechanical rewrite of replaceSymbol(A, B) to A->replace(B).
I also added a comment to Symbol::replace().

Technically this change is not necessary, but this change makes code a
bit more concise.

[MaskRay]

lld/ELF/Symbols.h

434 ↗

(On Diff #200197)

Consider renaming this since it is now a member function.

[ruiu]

lld/ELF/Symbols.h

434 ↗

(On Diff #200197)

I considered that but unfortunately we already have getSize function to return a size attribute of a symbol.

[smeenai]

lld/trunk/ELF/Symbols.h

86

Sorry for commenting on an old diff.

I'm trying to understand this comment and if it's still true. As far as I can tell, [replace()](https://github.com/llvm/llvm-project/blob/204d301bb1921431a853c0bfba32007c018df1d5/lld/ELF/Symbols.h#L529) is just doing a memcpy from the new symbol into the old symbol, and the binding field isn't restored afterwards, so replace will in fact overwrite the binding, right?

I imagine this comment is from a time when the behavior was different (particularly when SymbolBody used to be distinct from Symbol and the binding was part of Symbol and not SymbolBody), but it doesn't seem to be accurate anymore, as far as I can tell.

[MaskRay]

lld/trunk/ELF/Symbols.h

86

This is not overwritten by replace() is indeed misleading. replace overrides the binding. The only weird place is undefined weak + un-extracted archive member which is represented as a weak lazy symbol which is considered isUndefWeak.

An undefined weak is still weak when it resolves to a shared library. is normal rule which probably doesn't deserve a dedicated comment here.

lld/trunk/ELF/Driver.cpp

for (Symbol *Sym : Symtab->getSymbols()) {
     auto *S = dyn_cast<CommonSymbol>(Sym);
     if (!S)
       continue;
 
     auto *Bss = make<BssSection>("COMMON", S->Size, S->Alignment);
     Bss->File = S->File;
     Bss->Live = !Config->GcSections;
     InputSections.push_back(Bss);
-    replaceSymbol(S, Defined{S->File, S->getName(), S->Binding, S->StOther,
-                             S->Type,
-                             /*Value=*/0, S->Size, Bss});
+    S->replace(Defined{S->File, S->getName(), S->Binding, S->StOther, S->Type,
   }
 }
 
 // If all references to a DSO happen to be weak, the DSO is not added
 // to DT_NEEDED. If that happens, we need to eliminate shared symbols
 // created from the DSO. Otherwise, they become dangling references
 // that point to a non-existent DSO.
 static void demoteSharedSymbols() {
   for (Symbol *Sym : Symtab->getSymbols()) {
     auto *S = dyn_cast<SharedSymbol>(Sym);
     if (!S || S->getFile().IsNeeded)
       continue;
 
     bool Used = S->Used;
-    replaceSymbol(
-        S, Undefined{nullptr, S->getName(), STB_WEAK, S->StOther, S->Type});
+    S->replace(Undefined{nullptr, S->getName(), STB_WEAK, S->StOther, S->Type});
     S->Used = Used;
   }
 }
 
 // The section referred to by S is considered address-significant. Set the
 // KeepUnique flag on the section if appropriate.
 static void markAddrsig(Symbol *S) {
   if (auto *D = dyn_cast_or_null<Defined>(S))

lld/trunk/ELF/LTO.cpp

R.FinalDefinitionInLinkageUnit =
         // Skip absolute symbols from ELF objects, otherwise PC-rel relocations
         // will be generated by for them, triggering linker errors.
         // Symbol section is always null for bitcode symbols, hence the check
         // for isElf(). Skip linker script defined symbols as well: they have
         // no File defined.
         !(DR->Section == nullptr && (!Sym->File || Sym->File->isElf()));
 
     if (R.Prevailing)
-      replaceSymbol(Sym, Undefined{nullptr, Sym->getName(), STB_GLOBAL,
-                                   STV_DEFAULT, Sym->Type});
+      Sym->replace(Undefined{nullptr, Sym->getName(), STB_GLOBAL, STV_DEFAULT,
+                             Sym->Type});
 
     // We tell LTO to not apply interprocedural optimization for wrapped
     // (with --wrap) symbols because otherwise LTO would inline them while
     // their values are still not final.
     R.LinkerRedefined = !Sym->CanInline;
   }
   checkError(LTOObj->add(std::move(F.Obj), Resols));
 }

lld/trunk/ELF/LinkerScript.cpp

void LinkerScript::addSymbol(SymbolAssignment *Cmd) {
   // write expressions like this: `alignment = 16; . = ALIGN(., alignment)`.
   uint64_t SymValue = Value.Sec ? 0 : Value.getValue();
 
   Defined New(nullptr, Cmd->Name, STB_GLOBAL, Visibility, STT_NOTYPE, SymValue,
               0, Sec);
 
   Symbol *Sym = Symtab->insert(Cmd->Name);
   mergeSymbolProperties(Sym, New);
-  replaceSymbol(Sym, New);
+  Sym->replace(New);
   Cmd->Sym = cast<Defined>(Sym);
 }
 
 // This function is called from LinkerScript::declareSymbols.
 // It creates a placeholder symbol if needed.
 static void declareSymbol(SymbolAssignment *Cmd) {
   if (!shouldDefineSym(Cmd))
     return;
 
   uint8_t Visibility = Cmd->Hidden ? STV_HIDDEN : STV_DEFAULT;
   Defined New(nullptr, Cmd->Name, STB_GLOBAL, Visibility, STT_NOTYPE, 0, 0,
               nullptr);
 
   // We can't calculate final value right now.
   Symbol *Sym = Symtab->insert(Cmd->Name);
   mergeSymbolProperties(Sym, New);
-  replaceSymbol(Sym, New);
+  Sym->replace(New);
 
   Cmd->Sym = cast<Defined>(Sym);
   Cmd->Provide = false;
   Sym->ScriptDefined = true;
 }
 
 // This method is used to handle INSERT AFTER statement. Here we rebuild
 // the list of script commands to mix sections inserted into.

lld/trunk/ELF/Relocations.cpp

 // effectively a defined symbol. In the case of copy relocation the symbol is
 // in .bss and in the case of a canonical plt entry it is in .plt. This function
 // replaces the existing symbol with a Defined pointing to the appropriate
 // location.
 static void replaceWithDefined(Symbol &Sym, SectionBase *Sec, uint64_t Value,
                                uint64_t Size) {
   Symbol Old = Sym;
 
-  replaceSymbol(&Sym, Defined{Sym.File, Sym.getName(), Sym.Binding, Sym.StOther,
-                              Sym.Type, Value, Size, Sec});
+  Sym.replace(Defined{Sym.File, Sym.getName(), Sym.Binding, Sym.StOther,
 
   Sym.PltIndex = Old.PltIndex;
   Sym.GotIndex = Old.GotIndex;
   Sym.VerdefIndex = Old.VerdefIndex;
   Sym.PPC64BranchltIndex = Old.PPC64BranchltIndex;
   Sym.IsPreemptible = true;
   Sym.ExportDynamic = true;
   Sym.IsUsedInRegularObj = true;

lld/trunk/ELF/SymbolTable.cpp

Symbol *SymbolTable::addSymbol(const Symbol &New) {
   resolveSymbol(Old, New);
   return Old;
 }
 
 static void addUndefined(Symbol *Old, const Undefined &New) {
   // An undefined symbol with non default visibility must be satisfied
   // in the same DSO.
   if (Old->isShared() && New.Visibility != STV_DEFAULT) {
-    replaceSymbol(Old, New);
+    Old->replace(New);
     return;
   }
 
   if (Old->isShared() || Old->isLazy() ||
       (Old->isUndefined() && New.Binding != STB_WEAK))
     Old->Binding = New.Binding;
 
   if (Old->isLazy()) {

 }
 
 static void addCommon(Symbol *Old, const CommonSymbol &New) {
   int Cmp = compare(Old, &New);
   if (Cmp < 0)
     return;
 
   if (Cmp > 0) {
-    replaceSymbol(Old, New);
+    Old->replace(New);
     return;
   }
 
   CommonSymbol *OldSym = cast<CommonSymbol>(Old);
 
   OldSym->Alignment = std::max(OldSym->Alignment, New.Alignment);
   if (OldSym->Size < New.Size) {
     OldSym->File = New.File;
if (!Src2.empty())
     Msg += Src2 + "\n>>>            ";
   Msg += Obj2;
   error(Msg);
 }
 
 static void addDefined(Symbol *Old, const Defined &New) {
   int Cmp = compare(Old, &New);
   if (Cmp > 0)
-    replaceSymbol(Old, New);
+    Old->replace(New);
   else if (Cmp == 0)
     reportDuplicate(Old, New.File,
                     dyn_cast_or_null<InputSectionBase>(New.Section), New.Value);
 }
 
 static void addShared(Symbol *Old, const SharedSymbol &New) {
   if (Old->Visibility == STV_DEFAULT && (Old->isUndefined() || Old->isLazy())) {
     // An undefined symbol with non default visibility must be satisfied
     // in the same DSO.
     uint8_t Binding = Old->Binding;
-    replaceSymbol(Old, New);
+    Old->replace(New);
     Old->Binding = Binding;
   }
 }
 
 Symbol *SymbolTable::find(StringRef Name) {
   auto It = SymMap.find(CachedHashStringRef(Name));
   if (It == SymMap.end())
     return nullptr;
   if (It->second == -1)
     return nullptr;
   return SymVector[It->second];
 }
 
 template <class LazyT> static void addLazy(Symbol *Old, const LazyT &New) {
   if (!Old->isUndefined())
     return;
 
   // An undefined weak will not fetch archive members. See comment on Lazy in
   // Symbols.h for the details.
   if (Old->isWeak()) {
     uint8_t Type = Old->Type;
-    replaceSymbol(Old, New);
+    Old->replace(New);
     Old->Type = Type;
     Old->Binding = STB_WEAK;
     return;
   }
 
   if (InputFile *F = New.fetch())
     parseFile(F);
 }
void elf::mergeSymbolProperties(Symbol *Old, const Symbol &New) {
   if (!New.isShared())
     Old->Visibility = getMinVisibility(Old->Visibility, New.Visibility);
 }
 
 void elf::resolveSymbol(Symbol *Old, const Symbol &New) {
   mergeSymbolProperties(Old, New);
 
   if (Old->isPlaceholder()) {
-    replaceSymbol(Old, New);
+    Old->replace(New);
     return;
   }
 
   switch (New.kind()) {
   case Symbol::UndefinedKind:
     addUndefined(Old, cast<Undefined>(New));
     break;
   case Symbol::CommonKind:

lld/trunk/ELF/Symbols.h

public:
   uint32_t VerdefIndex = -1;
 
   // Version definition index.
   uint16_t VersionId;
 
   // An index into the .branch_lt section on PPC64.
   uint16_t PPC64BranchltIndex = -1;
 
-  // Symbol binding. This is not overwritten by replaceSymbol to track
+  // Symbol binding. This is not overwritten by replace() to track
   // changes during resolution. In particular:
   //  - An undefined weak is still weak when it resolves to a shared library.
   //  - An undefined weak will not fetch archive members, but we have to
   //    remember it is weak.
   uint8_t Binding;
 
   // The following fields have the same meaning as the ELF symbol attributes.
   uint8_t Type;    // symbol type
public:
   // False if LTO shouldn't inline whatever this symbol points to. If a symbol
   // is overwritten after LTO, LTO shouldn't inline the symbol because it
   // doesn't know the final contents of the symbol.
   unsigned CanInline : 1;
 
   // True if this symbol is specified by --trace-symbol option.
   unsigned Traced : 1;
 
+  inline void replace(const Symbol &New);
+
   bool includeInDynsym() const;
   uint8_t computeBinding() const;
   bool isWeak() const { return Binding == llvm::ELF::STB_WEAK; }
 
   bool isUndefined() const { return SymbolKind == UndefinedKind; }
   bool isCommon() const { return SymbolKind == CommonKind; }
   bool isDefined() const { return SymbolKind == DefinedKind; }
   bool isShared() const { return SymbolKind == SharedKind; }

 
 private:
   static bool isExportDynamic(Kind K, uint8_t Visibility) {
     if (K == SharedKind)
       return Visibility == llvm::ELF::STV_DEFAULT;
     return Config->Shared || Config->ExportDynamic;
   }
 
+  inline size_t getSymbolSize() const;
+
 protected:
   Symbol(Kind K, InputFile *File, StringRefZ Name, uint8_t Binding,
          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),
         IsUsedInRegularObj(!File || File->kind() == InputFile::ObjKind),
         ExportDynamic(isExportDynamic(K, Visibility)), CanInline(false),
         Traced(false), NeedsPltAddr(false), IsInIplt(false), GotInIgot(false),
static inline void assertSymbols() {
   AssertSymbol<Undefined>();
   AssertSymbol<SharedSymbol>();
   AssertSymbol<LazyArchive>();
   AssertSymbol<LazyObject>();
 }
 
 void printTraceSymbol(Symbol *Sym);
 
-static size_t getSymbolSize(const Symbol &Sym) {
-  switch (Sym.kind()) {
-  case Symbol::CommonKind:
+size_t Symbol::getSymbolSize() const {
+  switch (kind()) {
+  case CommonKind:
     return sizeof(CommonSymbol);
-  case Symbol::DefinedKind:
+  case DefinedKind:
     return sizeof(Defined);
-  case Symbol::LazyArchiveKind:
+  case LazyArchiveKind:
     return sizeof(LazyArchive);
-  case Symbol::LazyObjectKind:
+  case LazyObjectKind:
     return sizeof(LazyObject);
-  case Symbol::SharedKind:
+  case SharedKind:
     return sizeof(SharedSymbol);
-  case Symbol::UndefinedKind:
+  case UndefinedKind:
     return sizeof(Undefined);
-  case Symbol::PlaceholderKind:
+  case PlaceholderKind:
     return sizeof(Symbol);
   }
   llvm_unreachable("unknown symbol kind");
 }
 
-inline void replaceSymbol(Symbol *Sym, const Symbol &New) {
+// 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 &New) {
   using llvm::ELF::STT_TLS;
 
   // Symbols representing thread-local variables must be referenced by
   // TLS-aware relocations, and non-TLS symbols must be reference by
   // non-TLS relocations, so there's a clear distinction between TLS
   // and non-TLS symbols. It is an error if the same symbol is defined
   // as a TLS symbol in one file and as a non-TLS symbol in other file.
-  if (Sym->SymbolKind != Symbol::PlaceholderKind && !Sym->isLazy() &&
-      !New.isLazy()) {
-    bool TlsMismatch = (Sym->Type == STT_TLS && New.Type != STT_TLS) ||
-                       (Sym->Type != STT_TLS && New.Type == STT_TLS);
+  if (SymbolKind != PlaceholderKind && !isLazy() && !New.isLazy()) {
+    bool TlsMismatch = (Type == STT_TLS && New.Type != STT_TLS) ||
+                       (Type != STT_TLS && New.Type == STT_TLS);
     if (TlsMismatch)
-      error("TLS attribute mismatch: " + toString(*Sym) + "\n>>> defined in " +
-            toString(New.File) + "\n>>> defined in " + toString(Sym->File));
+      error("TLS attribute mismatch: " + toString(*this) + "\n>>> defined in " +
+            toString(New.File) + "\n>>> defined in " + toString(File));
   }
 
-  Symbol Old = *Sym;
-  memcpy(Sym, &New, getSymbolSize(New));
+  Symbol Old = *this;
+  memcpy(this, &New, New.getSymbolSize());
 
-  Sym->VersionId = Old.VersionId;
-  Sym->Visibility = Old.Visibility;
-  Sym->IsUsedInRegularObj = Old.IsUsedInRegularObj;
-  Sym->ExportDynamic = Old.ExportDynamic;
-  Sym->CanInline = Old.CanInline;
-  Sym->Traced = Old.Traced;
-  Sym->IsPreemptible = Old.IsPreemptible;
-  Sym->ScriptDefined = Old.ScriptDefined;
+  VersionId = Old.VersionId;
+  Visibility = Old.Visibility;
+  IsUsedInRegularObj = Old.IsUsedInRegularObj;
+  ExportDynamic = Old.ExportDynamic;
+  CanInline = Old.CanInline;
+  Traced = Old.Traced;
+  IsPreemptible = Old.IsPreemptible;
+  ScriptDefined = Old.ScriptDefined;
 
   // Print out a log message if --trace-symbol was specified.
   // This is for debugging.
-  if (Sym->Traced)
-    printTraceSymbol(Sym);
+  if (Traced)
+    printTraceSymbol(this);
 }
 
 void maybeWarnUnorderableSymbol(const Symbol *Sym);
 } // namespace elf
 } // namespace lld
 
 #endif