[ELF] Delete TargetInfo::getPltEntryOffset and simplify Symbol::getPltVA

[ruiu]

Now I think it is more clear than before that adding the size of the header in getPltVA is weird. Maybe that should be handled in getPltEntryOffset that's where we should compute an offset for an PLT entry. Also, please don't casually add an optional parameter to an existing function to "tweak" existing behavior in a subtle way. That kind of changes can quickly accumulate and make functions really hard to understand.

[ruiu]

I mean we probably should set PltHeaderSize to 0 if IPLT because IPLT's header size is actually zero (unless it is a retpoline IPLT, in which case it should be set to an appropriate value.)

[MaskRay]

In D54917#1310010, @ruiu wrote:

Now I think it is more clear than before that adding the size of the header in getPltVA is weird. Maybe that should be handled in getPltEntryOffset that's where we should compute an offset for an PLT entry. Also, please don't casually add an optional parameter to an existing function to "tweak" existing behavior in a subtle way. That kind of changes can quickly accumulate and make functions really hard to understand.

TargetInfo::getPltEntryOffset does not know which PLT section (normal PLT or IPLT) the caller is referring to.

[ruiu]

Yes, but you can teach which is the case.

[MaskRay]

In D54917#1310018, @ruiu wrote:

I mean we probably should set PltHeaderSize to 0 if IPLT because IPLT's header size is actually zero (unless it is a retpoline IPLT, in which case it should be set to an appropriate value.)

TargetInfo::PltHeaderSize is a field of TargetInfo, not PltSection. I cannot set PltHeaderSize to 0 for IPLT because otherwise when both normal PLT and IPLT are used, the PltHeaderSize of the normal PLT would be incorrect (0).

[ruiu]

Then maybe the function is currently at a wrong place?

[MaskRay]

In D54917#1310372, @ruiu wrote:

Then maybe the function is currently at a wrong place?

• InputSection::writeTo calls PltSection::writeTo. This may happen twice, one for normal PLT and the other for IPLT
• PltSection::writeTo calls Target->writePltHeader and Target->writePlt. The two methods are overriden by Arch/*.cpp targets.
• X86_64<ELFT>::writePlt (and some other targets) calls Target->getPltEntryOffset.

The bit of information (whether it is PLT or IPLT) is lost when PltSection::writeTo calls into TargetInfo methods. I am confusing about a better place to place the PltHeaderSize information.

[ruiu]

This is a rough sketch but I believe this is towards a right direction: https://gist.github.com/rui314/d919496a6db34483d673f9ebe34d6f89

[MaskRay]

Essentially used ruiu's suggestion to make HeaderSize public

[MaskRay]

Delete unintended change to Target.h

[ruiu]

Yeah, but the reason why this was just a sketch is because I believe TargetInfo::getPltEntryOffset no longer make much sense with this patch. Perhaps that should be removed.

[MaskRay]

Update title

Delete TargetInfo::getPltEntryOffset

[ruiu]

Hm, the new code doesn't look great as it contains too many repetitions. Do you have any suggestion to make this code better?

Let me play with this code too.

[MaskRay]

There are pros and cons. It makes the code slightly longer. The duplication mostly happens in getPltEntryOffset(Index); call sites in X86_64 X86 SPARCV9

- uint64_t Off = getPltEntryOffset(Index);
+ uint64_t Off = PltHeaderSize + Index * PltEntrySize;

The advantage is that a casual reader does not need to read the definition of getPltEntryOffset to understand how the offsets are computed. I am biased but I find such duplication tolerable.

[ruiu]

I see your point, and I agree with that. But the new code is also error-prone because you can make a mistake to not to add the header size, for example. So not sure which is better.

ELF/Arch/SPARCV9.cpp

const uint8_t PltData[] = {
       0x01, 0x00, 0x00, 0x00, // nop
       0x01, 0x00, 0x00, 0x00, // nop
       0x01, 0x00, 0x00, 0x00, // nop
       0x01, 0x00, 0x00, 0x00, // nop
       0x01, 0x00, 0x00, 0x00  // nop
   };
   memcpy(Buf, PltData, sizeof(PltData));
 
-  uint64_t Off = getPltEntryOffset(Index);
+  uint64_t Off = PltHeaderSize + Index * PltEntrySize;
   relocateOne(Buf, R_SPARC_22, Off);
   relocateOne(Buf + 4, R_SPARC_WDISP19, -(Off + 4 - PltEntrySize));
 }
 
 TargetInfo *elf::getSPARCV9TargetInfo() {
   static SPARCV9 Target;
   return &Target;
 }

ELF/Arch/X86.cpp

if (Config->Pic) {
     write32le(Buf + 2, GotPltEntryAddr - Ebx);
   } else {
     // jmp *foo_in_GOT
     Buf[1] = 0x25;
     write32le(Buf + 2, GotPltEntryAddr);
   }
 
   write32le(Buf + 7, RelOff);
-  write32le(Buf + 12, -getPltEntryOffset(Index) - 16);
+  write32le(Buf + 12, -(PltHeaderSize + Index * PltEntrySize + 16));
 }
 
 int64_t X86::getImplicitAddend(const uint8_t *Buf, RelType Type) const {
   switch (Type) {
   case R_386_8:
   case R_386_PC8:
     return SignExtend64<8>(*Buf);
   case R_386_16:
const uint8_t Insn[] = {
       0xe9, 0,    0,    0,    0,       // jmp plt+0x12
       0x68, 0,    0,    0,    0,       // pushl $reloc_offset
       0xe9, 0,    0,    0,    0,       // jmp plt+0
       0xcc, 0xcc, 0xcc, 0xcc, 0xcc,    // int3; padding
   };
   memcpy(Buf, Insn, sizeof(Insn));
 
   uint32_t Ebx = In.Got->getVA() + In.Got->getSize();
-  unsigned Off = getPltEntryOffset(Index);
+  unsigned Off = PltHeaderSize + Index * PltEntrySize;
   write32le(Buf + 3, GotPltEntryAddr - Ebx);
   write32le(Buf + 8, -Off - 12 + 32);
   write32le(Buf + 13, -Off - 17 + 18);
   write32le(Buf + 18, RelOff);
   write32le(Buf + 23, -Off - 27);
 }
 
 RetpolineNoPic::RetpolineNoPic() {
const uint8_t Insn[] = {
       0xe9, 0,    0,    0,    0,    // b:  jmp plt+0x11
       0x68, 0,    0,    0,    0,    // 10: pushl $reloc_offset
       0xe9, 0,    0,    0,    0,    // 15: jmp plt+0
       0xcc, 0xcc, 0xcc, 0xcc, 0xcc, // 1a: int3; padding
       0xcc,                         // 1f: int3; padding
   };
   memcpy(Buf, Insn, sizeof(Insn));
 
-  unsigned Off = getPltEntryOffset(Index);
+  unsigned Off = PltHeaderSize + Index * PltEntrySize;
   write32le(Buf + 2, GotPltEntryAddr);
   write32le(Buf + 7, -Off - 11 + 32);
   write32le(Buf + 12, -Off - 16 + 17);
   write32le(Buf + 17, RelOff);
   write32le(Buf + 22, -Off - 26);
 }
 
 TargetInfo *elf::getX86TargetInfo() {

ELF/Arch/X86_64.cpp

const uint8_t Inst[] = {
       0xff, 0x25, 0, 0, 0, 0, // jmpq *got(%rip)
       0x68, 0, 0, 0, 0,       // pushq <relocation index>
       0xe9, 0, 0, 0, 0,       // jmpq plt[0]
   };
   memcpy(Buf, Inst, sizeof(Inst));
 
   write32le(Buf + 2, GotPltEntryAddr - PltEntryAddr - 6);
   write32le(Buf + 7, Index);
-  write32le(Buf + 12, -getPltEntryOffset(Index) - 16);
+  write32le(Buf + 12, -(PltHeaderSize + Index * PltEntrySize + 16));
 }
 
 template <class ELFT> RelType X86_64<ELFT>::getDynRel(RelType Type) const {
   if (Type == R_X86_64_64 || Type == R_X86_64_PC64 || Type == R_X86_64_SIZE32 ||
       Type == R_X86_64_SIZE64)
     return Type;
   return R_X86_64_NONE;
 }
const uint8_t Insn[] = {
       0xe8, 0,    0,    0,    0,    // 7:  callq plt+0x20
       0xe9, 0,    0,    0,    0,    // c:  jmp plt+0x12
       0x68, 0,    0,    0,    0,    // 11: pushq <relocation index>
       0xe9, 0,    0,    0,    0,    // 16: jmp plt+0
       0xcc, 0xcc, 0xcc, 0xcc, 0xcc, // 1b: int3; padding
   };
   memcpy(Buf, Insn, sizeof(Insn));
 
-  uint64_t Off = TargetInfo::getPltEntryOffset(Index);
+  uint64_t Off = TargetInfo::PltHeaderSize + Index * TargetInfo::PltEntrySize;
 
   write32le(Buf + 3, GotPltEntryAddr - PltEntryAddr - 7);
   write32le(Buf + 8, -Off - 12 + 32);
   write32le(Buf + 13, -Off - 17 + 18);
   write32le(Buf + 18, Index);
   write32le(Buf + 23, -Off - 27);
 }
 
void RetpolineZNow<ELFT>::writePlt(uint8_t *Buf, uint64_t GotPltEntryAddr,
   const uint8_t Insn[] = {
       0x4c, 0x8b, 0x1d, 0,    0, 0, 0, // mov foo@GOTPLT(%rip), %r11
       0xe9, 0,    0,    0,    0,       // jmp plt+0
       0xcc, 0xcc, 0xcc, 0xcc,          // int3; padding
   };
   memcpy(Buf, Insn, sizeof(Insn));
 
   write32le(Buf + 3, GotPltEntryAddr - PltEntryAddr - 7);
-  write32le(Buf + 8, -TargetInfo::getPltEntryOffset(Index) - 12);
+  write32le(Buf + 8, -(TargetInfo::PltHeaderSize +
+                       Index * TargetInfo::PltEntrySize + 12));
 }
 
 template <class ELFT> static TargetInfo *getTargetInfo() {
   if (Config->ZRetpolineplt) {
     if (Config->ZNow) {
       static RetpolineZNow<ELFT> T;
       return &T;
     }

ELF/Symbols.cpp

 }
 
 uint64_t Symbol::getPPC64LongBranchOffset() const {
   assert(PPC64BranchltIndex != 0xffff);
   return PPC64BranchltIndex * Target->GotPltEntrySize;
 }
 
 uint64_t Symbol::getPltVA() const {
-  if (this->IsInIplt) {
-    if (Config->ZRetpolineplt)
-      return In.Iplt->getVA() + Target->getPltEntryOffset(PltIndex);
-    return In.Iplt->getVA() + PltIndex * Target->PltEntrySize;
-  }
-  return In.Plt->getVA() + Target->getPltEntryOffset(PltIndex);
+  PltSection *Plt = IsInIplt ? In.Iplt : In.Plt;
+  return Plt->getVA() + Plt->HeaderSize + PltIndex * Target->PltEntrySize;
 }
 
 uint64_t Symbol::getPltOffset() const {
   assert(!this->IsInIplt);
-  return Target->getPltEntryOffset(PltIndex);
+  return Target->PltHeaderSize + PltIndex * Target->PltEntrySize;
 }
 
 uint64_t Symbol::getPPC64LongBranchTableVA() const {
   assert(PPC64BranchltIndex != 0xffff);
   return In.PPC64LongBranchTarget->getVA() +
          PPC64BranchltIndex * Target->GotPltEntrySize;
 }
 

ELF/SyntheticSections.h

public:
   PltSection(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);
 
+  size_t HeaderSize;
+
 private:
   unsigned getPltRelocOff() const;
   std::vector<std::pair<const Symbol *, unsigned>> Entries;
-  size_t HeaderSize;
   bool IsIplt;
 };
 
 class GdbIndexSection final : public SyntheticSection {
 public:
   struct AddressEntry {
     InputSection *Section;
     uint64_t LowAddress;

ELF/Target.h

public:
   virtual void writePltHeader(uint8_t *Buf) const {}
 
   virtual void writePlt(uint8_t *Buf, uint64_t GotEntryAddr,
                         uint64_t PltEntryAddr, int32_t Index,
                         unsigned RelOff) const {}
   virtual void addPltHeaderSymbols(InputSection &IS) const {}
   virtual void addPltSymbols(InputSection &IS, uint64_t Off) const {}
 
-  unsigned getPltEntryOffset(unsigned Index) const {
-    return Index * PltEntrySize + PltHeaderSize;
-  }
-
   // Returns true if a relocation only uses the low bits of a value such that
   // all those bits are in the same page. For example, if the relocation
   // only uses the low 12 bits in a system with 4k pages. If this is true, the
   // bits will always have the same value at runtime and we don't have to emit
   // a dynamic relocation.
   virtual bool usesOnlyLowPageBits(RelType Type) const;
 
   // Decide whether a Thunk is needed for the relocation from File