[ELF] - Detemplate PltSection<ELFT> section.

[grimar]

Summary

Deemplating PltSection opens road for detemplating
SymbolBody::getVA() method which depends on
getPltVA() currently, which is easy to detemplate after this change.
That opens way to detemplate other parts of linker.

[ruiu]

This doesn't seem like an improvement. The new code look more complicated than the original one. Maybe you want to make these classes non-template classes, while keeping some member functions template?

[grimar]

In D31028#703332, @ruiu wrote:

This doesn't seem like an improvement. The new code look more complicated than the original one. Maybe you want to make these classes non-template classes, while keeping some member functions template?

I think it is.

I tried keeping single template method for PltSection in D30982. It also works for what I want to do finally, so committed it as rL298065;
But this patch not more complicated in my opinion. See original code:

In<ELFT>::Plt->addEntry<ELFT>(Body);
In<ELFT>::GotPlt->addEntry(Body);
In<ELFT>::RelaPlt->addReloc({Target->PltRel, In<ELFT>::GotPlt,
                             Body.getGotPltOffset(), !Preemptible,
                             &Body, 0});

Does it clear that Plt->addEntry depends on RelaPlt->addReloc ? If you switch their order code will be broken, because
Plt->addEntry inside refers to offset of relocation in RelaPlt.
Change made in this patch reveals this dependency making it explicit. I think that is itself simplifies logic.

[ruiu]

So you submitted a modified version of this patch as r298065? As a common practice, once you sent a patch for review, you are supposed to wait for an LGTM (if you think that a new patch is obvious, you can submit it to let other people do post-commit review, but it needs to be actually obvious and shouldn't be waiting for an LGTM). I appreciate your recent cleanup commits, but you want to slow down a bit here to follow the rule.

[grimar]

In D31028#703875, @ruiu wrote:

So you submitted a modified version of this patch as r298065? As a common practice, once you sent a patch for review, you are supposed to wait for an LGTM (if you think that a new patch is obvious, you can submit it to let other people do post-commit review, but it needs to be actually obvious and shouldn't be waiting for an LGTM). I appreciate your recent cleanup commits, but you want to slow down a bit here to follow the rule.

Not modified version of this patch, but a patch that changes nothing and just removes templating, leaving the single method templated. It is obvious trivial change I think.
And I kept this patch on review because think it is better way and can be committed on top.

Do you think https://reviews.llvm.org/rL298065 really needed pre-commit review ?

[ruiu]

So you submitted a modified version of this patch as r298065? As a common practice, once you sent a patch for review, you are supposed to wait for an LGTM (if you think that a new patch is obvious, you can submit it to let other people do post-commit review, but it needs to be actually obvious and shouldn't be waiting for an LGTM). I appreciate your recent cleanup commits, but you want to slow down a bit here to follow the rule.

Not modified version of this patch, but a patch that changes nothing and just removes templating, leaving the single method templated. It is obvious trivial change I think.
And I kept this patch on review because think it is better way and can be committed on top.

Do you think https://reviews.llvm.org/rL298065 really needed pre-commit review ?

No, r298065 is a modified version of this patch. I suggested doing the thing this patch is attempting to to do in a different way, and r298065 is an implementation of the suggestion. In general, if you get a comment to do a thing in a different way, the new patch to do in the new way is a continuation of the original patch. If my code review were slow, I could have understood, but you are actually getting very fast code review from me every day, so no need to haste that much, right? Please allow me to review your patches.

[grimar]

In D31028#703910, @ruiu wrote:

So you submitted a modified version of this patch as r298065? As a common practice, once you sent a patch for review, you are supposed to wait for an LGTM (if you think that a new patch is obvious, you can submit it to let other people do post-commit review, but it needs to be actually obvious and shouldn't be waiting for an LGTM). I appreciate your recent cleanup commits, but you want to slow down a bit here to follow the rule.

Not modified version of this patch, but a patch that changes nothing and just removes templating, leaving the single method templated. It is obvious trivial change I think.
And I kept this patch on review because think it is better way and can be committed on top.

Do you think https://reviews.llvm.org/rL298065 really needed pre-commit review ?

No, r298065 is a modified version of this patch. I suggested doing the thing this patch is attempting to to do in a different way, and r298065 is an implementation of the suggestion. In general, if you get a comment to do a thing in a different way, the new patch to do in the new way is a continuation of the original patch. If my code review were slow, I could have understood, but you are actually getting very fast code review from me every day, so no need to haste that much, right? Please allow me to review your patches.

I just asked. For me it was trivial thing, I did not think it needs review. First time I did the same what r298065 does in D30982, that was earlier than your suggestion.
Sorry, I will push things on review next time then.

ELF/Relocations.cpp

for (auto I = Rels.begin(), E = Rels.end(); I != E; ++I) {
     // also require us to create a got or plt entry.
 
     // If a relocation needs PLT, we create a PLT and a GOT slot for the symbol.
     if (needsPlt(Expr)) {
       if (Body.isInPlt())
         continue;
 
       if (Body.isGnuIFunc() && !Preemptible) {
-        In<ELFT>::Iplt->addEntry(Body);
+        Body.IsInIplt = true;
         In<ELFT>::IgotPlt->addEntry(Body);
-        In<ELFT>::RelaIplt->addReloc({Target->IRelativeRel, In<ELFT>::IgotPlt,
-                                      Body.getGotPltOffset(), !Preemptible,
-                                      &Body, 0});
+        unsigned RelOff = In<ELFT>::RelaIplt->addReloc(
+            {Target->IRelativeRel, In<ELFT>::IgotPlt, Body.getGotPltOffset(),
+             !Preemptible, &Body, 0});
+        In<ELFT>::Iplt->addEntry(Body, RelOff);
       } else {
-        In<ELFT>::Plt->addEntry(Body);
         In<ELFT>::GotPlt->addEntry(Body);
-        In<ELFT>::RelaPlt->addReloc({Target->PltRel, In<ELFT>::GotPlt,
-                                     Body.getGotPltOffset(), !Preemptible,
-                                     &Body, 0});
+        unsigned RelOff = In<ELFT>::RelaPlt->addReloc(
+            {Target->PltRel, In<ELFT>::GotPlt, Body.getGotPltOffset(),
+             !Preemptible, &Body, 0});
+        In<ELFT>::Plt->addEntry(Body, RelOff);
       }
       continue;
     }
 
     if (refersToGotEntry(Expr)) {
       if (Config->EMachine == EM_MIPS) {
         // MIPS ABI has special rules to process GOT entries and doesn't
         // require relocation entries for them. A special case is TLS

ELF/SyntheticSections.h

 
 template <class ELFT> class RelocationSection final : public SyntheticSection {
   typedef typename ELFT::Rel Elf_Rel;
   typedef typename ELFT::Rela Elf_Rela;
   typedef typename ELFT::uint uintX_t;
 
 public:
   RelocationSection(StringRef Name, bool Sort);
-  void addReloc(const DynamicReloc<ELFT> &Reloc);
+  unsigned addReloc(const DynamicReloc<ELFT> &Reloc);
   unsigned getRelocOffset();
   void finalizeContents() override;
   void writeTo(uint8_t *Buf) override;
   bool empty() const override { return Relocs.empty(); }
   size_t getSize() const override { return Relocs.size() * this->Entsize; }
   size_t getRelativeRelocCount() const { return NumRelativeRelocs; }
 
 private:

 private:
   size_t Size = 0;
 };
 
 // The PltSection is used for both the Plt and Iplt. The former always has a
 // header as its first entry that is used at run-time to resolve lazy binding.
 // The latter is used for GNU Ifunc symbols, that will be subject to a
 // Target->IRelativeRel.
-template <class ELFT> class PltSection : public SyntheticSection {
+class PltSection : public SyntheticSection {
 public:
   PltSection(size_t HeaderSize);
   void writeTo(uint8_t *Buf) override;
   size_t getSize() const override;
-  void addEntry(SymbolBody &Sym);
+  void addEntry(SymbolBody &Sym, unsigned RelOff);
   bool empty() const override { return Entries.empty(); }
   void addSymbols();
 
 private:
   void writeHeader(uint8_t *Buf){};
   void addHeaderSymbols(){};
   unsigned getPltRelocOff() const;
   std::vector<std::pair<const SymbolBody *, unsigned>> Entries;

 struct InX {
   static InputSection *ARMAttributes;
   static InputSection *Common;
   static StringTableSection *DynStrTab;
   static InputSection *Interp;
   static GotPltSection *GotPlt;
   static IgotPltSection *IgotPlt;
   static MipsRldMapSection *MipsRldMap;
+  static PltSection *Plt;
+  static PltSection *Iplt;
   static StringTableSection *ShStrTab;
   static StringTableSection *StrTab;
 };
 
 template <class ELFT> struct In : public InX {
   static BuildIdSection<ELFT> *BuildId;
   static DynamicSection<ELFT> *Dynamic;
   static SymbolTableSection<ELFT> *DynSymTab;
   static EhFrameHeader<ELFT> *EhFrameHdr;
   static GnuHashTableSection<ELFT> *GnuHashTab;
   static GdbIndexSection<ELFT> *GdbIndex;
   static GotSection<ELFT> *Got;
   static EhFrameSection<ELFT> *EhFrame;
   static MipsGotSection<ELFT> *MipsGot;
   static HashTableSection<ELFT> *HashTab;
-  static PltSection<ELFT> *Plt;
-  static PltSection<ELFT> *Iplt;
   static RelocationSection<ELFT> *RelaDyn;
   static RelocationSection<ELFT> *RelaPlt;
   static RelocationSection<ELFT> *RelaIplt;
   static SymbolTableSection<ELFT> *SymTab;
   static VersionDefinitionSection<ELFT> *VerDef;
   static VersionTableSection<ELFT> *VerSym;
   static VersionNeedSection<ELFT> *VerNeed;
 };
 
 template <class ELFT> BuildIdSection<ELFT> *In<ELFT>::BuildId;
 template <class ELFT> DynamicSection<ELFT> *In<ELFT>::Dynamic;
 template <class ELFT> SymbolTableSection<ELFT> *In<ELFT>::DynSymTab;
 template <class ELFT> EhFrameHeader<ELFT> *In<ELFT>::EhFrameHdr;
 template <class ELFT> GdbIndexSection<ELFT> *In<ELFT>::GdbIndex;
 template <class ELFT> GnuHashTableSection<ELFT> *In<ELFT>::GnuHashTab;
 template <class ELFT> GotSection<ELFT> *In<ELFT>::Got;
 template <class ELFT> EhFrameSection<ELFT> *In<ELFT>::EhFrame;
 template <class ELFT> MipsGotSection<ELFT> *In<ELFT>::MipsGot;
 template <class ELFT> HashTableSection<ELFT> *In<ELFT>::HashTab;
-template <class ELFT> PltSection<ELFT> *In<ELFT>::Plt;
-template <class ELFT> PltSection<ELFT> *In<ELFT>::Iplt;
 template <class ELFT> RelocationSection<ELFT> *In<ELFT>::RelaDyn;
 template <class ELFT> RelocationSection<ELFT> *In<ELFT>::RelaPlt;
 template <class ELFT> RelocationSection<ELFT> *In<ELFT>::RelaIplt;
 template <class ELFT> SymbolTableSection<ELFT> *In<ELFT>::SymTab;
 template <class ELFT> VersionDefinitionSection<ELFT> *In<ELFT>::VerDef;
 template <class ELFT> VersionTableSection<ELFT> *In<ELFT>::VerSym;
 template <class ELFT> VersionNeedSection<ELFT> *In<ELFT>::VerNeed;
 } // namespace elf
 } // namespace lld
 
 #endif

ELF/SyntheticSections.cpp

 RelocationSection<ELFT>::RelocationSection(StringRef Name, bool Sort)
     : SyntheticSection(SHF_ALLOC, Config->isRela() ? SHT_RELA : SHT_REL,
                        sizeof(uintX_t), Name),
       Sort(Sort) {
   this->Entsize = Config->isRela() ? sizeof(Elf_Rela) : sizeof(Elf_Rel);
 }
 
 template <class ELFT>
-void RelocationSection<ELFT>::addReloc(const DynamicReloc<ELFT> &Reloc) {
+unsigned RelocationSection<ELFT>::addReloc(const DynamicReloc<ELFT> &Reloc) {
+  unsigned Off = getRelocOffset();
   if (Reloc.Type == Target->RelativeRel)
     ++NumRelativeRelocs;
   Relocs.push_back(Reloc);
+  return Off;
 }
 
 template <class ELFT, class RelTy>
 static bool compRelocations(const RelTy &A, const RelTy &B) {
   bool AIsRel = A.getType(Config->isMips64EL()) == Target->RelativeRel;
   bool BIsRel = B.getType(Config->isMips64EL()) == Target->RelativeRel;
   if (AIsRel != BIsRel)
     return AIsRel;
for (const SymbolTableEntry &S : In<ELFT>::DynSymTab->getSymbols()) {
     StringRef Name = Body->getName();
     unsigned I = Body->DynsymIndex;
     uint32_t Hash = hashSysV(Name) % NumSymbols;
     Chains[I] = Buckets[Hash];
     Buckets[Hash] = I;
   }
 }
 
-template <class ELFT>
-PltSection<ELFT>::PltSection(size_t S)
+PltSection::PltSection(size_t S)
     : SyntheticSection(SHF_ALLOC | SHF_EXECINSTR, SHT_PROGBITS, 16, ".plt"),
       HeaderSize(S) {}
 
-template <class ELFT> void PltSection<ELFT>::writeTo(uint8_t *Buf) {
+void PltSection::writeTo(uint8_t *Buf) {
   // At beginning of PLT but not the IPLT, we have code to call the dynamic
   // linker to resolve dynsyms at runtime. Write such code.
   if (HeaderSize != 0)
     Target->writePltHeader(Buf);
   size_t Off = HeaderSize;
   // The IPlt is immediately after the Plt, account for this in RelOff
   unsigned PltOff = getPltRelocOff();
 
   for (auto &I : Entries) {
     const SymbolBody *B = I.first;
     unsigned RelOff = I.second + PltOff;
     uint64_t Got = B->getGotPltVA();
     uint64_t Plt = this->getVA() + Off;
     Target->writePlt(Buf + Off, Got, Plt, B->PltIndex, RelOff);
     Off += Target->PltEntrySize;
   }
 }
 
-template <class ELFT> void PltSection<ELFT>::addEntry(SymbolBody &Sym) {
+void PltSection::addEntry(SymbolBody &Sym, unsigned RelOff) {
   Sym.PltIndex = Entries.size();
-  RelocationSection<ELFT> *PltRelocSection = In<ELFT>::RelaPlt;
-  if (HeaderSize == 0) {
-    PltRelocSection = In<ELFT>::RelaIplt;
-    Sym.IsInIplt = true;
-  }
-  unsigned RelOff = PltRelocSection->getRelocOffset();
   Entries.push_back(std::make_pair(&Sym, RelOff));
 }
 
-template <class ELFT> size_t PltSection<ELFT>::getSize() const {
+size_t PltSection::getSize() const {
   return HeaderSize + Entries.size() * Target->PltEntrySize;
 }
 
 // Some architectures such as additional symbols in the PLT section. For
 // example ARM uses mapping symbols to aid disassembly
-template <class ELFT> void PltSection<ELFT>::addSymbols() {
+void PltSection::addSymbols() {
   // The PLT may have symbols defined for the Header, the IPLT has no header
   if (HeaderSize != 0)
     Target->addPltHeaderSymbols(this);
   size_t Off = HeaderSize;
   for (size_t I = 0; I < Entries.size(); ++I) {
     Target->addPltSymbols(this, Off);
     Off += Target->PltEntrySize;
   }
 }
 
-template <class ELFT> unsigned PltSection<ELFT>::getPltRelocOff() const {
-  return (HeaderSize == 0) ? In<ELFT>::Plt->getSize() : 0;
+unsigned PltSection::getPltRelocOff() const {
+  return (HeaderSize == 0) ? InX::Plt->getSize() : 0;
 }
 
 template <class ELFT>
 GdbIndexSection<ELFT>::GdbIndexSection()
     : SyntheticSection(0, SHT_PROGBITS, 1, ".gdb_index"),
       StringPool(llvm::StringTableBuilder::ELF) {}
 
 // Iterative hash function for symbol's name is described in .gdb_index format

 
 InputSection *InX::ARMAttributes;
 InputSection *InX::Common;
 StringTableSection *InX::DynStrTab;
 InputSection *InX::Interp;
 GotPltSection *InX::GotPlt;
 IgotPltSection *InX::IgotPlt;
 MipsRldMapSection *InX::MipsRldMap;
+PltSection *InX::Plt;
+PltSection *InX::Iplt;
 StringTableSection *InX::ShStrTab;
 StringTableSection *InX::StrTab;
 
 template InputSection *elf::createCommonSection<ELF32LE>();
 template InputSection *elf::createCommonSection<ELF32BE>();
 template InputSection *elf::createCommonSection<ELF64LE>();
 template InputSection *elf::createCommonSection<ELF64BE>();
 

 template class elf::GnuHashTableSection<ELF64LE>;
 template class elf::GnuHashTableSection<ELF64BE>;
 
 template class elf::HashTableSection<ELF32LE>;
 template class elf::HashTableSection<ELF32BE>;
 template class elf::HashTableSection<ELF64LE>;
 template class elf::HashTableSection<ELF64BE>;
 
-template class elf::PltSection<ELF32LE>;
-template class elf::PltSection<ELF32BE>;
-template class elf::PltSection<ELF64LE>;
-template class elf::PltSection<ELF64BE>;
-
 template class elf::GdbIndexSection<ELF32LE>;
 template class elf::GdbIndexSection<ELF32BE>;
 template class elf::GdbIndexSection<ELF64LE>;
 template class elf::GdbIndexSection<ELF64BE>;
 
 template class elf::EhFrameHeader<ELF32LE>;
 template class elf::EhFrameHeader<ELF32BE>;
 template class elf::EhFrameHeader<ELF64LE>;

ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::createSyntheticSections() {
 
   // The RelaIplt immediately follows .rel.plt (.rel.dyn for ARM) to ensure
   // that the IRelative relocations are processed last by the dynamic loader
   In<ELFT>::RelaIplt = make<RelocationSection<ELFT>>(
       (Config->EMachine == EM_ARM) ? ".rel.dyn" : In<ELFT>::RelaPlt->Name,
       false /*Sort*/);
   Add(In<ELFT>::RelaIplt);
 
-  In<ELFT>::Plt = make<PltSection<ELFT>>(Target->PltHeaderSize);
+  In<ELFT>::Plt = make<PltSection>(Target->PltHeaderSize);
   Add(In<ELFT>::Plt);
-  In<ELFT>::Iplt = make<PltSection<ELFT>>(0);
+  In<ELFT>::Iplt = make<PltSection>(0);
   Add(In<ELFT>::Iplt);
 
   if (!Config->Relocatable) {
     if (Config->EhFrameHdr) {
       In<ELFT>::EhFrameHdr = make<EhFrameHeader<ELFT>>();
       Add(In<ELFT>::EhFrameHdr);
     }
     In<ELFT>::EhFrame = make<EhFrameSection<ELFT>>();