[LLD][ELF] Use more specific method to calculate DT_PLTRELSZ

[peter.smith]

Summary

The DT_PLTRELSZ dynamic tag is calculated using the size of the OutputSection containing the In.RelaPlt InputSection. This will work for the default no linker script case and the majority of linker scripts. Unfortunately it doesn't work for some 'almost' sensible linker scripts. It is permitted by ELF to have a single OutputSection containing both In.RelaDyn, In.RelaPlt and In.RelaIPlt. It is also permissible for the range of memory [DT_RELA, DT_RELA + DT_RELASZ) and the range [DT_JMPREL, DT_JMPREL + DT_JMPRELSZ) to overlap as long as the the latter range is at the end (see comment in glibc dynamic loader) https://code.woboq.org/userspace/glibc/elf/dynamic-link.h.html#108).

To support a linker script containing something like: .rela.dyn : { *(.rela.dyn) *(.rela.plt) } use the InputSections to determine DT_JMPRELSZ rather than the OutputSection size. I've left the calculation of the .rela.dyn to be the size of the OutputSection as that is the most literal reading of ELF and the smallest change. This affects all targets, I've added tests for AArch64, x86 (cited in PR) and ARM (as the irelative relocations do not go in the .rel.plt).

There will be no change to any project using the default linker script or the more modern

.rela.dyn       : { *(.rela.dyn) }
.rela.plt         : { *(.rela.plt) *(.rela.iplt) }

fixes pr39678 (https://bugs.llvm.org/show_bug.cgi?id=39678)

I considered giving an error message rather than trying to support this case but the logic of working out when there is an error is no simpler. It also risked faulting existing programs.

[grimar]

It feels like some corner case. Though it still a bug we could fix.
I have a few suggestions/questions below.

ELF/SyntheticSections.cpp

1255 ↗

(On Diff #174783)

DT_JMPTREL -> DT_JMPREL

1267 ↗

(On Diff #174783)

I truncated this to

Entries.push_back({DT_PLTRELSZ, [=] {
                     size_t Size = In.RelaPlt->getSize();
                     if (In.RelaIplt->getParent() == In.RelaPlt->getParent())
                       Size += In.RelaIplt->getSize();
                     return Size;
                   }});

And all your tests pass.
That will break the case when !Config->AndroidPackDynReloc (so, In.RelaIplt is placed to .rel.dyn),
but do we really need to support it? I guess such script is a rare case and perhaps not worth to support all corner cases? I am not sure.

1359 ↗

(On Diff #174783)

So I suggest to inline addPltRelSize here and just update the existent comment above.

ELF/SyntheticSections.h

465 ↗

(On Diff #174783)

Perhaps I would not create the additional method, because it is very specific.
I think it can be inlined, see how DT_PPC64_GLINK is handled for example.

test/ELF/arm-combined-dynrel-ifunc.s

12 ↗

(On Diff #174783)

I would just include the comment here instead of referencing it. No need to save space so hard in test cases I think:)
The same applies to another test.

[peter.smith]

Thanks very much for the comments. I agree that this is a somewhat niche use case, most likely embedded systems related where someone has built their own loader. I've updated the diff with the following changes:

• The ARM test passed with your updated changes because it was always being skipped! The REQUIRES: ARM should have been REQUIRES: arm. There are a couple of other tests that have that pattern that I'll fix up later today.
• I've simplified the test for the non-Android Arm case in a simpler way (just check the In.RelaIplt name isn't .rel.dyn)
• I've inlined the function and updated the comments in the test as suggested.

[grimar]

ELF/SyntheticSections.cpp

1347 ↗

(On Diff #174889)

Seems it can be In.RelaIplt->Name == In.RelaPlt.
It feels like a slightly cleaner way probably?

1347 ↗

(On Diff #174889)

I mean In.RelaIplt->Name == In.RelaPlt->Name

[peter.smith]

Agreed, I've made the change to In.RelaIplt->Name == In.RelaPlt->Name.

[grimar]

LGTM.

[peter.smith]

Thanks, I'll wait a bit before committing to see if Rui has any comments.

[ruiu]

ELF/SyntheticSections.cpp

1343 ↗

(On Diff #174892)

It seems this lambda doesn't capture any variable, so this can be [], no? It might be better to write this as a regular (outline) function, as this seems indented a bit too much.

test/ELF/aarch64-combined-dynrel-ifunc.s

1 ↗

(On Diff #174892)

Needs a colon after REQUIRES?

[peter.smith]

Thanks for the comments. I've made the lambda a free function and have made sure REQUIRES has a colon after it in all the tests.

[ruiu]

LGTM

[ikudrin]

Hi @peter.smith! The tests arm-combined-dynrel.s and arm-combined-dynrel-ifunc.s do not run because of inaccurate REQUIRES. Could you fix them, please?

[peter.smith]

In D54759#3253576, @ikudrin wrote:

Hi @peter.smith! The tests arm-combined-dynrel.s and arm-combined-dynrel-ifunc.s do not run because of inaccurate REQUIRES. Could you fix them, please?

I'll take a look. It may be later on this week before I can get to it.

[peter.smith]

Created D117896 for the REQUIRES: update

lld/trunk/ELF/SyntheticSections.cpp

void DynamicSection<ELFT>::addSize(int32_t Tag, OutputSection *Sec) {
   Entries.push_back({Tag, [=] { return Sec->Size; }});
 }
 
 template <class ELFT>
 void DynamicSection<ELFT>::addSym(int32_t Tag, Symbol *Sym) {
   Entries.push_back({Tag, [=] { return Sym->getVA(); }});
 }
 
+// A Linker script may assign the RELA relocation sections to the same
+// output section. When this occurs we cannot just use the OutputSection
+// Size. Moreover the [DT_JMPREL, DT_JMPREL + DT_PLTRELSZ) is permitted to
+// overlap with the [DT_RELA, DT_RELA + DT_RELASZ).
+static uint64_t addPltRelSz() {
+  size_t Size = In.RelaPlt->getSize();
+  if (In.RelaIplt->getParent() == In.RelaPlt->getParent() &&
+      In.RelaIplt->Name == In.RelaPlt->Name)
+    Size += In.RelaIplt->getSize();
+  return Size;
+}
+
 // Add remaining entries to complete .dynamic contents.
 template <class ELFT> void DynamicSection<ELFT>::finalizeContents() {
   // Set DT_FLAGS and DT_FLAGS_1.
   uint32_t DtFlags = 0;
   uint32_t DtFlags1 = 0;
   if (Config->Bsymbolic)
     DtFlags |= DF_SYMBOLIC;
   if (Config->ZGlobal)
template <class ELFT> void DynamicSection<ELFT>::finalizeContents() {
   // .rel[a].plt section usually consists of two parts, containing plt and
   // iplt relocations. It is possible to have only iplt relocations in the
   // output. In that case RelaPlt is empty and have zero offset, the same offset
   // as RelaIplt have. And we still want to emit proper dynamic tags for that
   // case, so here we always use RelaPlt as marker for the begining of
   // .rel[a].plt section.
   if (In.RelaPlt->getParent()->Live) {
     addInSec(DT_JMPREL, In.RelaPlt);
-    addSize(DT_PLTRELSZ, In.RelaPlt->getParent());
+    Entries.push_back({DT_PLTRELSZ, addPltRelSz});
     switch (Config->EMachine) {
     case EM_MIPS:
       addInSec(DT_MIPS_PLTGOT, In.GotPlt);
       break;
     case EM_SPARCV9:
       addInSec(DT_PLTGOT, In.Plt);
       break;
     default:

lld/trunk/test/ELF/aarch64-combined-dynrel-ifunc.s

+// REQUIRES: AArch64
+// RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu %p/Inputs/shared.s -o %t-lib.o
+// RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu %s -o %t.o
+// RUN: ld.lld %t-lib.o --shared -o %t.so
+// RUN: echo "SECTIONS { \
+// RUN:         .text : { *(.text) } \
+// RUN:         .rela.dyn : { *(.rela.dyn) *(.rela.plt) } \
+// RUN: } " > %t.script
+// RUN: ld.lld %t.o -o %t.axf %t.so --script %t.script
+// RUN: llvm-readobj --section-headers --dynamic-table %t.axf | FileCheck %s
+
+// The linker script above combines the .rela.dyn and .rela.plt into a single
+// table. ELF is clear that the DT_PLTRELSZ should match the subset of
+// relocations that is associated with the PLT. It is less clear about what
+// the value of DT_RELASZ should be. ELF implies that it should be the size
+// of the single table so that DT_RELASZ includes DT_PLTRELSZ. The loader in
+// glibc permits this as long as .rela.plt comes after .rela.dyn in the
+// combined table. In the ARM case irelative relocations do not count as PLT
+// relocs. In the AArch64 case irelative relocations count as PLT relocs.
+
+.text
+.globl indirect
+.type indirect,@gnu_indirect_function
+indirect:
+ ret
+
+.globl bar // from Inputs/shared.s
+
+.text
+.globl _start
+.type _start,@function
+main:
+ bl indirect
+ bl bar
+ adrp x8, :got:indirect
+ ldr  x8, [x8, :got_lo12:indirect]
+ adrp    x8, :got:bar
+ ldr     x8, [x8, :got_lo12:bar]
+ ret
+
+// CHECK:     Name: .rela.dyn
+// CHECK-NEXT:     Type: SHT_RELA
+// CHECK-NEXT:     Flags [
+// CHECK-NEXT:       SHF_ALLOC
+// CHECK-NEXT:     ]
+// CHECK-NEXT:     Address:
+// CHECK-NEXT:     Offset:
+// CHECK-NEXT:     Size: 72
+
+// CHECK:      0x0000000000000008 RELASZ               72
+// CHECK:      0x0000000000000002 PLTRELSZ             48

lld/trunk/test/ELF/aarch64-combined-dynrel.s

+// REQUIRES: AArch64
+// RUN: llvm-mc --triple=aarch64-linux-gnu -filetype=obj -o %t.o %s
+// RUN: echo "SECTIONS { \
+// RUN:         .text : { *(.text) } \
+// RUN:         .rela.dyn : { *(.rela.dyn) *(.rela.plt) } \
+// RUN: } " > %t.script
+// RUN: ld.lld %t.o -o %t.so --shared --script %t.script
+// RUN: llvm-readobj --section-headers --dynamic-table %t.so | FileCheck %s
+
+// The linker script above combines the .rela.dyn and .rela.plt into a single
+// table. ELF is clear that the DT_PLTRELSZ should match the subset of
+// relocations that is associated with the PLT. It is less clear about what
+// the value of DT_RELASZ should be. ELF implies that it should be the size
+// of the single table so that DT_RELASZ includes DT_PLTRELSZ. The loader in
+// glibc permits this as long as .rela.plt comes after .rela.dyn in the
+// combined table.
+ .text
+ .globl func
+ .type func, %function
+ .globl foo
+ .type foo, %object
+
+ .globl _start
+ .type _start, %function
+_start:
+ bl func
+ adrp    x8, :got:foo
+ ldr     x8, [x8, :got_lo12:foo]
+ ret
+
+// CHECK:     Name: .rela.dyn
+// CHECK-NEXT:         Type: SHT_RELA
+// CHECK-NEXT:     Flags [
+// CHECK-NEXT:       SHF_ALLOC
+// CHECK-NEXT:     ]
+// CHECK-NEXT:     Address:
+// CHECK-NEXT:     Offset:
+// CHECK-NEXT:     Size: 48
+
+// CHECK:  0x0000000000000008 RELASZ               48
+// CHECK:  0x0000000000000002 PLTRELSZ             24

lld/trunk/test/ELF/arm-combined-dynrel-ifunc.s

+// REQUIRES: arm
+// RUN: llvm-mc -filetype=obj  -arm-add-build-attributes -triple=armv7a-linux-gnueabihf %p/Inputs/arm-shared.s -o %t-lib.o
+// RUN: llvm-mc -filetype=obj -arm-add-build-attributes -triple=armv7a-linux-gnueabihf %s -o %t.o
+// RUN: ld.lld %t-lib.o --shared -o %t.so
+// RUN: echo "SECTIONS { \
+// RUN:         .text : { *(.text) } \
+// RUN:         .rela.dyn : { *(.rel.dyn) *(.rel.plt) } \
+// RUN: } " > %t.script
+// RUN: ld.lld %t.o -o %t.axf %t.so --script %t.script
+// RUN: llvm-readobj --section-headers --dynamic-table %t.axf | FileCheck %s
+
+// The linker script above combines the .rela.dyn and .rela.plt into a single
+// table. ELF is clear that the DT_PLTRELSZ should match the subset of
+// relocations that is associated with the PLT. It is less clear about what
+// the value of DT_RELASZ should be. ELF implies that it should be the size
+// of the single table so that DT_RELASZ includes DT_PLTRELSZ. The loader in
+// glibc permits this as long as .rela.plt comes after .rela.dyn in the
+// combined table. In the ARM case irelative relocations do not count as PLT
+// relocs.
+
+.text
+.globl indirect
+.type indirect,%gnu_indirect_function
+indirect:
+ bx lr
+
+.globl bar2 // from Inputs/arm-shared.s
+
+.text
+.globl _start
+.type _start,%function
+main:
+ bl indirect
+ bl bar2
+ .word indirect(got)
+ .word bar2(got)
+ bx lr
+
+// CHECK:     Name: .rela.dyn
+// CHECK-NEXT:     Type: SHT_REL
+// CHECK-NEXT:     Flags [
+// CHECK-NEXT:       SHF_ALLOC
+// CHECK-NEXT:     ]
+// CHECK-NEXT:     Address:
+// CHECK-NEXT:     Offset:
+// CHECK-NEXT:     Size: 24
+
+// CHECK: 0x00000012 RELSZ                24 (bytes)
+// CHECK: 0x00000002 PLTRELSZ             8 (bytes)

lld/trunk/test/ELF/x86-64-combined-dynrel.s

+# REQUIRES: x86
+# RUN: llvm-mc --triple=x86_64-pc-linux -filetype=obj -o %t.o %s
+# RUN: echo "SECTIONS { \
+# RUN:         .text : { *(.text) } \
+# RUN:         .rela.dyn : { *(.rela.dyn) *(.rela.plt) } \
+# RUN: } " > %t.script
+# RUN: ld.lld %t.o -o %t.so --shared --script %t.script
+# RUN: llvm-readobj --section-headers --dynamic-table %t.so | FileCheck %s
+
+// The linker script above combines the .rela.dyn and .rela.plt into a single
+// table. ELF is clear that the DT_PLTRELSZ should match the subset of
+// relocations that is associated with the PLT. It is less clear about what
+// the value of DT_RELASZ should be. ELF implies that it should be the size
+// of the single table so that DT_RELASZ includes DT_PLTRELSZ. The loader in
+// glibc permits this as long as .rela.plt comes after .rela.dyn in the
+// combined table.
+
+ .text
+ .globl func
+ .type func, %function
+ .globl foo
+ .type foo, %object
+
+ .globl _start
+ .type _start, %function
+_start:
+ call func@plt
+ movq func@GOTPCREL (%rip), %rax
+
+# CHECK:     Name: .rela.dyn
+# CHECK-NEXT:         Type: SHT_RELA
+# CHECK-NEXT:     Flags [
+# CHECK-NEXT:       SHF_ALLOC
+# CHECK-NEXT:     ]
+# CHECK-NEXT:     Address:
+# CHECK-NEXT:     Offset:
+# CHECK-NEXT:     Size: 48
+
+# CHECK:  0x0000000000000008 RELASZ               48
+# CHECK:  0x0000000000000002 PLTRELSZ             24