[lld/ELF] Avoid unnecessary TPOFF relocations in GOT for -pie #81739

uweigand · 2024-02-14T14:24:40Z

With the new SystemZ port we noticed that -pie executables generated from files containing R_390_TLS_IEENT relocations will have unnecessary relocations in their GOT:

                    9e8d8: R_390_TLS_TPOFF  *ABS*+0x18

This is caused by the config->isPic conditon in addTpOffsetGotEntry:

static void addTpOffsetGotEntry(Symbol &sym) {
in.got->addEntry(sym);
uint64_t off = sym.getGotOffset();
if (!sym.isPreemptible && !config->isPic) {
in.got->addConstant({R_TPREL, target->symbolicRel, off, 0, &sym});
return;
}

It is correct that we need to retain a TPOFF relocation if the target symbol is preemptible or if we're building a shared library. But when building a -pie executable, those values are fixed at link time and there's no need for any remaining dynamic relocation.

Note that the equivalent MIPS-specific code in MipsGotSection::build checks for config->shared instead of config->isPic; we should use the same check here. (Note also that on many other platforms we're not even using addTpOffsetGotEntry in this case as an IE->LE relaxation is applied before; we don't have this type of relaxation on SystemZ.)

With the new SystemZ port we noticed that -pie executables generated from files containing R_390_TLS_IEENT relocations will have unnecessary relocations in their GOT: 9e8d8: R_390_TLS_TPOFF *ABS*+0x18 This is caused by the config->isPic conditon in addTpOffsetGotEntry: static void addTpOffsetGotEntry(Symbol &sym) { in.got->addEntry(sym); uint64_t off = sym.getGotOffset(); if (!sym.isPreemptible && !config->isPic) { in.got->addConstant({R_TPREL, target->symbolicRel, off, 0, &sym}); return; } It is correct that we need to retain a TPOFF relocation if the target symbol is preemptible or if we're building a shared library. But when building a -pie executable, those values are fixed at link time and there's no need for any remaining dynamic relocation. Note that the equivalent MIPS-specific code in MipsGotSection::build checks for config->shared instead of config->isPic; we should use the same check here. (Note also that on many other platforms we're not even using addTpOffsetGotEntry in this case as an IE->LE relaxation is applied before; we don't have this type of relaxation on SystemZ.)

llvmbot · 2024-02-14T14:25:11Z

@llvm/pr-subscribers-lld-elf

Author: Ulrich Weigand (uweigand)

Changes

With the new SystemZ port we noticed that -pie executables generated from files containing R_390_TLS_IEENT relocations will have unnecessary relocations in their GOT:

                    9e8d8: R_390_TLS_TPOFF  *ABS*+0x18

This is caused by the config->isPic conditon in addTpOffsetGotEntry:

static void addTpOffsetGotEntry(Symbol &sym) {
in.got->addEntry(sym);
uint64_t off = sym.getGotOffset();
if (!sym.isPreemptible && !config->isPic) {
in.got->addConstant({R_TPREL, target->symbolicRel, off, 0, &sym});
return;
}

It is correct that we need to retain a TPOFF relocation if the target symbol is preemptible or if we're building a shared library. But when building a -pie executable, those values are fixed at link time and there's no need for any remaining dynamic relocation.

Note that the equivalent MIPS-specific code in MipsGotSection::build checks for config->shared instead of config->isPic; we should use the same check here. (Note also that on many other platforms we're not even using addTpOffsetGotEntry in this case as an IE->LE relaxation is applied before; we don't have this type of relaxation on SystemZ.)

Full diff: https://github.com/llvm/llvm-project/pull/81739.diff

2 Files Affected:

(modified) lld/ELF/Relocations.cpp (+1-1)
(modified) lld/test/ELF/systemz-tls-ie.s (+34)

diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index f64b4219e0acc1..619fbaf5dc5452 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -940,7 +940,7 @@ void elf::addGotEntry(Symbol &sym) {
 static void addTpOffsetGotEntry(Symbol &sym) {
   in.got->addEntry(sym);
   uint64_t off = sym.getGotOffset();
-  if (!sym.isPreemptible && !config->isPic) {
+  if (!sym.isPreemptible && !config->shared) {
     in.got->addConstant({R_TPREL, target->symbolicRel, off, 0, &sym});
     return;
   }
diff --git a/lld/test/ELF/systemz-tls-ie.s b/lld/test/ELF/systemz-tls-ie.s
index 27b642ed2dfc5f..85e2f24cb61f62 100644
--- a/lld/test/ELF/systemz-tls-ie.s
+++ b/lld/test/ELF/systemz-tls-ie.s
@@ -12,6 +12,14 @@
 # RUN: llvm-objdump --section .data --full-contents %t | FileCheck --check-prefix=LE-DATA %s
 # RUN: llvm-objdump --section .got --full-contents %t | FileCheck --check-prefix=LE-GOT %s
 
+## With -pie we still have the R_390_RELATIVE for the data element, but all GOT
+## entries should be fully resolved without any remaining R_390_TLS_TPOFF.
+# RUN: ld.lld -pie %t.o -o %t.pie
+# RUN: llvm-readelf -r %t.pie | FileCheck --check-prefix=PIE-REL %s
+# RUN: llvm-objdump -d --no-show-raw-insn %t.pie | FileCheck --check-prefix=PIE %s
+# RUN: llvm-objdump --section .data --full-contents %t.pie | FileCheck --check-prefix=PIE-DATA %s
+# RUN: llvm-objdump --section .got --full-contents %t.pie | FileCheck --check-prefix=PIE-GOT %s
+
 # IE-REL: Relocation section '.rela.dyn' at offset {{.*}} contains 4 entries:
 # IE-REL: 0000000000003478 000000000000000c R_390_RELATIVE 2460
 # IE-REL: 0000000000002460 0000000100000038 R_390_TLS_TPOFF 0000000000000008 a + 0
@@ -58,6 +66,32 @@
 # LE-GOT: 1002248 00000000 00000000 ffffffff fffffff8
 # LE-GOT: 1002258 ffffffff fffffffc 00000000 00000000
 
+# PIE-REL: Relocation section '.rela.dyn' at offset {{.*}} contains 1 entries:
+# PIE-REL: 00000000000033d0 000000000000000c R_390_RELATIVE 23b8
+
+## TP offset for a is at 0x23b8
+# PIE:      lgrl    %r1, 0x23b8
+# PIE-NEXT: lgf     %r1, 0(%r1,%r7)
+
+## TP offset for b is at 0x23c0
+# PIE-NEXT: lgrl    %r1, 0x23c0
+# PIE-NEXT: lgf     %r1, 0(%r1,%r7)
+
+## TP offset for c is at 0x23c8
+# PIE-NEXT: lgrl    %r1, 0x23c8
+# PIE-NEXT: lgf     %r1, 0(%r1,%r7)
+
+## Data element: TP offset for a is at 0x23b8 (relocated via R_390_RELATIVE above)
+# PIE-DATA: 33d0 00000000 00000000
+
+## TP offsets in GOT:
+# a: -8
+# b: -4
+# c: 0
+# PIE-GOT: 23a0 00000000 000022d0 00000000 00000000
+# PIE-GOT: 23b0 00000000 00000000 ffffffff fffffff8
+# PIE-GOT: 23c0 ffffffff fffffffc 00000000 00000000
+
 ear     %r7,%a0
 sllg    %r7,%r1,32
 ear     %r7,%a1

llvmbot · 2024-02-14T14:25:11Z

@llvm/pr-subscribers-lld

Author: Ulrich Weigand (uweigand)

Changes

With the new SystemZ port we noticed that -pie executables generated from files containing R_390_TLS_IEENT relocations will have unnecessary relocations in their GOT:

                    9e8d8: R_390_TLS_TPOFF  *ABS*+0x18

This is caused by the config->isPic conditon in addTpOffsetGotEntry:

static void addTpOffsetGotEntry(Symbol &sym) {
in.got->addEntry(sym);
uint64_t off = sym.getGotOffset();
if (!sym.isPreemptible && !config->isPic) {
in.got->addConstant({R_TPREL, target->symbolicRel, off, 0, &sym});
return;
}

It is correct that we need to retain a TPOFF relocation if the target symbol is preemptible or if we're building a shared library. But when building a -pie executable, those values are fixed at link time and there's no need for any remaining dynamic relocation.

Note that the equivalent MIPS-specific code in MipsGotSection::build checks for config->shared instead of config->isPic; we should use the same check here. (Note also that on many other platforms we're not even using addTpOffsetGotEntry in this case as an IE->LE relaxation is applied before; we don't have this type of relaxation on SystemZ.)

Full diff: https://github.com/llvm/llvm-project/pull/81739.diff

2 Files Affected:

(modified) lld/ELF/Relocations.cpp (+1-1)
(modified) lld/test/ELF/systemz-tls-ie.s (+34)

diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index f64b4219e0acc1..619fbaf5dc5452 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -940,7 +940,7 @@ void elf::addGotEntry(Symbol &sym) {
 static void addTpOffsetGotEntry(Symbol &sym) {
   in.got->addEntry(sym);
   uint64_t off = sym.getGotOffset();
-  if (!sym.isPreemptible && !config->isPic) {
+  if (!sym.isPreemptible && !config->shared) {
     in.got->addConstant({R_TPREL, target->symbolicRel, off, 0, &sym});
     return;
   }
diff --git a/lld/test/ELF/systemz-tls-ie.s b/lld/test/ELF/systemz-tls-ie.s
index 27b642ed2dfc5f..85e2f24cb61f62 100644
--- a/lld/test/ELF/systemz-tls-ie.s
+++ b/lld/test/ELF/systemz-tls-ie.s
@@ -12,6 +12,14 @@
 # RUN: llvm-objdump --section .data --full-contents %t | FileCheck --check-prefix=LE-DATA %s
 # RUN: llvm-objdump --section .got --full-contents %t | FileCheck --check-prefix=LE-GOT %s
 
+## With -pie we still have the R_390_RELATIVE for the data element, but all GOT
+## entries should be fully resolved without any remaining R_390_TLS_TPOFF.
+# RUN: ld.lld -pie %t.o -o %t.pie
+# RUN: llvm-readelf -r %t.pie | FileCheck --check-prefix=PIE-REL %s
+# RUN: llvm-objdump -d --no-show-raw-insn %t.pie | FileCheck --check-prefix=PIE %s
+# RUN: llvm-objdump --section .data --full-contents %t.pie | FileCheck --check-prefix=PIE-DATA %s
+# RUN: llvm-objdump --section .got --full-contents %t.pie | FileCheck --check-prefix=PIE-GOT %s
+
 # IE-REL: Relocation section '.rela.dyn' at offset {{.*}} contains 4 entries:
 # IE-REL: 0000000000003478 000000000000000c R_390_RELATIVE 2460
 # IE-REL: 0000000000002460 0000000100000038 R_390_TLS_TPOFF 0000000000000008 a + 0
@@ -58,6 +66,32 @@
 # LE-GOT: 1002248 00000000 00000000 ffffffff fffffff8
 # LE-GOT: 1002258 ffffffff fffffffc 00000000 00000000
 
+# PIE-REL: Relocation section '.rela.dyn' at offset {{.*}} contains 1 entries:
+# PIE-REL: 00000000000033d0 000000000000000c R_390_RELATIVE 23b8
+
+## TP offset for a is at 0x23b8
+# PIE:      lgrl    %r1, 0x23b8
+# PIE-NEXT: lgf     %r1, 0(%r1,%r7)
+
+## TP offset for b is at 0x23c0
+# PIE-NEXT: lgrl    %r1, 0x23c0
+# PIE-NEXT: lgf     %r1, 0(%r1,%r7)
+
+## TP offset for c is at 0x23c8
+# PIE-NEXT: lgrl    %r1, 0x23c8
+# PIE-NEXT: lgf     %r1, 0(%r1,%r7)
+
+## Data element: TP offset for a is at 0x23b8 (relocated via R_390_RELATIVE above)
+# PIE-DATA: 33d0 00000000 00000000
+
+## TP offsets in GOT:
+# a: -8
+# b: -4
+# c: 0
+# PIE-GOT: 23a0 00000000 000022d0 00000000 00000000
+# PIE-GOT: 23b0 00000000 00000000 ffffffff fffffff8
+# PIE-GOT: 23c0 ffffffff fffffffc 00000000 00000000
+
 ear     %r7,%a0
 sllg    %r7,%r1,32
 ear     %r7,%a1

MaskRay · 2024-02-14T16:54:11Z

lld/test/ELF/systemz-tls-ie.s

+# RUN: ld.lld -pie %t.o -o %t.pie
+# RUN: llvm-readelf -r %t.pie | FileCheck --check-prefix=PIE-REL %s
+# RUN: llvm-objdump -d --no-show-raw-insn %t.pie | FileCheck --check-prefix=PIE %s
+# RUN: llvm-objdump --section .data --full-contents %t.pie | FileCheck --check-prefix=PIE-DATA %s


To inspect section content, llvm-readelf -x .got -x .data is shorter:)

MaskRay

Thanks! This for arm/hexagon/loongarch/riscv/s390x/etc that do not support IE to LE TLS optimization.

MaskRay · 2024-02-14T20:49:41Z

/cherry-pick 6f90773

llvmbot · 2024-02-14T20:54:35Z

Failed to cherry-pick: 6f90773

https://github.com/llvm/llvm-project/actions/runs/7907226425

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

MaskRay · 2024-02-14T22:25:16Z

This backport requires the the lld/SystemZ patch to be cherry-picked first.

uweigand · 2024-02-16T13:22:26Z

/cherry-pick 6f90773

…1739) With the new SystemZ port we noticed that -pie executables generated from files containing R_390_TLS_IEENT relocations will have unnecessary relocations in their GOT: 9e8d8: R_390_TLS_TPOFF *ABS*+0x18 This is caused by the config->isPic conditon in addTpOffsetGotEntry: static void addTpOffsetGotEntry(Symbol &sym) { in.got->addEntry(sym); uint64_t off = sym.getGotOffset(); if (!sym.isPreemptible && !config->isPic) { in.got->addConstant({R_TPREL, target->symbolicRel, off, 0, &sym}); return; } It is correct that we need to retain a TPOFF relocation if the target symbol is preemptible or if we're building a shared library. But when building a -pie executable, those values are fixed at link time and there's no need for any remaining dynamic relocation. Note that the equivalent MIPS-specific code in MipsGotSection::build checks for config->shared instead of config->isPic; we should use the same check here. (Note also that on many other platforms we're not even using addTpOffsetGotEntry in this case as an IE->LE relaxation is applied before; we don't have this type of relaxation on SystemZ.) (cherry picked from commit 6f90773)

llvmbot · 2024-02-16T13:27:21Z

/pull-request #81990

…1739) With the new SystemZ port we noticed that -pie executables generated from files containing R_390_TLS_IEENT relocations will have unnecessary relocations in their GOT: 9e8d8: R_390_TLS_TPOFF *ABS*+0x18 This is caused by the config->isPic conditon in addTpOffsetGotEntry: static void addTpOffsetGotEntry(Symbol &sym) { in.got->addEntry(sym); uint64_t off = sym.getGotOffset(); if (!sym.isPreemptible && !config->isPic) { in.got->addConstant({R_TPREL, target->symbolicRel, off, 0, &sym}); return; } It is correct that we need to retain a TPOFF relocation if the target symbol is preemptible or if we're building a shared library. But when building a -pie executable, those values are fixed at link time and there's no need for any remaining dynamic relocation. Note that the equivalent MIPS-specific code in MipsGotSection::build checks for config->shared instead of config->isPic; we should use the same check here. (Note also that on many other platforms we're not even using addTpOffsetGotEntry in this case as an IE->LE relaxation is applied before; we don't have this type of relaxation on SystemZ.) (cherry picked from commit 6f90773)

uweigand requested a review from MaskRay February 14, 2024 14:24

llvmbot added lld lld:ELF labels Feb 14, 2024

MaskRay reviewed Feb 14, 2024

View reviewed changes

MaskRay approved these changes Feb 14, 2024

View reviewed changes

uweigand merged commit 6f90773 into llvm:main Feb 14, 2024
7 checks passed

uweigand deleted the lld-pie-tpoff branch February 14, 2024 17:26

uweigand mentioned this pull request Feb 14, 2024

release/18.x: [lld] Add target support for SystemZ (s390x) (#75643) #81675

Merged

MaskRay added this to the LLVM 18.X Release milestone Feb 14, 2024

llvmbot added the release:cherry-pick-failed label Feb 14, 2024

llvmbot removed the release:cherry-pick-failed label Feb 16, 2024

pointhex mentioned this pull request May 7, 2024

getStyleDiagHandler #91314

Closed

aemerson mentioned this pull request May 9, 2024

release/18.x: [AArc64][GlobalISel] Fix legalizer assert for G_INSERT_VECTOR_ELT - manual merge #91672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lld/ELF] Avoid unnecessary TPOFF relocations in GOT for -pie #81739

[lld/ELF] Avoid unnecessary TPOFF relocations in GOT for -pie #81739

uweigand commented Feb 14, 2024

llvmbot commented Feb 14, 2024

llvmbot commented Feb 14, 2024

MaskRay Feb 14, 2024 •

edited

MaskRay left a comment •

edited

MaskRay commented Feb 14, 2024

llvmbot commented Feb 14, 2024

MaskRay commented Feb 14, 2024

uweigand commented Feb 16, 2024

llvmbot commented Feb 16, 2024

[lld/ELF] Avoid unnecessary TPOFF relocations in GOT for -pie #81739

[lld/ELF] Avoid unnecessary TPOFF relocations in GOT for -pie #81739

Conversation

uweigand commented Feb 14, 2024

llvmbot commented Feb 14, 2024

llvmbot commented Feb 14, 2024

MaskRay Feb 14, 2024 • edited

Choose a reason for hiding this comment

MaskRay left a comment • edited

Choose a reason for hiding this comment

MaskRay commented Feb 14, 2024

llvmbot commented Feb 14, 2024

MaskRay commented Feb 14, 2024

uweigand commented Feb 16, 2024

llvmbot commented Feb 16, 2024

MaskRay Feb 14, 2024 •

edited

MaskRay left a comment •

edited