[ELF] Don't special case symbolic relocations with 0 addend to ifunc in writable locations

[MaskRay]

Summary

Currently the following 3 relocation types do not trigger the creation
of a canonical PLT (which changes STT_GNU_IFUNC to STT_FUNC and
redirects all references):

1. GOT-generating (needsGot)
2. PLT-generating (needsPlt)
3. R_ABS with 0 addend in a writable location. This is used for for ifunc function pointers in writable sections such as .data and .toc.

This patch deletes case 3) to simplify the R_*_IRELATIVE generating
logic added in D57371. Other advantages:

• It is guaranteed no more than 1 R_*_IRELATIVE is created for an ifunc.
• PPC64: no need to special case ifunc in toc-indirect to toc-relative relaxation. See D65755

[MaskRay]

Improve aarch64-gnu-ifunc-nonpreemptable2.s to test .data as well

[MaskRay]

Fix a comment

[ruiu]

Currently the following 3 relocation types do not trigger the creation of a canonical PLT:

• GOT-generating
• PLT-generating
• symbolic relocation with 0 addend in a writable location

Is it a typo that a PLT-generating relocation do *not* trigger the creation a canonical PLT, or am I missing something?

[MaskRay]

In D65995#1622445, @ruiu wrote:

Currently the following 3 relocation types do not trigger the creation of a canonical PLT:

• GOT-generating
• PLT-generating
• symbolic relocation with 0 addend in a writable location

Is it a typo that a PLT-generating relocation do *not* trigger the creation a canonical PLT, or am I missing something?

It isn't a typo. The creation of a canonical PLT refers to this piece of code:

} else if (!needsPlt(expr)) {
  // Make the ifunc's PLT entry canonical by changing the value of its
  // symbol to redirect all references to point to it.
  unsigned entryOffset = sym.pltIndex * target->pltEntrySize;
  if (config->zRetpolineplt)
    entryOffset += target->pltHeaderSize;

  auto &d = cast<Defined>(sym);
  d.section = in.iplt;
  d.value = entryOffset;
  d.size = 0;
  // It's important to set the symbol type here so that dynamic loaders
  // don't try to call the PLT as if it were an ifunc resolver.
  d.type = STT_FUNC;

It changes the type from STT_GNU_IFUNC to STT_FUNC.

If a non-preemptable ifunc just has PLT-generating and GOT-generating relocations. The type doesn't need a change.

[MaskRay]

Fix comments and clarify "canonical PLT"

[peter.smith]

If I've understood correctly, this will make the case "address of an ifunc is taken from RW but there are no other relocations that would create a canonical PLT entry" worse as we would always create the canonical PLT entry even if it isn't strictly needed. The trade off is simpler code and a possible size saving from only needing one irelative relocation?

I think that taking the address of Ifunc's is rare in most user code, so even if we make this case worse it isn't likely to make a lot of difference, however it would be good to hear from PCC as he may have a special use case using lots more ifuncs than usual. I think FreeBSD also make heavy use of ifuncs so they may have some concerns.

Personally I'd like to avoid canonical PLT entries as much as possible as if we can show that the PLT addresses don't leak, the BTI PLT entries can be optimised to remove the BTI as it is only needed if the PLT entry is called indirectly. I recognise that this is only of theoretical interest at the moment though so I'm happy to go with the consensus.

ELF/Relocations.cpp

1095 ↗

(On Diff #214325)

I think that this is only used by code that you have deleted. Could be removed as well?

test/ELF/aarch64-gnu-ifunc-nonpreemptable2.s

1 ↗

(On Diff #214325)

Assuming I've not made a mistake somewhere, this test is passing with lld prior to this change. Is this intentional?

Possibly the reference from RO is causing the canonical PLT entry even with the previous LLD?

[MaskRay]

Delete IRelativeReloc

[MaskRay]

If I've understood correctly, this will make the case "address of an ifunc is taken from RW but there are no other relocations that would create a canonical PLT entry" worse as we would always create the canonical PLT entry even if it isn't strictly needed. The trade off is simpler code and a possible size saving from only needing one irelative relocation?

We have probably overloaded the meaning of "canonical PLT" here:) Let me give a bit more details about the two types of canonical PLT:

1. STT_FUNC, Undefined or SharedSymbol. The canonical PLT makes it a Defined and exports it (replaceWithDefined sets the exportDynamic field) in the dynamic symbol table. This fake definition (st_value=addr(PLT)!=0, st_shndx=0, ld.so has logic to handle such symbols) can preempt a real definition in a DSO. This can be seen as an address leak (in AArch64 BTI protection such PLT needs BTI c).
2. STT_IFUNC, Defined. The canonical PLT is created because the symbol is referenced by a non-PLT-generating-non-GOT-generating relocation. This case is already a Defined, so there is no new Defined (exportDynamic field is not set). We just change some fields of the symbol (st_value and st_type are important to ld.so. For st_shndx, as long as is non-zero, it doesn't matter what its actual value is). If the STT_GNU_IFUNC symbol was exported before, the converted STT_FUNC is still exported. If the STT_GNU_IFUNC was not (e.g. local/hidden), the new STT_FUNC is not.

Both are created for pointer equality. This patch deals with 2).

If the ifunc is referenced by another component.

• Without a canonical PLT, its type is STT_GNU_IFUNC. A reference (symbolic relocation/GLOB_DAT/JUMP_SLOT) has to call the ifunc resolver to get the real address.
• With a canonical PLT, its type is STT_FUNC. A reference does not have to call the ifunc resolver, but every subsequent function call has to go through the canonical PLT. Address taken of a non-preemptable ifunc in a static storage (.rodata, .data, etc) is rare, so when making the trade-off, we can lean toward implementation complexity.

ELF/Relocations.cpp

1095 ↗

(On Diff #214325)

Thanks! Will delete.

test/ELF/aarch64-gnu-ifunc-nonpreemptable2.s

1 ↗

(On Diff #214325)

Yes, the reference from .rodata creates the canonical PLT. This test is to increase test coverage. It passes prior to this change.

If I delete .data, there is a canonical PLT.

If I delete .rodata, there is no canonical PLT, but there are 2 R_*_IRELATIVE. The R_*_IRELATIVE relocating .got.plt is redundant. We can move the if (expr == R_ABS ... code before if (!sym.isInPlt()) to decrease one. That was what I experimented before I thought: we should probably just simplify the cases.

[peter.smith]

In D65995#1623285, @MaskRay wrote:

If I've understood correctly, this will make the case "address of an ifunc is taken from RW but there are no other relocations that would create a canonical PLT entry" worse as we would always create the canonical PLT entry even if it isn't strictly needed. The trade off is simpler code and a possible size saving from only needing one irelative relocation?

We have probably overloaded the meaning of "canonical PLT" here:) Let me give a bit more details about the two types of canonical PLT:

1. STT_FUNC, Undefined or SharedSymbol. The canonical PLT makes it a Defined and exports it (replaceWithDefined sets the exportDynamic field) in the dynamic symbol table. This fake definition (st_value=addr(PLT)!=0, st_shndx=0, ld.so has logic to handle such symbols) can preempt a real definition in a DSO. This can be seen as an address leak (in AArch64 BTI protection such PLT needs BTI c).
2. STT_IFUNC, Defined. The canonical PLT is created because the symbol is referenced by a non-PLT-generating-non-GOT-generating relocation. This case is already a Defined, so there is no new Defined (exportDynamic field is not set). We just change some fields of the symbol (st_value and st_type are important to ld.so. For st_shndx, as long as is non-zero, it doesn't matter what its actual value is). If the STT_GNU_IFUNC symbol was exported before, the converted STT_FUNC is still exported. If the STT_GNU_IFUNC was not (e.g. local/hidden), the new STT_FUNC is not.

Both are created for pointer equality. This patch deals with 2).

If the ifunc is referenced by another component.

• Without a canonical PLT, its type is STT_GNU_IFUNC. A reference (symbolic relocation/GLOB_DAT/JUMP_SLOT) has to call the ifunc resolver to get the real address.
• With a canonical PLT, its type is STT_FUNC. A reference does not have to call the ifunc resolver, but every subsequent function call has to go through the canonical PLT. Address taken of a non-preemptable ifunc in a static storage (.rodata, .data, etc) is rare, so when making the trade-off, we can lean toward implementation complexity.

I agree that in hand written code it is not common to take the address of an ifunc in such a way that a canonical PLT entry isn't needed. My concern is that this part of the code is motivated by something like HWASAN, which I believe may make compiler generated use of ifuncs to access shadow memory for example: D50544 . I must confess to not knowing too much about the details about HWASAN so I'm hoping that PCC can let us know if this will be a real performance problem or not.

[pcc]

HWASAN should only be using GOT relative relocations to access shadow memory, so I wouldn't expect this change to have an impact on HWASAN.

I wouldn't object too much to removing this code at this point, but it may be worth bringing it back if we ever manage to improve relocation processing here to make this simpler to implement.

[MaskRay]

In D65995#1626365, @pcc wrote:

HWASAN should only be using GOT relative relocations to access shadow memory, so I wouldn't expect this change to have an impact on HWASAN.

Thanks for clarification!

I wouldn't object too much to removing this code at this point, but it may be worth bringing it back if we ever manage to improve relocation processing here to make this simpler to implement.

Do you have other things in mind that this may be beneficial?

[peter.smith]

In D65995#1626470, @MaskRay wrote:

In D65995#1626365, @pcc wrote:

HWASAN should only be using GOT relative relocations to access shadow memory, so I wouldn't expect this change to have an impact on HWASAN.

Thanks for clarification!

Thanks, I've no more objections.

I wouldn't object too much to removing this code at this point, but it may be worth bringing it back if we ever manage to improve relocation processing here to make this simpler to implement.

Do you have other things in mind that this may be beneficial?

I think that there are a few things that are made difficult due to a single pass through the relocations. For correctness we can always make decisions based on considering a relocation in isolation, but there are a few optimizations where we need to know about all the relocations. For example if we know that there is no need to create a canonical PLT then we could very easily avoid a lot of the complexity. This is easier said than done though, we'd need to be careful of performance as there can be a lot of relocations in a program.

[peter.smith]

Given that this is mostly deleting code, I'm happy to approve.

lld/trunk/ELF/Arch/PPC64.cpp

bool elf::tryRelaxPPC64TocIndirection(RelType type, const Relocation &rel,
   Defined *d;
   int64_t addend;
   auto *tocISB = cast<InputSectionBase>(defSym->section);
   std::tie(d, addend) =
       config->isLE ? getRelaTocSymAndAddend<ELF64LE>(tocISB, rel.addend)
                    : getRelaTocSymAndAddend<ELF64BE>(tocISB, rel.addend);
 
   // Only non-preemptable defined symbols can be relaxed.
-  //
-  // The toc entry of a non-preemptable ifunc is relocated by R_PPC64_IRELATIVE,
-  // which will run at load time to determine the relocated value. It is not
-  // known until load time, so the access cannot be relaxed.
-  if (!d || d->isPreemptible || d->isGnuIFunc())
+  if (!d || d->isPreemptible)
     return false;
 
+  // R_PPC64_ADDR64 should have created a canonical PLT for the non-preemptable
+  // ifunc and changed its type to STT_FUNC.
+  assert(!d->isGnuIFunc());
+
   // Two instructions can materialize a 32-bit signed offset from the toc base.
   uint64_t tocRelative = d->getVA(addend) - getPPC64TocBase();
   if (!isInt<32>(tocRelative))
     return false;
 
   // Add PPC64TocOffset that will be subtracted by relocateOne().
   target->relaxGot(bufLoc, type, tocRelative + ppc64TocOffset);
   return true;

lld/trunk/ELF/Relocations.cpp

if (sym.isFunc()) {
     sec.relocations.push_back({expr, type, offset, addend, &sym});
     return;
   }
 
   errorOrWarn("symbol '" + toString(sym) + "' has no type" +
               getLocation(sec, sym, offset));
 }
 
-struct IRelativeReloc {
-  RelType type;
-  InputSectionBase *sec;
-  uint64_t offset;
-  Symbol *sym;
-};
-
-static std::vector<IRelativeReloc> iRelativeRelocs;
-
 template <class ELFT, class RelTy>
 static void scanReloc(InputSectionBase &sec, OffsetGetter &getOffset, RelTy *&i,
                       RelTy *end) {
   const RelTy &rel = *i;
   uint32_t symIndex = rel.getSymbol(config->isMips64EL);
   Symbol &sym = sec.getFile<ELFT>()->getSymbol(symIndex);
   RelType type;
 
if (!sym.isGnuIFunc() || sym.isPreemptible) {
     //   GOT-generating relocations.
     //
     // - The fact that these symbols do not have a fixed value makes them an
     //   exception to the general rule that a statically linked executable does
     //   not require any form of dynamic relocation. To handle these relocations
     //   correctly, the IRELATIVE relocations are stored in an array which a
     //   statically linked executable's startup code must enumerate using the
     //   linker-defined symbols __rela?_iplt_{start,end}.
-    //
-    // - An absolute relocation to a non-preemptible ifunc (such as a global
-    //   variable containing a pointer to the ifunc) needs to be relocated in
-    //   the exact same way as a GOT entry, so we can avoid needing to make the
-    //   PLT entry canonical by translating such relocations into IRELATIVE
-    //   relocations in the relaIplt.
     if (!sym.isInPlt()) {
       // Create PLT and GOTPLT slots for the symbol.
       sym.isInIplt = true;
 
       // Create a copy of the symbol to use as the target of the IRELATIVE
       // relocation in the igotPlt. This is in case we make the PLT canonical
       // later, which would overwrite the original symbol.
       //
       // FIXME: Creating a copy of the symbol here is a bit of a hack. All
       // that's really needed to create the IRELATIVE is the section and value,
       // so ideally we should just need to copy those.
       auto *directSym = make<Defined>(cast<Defined>(sym));
       addPltEntry<ELFT>(in.iplt, in.igotPlt, in.relaIplt, target->iRelativeRel,
                         *directSym);
       sym.pltIndex = directSym->pltIndex;
     }
-    if (expr == R_ABS && addend == 0 && (sec.flags & SHF_WRITE)) {
-      // We might be able to represent this as an IRELATIVE. But we don't know
-      // yet whether some later relocation will make the symbol point to a
-      // canonical PLT, which would make this either a dynamic RELATIVE (PIC) or
-      // static (non-PIC) relocation. So we keep a record of the information
-      // required to process the relocation, and after scanRelocs() has been
-      // called on all relocations, the relocation is resolved by
-      // addIRelativeRelocs().
-      iRelativeRelocs.push_back({type, &sec, offset, &sym});
-      return;
-    }
     if (needsGot(expr)) {
       // Redirect GOT accesses to point to the Igot.
       //
       // This field is also used to keep track of whether we ever needed a GOT
       // entry. If we did and we make the PLT canonical later, we'll need to
       // create a GOT entry pointing to the PLT entry for Sym.
       sym.gotInIgot = true;
     } else if (!needsPlt(expr)) {

 
 template <class ELFT> void elf::scanRelocations(InputSectionBase &s) {
   if (s.areRelocsRela)
     scanRelocs<ELFT>(s, s.relas<ELFT>());
   else
     scanRelocs<ELFT>(s, s.rels<ELFT>());
 }
 
-// Figure out which representation to use for any absolute relocs to
-// non-preemptible ifuncs that we visited during scanRelocs().
-void elf::addIRelativeRelocs() {
-  for (IRelativeReloc &r : iRelativeRelocs) {
-    if (r.sym->type == STT_GNU_IFUNC)
-      in.relaIplt->addReloc(
-          {target->iRelativeRel, r.sec, r.offset, true, r.sym, 0});
-    else if (config->isPic)
-      addRelativeReloc(r.sec, r.offset, r.sym, 0, R_ABS, r.type);
-    else
-      r.sec->relocations.push_back({R_ABS, r.type, r.offset, 0, r.sym});
-  }
-  iRelativeRelocs.clear();
-}
-
 static bool mergeCmp(const InputSection *a, const InputSection *b) {
   // std::merge requires a strict weak ordering.
   if (a->outSecOff < b->outSecOff)
     return true;
 
   if (a->outSecOff == b->outSecOff) {
     auto *ta = dyn_cast<ThunkSection>(a);
     auto *tb = dyn_cast<ThunkSection>(b);

lld/trunk/ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::finalizeSections() {
 
   // Scan relocations. This must be done after every symbol is declared so that
   // we can correctly decide if a dynamic relocation is needed.
   if (!config->relocatable) {
     forEachRelSec(scanRelocations<ELFT>);
     reportUndefinedSymbols<ELFT>();
   }
 
-  addIRelativeRelocs();
-
   if (in.plt && in.plt->isNeeded())
     in.plt->addSymbols();
   if (in.iplt && in.iplt->isNeeded())
     in.iplt->addSymbols();
 
   if (!config->allowShlibUndefined) {
     // Error on undefined symbols in a shared object, if all of its DT_NEEDED
     // entires are seen. These cases would otherwise lead to runtime errors

lld/trunk/test/ELF/aarch64-gnu-ifunc-nonpreemptable2.s

+# REQUIRES: aarch64
+# RUN: llvm-mc -filetype=obj -triple=aarch64-none-linux-gnu %s -o %t.o
+# RUN: ld.lld %t.o -o %t
+# RUN: llvm-readelf -S -s %t | FileCheck %s --check-prefix=SEC
+# RUN: llvm-readelf -x .rodata -x .data %t | FileCheck --check-prefix=HEX %s
+# RUN: llvm-readobj -r %t | FileCheck %s --check-prefix=RELOC
+
+## ifunc is a non-preemptable STT_GNU_IFUNC. Check we create a canonical PLT
+## and redirect .rodata and .data references to it.
+
+# SEC: .text    PROGBITS 0000000000210000
+# SEC: .got.plt PROGBITS 0000000000220008
+# SEC: 0000000000210010 0 FUNC GLOBAL DEFAULT 4 ifunc
+
+## .rodata[0] and .data[0] store the address of the canonical PLT.
+# HEX:      section '.rodata':
+# HEX-NEXT: 0x00200170 10002100 00000000
+# HEX:      section '.data':
+# HEX-NEXT: 0x00220000 10002100 00000000
+
+# RELOC:      .rela.dyn {
+# RELOC-NEXT:   0x220008 R_AARCH64_IRELATIVE - 0x210000
+# RELOC-NEXT: }
+
+.globl ifunc
+.type ifunc,@gnu_indirect_function
+ifunc:
+  ret
+
+.rodata
+.p2align 3
+.xword ifunc
+
+.data
+.p2align 3
+.xword ifunc

lld/trunk/test/ELF/gnu-ifunc-canon.s

 // REQUIRES: x86
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %S/Inputs/gnu-ifunc-canon-ro-pcrel.s -o %t-ro-pcrel.o
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %S/Inputs/gnu-ifunc-canon-ro-abs.s -o %t-ro-abs.o
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %S/Inputs/gnu-ifunc-canon-rw-addend.s -o %t-rw-addend.o
 // RUN: ld.lld %t.o -o %t1
-// RUN: llvm-readobj -r %t1 | FileCheck --check-prefix=IREL2 %s
+// RUN: llvm-readobj -r %t1 | FileCheck --check-prefix=IREL1 %s
 // RUN: ld.lld %t.o %t-ro-pcrel.o -o %t2
 // RUN: llvm-readobj -r %t2 | FileCheck --check-prefix=IREL1 %s
 // RUN: ld.lld %t.o %t-ro-abs.o -o %t3
 // RUN: llvm-readobj -r %t3 | FileCheck --check-prefix=IREL1 %s
 // RUN: ld.lld %t.o %t-rw-addend.o -o %t4
 // RUN: llvm-readobj -r %t4 | FileCheck --check-prefix=IREL1 %s
 // RUN: llvm-objdump -s %t4 | FileCheck --check-prefix=DUMP %s
 // RUN: ld.lld %t.o %t-rw-addend.o -o %t4a -z retpolineplt
 // RUN: llvm-readobj -r %t4a | FileCheck --check-prefix=IREL1 %s
 // RUN: llvm-objdump -s %t4a | FileCheck --check-prefix=DUMP2 %s
 // RUN: ld.lld %t-ro-pcrel.o %t.o -o %t5
 // RUN: llvm-readobj -r %t5 | FileCheck --check-prefix=IREL1 %s
 // RUN: ld.lld %t-ro-abs.o %t.o -o %t6
 // RUN: llvm-readobj -r %t6 | FileCheck --check-prefix=IREL1 %s
 // RUN: ld.lld %t-rw-addend.o %t.o -o %t7
 // RUN: llvm-readobj -r %t7 | FileCheck --check-prefix=IREL1 %s
 // RUN: ld.lld %t.o -o %t8 -pie
-// RUN: llvm-readobj -r %t8 | FileCheck --check-prefix=IREL2 %s
+// RUN: llvm-readobj -r %t8 | FileCheck --check-prefix=IREL1-REL2 %s
 // RUN: ld.lld %t.o %t-ro-pcrel.o -o %t9 -pie
 // RUN: llvm-readobj -r %t9 | FileCheck --check-prefix=IREL1-REL2 %s
 // RUN: ld.lld %t.o %t-rw-addend.o -o %t10 -pie
 // RUN: llvm-readobj -r %t10 | FileCheck --check-prefix=IREL1-REL3 %s
 // RUN: ld.lld %t-ro-pcrel.o %t.o -o %t11 -pie
 // RUN: llvm-readobj -r %t11 | FileCheck --check-prefix=IREL1-REL2 %s
 // RUN: ld.lld %t-rw-addend.o %t.o -o %t12 -pie
 // RUN: llvm-readobj -r %t12 | FileCheck --check-prefix=IREL1-REL3 %s
 
-// Two relocs, one for the GOT and the other for .data.
-// IREL2-NOT: R_X86_64_
-// IREL2: .rela.dyn
-// IREL2-NEXT: R_X86_64_IRELATIVE
-// IREL2-NEXT: R_X86_64_IRELATIVE
-// IREL2-NOT: R_X86_64_
-
 // One reloc for the canonical PLT.
 // IREL1-NOT: R_X86_64_
 // IREL1: .rela.dyn
 // IREL1-NEXT: R_X86_64_IRELATIVE
 // IREL1-NOT: R_X86_64_
 
 // One reloc for the canonical PLT and two RELATIVE relocations pointing to it,
 // one in the GOT and one in .data.

lld/trunk/test/ELF/ppc64-toc-relax-ifunc.s

 # REQUIRES: ppc
 
 # RUN: llvm-mc -filetype=obj -triple=powerpc64le %s -o %t.o
 # RUN: echo '.globl ifunc; .type ifunc, %gnu_indirect_function; ifunc:' | \
 # RUN:   llvm-mc -filetype=obj -triple=powerpc64le - -o %t1.o
 # RUN: ld.lld %t.o %t1.o -o %t
-# RUN: llvm-objdump -d %t | FileCheck %s
+# RUN: llvm-readelf -S -s %t | FileCheck --check-prefix=SEC %s
+# RUN: llvm-readelf -x .toc %t | FileCheck --check-prefix=HEX %s
+# RUN: llvm-objdump -d %t | FileCheck --check-prefix=DIS %s
 
-## ifunc is a non-preemptable STT_GNU_IFUNC. Its toc entry will be
-## relocated by R_PPC64_IRELATIVE, not representable by a toc-relative value.
-## Check the toc-indirect access is not relaxed.
+## ifunc is a non-preemptable STT_GNU_IFUNC. The R_PPC64_ADDR64 in .toc
+## creates a canonical PLT for it and changes its type to STT_FUNC. We can thus
+## still perform toc-indirect to toc-relative relaxation because the distance
+## to the address of the canonical PLT is fixed.
 
-# CHECK:      nop
-# CHECK-NEXT: ld 3, -32768(2)
+# SEC: .text PROGBITS 0000000010010000
+# SEC: .plt  NOBITS   0000000010030000
+# SEC: 0000000010010010 0 FUNC GLOBAL DEFAULT 3 ifunc
+
+## .toc[0] stores the address of the canonical PLT.
+# HEX:      section '.toc':
+# HEX-NEXT: 0x10020000 10000110 00000000
+
+# REL:      .rela.dyn {
+# REL-NEXT:   0x10030000 R_PPC64_IRELATIVE - 0x10010008
+# REL-NEXT: }
+
+# DIS: addi 3, 3,
 
 addis 3, 2, .toc@toc@ha
 ld 3, .toc@toc@l(3)
 
 .section .toc,"aw",@progbits
   .quad ifunc