[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

883

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

883

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 addPredefinedSections();
   bool needsGot();
 
   template <class RelTy>
   void scanRelocs(InputSectionBase<ELFT> &C, ArrayRef<RelTy> Rels);
 
   void scanRelocs(InputSection<ELFT> &C);
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();

 static bool compareSections(OutputSectionBase<ELFT> *A,
                             OutputSectionBase<ELFT> *B) {
   typedef typename ELFT::uint uintX_t;
 
   int Comp = Script<ELFT>::X->compareSections(A->getName(), B->getName());
   if (Comp != 0)
     return Comp < 0;
 
+  // Add .interp first because some loaders want to see that section
+  // on the first page of the executable file when loaded into memory.
+  if (A == Out<ELFT>::Interp || B == Out<ELFT>::Interp)
+    return A == 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 (A == Out<ELFT>::BuildId || B == Out<ELFT>::BuildId)
+    return A == Out<ELFT>::BuildId;
+
   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)
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);
-
-  // 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);
-
   // 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) {

 template <class ELFT> void Writer<ELFT>::addPredefinedSections() {
   auto Add = [&](OutputSectionBase<ELFT> *C) {
     if (C)
       OutputSections.push_back(C);
   };
 
   // This order is not the same as the final output order
   // because we sort the sections using their attributes below.
+  if (needsInterpSection())
+    Add(Out<ELFT>::Interp);
+  if (Out<ELFT>::BuildId)
+    Add(Out<ELFT>::BuildId);
   Add(Out<ELFT>::SymTab);
   Add(Out<ELFT>::ShStrTab);
   Add(Out<ELFT>::StrTab);
   if (isOutputDynamic()) {
     Add(Out<ELFT>::DynSymTab);
     Add(Out<ELFT>::GnuHashTab);
     Add(Out<ELFT>::HashTab);
     Add(Out<ELFT>::Dynamic);