[ELF] - Some cleanup of Writer<ELFT>::createSections()

[grimar]

Summary

Patch performs the next changes:

• Create reserved symbols at one place.
• Add predefined sections at one place to output list.
• Separate processing commons to addCommonSymbols().
• Added fillSymTables() to keep logic about filling SymTab and DynSymTab.
• Moved scan relocations logic to scanRelocs().

I am trying to clean up this method as it do too much now, and not just what
it's name says. That hopefully will help to reuse sub-methods later for redesigning
linkerscript functionality.

[• rafael]

ELF/Writer.cpp

895

Please use the same structure we use for the other comparisons:

AIsFoo = ...;
BIsFoo = ...;
if (AIsFoo != BIsFoo)
return ...;

I wonder if it wouldn't be nicer to just put Interp and BuildId at the start of the section list in addPredefinedSections and avoid having this special cases in the comparison function.

[ruiu]

I'm not sure if how this patch would work towards full linker script support. Can you describe your plan? Thanks!

[grimar]

In D19538#413090, @ruiu wrote:

I'm not sure if how this patch would work towards full linker script support. Can you describe your plan? Thanks!

There is no real plan yet, I am still investigating possible redesign of LS. This patch is just clean up as I noticed we are creating symbols, adding sections in different places/methods now,
what is in my opinion does not look clean when we have everything to localize that in one place.
So patch does not directly related to linkerscript support except one moment:
void Writer<ELFT>::createSections() is used now not only for just creating some sections. It do a lot of things: updates symbol tables, scans relocations, finalizes the sections, something else probably. Some of these parts I hope can be reused after delegating create sections functionality to linkerscript. So I guess this function needs some cleanup and first step I would do is splitting it. But before doing that initial cleanup can be done and that is what patch do.

[grimar]

• Addressed review comment.
• Separate processing commons to addCommonSymbols().
• Added fillSymTables() to keep logic about filling SymTab and DynSymTab.
• Moved scan relocations logic to scanRelocs().

ELF/Writer.cpp

895

I am not sure, because that is not so obvious, but since we had very close logic before
and its really shorter, lets try your suggestion.

[grimar]

In D19538#413090, @ruiu wrote:

I'm not sure if how this patch would work towards full linker script support. Can you describe your plan? Thanks!

So, I upgraded this patch to make it's general usefulness (hopefully) more visible.

[grimar]

Something wrong with this ? I thought that it at least useful cleanup.

[grimar]

Let's start from litle: D19981

ELF/Writer.cpp

Phdr(unsigned Type, unsigned Flags) {
       H.p_flags = Flags;
     }
     Elf_Phdr H = {};
     OutputSectionBase<ELFT> *First = nullptr;
     OutputSectionBase<ELFT> *Last = nullptr;
   };
 
   void copyLocalSymbols();
-  void addReservedSymbols();
+  void addReservedSymbols(ArrayRef<OutputSectionBase<ELFT> *> Sections);
   void createSections();
+  void fillSymTables();
   void addPredefinedSections();
   bool needsGot();
 
   template <class RelTy>
   void scanRelocs(InputSectionBase<ELFT> &C, ArrayRef<RelTy> Rels);
 
-  void scanRelocs(InputSection<ELFT> &C);
+  void scanRelocs();
   void scanRelocs(InputSectionBase<ELFT> &S, const Elf_Shdr &RelSec);
   void createPhdrs();
   void assignAddresses();
   void assignFileOffsets();
   void setPhdrs();
   void fixHeaders();
   void fixSectionAlignments();
   void fixAbsoluteSymbols();
private:
   bool isOutputDynamic() const {
     return !Symtab.getSharedFiles().empty() || Config->Pic;
   }
   template <class RelTy>
   void scanRelocsForThunks(const elf::ObjectFile<ELFT> &File,
                            ArrayRef<RelTy> Rels);
 
   void ensureBss();
-  void addCommonSymbols(std::vector<DefinedCommon *> &Syms);
+  void addCommonSymbols();
   void addCopyRelSymbol(SharedSymbol<ELFT> *Sym);
 
   std::unique_ptr<llvm::FileOutputBuffer> Buffer;
 
   BumpPtrAllocator Alloc;
   std::vector<OutputSectionBase<ELFT> *> OutputSections;
   std::vector<std::unique_ptr<OutputSectionBase<ELFT>>> OwningSections;
 
template <class ELFT> void elf::writeResult(SymbolTable<ELFT> *Symtab) {
 
   Writer<ELFT>(*Symtab).run();
 }
 
 // The main function of the writer.
 template <class ELFT> void Writer<ELFT>::run() {
   if (!Config->DiscardAll)
     copyLocalSymbols();
-  addReservedSymbols();
   createSections();
   if (HasError)
     return;
 
   if (Config->Relocatable) {
     assignFileOffsets();
   } else {
     createPhdrs();
for (auto I = Rels.begin(), E = Rels.end(); I != E; ++I) {
     C.Relocations.push_back({R_ABS, Type, Offset, Addend, &Body});
   }
 
   // Scan relocations for necessary thunks.
   if (Config->EMachine == EM_MIPS)
     scanRelocsForThunks(File, Rels);
 }
 
-template <class ELFT> void Writer<ELFT>::scanRelocs(InputSection<ELFT> &C) {
-  for (const Elf_Shdr *RelSec : C.RelocSections)
-    scanRelocs(C, *RelSec);
+template <class ELFT> void Writer<ELFT>::scanRelocs() {
+  // Scan relocations. This must be done after every symbol is declared so that
+  // we can correctly decide if a dynamic relocation is needed.
+  // Check size() each time to guard against .bss being created.
+  for (unsigned I = 0; I < OutputSections.size(); ++I) {
+    OutputSectionBase<ELFT> *Sec = OutputSections[I];
+    Sec->forEachInputSection([&](InputSectionBase<ELFT> *S) {
+      if (auto *IS = dyn_cast<InputSection<ELFT>>(S)) {
+        // Set OutSecOff so that scanRelocs can use it.
+        uintX_t Off = alignTo(Sec->getSize(), S->Align);
+        IS->OutSecOff = Off;
+
+        for (const Elf_Shdr *RelSec : IS->RelocSections)
+          scanRelocs(*IS, *RelSec);
+
+        // Now that scan relocs possibly changed the size, update the offset.
+        Sec->setSize(Off + S->getSize());
+      } else if (auto *EH = dyn_cast<EHInputSection<ELFT>>(S)) {
+        if (EH->RelocSection)
+          scanRelocs(*EH, *EH->RelocSection);
+      }
+    });
+  }
 }
 
 template <class ELFT>
 void Writer<ELFT>::scanRelocs(InputSectionBase<ELFT> &S,
                               const Elf_Shdr &RelSec) {
   ELFFile<ELFT> &EObj = S.getFile()->getObj();
   if (RelSec.sh_type == SHT_RELA)
     scanRelocs(S, EObj.relas(&RelSec));
static bool compareSections(OutputSectionBase<ELFT> *A,
   typedef typename ELFT::uint uintX_t;
 
   int Comp = Script<ELFT>::X->compareSections(A->getName(), B->getName());
   if (Comp != 0)
     return Comp < 0;
 
   uintX_t AFlags = A->getFlags();
   uintX_t BFlags = B->getFlags();
 
   // Allocatable sections go first to reduce the total PT_LOAD size and
   // so debug info doesn't change addresses in actual code.
   bool AIsAlloc = AFlags & SHF_ALLOC;
   bool BIsAlloc = BFlags & SHF_ALLOC;
   if (AIsAlloc != BIsAlloc)
     return AIsAlloc;
 
   // We don't have any special requirements for the relative order of
template <class ELFT> void Writer<ELFT>::ensureBss() {
   Out<ELFT>::Bss =
       new OutputSection<ELFT>(".bss", SHT_NOBITS, SHF_ALLOC | SHF_WRITE);
   OwningSections.emplace_back(Out<ELFT>::Bss);
   OutputSections.push_back(Out<ELFT>::Bss);
 }
 
 // Until this function is called, common symbols do not belong to any section.
 // This function adds them to end of BSS section.
-template <class ELFT>
-void Writer<ELFT>::addCommonSymbols(std::vector<DefinedCommon *> &Syms) {
+template <class ELFT> void Writer<ELFT>::addCommonSymbols() {
+  std::vector<DefinedCommon *> Syms;
+  for (Symbol *S : Symtab.getSymbols())
+    if (auto *C = dyn_cast<DefinedCommon>(S->Body))
+      Syms.push_back(C);
+
   if (Syms.empty())
     return;
 
   // Sort the common symbols by alignment as an heuristic to pack them better.
   std::stable_sort(Syms.begin(), Syms.end(),
                    [](const DefinedCommon *A, const DefinedCommon *B) {
                      return A->Alignment > B->Alignment;
                    });
if (Type == SHT_PROGBITS && Config->EMachine == EM_X86_64 &&
       isa<EHInputSection<ELFT>>(C))
     Type = SHT_X86_64_UNWIND;
 
   return SectionKey<ELFT::Is64Bits>{OutsecName, Type, Flags, Alignment};
 }
 
 // The linker is expected to define some symbols depending on
 // the linking result. This function defines such symbols.
-template <class ELFT> void Writer<ELFT>::addReservedSymbols() {
+template <class ELFT>
+void Writer<ELFT>::addReservedSymbols(
+    ArrayRef<OutputSectionBase<ELFT> *> Sections) {
   if (Config->EMachine == EM_MIPS) {
     // Define _gp for MIPS. st_value of _gp symbol will be updated by Writer
     // so that it points to an absolute address which is relative to GOT.
     // See "Global Data Symbols" in Chapter 6 in the following document:
     // ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/mipsabi.pdf
     ElfSym<ELFT>::MipsGp =
         Symtab.addSynthetic("_gp", *Out<ELFT>::Got, MipsGPOffset);
 
auto Define = [this](StringRef S, DefinedRegular<ELFT> *&Sym1,
     if (SymbolBody *B = Symtab.find(S))
       if (B->isUndefined())
         Sym2 = Symtab.addAbsolute(S, STV_DEFAULT);
   };
 
   Define("_end", ElfSym<ELFT>::End, ElfSym<ELFT>::End2);
   Define("_etext", ElfSym<ELFT>::Etext, ElfSym<ELFT>::Etext2);
   Define("_edata", ElfSym<ELFT>::Edata, ElfSym<ELFT>::Edata2);
+
+  // The linker needs to define SECNAME_start, SECNAME_end and SECNAME_stop
+  // symbols for sections, so that the runtime can get the start and end
+  // addresses of each section by section name. Add such symbols.
+  if (!Config->Relocatable) {
+    addStartEndSymbols();
+    for (OutputSectionBase<ELFT> *Sec : Sections)
+      addStartStopSymbols(Sec);
+  }
+
+  // Add _DYNAMIC symbol. Unlike GNU gold, our _DYNAMIC symbol has no type.
+  // It should be okay as no one seems to care about the type.
+  // Even the author of gold doesn't remember why gold behaves that way.
+  // https://sourceware.org/ml/binutils/2002-03/msg00360.html
+  if (isOutputDynamic())
+    Symtab.addSynthetic("_DYNAMIC", *Out<ELFT>::Dynamic, 0);
+
+  // Define __rel[a]_iplt_{start,end} symbols if needed.
+  addRelIpltSymbols();
 }
 
 // Sort input sections by section name suffixes for
 // __attribute__((init_priority(N))).
 template <class ELFT> static void sortInitFini(OutputSectionBase<ELFT> *S) {
   if (S)
     reinterpret_cast<OutputSection<ELFT> *>(S)->sortInitFini();
 }
 
 // Sort input sections by the special rule for .ctors and .dtors.
 template <class ELFT> static void sortCtorsDtors(OutputSectionBase<ELFT> *S) {
   if (S)
     reinterpret_cast<OutputSection<ELFT> *>(S)->sortCtorsDtors();
 }
 
-// Create output section objects and add them to OutputSections.
-template <class ELFT> void Writer<ELFT>::createSections() {
-  // Add .interp first because some loaders want to see that section
-  // on the first page of the executable file when loaded into memory.
-  if (needsInterpSection())
-    OutputSections.push_back(Out<ELFT>::Interp);
+template <class ELFT> void Writer<ELFT>::fillSymTables() {
+  // Now that we have defined all possible symbols including linker-
+  // synthesized ones. Visit all symbols to give the finishing touches.
+  for (Symbol *S : Symtab.getSymbols()) {
+    SymbolBody *Body = S->Body;
 
-  // A core file does not usually contain unmodified segments except
-  // the first page of the executable. Add the build ID section now
-  // so that the section is included in the first page.
-  if (Out<ELFT>::BuildId)
-    OutputSections.push_back(Out<ELFT>::BuildId);
+    // Set "used" bit for --as-needed.
+    if (S->IsUsedInRegularObj && !S->isWeak())
+      if (auto *SS = dyn_cast<SharedSymbol<ELFT>>(Body))
+        SS->File->IsUsed = true;
+
+    if (Body->isUndefined() && !S->isWeak())
+      reportUndefined<ELFT>(Symtab, Body);
+
+    if (!includeInSymtab<ELFT>(*Body))
+      continue;
+    if (Out<ELFT>::SymTab)
+      Out<ELFT>::SymTab->addSymbol(Body);
+
+    if (isOutputDynamic() && S->includeInDynsym())
+      Out<ELFT>::DynSymTab->addSymbol(Body);
+  }
+}
 
+// Create output section objects and add them to OutputSections.
+template <class ELFT> void Writer<ELFT>::createSections() {
   // Create output sections for input object file sections.
   std::vector<OutputSectionBase<ELFT> *> RegularSections;
   OutputSectionFactory<ELFT> Factory;
   for (const std::unique_ptr<elf::ObjectFile<ELFT>> &F :
        Symtab.getObjectFiles()) {
     for (InputSectionBase<ELFT> *C : F->getSections()) {
       if (isDiscarded(C)) {
         reportDiscarded(C, F);
Out<ELFT>::Dynamic->FiniArraySec =
       Factory.lookup(".fini_array", SHT_FINI_ARRAY, SHF_WRITE | SHF_ALLOC);
 
   // Sort section contents for __attribute__((init_priority(N)).
   sortInitFini(Out<ELFT>::Dynamic->InitArraySec);
   sortInitFini(Out<ELFT>::Dynamic->FiniArraySec);
   sortCtorsDtors(Factory.lookup(".ctors", SHT_PROGBITS, SHF_WRITE | SHF_ALLOC));
   sortCtorsDtors(Factory.lookup(".dtors", SHT_PROGBITS, SHF_WRITE | SHF_ALLOC));
 
-  // The linker needs to define SECNAME_start, SECNAME_end and SECNAME_stop
-  // symbols for sections, so that the runtime can get the start and end
-  // addresses of each section by section name. Add such symbols.
-  if (!Config->Relocatable) {
-    addStartEndSymbols();
-    for (OutputSectionBase<ELFT> *Sec : RegularSections)
-      addStartStopSymbols(Sec);
-  }
-
-  // Add _DYNAMIC symbol. Unlike GNU gold, our _DYNAMIC symbol has no type.
-  // It should be okay as no one seems to care about the type.
-  // Even the author of gold doesn't remember why gold behaves that way.
-  // https://sourceware.org/ml/binutils/2002-03/msg00360.html
-  if (isOutputDynamic())
-    Symtab.addSynthetic("_DYNAMIC", *Out<ELFT>::Dynamic, 0);
-
-  // Define __rel[a]_iplt_{start,end} symbols if needed.
-  addRelIpltSymbols();
+  addReservedSymbols(RegularSections);
 
   if (Out<ELFT>::EhFrameHdr->Sec)
     Out<ELFT>::EhFrameHdr->Sec->finalize();
 
-  // Scan relocations. This must be done after every symbol is declared so that
-  // we can correctly decide if a dynamic relocation is needed.
-  // Check size() each time to guard against .bss being created.
-  for (unsigned I = 0; I < OutputSections.size(); ++I) {
-    OutputSectionBase<ELFT> *Sec = OutputSections[I];
-    Sec->forEachInputSection([&](InputSectionBase<ELFT> *S) {
-      if (auto *IS = dyn_cast<InputSection<ELFT>>(S)) {
-        // Set OutSecOff so that scanRelocs can use it.
-        uintX_t Off = alignTo(Sec->getSize(), S->Align);
-        IS->OutSecOff = Off;
-
-        scanRelocs(*IS);
-
-        // Now that scan relocs possibly changed the size, update the offset.
-        Sec->setSize(Off + S->getSize());
-      } else if (auto *EH = dyn_cast<EHInputSection<ELFT>>(S)) {
-        if (EH->RelocSection)
-          scanRelocs(*EH, *EH->RelocSection);
-      }
-    });
-  }
-
-  // Now that we have defined all possible symbols including linker-
-  // synthesized ones. Visit all symbols to give the finishing touches.
-  std::vector<DefinedCommon *> CommonSymbols;
-  for (Symbol *S : Symtab.getSymbols()) {
-    SymbolBody *Body = S->Body;
-
-    // Set "used" bit for --as-needed.
-    if (S->IsUsedInRegularObj && !S->isWeak())
-      if (auto *SS = dyn_cast<SharedSymbol<ELFT>>(Body))
-        SS->File->IsUsed = true;
-
-    if (Body->isUndefined() && !S->isWeak())
-      reportUndefined<ELFT>(Symtab, Body);
-
-    if (auto *C = dyn_cast<DefinedCommon>(Body))
-      CommonSymbols.push_back(C);
-
-    if (!includeInSymtab<ELFT>(*Body))
-      continue;
-    if (Out<ELFT>::SymTab)
-      Out<ELFT>::SymTab->addSymbol(Body);
-
-    if (isOutputDynamic() && S->includeInDynsym())
-      Out<ELFT>::DynSymTab->addSymbol(Body);
-  }
+  scanRelocs();
+  fillSymTables();
 
   // Do not proceed if there was an undefined symbol.
   if (HasError)
     return;
 
-  addCommonSymbols(CommonSymbols);
+  addCommonSymbols();
 
   // So far we have added sections from input object files.
   // This function adds linker-created Out<ELFT>::* sections.
   addPredefinedSections();
 
   std::stable_sort(OutputSections.begin(), OutputSections.end(),
                    compareSections<ELFT>);
 

 
 // 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);
   };
 
+  // Add .interp at first because some loaders want to see that section
+  // on the first page of the executable file when loaded into memory.
+  auto I = OutputSections.begin();
+  if (needsInterpSection())
+    I = OutputSections.insert(I, Out<ELFT>::Interp);
+
+  // A core file does not usually contain unmodified segments except
+  // the first page of the executable. Add the build ID section now
+  // so that the section is included in the first page.
+  if (Out<ELFT>::BuildId)
+    OutputSections.insert(I, Out<ELFT>::BuildId);
+
   // This order is not the same as the final output order
   // because we sort the sections using their attributes below.
   Add(Out<ELFT>::SymTab);
   Add(Out<ELFT>::ShStrTab);
   Add(Out<ELFT>::StrTab);
   if (isOutputDynamic()) {
     Add(Out<ELFT>::DynSymTab);
     Add(Out<ELFT>::GnuHashTab);