[PATCH] D29664: [LLD][ELF] Refactor PltSection and IPltSection common code into a base class NFC

[Rafael Avila de Espindola via llvm-commits]

I think this is an improvement. Rui, do you agree?

Cheers,
Rafael

Peter Smith via Phabricator <reviews at reviews.llvm.org> writes:

> peter.smith created this revision.
>
> In a comment via email on https://reviews.llvm.org/D28956 Rafael suggested:
> "Would it be possible to add a common base class to PltSection and Ipltsection and share this code?"
>
> This is an attempt to add a common base class PltSectionBase that moves out most of the more complex code into the base class. It is using the same logic and interface as previously.
>
>
> https://reviews.llvm.org/D29664
>
> Files:
>   ELF/SyntheticSections.cpp
>   ELF/SyntheticSections.h
>
> Index: ELF/SyntheticSections.h
> ===================================================================
> --- ELF/SyntheticSections.h
> +++ ELF/SyntheticSections.h
> @@ -457,33 +457,47 @@
>    size_t Size = 0;
>  };
>  
> -template <class ELFT> class PltSection final : public SyntheticSection<ELFT> {
> +// Common base class for Plt and IPlt.
> +template <class ELFT> class PltSectionBase : public SyntheticSection<ELFT> {
>  public:
> -  PltSection();
> +  PltSectionBase(size_t HeaderSize, RelocationSection<ELFT> *PltRelSec);
>    void writeTo(uint8_t *Buf) override;
>    size_t getSize() const override;
>    void addEntry(SymbolBody &Sym);
>    bool empty() const override { return Entries.empty(); }
>    void addSymbols();
>  
>  private:
> +  virtual void writeHeader(uint8_t *Buf){};
> +  virtual void addHeaderSymbols(){};
> +  virtual unsigned getPltRelocOff() const = 0;
>    std::vector<std::pair<const SymbolBody *, unsigned>> Entries;
> +  // IPlt always has HeaderSize of 0
> +  size_t HeaderSize;
> +  RelocationSection<ELFT> *PltRelocSection;
> +};
> +
> +// The traditional Plt section for non GNU Ifunc symbols. This always has a
> +// header as the first entry to facilitate lazy binding at run time.
> +template <class ELFT> class PltSection final : public PltSectionBase<ELFT> {
> +public:
> +  PltSection();
> +
> +private:
> +  virtual void writeHeader(uint8_t *Buf) override;
> +  virtual void addHeaderSymbols() override;
> +  virtual unsigned getPltRelocOff() const override { return 0; }
>  };
>  
>  // The IpltSection is a variant of Plt for recording entries for GNU Ifunc
>  // symbols that will be subject to a Target->IRelativeRel
>  // The IpltSection immediately follows the Plt section in the Output Section
> -template <class ELFT> class IpltSection final : public SyntheticSection<ELFT> {
> +template <class ELFT> class IpltSection final : public PltSectionBase<ELFT> {
>  public:
>    IpltSection();
> -  void writeTo(uint8_t *Buf) override;
> -  size_t getSize() const override;
> -  void addEntry(SymbolBody &Sym);
> -  bool empty() const override { return Entries.empty(); }
> -  void addSymbols();
>  
>  private:
> -  std::vector<std::pair<const SymbolBody *, unsigned>> Entries;
> +  virtual unsigned getPltRelocOff() const override;
>  };
>  
>  template <class ELFT>
> Index: ELF/SyntheticSections.cpp
> ===================================================================
> --- ELF/SyntheticSections.cpp
> +++ ELF/SyntheticSections.cpp
> @@ -1427,83 +1427,74 @@
>  }
>  
>  template <class ELFT>
> -PltSection<ELFT>::PltSection()
> +PltSectionBase<ELFT>::PltSectionBase(size_t S,
> +                                     RelocationSection<ELFT> *PltRelSec)
>      : SyntheticSection<ELFT>(SHF_ALLOC | SHF_EXECINSTR, SHT_PROGBITS, 16,
> -                             ".plt") {}
> +                             ".plt"),
> +      HeaderSize(S), PltRelocSection(PltRelSec) {}
>  
> -template <class ELFT> void PltSection<ELFT>::writeTo(uint8_t *Buf) {
> -  // At beginning of PLT, we have code to call the dynamic linker
> -  // to resolve dynsyms at runtime. Write such code.
> -  Target->writePltHeader(Buf);
> -  size_t Off = Target->PltHeaderSize;
> +template <class ELFT> void PltSectionBase<ELFT>::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.
> +  writeHeader(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;
> +    unsigned RelOff = I.second + PltOff;
>      uint64_t Got = B->getGotPltVA<ELFT>();
>      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) {
> +template <class ELFT> void PltSectionBase<ELFT>::addEntry(SymbolBody &Sym) {
>    Sym.PltIndex = Entries.size();
> -  unsigned RelOff = In<ELFT>::RelaPlt->getRelocOffset();
> +  unsigned RelOff = PltRelocSection->getRelocOffset();
> +  Sym.IsInIplt = HeaderSize == 0;
>    Entries.push_back(std::make_pair(&Sym, RelOff));
>  }
>  
> -template <class ELFT> size_t PltSection<ELFT>::getSize() const {
> -  return Target->PltHeaderSize + Entries.size() * Target->PltEntrySize;
> +template <class ELFT> size_t PltSectionBase<ELFT>::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() {
> -  Target->addPltHeaderSymbols(this);
> -  size_t Off = Target->PltHeaderSize;
> +template <class ELFT> void PltSectionBase<ELFT>::addSymbols() {
> +  // The PLT may have symbols defined for the Header, the IPLT has no header
> +  addHeaderSymbols();
> +  size_t Off = HeaderSize;
> +
>    for (size_t I = 0; I < Entries.size(); ++I) {
>      Target->addPltSymbols(this, Off);
>      Off += Target->PltEntrySize;
>    }
>  }
>  
>  template <class ELFT>
> -IpltSection<ELFT>::IpltSection()
> -    : SyntheticSection<ELFT>(SHF_ALLOC | SHF_EXECINSTR, SHT_PROGBITS, 16,
> -                             ".plt") {}
> +PltSection<ELFT>::PltSection()
> +    : PltSectionBase<ELFT>(Target->PltHeaderSize, In<ELFT>::RelaPlt) {}
>  
> -template <class ELFT> void IpltSection<ELFT>::writeTo(uint8_t *Buf) {
> -  // The IRelative relocations do not support lazy binding so no header is
> -  // needed
> -  size_t Off = 0;
> -  for (auto &I : Entries) {
> -    const SymbolBody *B = I.first;
> -    unsigned RelOff = I.second + In<ELFT>::Plt->getSize();
> -    uint64_t Got = B->getGotPltVA<ELFT>();
> -    uint64_t Plt = this->getVA() + Off;
> -    Target->writePlt(Buf + Off, Got, Plt, B->PltIndex, RelOff);
> -    Off += Target->PltEntrySize;
> -  }
> +template <class ELFT> void PltSection<ELFT>::writeHeader(uint8_t *Buf) {
> +  Target->writePltHeader(Buf);
>  }
>  
> -template <class ELFT> void IpltSection<ELFT>::addEntry(SymbolBody &Sym) {
> -  Sym.PltIndex = Entries.size();
> -  Sym.IsInIplt = true;
> -  unsigned RelOff = In<ELFT>::RelaIplt->getRelocOffset();
> -  Entries.push_back(std::make_pair(&Sym, RelOff));
> +// 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>::addHeaderSymbols() {
> +  Target->addPltHeaderSymbols(this);
>  }
>  
> -template <class ELFT> size_t IpltSection<ELFT>::getSize() const {
> -  return Entries.size() * Target->PltEntrySize;
> -}
> +template <class ELFT>
> +IpltSection<ELFT>::IpltSection()
> +    : PltSectionBase<ELFT>(0, In<ELFT>::RelaIplt) {}
>  
> -template <class ELFT> void IpltSection<ELFT>::addSymbols() {
> -  size_t Off = 0;
> -  for (size_t I = 0; I < Entries.size(); ++I) {
> -    Target->addPltSymbols(this, Off);
> -    Off += Target->PltEntrySize;
> -  }
> +template <class ELFT> unsigned IpltSection<ELFT>::getPltRelocOff() const {
> +  return In<ELFT>::Plt->getSize();
>  }
>  
>  template <class ELFT>
> @@ -2101,6 +2092,11 @@
>  template class elf::HashTableSection<ELF64LE>;
>  template class elf::HashTableSection<ELF64BE>;
>  
> +template class elf::PltSectionBase<ELF32LE>;
> +template class elf::PltSectionBase<ELF32BE>;
> +template class elf::PltSectionBase<ELF64LE>;
> +template class elf::PltSectionBase<ELF64BE>;
> +
>  template class elf::PltSection<ELF32LE>;
>  template class elf::PltSection<ELF32BE>;
>  template class elf::PltSection<ELF64LE>;