Refactor SharedFile::parseRest. NFC.

[ruiu]

Summary

SharedFile::parseRest function grew organically and got a bit hard to
understand. This patch refactor it. This patch also adds comments.

[grimar]

This LGTM. Minor nits below.

lld/ELF/InputFiles.cpp

847 ↗

(On Diff #139692)

I would probably inline SecAlign

857 ↗

(On Diff #139692)

I think it should be "a DSO"

913 ↗

(On Diff #139692)

Lets early continue?

if (Idx == VER_NDX_GLOBAL)
  continue;

921 ↗

(On Diff #139692)

I would inline Ver.

lld/trunk/ELF/InputFiles.h

public:
   static bool classof(const InputFile *F) {
     return F->kind() == Base::SharedKind;
   }
 
   SharedFile(MemoryBufferRef M, StringRef DefaultSoName);
 
   void parseSoName();
   void parseRest();
-  std::vector<const Elf_Verdef *> parseVerdefs(const Elf_Versym *&Versym);
+  uint32_t getAlignment(ArrayRef<Elf_Shdr> Sections, const Elf_Sym &Sym);
+  std::vector<const Elf_Verdef *> parseVerdefs();
+  std::vector<uint32_t> parseVersyms();
 
   struct NeededVer {
     // The string table offset of the version name in the output file.
     size_t StrTab;
 
     // The version identifier for this version name.
     uint16_t Index;
   };

lld/trunk/ELF/InputFiles.cpp

if (Dyn.d_tag == DT_SONAME) {
       if (Val >= this->StringTable.size())
         fatal(toString(this) + ": invalid DT_SONAME entry");
       SoName = this->StringTable.data() + Val;
       return;
     }
   }
 }
 
+// Parses ".gnu.version" section which is a parallel array for the symbol table.
+// If a given file doesn't have ".gnu.version" section, returns VER_NDX_GLOBAL.
+template <class ELFT> std::vector<uint32_t> SharedFile<ELFT>::parseVersyms() {
+  size_t Size = this->ELFSyms.size() - this->FirstNonLocal;
+  if (!VersymSec)
+    return std::vector<uint32_t>(Size, VER_NDX_GLOBAL);
+
+  const char *Base = this->MB.getBuffer().data();
+  const Elf_Versym *Versym =
+      reinterpret_cast<const Elf_Versym *>(Base + VersymSec->sh_offset) +
+      this->FirstNonLocal;
+
+  std::vector<uint32_t> Ret(Size);
+  for (size_t I = 0; I < Size; ++I)
+    Ret[I] = Versym[I].vs_index;
+  return Ret;
+}
+
 // Parse the version definitions in the object file if present. Returns a vector
 // whose nth element contains a pointer to the Elf_Verdef for version identifier
 // n. Version identifiers that are not definitions map to nullptr.
 template <class ELFT>
-std::vector<const typename ELFT::Verdef *>
-SharedFile<ELFT>::parseVerdefs(const Elf_Versym *&Versym) {
-  // We only need to process symbol versions for this DSO if it has both a
-  // versym and a verdef section, which indicates that the DSO contains symbol
-  // version definitions.
-  if (!VersymSec || !VerdefSec)
+std::vector<const typename ELFT::Verdef *> SharedFile<ELFT>::parseVerdefs() {
+  if (!VerdefSec)
     return {};
 
-  // The location of the first global versym entry.
-  const char *Base = this->MB.getBuffer().data();
-  Versym = reinterpret_cast<const Elf_Versym *>(Base + VersymSec->sh_offset) +
-           this->FirstNonLocal;
-
   // We cannot determine the largest verdef identifier without inspecting
   // every Elf_Verdef, but both bfd and gold assign verdef identifiers
   // sequentially starting from 1, so we predict that the largest identifier
   // will be VerdefCount.
   unsigned VerdefCount = VerdefSec->sh_info;
   std::vector<const Elf_Verdef *> Verdefs(VerdefCount + 1);
 
   // Build the Verdefs array by following the chain of Elf_Verdef objects
   // from the start of the .gnu.version_d section.
+  const char *Base = this->MB.getBuffer().data();
   const char *Verdef = Base + VerdefSec->sh_offset;
   for (unsigned I = 0; I != VerdefCount; ++I) {
     auto *CurVerdef = reinterpret_cast<const Elf_Verdef *>(Verdef);
     Verdef += CurVerdef->vd_next;
     unsigned VerdefIndex = CurVerdef->vd_ndx;
     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() {
-  // Create mapping from version identifiers to Elf_Verdef entries.
-  const Elf_Versym *Versym = nullptr;
-  Verdefs = parseVerdefs(Versym);
-
+  Verdefs = parseVerdefs();                       // parse .gnu.version_d
+  std::vector<uint32_t> Versyms = parseVersyms(); // parse .gnu.version
   ArrayRef<Elf_Shdr> Sections = CHECK(this->getObj().sections(), this);
 
   // Add symbols to the symbol table.
-  Elf_Sym_Range Syms = this->getGlobalELFSyms();
-  for (const Elf_Sym &Sym : Syms) {
-    unsigned VersymIndex = VER_NDX_GLOBAL;
-    if (Versym) {
-      VersymIndex = Versym->vs_index;
-      ++Versym;
-    }
-    bool Hidden = VersymIndex & VERSYM_HIDDEN;
-    VersymIndex = VersymIndex & ~VERSYM_HIDDEN;
+  ArrayRef<Elf_Sym> Syms = this->getGlobalELFSyms();
+  for (size_t I = 0; I < Syms.size(); ++I) {
+    const Elf_Sym &Sym = Syms[I];
 
     StringRef Name = CHECK(Sym.getName(this->StringTable), this);
     if (Sym.isUndefined()) {
       Symbol *S = Symtab->addUndefined<ELFT>(Name, Sym.getBinding(),
                                              Sym.st_other, Sym.getType(),
                                              /*CanOmitFromDynSym=*/false, this);
       S->ExportDynamic = true;
       continue;
     }
 
     // ELF spec requires that all local symbols precede weak or global
     // symbols in each symbol table, and the index of first non-local symbol
     // is stored to sh_info. If a local symbol appears after some non-local
     // symbol, that's a violation of the spec.
     if (Sym.getBinding() == STB_LOCAL) {
       warn("found local symbol '" + Name +
            "' in global part of symbol table in file " + toString(this));
       continue;
     }
 
     // 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.
-    if (Config->EMachine == EM_MIPS && VersymIndex == VER_NDX_LOCAL &&
+    uint32_t Idx = Versyms[I] & ~VERSYM_HIDDEN;
+    if (Config->EMachine == EM_MIPS && Idx == VER_NDX_LOCAL &&
         Name == "_gp_disp")
       continue;
 
-    const Elf_Verdef *Ver = nullptr;
-    if (VersymIndex != VER_NDX_GLOBAL) {
-      if (VersymIndex >= Verdefs.size() || VersymIndex == VER_NDX_LOCAL) {
-        error("corrupt input file: version definition index " +
-              Twine(VersymIndex) + " for symbol " + Name +
-              " is out of bounds\n>>> defined in " + toString(this));
-        continue;
-      }
-      Ver = Verdefs[VersymIndex];
-    }
+    uint64_t Alignment = getAlignment(Sections, Sym);
+    if (!(Versyms[I] & VERSYM_HIDDEN))
+      Symtab->addShared(Name, *this, Sym, Alignment, Idx);
+
+    // Also add the symbol with the versioned name to handle undefined symbols
+    // with explicit versions.
+    if (Idx == VER_NDX_GLOBAL)
 
-    // 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 here.
-    uint64_t Alignment = 1;
-    if (Sym.st_value)
-      Alignment = 1ULL << countTrailingZeros((uint64_t)Sym.st_value);
-    if (0 < Sym.st_shndx && Sym.st_shndx < Sections.size()) {
-      uint64_t SecAlign = Sections[Sym.st_shndx].sh_addralign;
-      Alignment = std::min(Alignment, SecAlign);
+    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;
     }
-    if (Alignment > UINT32_MAX)
-      error(toString(this) + ": alignment too large: " + Name);
 
-    if (!Hidden)
-      Symtab->addShared(Name, *this, Sym, Alignment, VersymIndex);
-
-    // Also add the symbol with the versioned name to handle undefined symbols
-    // with explicit versions.
-    if (Ver) {
-      StringRef VerName = this->StringTable.data() + Ver->getAux()->vda_name;
-      Name = Saver.save(Name + "@" + VerName);
-      Symtab->addShared(Name, *this, Sym, Alignment, VersymIndex);
-    }
+    StringRef VerName =
+        this->StringTable.data() + Verdefs[Idx]->getAux()->vda_name;
+    Symtab->addShared(Name, *this, Sym, Alignment, Idx);
   }
 }
 
 static ELFKind getBitcodeELFKind(const Triple &T) {
   if (T.isLittleEndian())
     return T.isArch64Bit() ? ELF64LEKind : ELF32LEKind;
   return T.isArch64Bit() ? ELF64BEKind : ELF32BEKind;
 }