[ELF] - Do not use HeaderSize for conditions in PltSection.

[grimar]

Summary

Found this during experiments with refactoring.

Our PltSection code assumes that InX::Iplt section has zero header size
and that InX::Plt always has non-zero header:
https://github.com/llvm-mirror/lld/blob/master/ELF/SyntheticSections.cpp#L1870

But PPC64 target sets PltHeaderSize to zero:
https://github.com/llvm-mirror/lld/blob/master/ELF/Arch/PPC64.cpp#L68
(I could be wrong about reasons, but I think PPC64 target just does not
implement lazy binding).

Because of that, ppc64-toc-restore.s testcase, for example, sets IsInIplt flag for
symbols, what does not seem to be correct.

I suggest to stop using HeaderSize == 0 in code as condition to find out whether
we are in Plt or Iplt section.

[grimar]

• Addressed review comment (IsPlt->IsIplt).

[grimar]

LGTMed by Rafael in mail list. Rui, are you OK with this change ?

[grimar]

Ping. Rui ?

[ruiu]

Isn't it an ABI violation not to write the PLT header for PPC64? If so, we fix that ABI violation rather than "correctly" emitting ABI-violating executables.

[ruiu]

ELF/SyntheticSections.cpp

1863

Remove HeaderSize member because it is always Target->PltHeaderSize

ELF/Writer.cpp

372–374

Passing (PltHeaderSize, false) or (0, true) is strange. You should just pass true or false and infer the header size inside the class.

[grimar]

• Addressed review comments.

ELF/Writer.cpp

372–374

With this suggestion patch looks better IMO, thanks.

[ruiu]

LGTM with these changes.

ELF/SyntheticSections.cpp

1875

OK this change isn't pretty. Pleasse keep HeaderSize and remove getHeaderSize.

ELF/SyntheticSections.h

507–508

That IsIplt is true if it is Iplt is obvious, no? If you change this to something like "true if IPLT. false if the regular PLT", that would be nontrivial. But I doubt we need it though.

[ruiu]

ELF/SyntheticSections.cpp

1875

Sorry for not being clear, but I didn't mean you should revert all these changes. I'll make a change to redo what your change originally contained.

ELF/SyntheticSections.h

public:
   void finalizeContents() override;
   void writeTo(uint8_t *Buf) override;
   size_t getSize() const override { return Size; }
 
 private:
   size_t Size = 0;
 };
 
-// The PltSection is used for both the Plt and Iplt. The former always has a
+// The PltSection is used for both the Plt and Iplt. The former usually 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.
 class PltSection : public SyntheticSection {
 public:
-  PltSection(size_t HeaderSize);
+  PltSection(size_t HeaderSize, bool IsIplt);
   void writeTo(uint8_t *Buf) override;
   size_t getSize() const override;
   bool empty() const override { return Entries.empty(); }
   void addSymbols();
 
   template <class ELFT> void addEntry(Symbol &Sym);
 
 private:
   unsigned getPltRelocOff() const;
   std::vector<std::pair<const Symbol *, unsigned>> Entries;
-  // Iplt always has HeaderSize of 0, the Plt HeaderSize is always non-zero
   size_t HeaderSize;
+  bool IsIplt;
 };
 
 // GdbIndexChunk is created for each .debug_info section and contains
 // information to create a part of .gdb_index for a given input section.
 struct GdbIndexChunk {
   struct AddressEntry {
     InputSection *Section;
     uint64_t LowAddress;

ELF/SyntheticSections.cpp

for (const SymbolTableEntry &S : InX::DynSymTab->getSymbols()) {
     StringRef Name = Sym->getName();
     unsigned I = Sym->DynsymIndex;
     uint32_t Hash = hashSysV(Name) % NumSymbols;
     Chains[I] = Buckets[Hash];
     write32(Buckets + Hash, I);
   }
 }
 
-PltSection::PltSection(size_t S)
+PltSection::PltSection(size_t HeaderSize, bool IsIplt)
     : SyntheticSection(SHF_ALLOC | SHF_EXECINSTR, SHT_PROGBITS, 16, ".plt"),
-      HeaderSize(S) {
+      HeaderSize(HeaderSize), IsIplt(IsIplt) {
   // The PLT needs to be writable on SPARC as the dynamic linker will
   // modify the instructions in the PLT entries.
   if (Config->EMachine == EM_SPARCV9)
     this->Flags |= SHF_WRITE;
 }
 
 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)
+  if (!IsIplt)
     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 Symbol *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::addEntry(Symbol &Sym) {
   Sym.PltIndex = Entries.size();
   RelocationBaseSection *PltRelocSection = InX::RelaPlt;
-  if (HeaderSize == 0) {
+  if (IsIplt) {
     PltRelocSection = InX::RelaIplt;
     Sym.IsInIplt = true;
   }
   unsigned RelOff =
       static_cast<RelocationSection<ELFT> *>(PltRelocSection)->getRelocOffset();
   Entries.push_back(std::make_pair(&Sym, RelOff));
 }
 
 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
 void PltSection::addSymbols() {
   // The PLT may have symbols defined for the Header, the IPLT has no header
-  if (HeaderSize != 0)
+  if (!IsIplt)
     Target->addPltHeaderSymbols(*this);
   size_t Off = HeaderSize;
   for (size_t I = 0; I < Entries.size(); ++I) {
     Target->addPltSymbols(*this, Off);
     Off += Target->PltEntrySize;
   }
 }
 
 unsigned PltSection::getPltRelocOff() const {
-  return (HeaderSize == 0) ? InX::Plt->getSize() : 0;
+  return IsIplt ? InX::Plt->getSize() : 0;
 }
 
 // The string hash function for .gdb_index.
 static uint32_t computeGdbHash(StringRef S) {
   uint32_t H = 0;
   for (uint8_t C : S)
     H = H * 67 + tolower(C) - 113;
   return H;

ELF/Writer.cpp

template <class ELFT> static void createSyntheticSections() {
   // we can get the desired behaviour by placing the iplt section in .rel.plt.
   InX::RelaIplt = make<RelocationSection<ELFT>>(
       (Config->EMachine == EM_ARM && !Config->AndroidPackDynRelocs)
           ? ".rel.dyn"
           : InX::RelaPlt->Name,
       false /*Sort*/);
   Add(InX::RelaIplt);
 
-  InX::Plt = make<PltSection>(Target->PltHeaderSize);
+  InX::Plt = make<PltSection>(Target->PltHeaderSize, false /*IsIplt*/);
   Add(InX::Plt);
-  InX::Iplt = make<PltSection>(0);
+  InX::Iplt = make<PltSection>(0 /*HeaderSize*/, true /*IsIplt*/);
   Add(InX::Iplt);
 
   if (!Config->Relocatable) {
     if (Config->EhFrameHdr) {
       InX::EhFrameHdr = make<EhFrameHeader>();
       Add(InX::EhFrameHdr);
     }
     InX::EhFrame = make<EhFrameSection>();

test/ELF/ppc64-ifunc.s

+# REQUIRES: ppc
+# RUN: llvm-mc -filetype=obj -triple=powerpc64-unknown-linux %s -o %t.o
+# RUN: llvm-mc -filetype=obj -triple=powerpc64-unknown-linux %p/Inputs/shared-ppc64.s -o %t2.o
+# RUN: ld.lld -shared %t2.o -o %t2.so
+# RUN: ld.lld %t.o %t2.so -o %t
+# RUN: llvm-objdump -d %t | FileCheck %s
+
+# CHECK:      _start:
+# CHECK-NEXT:  10010004: {{.*}} bl .+12
+# CHECK-NEXT:  10010008: {{.*}} bl .+40
+
+# 0x10010004 + 12 = 0x10010010 (PLT entry 0)
+# 0x10010008 + 40 = 0x10010030 (PLT entry 1)
+
+# CHECK:     Disassembly of section .plt:
+# CHECK:       10010010: {{.*}} std 2, 40(1)
+# CHECK-NEXT:  10010014: {{.*}} addis 11, 2, 4098
+# CHECK-NEXT:  10010018: {{.*}} ld 12, -32744(11)
+# CHECK-NEXT:  1001001c: {{.*}} ld 11, 0(12)
+# CHECK-NEXT:  10010020: {{.*}} mtctr 11
+# CHECK-NEXT:  10010024: {{.*}} ld 2, 8(12)
+# CHECK-NEXT:  10010028: {{.*}} ld 11, 16(12)
+# CHECK-NEXT:  1001002c: {{.*}} bctr
+# CHECK-NEXT:  10010030: {{.*}} std 2, 40(1)
+# CHECK-NEXT:  10010034: {{.*}} addis 11, 2, 4098
+# CHECK-NEXT:  10010038: {{.*}} ld 12, -32736(11)
+# CHECK-NEXT:  1001003c: {{.*}} ld 11, 0(12)
+# CHECK-NEXT:  10010040: {{.*}} mtctr 11
+# CHECK-NEXT:  10010044: {{.*}} ld 2, 8(12)
+# CHECK-NEXT:  10010048: {{.*}} ld 11, 16(12)
+# CHECK-NEXT:  1001004c: {{.*}} bctr
+
+.type ifunc STT_GNU_IFUNC
+.globl ifunc
+ifunc:
+ nop
+
+.global _start
+_start:
+  bl bar
+  bl ifunc