ELF: Don't query a shared symbol's alignment unless necessary.

[pcc]

Summary

There are some cases where cannot infer a shared symbol's alignment.
But a shared symbol's alignment only matters when creating a copy
relocation. This change moves the alignment computation into the code
that creates copy relocations.

[ruiu]

This patch itself looks decent, but I think the principle here is to avoid referring ELF-file data structures directly from Symbol to reduce dependency between Symbol and the underlying ELF files. Previously, we store a pointer to ELFSym to each Symbol, and at some point we rewrote to eliminate that. That makes it easy to create a fake Shared symbol too.

[pcc]

That certainly makes sense for Defined, but maybe not so much for SharedSymbol, at least when we don't have any code which actually creates fake shared symbols. You'll note that there are already dependencies on there being an actual .so file, e.g. with VerdefIndex, and I reckon that it's probably better to depend on the .so fully rather than being in a half dependent state.

[ruiu]

That's true that a shared symbol can refer an underlying ELF data structure using VerdefIndex, but that's I believe optional. You can leave it out with the default value to represent "no version". For ELFSym, it is mandatory, and we are not doing that in other places, so I'm inclined to not do this.

[pcc]

There are other cases as well: http://llvm-cs.pcc.me.uk/tools/lld/ELF/Symbols.h/rgetFile

But I guess I don't feel too strongly so I'll abandon this.

lld/ELF/InputFiles.cpp

for (unsigned I = 0; I != VerdefCount; ++I) {
     if (Verdefs.size() <= VerdefIndex)
       Verdefs.resize(VerdefIndex + 1);
     Verdefs[VerdefIndex] = CurVerdef;
   }
 
   return Verdefs;
 }
 
-// We do not usually care about alignments of data in shared object
-// files because the loader takes care of it. However, if we promote a
-// DSO symbol to point to .bss due to copy relocation, we need to keep
-// the original alignment requirements. We infer it in this function.
-template <class ELFT>
-uint32_t SharedFile<ELFT>::getAlignment(ArrayRef<Elf_Shdr> Sections,
-                                        const Elf_Sym &Sym) {
-  uint64_t Ret = 1;
-  if (Sym.st_value)
-    Ret = 1ULL << countTrailingZeros((uint64_t)Sym.st_value);
-  if (0 < Sym.st_shndx && Sym.st_shndx < Sections.size())
-    Ret = std::min<uint64_t>(Ret, Sections[Sym.st_shndx].sh_addralign);
-
-  if (Ret > UINT32_MAX)
-    error(toString(this) + ": alignment too large: " +
-          CHECK(Sym.getName(this->StringTable), this));
-  return Ret;
-}
-
 // Fully parse the shared object file. This must be called after parseSoName().
 //
 // This function parses symbol versions. If a DSO has version information,
 // the file has a ".gnu.version_d" section which contains symbol version
 // definitions. Each symbol is associated to one version through a table in
 // ".gnu.version" section. That table is a parallel array for the symbol
 // table, and each table entry contains an index in ".gnu.version_d".
 //
 // The special index 0 is reserved for VERF_NDX_LOCAL and 1 is for
 // VER_NDX_GLOBAL. There's no table entry for these special versions in
 // ".gnu.version_d".
 //
 // The file format for symbol versioning is perhaps a bit more complicated
 // than necessary, but you can easily understand the code if you wrap your
 // head around the data structure described above.
 template <class ELFT> void SharedFile<ELFT>::parseRest() {
   Verdefs = parseVerdefs();                       // parse .gnu.version_d
   std::vector<uint32_t> Versyms = parseVersyms(); // parse .gnu.version
-  ArrayRef<Elf_Shdr> Sections = CHECK(this->getObj().sections(), this);
 
   // System libraries can have a lot of symbols with versions. Using a
   // fixed buffer for computing the versions name (foo@ver) can save a
   // lot of allocations.
   SmallString<0> VersionedNameBuffer;
 
   // Add symbols to the symbol table.
   ArrayRef<Elf_Sym> Syms = this->getGlobalELFSyms();
for (size_t I = 0; I < Syms.size(); ++I) {
     // MIPS BFD linker puts _gp_disp symbol into DSO files and incorrectly
     // assigns VER_NDX_LOCAL to this section global symbol. Here is a
     // workaround for this bug.
     uint32_t Idx = Versyms[I] & ~VERSYM_HIDDEN;
     if (Config->EMachine == EM_MIPS && Idx == VER_NDX_LOCAL &&
         Name == "_gp_disp")
       continue;
 
-    uint64_t Alignment = getAlignment(Sections, Sym);
     if (!(Versyms[I] & VERSYM_HIDDEN))
-      Symtab->addShared(Name, *this, Sym, Alignment, Idx);
+      Symtab->addShared(Name, *this, Sym, Idx);
 
     // Also add the symbol with the versioned name to handle undefined symbols
     // with explicit versions.
     if (Idx == VER_NDX_GLOBAL)
       continue;
 
     if (Idx >= Verdefs.size() || Idx == VER_NDX_LOCAL) {
       error("corrupt input file: version definition index " + Twine(Idx) +
             " for symbol " + Name + " is out of bounds\n>>> defined in " +
             toString(this));
       continue;
     }
 
     StringRef VerName =
         this->StringTable.data() + Verdefs[Idx]->getAux()->vda_name;
     VersionedNameBuffer.clear();
     Name = (Name + "@" + VerName).toStringRef(VersionedNameBuffer);
-    Symtab->addShared(Saver.save(Name), *this, Sym, Alignment, Idx);
+    Symtab->addShared(Saver.save(Name), *this, Sym, Idx);
   }
 }
 
 static ELFKind getBitcodeELFKind(const Triple &T) {
   if (T.isLittleEndian())
     return T.isArch64Bit() ? ELF64LEKind : ELF32LEKind;
   return T.isArch64Bit() ? ELF64BEKind : ELF32BEKind;
 }

lld/ELF/Relocations.cpp

 }
 
 // Returns true if a given shared symbol is in a read-only segment in a DSO.
 template <class ELFT> static bool isReadOnly(SharedSymbol &SS) {
   typedef typename ELFT::Phdr Elf_Phdr;
 
   // Determine if the symbol is read-only by scanning the DSO's program headers.
   const SharedFile<ELFT> &File = SS.getFile<ELFT>();
+  uint64_t Value = SS.getSym<ELFT>().st_value;
   for (const Elf_Phdr &Phdr : check(File.getObj().program_headers()))
     if ((Phdr.p_type == ELF::PT_LOAD || Phdr.p_type == ELF::PT_GNU_RELRO) &&
-        !(Phdr.p_flags & ELF::PF_W) && SS.Value >= Phdr.p_vaddr &&
-        SS.Value < Phdr.p_vaddr + Phdr.p_memsz)
+        !(Phdr.p_flags & ELF::PF_W) && Value >= Phdr.p_vaddr &&
+        Value < Phdr.p_vaddr + Phdr.p_memsz)
       return true;
   return false;
 }
 
 // Returns symbols at the same offset as a given symbol, including SS itself.
 //
 // If two or more symbols are at the same offset, and at least one of
 // them are copied by a copy relocation, all of them need to be copied.
 // Otherwise, they would refer different places at runtime.
 template <class ELFT>
 static std::vector<SharedSymbol *> getSymbolsAt(SharedSymbol &SS) {
   typedef typename ELFT::Sym Elf_Sym;
 
   SharedFile<ELFT> &File = SS.getFile<ELFT>();
 
   std::vector<SharedSymbol *> Ret;
+  uint64_t Value = SS.getSym<ELFT>().st_value;
   for (const Elf_Sym &S : File.getGlobalELFSyms()) {
     if (S.st_shndx == SHN_UNDEF || S.st_shndx == SHN_ABS ||
-        S.st_value != SS.Value)
+        S.st_value != Value)
       continue;
     StringRef Name = check(S.getName(File.getStringTable()));
     Symbol *Sym = Symtab->find(Name);
     if (auto *Alias = dyn_cast_or_null<SharedSymbol>(Sym))
       Ret.push_back(Alias);
   }
   return Ret;
 }

 // existing members. That's because the space for the new members are not
 // reserved in .bss unless you recompile the main program. That means they
 // are likely to overlap with other data that happens to be laid out next
 // to the variable in .bss. This kind of issue is sometimes very hard to
 // debug. What's a solution? Instead of exporting a varaible V from a DSO,
 // define an accessor getV().
 template <class ELFT> static void addCopyRelSymbol(SharedSymbol &SS) {
   // Copy relocation against zero-sized symbol doesn't make sense.
-  uint64_t SymSize = SS.getSize();
+  uint64_t SymSize = SS.getSym<ELFT>().st_size;
   if (SymSize == 0)
     fatal("cannot create a copy relocation for symbol " + toString(SS));
 
   // See if this symbol is in a read-only segment. If so, preserve the symbol's
   // memory protection by reserving space in the .bss.rel.ro section.
   bool IsReadOnly = isReadOnly<ELFT>(SS);
   BssSection *Sec = make<BssSection>(IsReadOnly ? ".bss.rel.ro" : ".bss",
-                                     SymSize, SS.Alignment);
+                                     SymSize, SS.getAlignment<ELFT>());
   if (IsReadOnly)
     InX::BssRelRo->getParent()->addSection(Sec);
   else
     InX::Bss->getParent()->addSection(Sec);
 
   // Look through the DSO's dynamic symbol table for aliases and create a
   // dynamic symbol for each one. This causes the copy relocation to correctly
   // interpose any aliases.
   for (SharedSymbol *Sym : getSymbolsAt<ELFT>(SS))
-    replaceWithDefined(*Sym, Sec, 0, Sym->Size);
+    replaceWithDefined(*Sym, Sec, 0, SymSize);
 
   InX::RelaDyn->addReloc(Target->CopyRel, Sec, 0, &SS);
 }
 
 // MIPS has an odd notion of "paired" relocations to calculate addends.
 // For example, if a relocation is of R_MIPS_HI16, there must be a
 // R_MIPS_LO16 relocation after that, and an addend is calculated using
 // the two relocations.

lld/ELF/SymbolTable.h

public:
   Symbol *addUndefined(StringRef Name, uint8_t Binding, uint8_t StOther,
                        uint8_t Type, bool CanOmitFromDynSym, InputFile *File);
   Symbol *addRegular(StringRef Name, uint8_t StOther, uint8_t Type,
                      uint64_t Value, uint64_t Size, uint8_t Binding,
                      SectionBase *Section, InputFile *File);
 
   template <class ELFT>
   void addShared(StringRef Name, SharedFile<ELFT> &F,
-                 const typename ELFT::Sym &Sym, uint32_t Alignment,
-                 uint32_t VerdefIndex);
+                 const typename ELFT::Sym &Sym, uint32_t VerdefIndex);
 
   template <class ELFT>
   void addLazyArchive(StringRef Name, ArchiveFile &F,
                       const llvm::object::Archive::Symbol S);
 
   template <class ELFT> void addLazyObject(StringRef Name, LazyObjFile &Obj);
 
   Symbol *addBitcode(StringRef Name, uint8_t Binding, uint8_t StOther,

lld/ELF/SymbolTable.cpp

Symbol *SymbolTable::addRegular(StringRef Name, uint8_t StOther, uint8_t Type,
   else if (Cmp == 0)
     reportDuplicate(S, File, dyn_cast_or_null<InputSectionBase>(Section),
                     Value);
   return S;
 }
 
 template <typename ELFT>
 void SymbolTable::addShared(StringRef Name, SharedFile<ELFT> &File,
-                            const typename ELFT::Sym &Sym, uint32_t Alignment,
+                            const typename ELFT::Sym &Sym,
                             uint32_t VerdefIndex) {
   // DSO symbols do not affect visibility in the output, so we pass STV_DEFAULT
   // as the visibility, which will leave the visibility in the symbol table
   // unchanged.
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name, Sym.getType(), STV_DEFAULT,
                                     /*CanOmitFromDynSym*/ true, &File);
   // Make sure we preempt DSO symbols with default visibility.
   if (Sym.getVisibility() == STV_DEFAULT)
     S->ExportDynamic = true;
 
   // An undefined symbol with non default visibility must be satisfied
   // in the same DSO.
   if (WasInserted ||
       ((S->isUndefined() || S->isLazy()) && S->Visibility == STV_DEFAULT)) {
     uint8_t Binding = S->Binding;
     bool WasUndefined = S->isUndefined();
-    replaceSymbol<SharedSymbol>(S, File, Name, Sym.getBinding(), Sym.st_other,
-                                Sym.getType(), Sym.st_value, Sym.st_size,
-                                Alignment, VerdefIndex);
+    replaceSymbol<SharedSymbol>(
+        S, File, Name, Sym.getBinding(), Sym.st_other, Sym.getType(),
+        reinterpret_cast<const void *>(&Sym), VerdefIndex);
     if (!WasInserted) {
       S->Binding = Binding;
       if (!S->isWeak() && !Config->GcSections && WasUndefined)
         File.IsNeeded = true;
     }
   }
 }
 

 
 template void SymbolTable::fetchLazy<ELF32LE>(Symbol *);
 template void SymbolTable::fetchLazy<ELF32BE>(Symbol *);
 template void SymbolTable::fetchLazy<ELF64LE>(Symbol *);
 template void SymbolTable::fetchLazy<ELF64BE>(Symbol *);
 
 template void SymbolTable::addShared<ELF32LE>(StringRef, SharedFile<ELF32LE> &,
                                               const typename ELF32LE::Sym &,
-                                              uint32_t Alignment, uint32_t);
+                                              uint32_t);
 template void SymbolTable::addShared<ELF32BE>(StringRef, SharedFile<ELF32BE> &,
                                               const typename ELF32BE::Sym &,
-                                              uint32_t Alignment, uint32_t);
+                                              uint32_t);
 template void SymbolTable::addShared<ELF64LE>(StringRef, SharedFile<ELF64LE> &,
                                               const typename ELF64LE::Sym &,
-                                              uint32_t Alignment, uint32_t);
+                                              uint32_t);
 template void SymbolTable::addShared<ELF64BE>(StringRef, SharedFile<ELF64BE> &,
                                               const typename ELF64BE::Sym &,
-                                              uint32_t Alignment, uint32_t);
+                                              uint32_t);

lld/ELF/Symbols.h

public:
   static bool classof(const Symbol *S) { return S->kind() == UndefinedKind; }
 };
 
 class SharedSymbol : public Symbol {
 public:
   static bool classof(const Symbol *S) { return S->kind() == SharedKind; }
 
   SharedSymbol(InputFile &File, StringRef Name, uint8_t Binding,
-               uint8_t StOther, uint8_t Type, uint64_t Value, uint64_t Size,
-               uint32_t Alignment, uint32_t VerdefIndex)
-      : Symbol(SharedKind, &File, Name, Binding, StOther, Type),
-        Alignment(Alignment), Value(Value), Size(Size) {
+               uint8_t StOther, uint8_t Type, const void *Sym,
+               uint32_t VerdefIndex)
+      : Symbol(SharedKind, &File, Name, Binding, StOther, Type), Sym(Sym) {
     this->VerdefIndex = VerdefIndex;
     // GNU ifunc is a mechanism to allow user-supplied functions to
     // resolve PLT slot values at load-time. This is contrary to the
     // regular symbol resolution scheme in which symbols are resolved just
     // by name. Using this hook, you can program how symbols are solved
     // for you program. For example, you can make "memcpy" to be resolved
     // to a SSE-enabled version of memcpy only when a machine running the
     // program supports the SSE instruction set.
SharedSymbol(InputFile &File, StringRef Name, uint8_t Binding,
     if (this->Type == llvm::ELF::STT_GNU_IFUNC)
       this->Type = llvm::ELF::STT_FUNC;
   }
 
   template <class ELFT> SharedFile<ELFT> &getFile() const {
     return *cast<SharedFile<ELFT>>(File);
   }
 
-  uint32_t Alignment;
+  const void *Sym;
+  template <class ELFT> const typename ELFT::Sym &getSym() const {
+    return *static_cast<const typename ELFT::Sym *>(Sym);
+  }
 
-  uint64_t Value; // st_value
-  uint64_t Size;  // st_size
+  template <class ELFT> uint32_t getAlignment() const;
 };
 
 // LazyArchive and LazyObject represent a symbols that is not yet in the link,
 // but we know where to find it if needed. If the resolver finds both Undefined
 // and Lazy for the same name, it will ask the Lazy to load a file.
 //
 // A special complication is the handling of weak undefined symbols. They should
 // not load a file, but we have to remember we have seen both the weak undefined

lld/ELF/Symbols.cpp

 uint64_t Symbol::getPltOffset() const {
   assert(!this->IsInIplt);
   return Target->getPltEntryOffset(PltIndex);
 }
 
 uint64_t Symbol::getSize() const {
   if (const auto *DR = dyn_cast<Defined>(this))
     return DR->Size;
-  if (const auto *S = dyn_cast<SharedSymbol>(this))
-    return S->Size;
   return 0;
 }
 
 OutputSection *Symbol::getOutputSection() const {
   if (auto *S = dyn_cast<Defined>(this)) {
     if (auto *Sec = S->Section)
       return Sec->Repl->getOutputSection();
     return nullptr;
if (!Config->HasDynSymTab)
     return false;
   if (computeBinding() == STB_LOCAL)
     return false;
   if (!isDefined())
     return true;
   return ExportDynamic;
 }
 
+// We do not usually care about alignments of data in shared object
+// files because the loader takes care of it. However, if we promote a
+// DSO symbol to point to .bss due to copy relocation, we need to keep
+// the original alignment requirements. We infer it in this function.
+template <class ELFT> uint32_t SharedSymbol::getAlignment() const {
+  SharedFile<ELFT> &File = this->getFile<ELFT>();
+  ArrayRef<typename ELFT::Shdr> Sections =
+      CHECK(File.getObj().sections(), &File);
+  const typename ELFT::Sym &Sym = this->getSym<ELFT>();
+
+  uint64_t Ret = 1;
+  if (Sym.st_value)
+    Ret = 1ULL << countTrailingZeros((uint64_t)Sym.st_value);
+  if (0 < Sym.st_shndx && Sym.st_shndx < Sections.size())
+    Ret = std::min<uint64_t>(Ret, Sections[Sym.st_shndx].sh_addralign);
+
+  if (Ret > UINT32_MAX)
+    error(toString(&File) + ": alignment too large: " +
+          CHECK(Sym.getName(File.getStringTable()), &File));
+  return Ret;
+}
+
 // Print out a log message for --trace-symbol.
 void elf::printTraceSymbol(Symbol *Sym) {
   std::string S;
   if (Sym->isUndefined())
     S = ": reference to ";
   else if (Sym->isLazy())
     S = ": lazy definition of ";
   else if (Sym->isShared())

 
 // Returns a symbol for an error message.
 std::string lld::toString(const Symbol &B) {
   if (Config->Demangle)
     if (Optional<std::string> S = demangleItanium(B.getName()))
       return *S;
   return B.getName();
 }
+
+template uint32_t SharedSymbol::getAlignment<ELF32LE>() const;
+template uint32_t SharedSymbol::getAlignment<ELF32BE>() const;
+template uint32_t SharedSymbol::getAlignment<ELF64LE>() const;
+template uint32_t SharedSymbol::getAlignment<ELF64BE>() const;

lld/test/ELF/shared-abs.s

+// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+// RUN: ld.lld -shared %t.o -o %t.so
+// RUN: ld.lld %t.so -o %t
+
+.globl abs
+abs = 1 << 33