[ELF] - Get rid of postThunkContents().

[grimar]

Summary

It turns out that postThunkContents() is only used for
sorting symbols in .symtab.

Though we can instead move the logic to finalizeContents,
postpone calling it and then get rid of postThunkContents() completely.

[grimar]

What about doing this change?

[peter.smith]

Looks reasonable to me. I've made a suggestion that might simplify a bit further, I've not tried or tested it though.

Apologies for the late response; I've had this stuck in a draft state for a while, must have forgotten to press submit.

ELF/SyntheticSections.cpp

1822 ↗

(On Diff #156274)

If the only line of code that SymbolTableSection (SHT_SYMTAB) and SymbolTableBaseSection (SHT_DYNSYM) share is getParent()->Link ... Would it be possible to move sortSymTabSymbols into SymbolTableSection::finalizeContents() add the getParent()->Link and let the virtual call mechanism replace the test?

[grimar]

Thanks for the feedback, Peter!

ELF/SyntheticSections.cpp

1822 ↗

(On Diff #156274)

I am not sure I follow here.

SHT_SYMTAB and SHT_DYNSYM are both SymbolTableSection<ELFT>:

InX::SymTab = make<SymbolTableSection<ELFT>>(*InX::StrTab);
...
InX::DynSymTab = make<SymbolTableSection<ELFT>>(*InX::DynStrTab);

So I think the virtual call mechanism will not be helpful here?
(Since we do not have different classes for them atm. And need them both to be SymbolTableSection for writeTo I think).

[peter.smith]

LGTM. Probably worth waiting a bit to see if Rui has any opinions.

ELF/SyntheticSections.cpp

1822 ↗

(On Diff #156274)

My apologies, I think I must have assumed that the DynSym and the SymTab where of different types. Please ignore the suggestion.

[grimar]

Rui?

[grimar]

Ping. Rui?

[grimar]

Since there are no objections, I am going to commit this tomorrow.

lld/trunk/ELF/SyntheticSections.h

public:
 
   virtual ~SyntheticSection() = default;
   virtual void writeTo(uint8_t *Buf) = 0;
   virtual size_t getSize() const = 0;
   virtual void finalizeContents() {}
   // If the section has the SHF_ALLOC flag and the size may be changed if
   // thunks are added, update the section size.
   virtual bool updateAllocSize() { return false; }
-  // If any additional finalization of contents are needed post thunk creation.
-  virtual void postThunkContents() {}
   virtual bool empty() const { return false; }
 
   static bool classof(const SectionBase *D) {
     return D->kind() == InputSectionBase::Synthetic;
   }
 };
 
 struct CieRecord {
struct SymbolTableEntry {
   Symbol *Sym;
   size_t StrTabOffset;
 };
 
 class SymbolTableBaseSection : public SyntheticSection {
 public:
   SymbolTableBaseSection(StringTableSection &StrTabSec);
   void finalizeContents() override;
-  void postThunkContents() override;
   size_t getSize() const override { return getNumSymbols() * Entsize; }
   void addSymbol(Symbol *Sym);
   unsigned getNumSymbols() const { return Symbols.size() + 1; }
   size_t getSymbolIndex(Symbol *Sym);
   ArrayRef<SymbolTableEntry> getSymbols() const { return Symbols; }
 
 protected:
+  void sortSymTabSymbols();
+
   // A vector of symbols and their string table offsets.
   std::vector<SymbolTableEntry> Symbols;
 
   StringTableSection &StrTabSec;
 
   llvm::once_flag OnceFlag;
   llvm::DenseMap<Symbol *, size_t> SymbolIndexMap;
   llvm::DenseMap<OutputSection *, size_t> SectionIndexMap;

lld/trunk/ELF/SyntheticSections.cpp

static bool sortMipsSymbols(const SymbolTableEntry &L,
   if (!L.Sym->isInGot() && !R.Sym->isInGot())
     return false;
   return !L.Sym->isInGot();
 }
 
 void SymbolTableBaseSection::finalizeContents() {
   getParent()->Link = StrTabSec.getParent()->SectionIndex;
 
-  if (this->Type != SHT_DYNSYM)
+  if (this->Type != SHT_DYNSYM) {
+    sortSymTabSymbols();
     return;
+  }
 
   // If it is a .dynsym, there should be no local symbols, but we need
   // to do a few things for the dynamic linker.
 
   // Section's Info field has the index of the first non-local symbol.
   // Because the first symbol entry is a null entry, 1 is the first.
   getParent()->Info = 1;
 

 
 // The ELF spec requires that all local symbols precede global symbols, so we
 // sort symbol entries in this function. (For .dynsym, we don't do that because
 // symbols for dynamic linking are inherently all globals.)
 //
 // Aside from above, we put local symbols in groups starting with the STT_FILE
 // symbol. That is convenient for purpose of identifying where are local symbols
 // coming from.
-void SymbolTableBaseSection::postThunkContents() {
-  assert(this->Type == SHT_SYMTAB);
-
+void SymbolTableBaseSection::sortSymTabSymbols() {
   // Move all local symbols before global symbols.
   auto E = std::stable_partition(
       Symbols.begin(), Symbols.end(), [](const SymbolTableEntry &S) {
         return S.Sym->isLocal() || S.Sym->computeBinding() == STB_LOCAL;
       });
   size_t NumLocals = E - Symbols.begin();
   getParent()->Info = NumLocals + 1;
 

lld/trunk/ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::finalizeSections() {
 
   // Some symbols are defined in term of program headers. Now that we
   // have the headers, we can find out which sections they point to.
   setReservedSymbolSections();
 
   // Dynamic section must be the last one in this list and dynamic
   // symbol table section (DynSymTab) must be the first one.
   applySynthetic(
-      {InX::DynSymTab, InX::Bss,         InX::BssRelRo,    InX::GnuHashTab,
-       InX::HashTab,   InX::SymTab,      InX::SymTabShndx, InX::ShStrTab,
-       InX::StrTab,    In<ELFT>::VerDef, InX::DynStrTab,   InX::Got,
-       InX::MipsGot,   InX::IgotPlt,     InX::GotPlt,      InX::RelaDyn,
-       InX::RelrDyn,   InX::RelaIplt,    InX::RelaPlt,     InX::Plt,
-       InX::Iplt,      InX::EhFrameHdr,  In<ELFT>::VerSym, In<ELFT>::VerNeed,
-       InX::Dynamic},
+      {InX::DynSymTab,   InX::Bss,         InX::BssRelRo,     InX::GnuHashTab,
+       InX::HashTab,     InX::SymTabShndx, InX::ShStrTab,     InX::StrTab,
+       In<ELFT>::VerDef, InX::DynStrTab,   InX::Got,          InX::MipsGot,
+       InX::IgotPlt,     InX::GotPlt,      InX::RelaDyn,      InX::RelrDyn,
+       InX::RelaIplt,    InX::RelaPlt,     InX::Plt,          InX::Iplt,
+       InX::EhFrameHdr,  In<ELFT>::VerSym, In<ELFT>::VerNeed, InX::Dynamic},
       [](SyntheticSection *SS) { SS->finalizeContents(); });
 
   if (!Script->HasSectionsCommand && !Config->Relocatable)
     fixSectionAlignments();
 
   // After link order processing .ARM.exidx sections can be deduplicated, which
   // needs to be resolved before any other address dependent operation.
   resolveShfLinkOrder();
do {
       Changed |= InX::RelaDyn->updateAllocSize();
       if (InX::RelrDyn)
         Changed |= InX::RelrDyn->updateAllocSize();
     } while (Changed);
   }
 
   // createThunks may have added local symbols to the static symbol table
   applySynthetic({InX::SymTab},
-                 [](SyntheticSection *SS) { SS->postThunkContents(); });
+                 [](SyntheticSection *SS) { SS->finalizeContents(); });
 
   // Fill other section headers. The dynamic table is finalized
   // at the end because some tags like RELSZ depend on result
   // of finalizing other sections.
   for (OutputSection *Sec : OutputSections)
     Sec->finalize<ELFT>();
 }