Avoid almost duplication in .dynamic finalize and write.

[• rafael]

Summary

There are a few cases where we have almost duplicated code.

This patches fixes the simplest: the finalize and write of dynamic section. Right now they have to have exactly the same structure to decide if a DT_* entry is needed and then to actually write it.

We cannot just write it to a std::vector in the first pass since addresses have not been computed yet.

[• rafael]

rebased

[ruiu]

LGTM. This is a very nice change. Thank you for doing this.

ELF/OutputSections.cpp

596

It is better to describe a bit more detail. Because only this DynamicSection adds strings to DynStrTab, we can compute the size of DynStrTab early in this function, and that helps us to fill DT_STRSZ field in this function.

[ruiu]

ELF/OutputSections.cpp

713–723

These helper functions don't make sense any more. You can inline them.

[emaste]

I wondered about this when I added DT_DEBUG -- thanks for doing this!

[• rafael]

r258723

ELF/OutputSections.h

 template <class ELFT>
 class DynamicSection final : public OutputSectionBase<ELFT> {
   typedef OutputSectionBase<ELFT> Base;
   typedef typename llvm::object::ELFFile<ELFT>::Elf_Dyn Elf_Dyn;
   typedef typename llvm::object::ELFFile<ELFT>::Elf_Rel Elf_Rel;
   typedef typename llvm::object::ELFFile<ELFT>::Elf_Rela Elf_Rela;
   typedef typename llvm::object::ELFFile<ELFT>::Elf_Shdr Elf_Shdr;
   typedef typename llvm::object::ELFFile<ELFT>::Elf_Sym Elf_Sym;
+  typedef typename llvm::object::ELFFile<ELFT>::uintX_t uintX_t;
+
+  struct Entry {
+    int32_t Tag;
+    union {
+      OutputSectionBase<ELFT> *OutSec;
+      uint64_t Val;
+      const SymbolBody *Sym;
+    };
+    enum KindT { SecAddr, SymAddr, PlainInt } Kind;
+    Entry(int32_t Tag, OutputSectionBase<ELFT> *OutSec)
+        : Tag(Tag), OutSec(OutSec), Kind(SecAddr) {}
+    Entry(int32_t Tag, uint64_t Val) : Tag(Tag), Val(Val), Kind(PlainInt) {}
+    Entry(int32_t Tag, const SymbolBody *Sym)
+        : Tag(Tag), Sym(Sym), Kind(SymAddr) {}
+  };
+  std::vector<Entry> Entries;
 
 public:
   DynamicSection(SymbolTable<ELFT> &SymTab);
   void finalize() override;
   void writeTo(uint8_t *Buf) override;
 
   OutputSectionBase<ELFT> *PreInitArraySec = nullptr;
   OutputSectionBase<ELFT> *InitArraySec = nullptr;

ELF/OutputSections.cpp

 
 template <class ELFT> void DynamicSection<ELFT>::finalize() {
   if (this->Header.sh_size)
     return; // Already finalized.
 
   Elf_Shdr &Header = this->Header;
   Header.sh_link = Out<ELFT>::DynStrTab->SectionIndex;
 
-  unsigned NumEntries = 0;
+  // Reserve strings
+  if (!Config->RPath.empty())
+    Out<ELFT>::DynStrTab->reserve(Config->RPath);
+  if (!Config->SoName.empty())
+    Out<ELFT>::DynStrTab->reserve(Config->SoName);
+  for (const std::unique_ptr<SharedFile<ELFT>> &F : SymTab.getSharedFiles())
+    if (F->isNeeded())
+      Out<ELFT>::DynStrTab->reserve(F->getSoName());
+  Out<ELFT>::DynStrTab->finalize();
+
   if (Out<ELFT>::RelaDyn->hasRelocs()) {
-    ++NumEntries; // DT_RELA / DT_REL
-    ++NumEntries; // DT_RELASZ / DT_RELSZ
-    ++NumEntries; // DT_RELAENT / DT_RELENT
+    bool IsRela = Out<ELFT>::RelaDyn->isRela();
+    Entries.push_back({IsRela ? DT_RELA : DT_REL, Out<ELFT>::RelaDyn});
+    Entries.push_back(
+        {IsRela ? DT_RELASZ : DT_RELSZ, Out<ELFT>::RelaDyn->getSize()});
+    Entries.push_back({IsRela ? DT_RELAENT : DT_RELENT,
+                       uintX_t(IsRela ? sizeof(Elf_Rela) : sizeof(Elf_Rel))});
   }
   if (Out<ELFT>::RelaPlt && Out<ELFT>::RelaPlt->hasRelocs()) {
-    ++NumEntries; // DT_JMPREL
-    ++NumEntries; // DT_PLTRELSZ
-    ++NumEntries; // DT_PLTGOT / DT_MIPS_PLTGOT
-    ++NumEntries; // DT_PLTREL
+    Entries.push_back({DT_JMPREL, Out<ELFT>::RelaPlt});
+    Entries.push_back({DT_PLTRELSZ, Out<ELFT>::RelaPlt->getSize()});
+    Entries.push_back({Config->EMachine == EM_MIPS ? DT_MIPS_PLTGOT : DT_PLTGOT,
+                       Out<ELFT>::GotPlt});
+    Entries.push_back(
+        {DT_PLTREL, Out<ELFT>::RelaPlt->isRela() ? DT_RELA : DT_REL});
   }
 
-  ++NumEntries; // DT_SYMTAB
-  ++NumEntries; // DT_SYMENT
-  ++NumEntries; // DT_STRTAB
-  ++NumEntries; // DT_STRSZ
+  Entries.push_back({DT_SYMTAB, Out<ELFT>::DynSymTab});
+  Entries.push_back({DT_SYMENT, sizeof(Elf_Sym)});
+  Entries.push_back({DT_STRTAB, Out<ELFT>::DynStrTab});
+  Entries.push_back({DT_STRSZ, Out<ELFT>::DynStrTab->getSize()});
   if (Out<ELFT>::GnuHashTab)
-    ++NumEntries; // DT_GNU_HASH
+    Entries.push_back({DT_GNU_HASH, Out<ELFT>::GnuHashTab});
   if (Out<ELFT>::HashTab)
-    ++NumEntries; // DT_HASH
-
-  if (!Config->RPath.empty()) {
-    ++NumEntries; // DT_RUNPATH / DT_RPATH
-    Out<ELFT>::DynStrTab->reserve(Config->RPath);
-  }
+    Entries.push_back({DT_HASH, Out<ELFT>::HashTab});
 
-  if (!Config->SoName.empty()) {
-    ++NumEntries; // DT_SONAME
-    Out<ELFT>::DynStrTab->reserve(Config->SoName);
-  }
+  if (!Config->RPath.empty())
+    Entries.push_back({Config->EnableNewDtags ? DT_RUNPATH : DT_RPATH,
+                       Out<ELFT>::DynStrTab->addString(Config->RPath)});
 
-  if (PreInitArraySec)
-    NumEntries += 2;
-  if (InitArraySec)
-    NumEntries += 2;
-  if (FiniArraySec)
-    NumEntries += 2;
+  if (!Config->SoName.empty())
+    Entries.push_back(
+        {DT_SONAME, Out<ELFT>::DynStrTab->addString(Config->SoName)});
 
-  for (const std::unique_ptr<SharedFile<ELFT>> &F : SymTab.getSharedFiles()) {
-    if (!F->isNeeded())
-      continue;
-    Out<ELFT>::DynStrTab->reserve(F->getSoName());
-    ++NumEntries;
+  if (PreInitArraySec) {
+    Entries.push_back({DT_PREINIT_ARRAY, PreInitArraySec});
+    Entries.push_back({DT_PREINIT_ARRAYSZ, PreInitArraySec->getSize()});
+  }
+  if (InitArraySec) {
+    Entries.push_back({DT_INIT_ARRAY, InitArraySec});
+    Entries.push_back({DT_INIT_ARRAYSZ, (uintX_t)InitArraySec->getSize()});
+  }
+  if (FiniArraySec) {
+    Entries.push_back({DT_FINI_ARRAY, FiniArraySec});
+    Entries.push_back({DT_FINI_ARRAYSZ, (uintX_t)FiniArraySec->getSize()});
   }
 
+  for (const std::unique_ptr<SharedFile<ELFT>> &F : SymTab.getSharedFiles())
+    if (F->isNeeded())
+      Entries.push_back(
+          {DT_NEEDED, Out<ELFT>::DynStrTab->addString(F->getSoName())});
+
   if (Symbol *S = SymTab.getSymbols().lookup(Config->Init))
     InitSym = S->Body;
   if (Symbol *S = SymTab.getSymbols().lookup(Config->Fini))
     FiniSym = S->Body;
   if (InitSym)
-    ++NumEntries; // DT_INIT
+    Entries.push_back({DT_INIT, InitSym});
   if (FiniSym)
-    ++NumEntries; // DT_FINI
+    Entries.push_back({DT_FINI, FiniSym});
 
   if (Config->Bsymbolic)
     DtFlags |= DF_SYMBOLIC;
   if (Config->ZNodelete)
     DtFlags1 |= DF_1_NODELETE;
   if (Config->ZNow) {
     DtFlags |= DF_BIND_NOW;
     DtFlags1 |= DF_1_NOW;
   }
   if (Config->ZOrigin) {
     DtFlags |= DF_ORIGIN;
     DtFlags1 |= DF_1_ORIGIN;
   }
 
   if (DtFlags)
-    ++NumEntries; // DT_FLAGS
+    Entries.push_back({DT_FLAGS, DtFlags});
   if (DtFlags1)
-    ++NumEntries; // DT_FLAGS_1
+    Entries.push_back({DT_FLAGS_1, DtFlags1});
 
   if (!Config->Entry.empty())
-    ++NumEntries; // DT_DEBUG
+    Entries.push_back({DT_DEBUG, (uintX_t)0});
 
   if (Config->EMachine == EM_MIPS) {
-    ++NumEntries; // DT_MIPS_RLD_VERSION
-    ++NumEntries; // DT_MIPS_FLAGS
-    ++NumEntries; // DT_MIPS_BASE_ADDRESS
-    ++NumEntries; // DT_MIPS_SYMTABNO
-    ++NumEntries; // DT_MIPS_LOCAL_GOTNO
-    ++NumEntries; // DT_MIPS_GOTSYM;
-    ++NumEntries; // DT_PLTGOT
+    Entries.push_back({DT_MIPS_RLD_VERSION, 1});
+    Entries.push_back({DT_MIPS_FLAGS, RHF_NOTPOT});
+    Entries.push_back({DT_MIPS_BASE_ADDRESS, (uintX_t)Target->getVAStart()});
+    Entries.push_back(
+        {DT_MIPS_SYMTABNO, Out<ELFT>::DynSymTab->getNumSymbols()});
+    Entries.push_back(
+        {DT_MIPS_LOCAL_GOTNO, Out<ELFT>::Got->getMipsLocalEntriesNum()});
+    if (const SymbolBody *B = Out<ELFT>::Got->getMipsFirstGlobalEntry())
+      Entries.push_back({DT_MIPS_GOTSYM, B->DynamicSymbolTableIndex});
+    else
+      Entries.push_back(
+          {DT_MIPS_GOTSYM, Out<ELFT>::DynSymTab->getNumSymbols()});
+    Entries.push_back({DT_PLTGOT, Out<ELFT>::Got});
     if (Out<ELFT>::MipsRldMap)
-      ++NumEntries; // DT_MIPS_RLD_MAP
+      Entries.push_back({DT_MIPS_RLD_MAP, Out<ELFT>::MipsRldMap});
   }
 
-  ++NumEntries; // DT_NULL
-
-  Header.sh_size = NumEntries * Header.sh_entsize;
+  // +1 for DT_NULL
+  Header.sh_size = (Entries.size() + 1) * Header.sh_entsize;
 }
 
 template <class ELFT> void DynamicSection<ELFT>::writeTo(uint8_t *Buf) {
   auto *P = reinterpret_cast<Elf_Dyn *>(Buf);
 
   auto WritePtr = [&](int32_t Tag, uint64_t Val) {
     P->d_tag = Tag;
     P->d_un.d_ptr = Val;
     ++P;
   };
 
   auto WriteVal = [&](int32_t Tag, uint32_t Val) {
     P->d_tag = Tag;
     P->d_un.d_val = Val;
     ++P;
   };
 
-  if (Out<ELFT>::RelaDyn->hasRelocs()) {
-    bool IsRela = Out<ELFT>::RelaDyn->isRela();
-    WritePtr(IsRela ? DT_RELA : DT_REL, Out<ELFT>::RelaDyn->getVA());
-    WriteVal(IsRela ? DT_RELASZ : DT_RELSZ, Out<ELFT>::RelaDyn->getSize());
-    WriteVal(IsRela ? DT_RELAENT : DT_RELENT,
-             IsRela ? sizeof(Elf_Rela) : sizeof(Elf_Rel));
-  }
-  if (Out<ELFT>::RelaPlt && Out<ELFT>::RelaPlt->hasRelocs()) {
-    WritePtr(DT_JMPREL, Out<ELFT>::RelaPlt->getVA());
-    WriteVal(DT_PLTRELSZ, Out<ELFT>::RelaPlt->getSize());
-    // On MIPS, the address of the .got.plt section is stored in
-    // the DT_MIPS_PLTGOT entry because the DT_PLTGOT entry points to
-    // the .got section. See "Dynamic Section" in the following document:
-    // https://sourceware.org/ml/binutils/2008-07/txt00000.txt
-    WritePtr((Config->EMachine == EM_MIPS) ? DT_MIPS_PLTGOT : DT_PLTGOT,
-             Out<ELFT>::GotPlt->getVA());
-    WriteVal(DT_PLTREL, Out<ELFT>::RelaPlt->isRela() ? DT_RELA : DT_REL);
+  for (const Entry &E : Entries) {
+    switch (E.Kind) {
+    case Entry::SecAddr:
+      WritePtr(E.Tag, E.OutSec->getVA());
+      break;
+    case Entry::SymAddr:
+      WritePtr(E.Tag, getSymVA<ELFT>(*E.Sym));
+      break;
+    case Entry::PlainInt:
+      WriteVal(E.Tag, E.Val);
+      break;
     }
-
-  WritePtr(DT_SYMTAB, Out<ELFT>::DynSymTab->getVA());
-  WritePtr(DT_SYMENT, sizeof(Elf_Sym));
-  WritePtr(DT_STRTAB, Out<ELFT>::DynStrTab->getVA());
-  WriteVal(DT_STRSZ, Out<ELFT>::DynStrTab->getSize());
-  if (Out<ELFT>::GnuHashTab)
-    WritePtr(DT_GNU_HASH, Out<ELFT>::GnuHashTab->getVA());
-  if (Out<ELFT>::HashTab)
-    WritePtr(DT_HASH, Out<ELFT>::HashTab->getVA());
-
-  // If --enable-new-dtags is set, lld emits DT_RUNPATH
-  // instead of DT_RPATH. The two tags are functionally
-  // equivalent except for the following:
-  // - DT_RUNPATH is searched after LD_LIBRARY_PATH, while
-  //   DT_RPATH is searched before.
-  // - DT_RUNPATH is used only to search for direct
-  //   dependencies of the object it's contained in, while
-  //   DT_RPATH is used for indirect dependencies as well.
-  if (!Config->RPath.empty())
-    WriteVal(Config->EnableNewDtags ? DT_RUNPATH : DT_RPATH,
-             Out<ELFT>::DynStrTab->addString(Config->RPath));
-
-  if (!Config->SoName.empty())
-    WriteVal(DT_SONAME, Out<ELFT>::DynStrTab->addString(Config->SoName));
-
-  auto WriteArray = [&](int32_t T1, int32_t T2,
-                        const OutputSectionBase<ELFT> *Sec) {
-    if (!Sec)
-      return;
-    WritePtr(T1, Sec->getVA());
-    WriteVal(T2, Sec->getSize());
-  };
-  WriteArray(DT_PREINIT_ARRAY, DT_PREINIT_ARRAYSZ, PreInitArraySec);
-  WriteArray(DT_INIT_ARRAY, DT_INIT_ARRAYSZ, InitArraySec);
-  WriteArray(DT_FINI_ARRAY, DT_FINI_ARRAYSZ, FiniArraySec);
-
-  for (const std::unique_ptr<SharedFile<ELFT>> &F : SymTab.getSharedFiles())
-    if (F->isNeeded())
-      WriteVal(DT_NEEDED, Out<ELFT>::DynStrTab->addString(F->getSoName()));
-
-  if (InitSym)
-    WritePtr(DT_INIT, getSymVA<ELFT>(*InitSym));
-  if (FiniSym)
-    WritePtr(DT_FINI, getSymVA<ELFT>(*FiniSym));
-  if (DtFlags)
-    WriteVal(DT_FLAGS, DtFlags);
-  if (DtFlags1)
-    WriteVal(DT_FLAGS_1, DtFlags1);
-  if (!Config->Entry.empty())
-    WriteVal(DT_DEBUG, 0);
-
-  // See "Dynamic Section" in Chapter 5 in the following document
-  // for detailed description:
-  // ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/mipsabi.pdf
-  if (Config->EMachine == EM_MIPS) {
-    WriteVal(DT_MIPS_RLD_VERSION, 1);
-    WriteVal(DT_MIPS_FLAGS, RHF_NOTPOT);
-    WritePtr(DT_MIPS_BASE_ADDRESS, Target->getVAStart());
-    WriteVal(DT_MIPS_SYMTABNO, Out<ELFT>::DynSymTab->getNumSymbols());
-    WriteVal(DT_MIPS_LOCAL_GOTNO, Out<ELFT>::Got->getMipsLocalEntriesNum());
-    if (const SymbolBody *B = Out<ELFT>::Got->getMipsFirstGlobalEntry())
-      WriteVal(DT_MIPS_GOTSYM, B->DynamicSymbolTableIndex);
-    else
-      WriteVal(DT_MIPS_GOTSYM, Out<ELFT>::DynSymTab->getNumSymbols());
-    WritePtr(DT_PLTGOT, Out<ELFT>::Got->getVA());
-    if (Out<ELFT>::MipsRldMap)
-      WritePtr(DT_MIPS_RLD_MAP, Out<ELFT>::MipsRldMap->getVA());
   }
-
-  WriteVal(DT_NULL, 0);
 }
 
 template <class ELFT>
 EhFrameHeader<ELFT>::EhFrameHeader()
     : OutputSectionBase<ELFT>(".eh_frame_hdr", llvm::ELF::SHT_PROGBITS,
                               SHF_ALLOC) {
   // It's a 4 bytes of header + pointer to the contents of the .eh_frame section
   // + the number of FDE pointers in the table.

ELF/Writer.cpp

for (unsigned I = 0, N = OutputSections.size(); I < N; ++I) {
     OutputSections[I]->SectionIndex = I + 1;
     HasRelro |= (Config->ZRelro && isRelroSection(OutputSections[I]));
   }
 
   for (OutputSectionBase<ELFT> *Sec : OutputSections)
     Out<ELFT>::ShStrTab->reserve(Sec->getName());
 
   // Finalizers fix each section's size.
-  // .dynamic section's finalizer may add strings to .dynstr,
-  // so finalize that early.
-  // Likewise, .dynsym is finalized early since that may fill up .gnu.hash.
-  Out<ELFT>::Dynamic->finalize();
+  // .dynsym is finalized early since that may fill up .gnu.hash.
   if (isOutputDynamic())
     Out<ELFT>::DynSymTab->finalize();
 
-  // Fill other section headers.
+  // Fill other section headers. The dynamic string table in finalized
+  // once the .dynamic finalizer has added a few last strings.
   for (OutputSectionBase<ELFT> *Sec : OutputSections)
+    if (Sec != Out<ELFT>::DynStrTab)
       Sec->finalize();
 }
 
 // This function add Out<ELFT>::* sections to OutputSections.
 template <class ELFT> void Writer<ELFT>::addPredefinedSections() {
   auto Add = [&](OutputSectionBase<ELFT> *C) {
     if (C)
       OutputSections.push_back(C);
   };