-
Notifications
You must be signed in to change notification settings - Fork 12.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ELF,RISCV] Fix TLSDESC=>IE when there is no TLS section #98569
[ELF,RISCV] Fix TLSDESC=>IE when there is no TLS section #98569
Conversation
Created using spr 1.3.5-bogner
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-elf Author: Fangrui Song (MaskRay) ChangesSee the comment in handleTlsRelocation. For TLSDESC=>IE (the TLS symbol If there is no TLS section, Full diff: https://github.com/llvm/llvm-project/pull/98569.diff 3 Files Affected:
diff --git a/lld/ELF/Arch/RISCV.cpp b/lld/ELF/Arch/RISCV.cpp
index faacc8f834be7..6af89ce3517b7 100644
--- a/lld/ELF/Arch/RISCV.cpp
+++ b/lld/ELF/Arch/RISCV.cpp
@@ -631,8 +631,8 @@ void RISCV::relocateAlloc(InputSectionBase &sec, uint8_t *buf) const {
continue;
case R_RELAX_TLS_GD_TO_LE:
// See the comment in handleTlsRelocation. For TLSDESC=>IE,
- // R_RISCV_TLSDESC_{LOAD_LO12,ADD_LO12,CALL} also reach here. If isToIe is
- // true, this is actually TLSDESC=>IE optimization.
+ // R_RISCV_TLSDESC_{LOAD_LO12,ADD_LO12,CALL} also reach here. If isToLe is
+ // false, this is actually TLSDESC=>IE optimization.
if (rel.type == R_RISCV_TLSDESC_HI20) {
tlsdescVal = val;
isToLe = true;
diff --git a/lld/ELF/InputSection.cpp b/lld/ELF/InputSection.cpp
index 4420be77f6685..6ea4453581894 100644
--- a/lld/ELF/InputSection.cpp
+++ b/lld/ELF/InputSection.cpp
@@ -660,7 +660,12 @@ static int64_t getTlsTpOffset(const Symbol &s) {
return s.getVA(0) + (tls->p_vaddr & (tls->p_align - 1)) - 0x7000;
case EM_LOONGARCH:
case EM_RISCV:
- return s.getVA(0) + (tls->p_vaddr & (tls->p_align - 1));
+ // See the comment in handleTlsRelocation. For TLSDESC=>IE,
+ // R_RISCV_TLSDESC_{LOAD_LO12,ADD_LO12_I,CALL} also reach here. While
+ // `tls` may be null, the return value is ignored.
+ if (s.type != STT_TLS)
+ return 0;
+ return s.getVA(0) + (tls ? tls->p_vaddr & (tls->p_align - 1) : 0);
// Variant 2.
case EM_HEXAGON:
diff --git a/lld/test/ELF/riscv-tlsdesc.s b/lld/test/ELF/riscv-tlsdesc.s
index 935ecbddfbfff..05d94d7538f92 100644
--- a/lld/test/ELF/riscv-tlsdesc.s
+++ b/lld/test/ELF/riscv-tlsdesc.s
@@ -37,6 +37,11 @@
# RUN: llvm-mc -triple=riscv32 -filetype=obj d.s -o d.32.o --defsym ELF32=1
# RUN: ld.lld -shared -soname=d.32.so -o d.32.so d.32.o --fatal-warnings
+## The output has no TLS section.
+# RUN: llvm-mc -filetype=obj -triple=riscv64 a1.s -o a1.64.o
+# RUN: ld.lld -pie a1.64.o c.64.so -o a1.64
+# RUN: llvm-objdump --no-show-raw-insn -M no-aliases -Rd a1.64 | FileCheck %s --check-prefix=IE64A
+
# GD64-RELA: .rela.dyn {
# GD64-RELA-NEXT: 0x2408 R_RISCV_TLSDESC - 0x7FF
# GD64-RELA-NEXT: 0x23E8 R_RISCV_TLSDESC a 0x0
@@ -164,6 +169,15 @@
# IE32-NEXT: lw a0, 0x80(a0)
# IE32-NEXT: add a0, a0, tp
+# IE64A-LABEL: 0000000000002340 R_RISCV_TLS_TPREL64 c
+## &.got[c]-. = 0x2340 - 0x1258 = 0x10e8
+# IE64A-LABEL: <.Ltlsdesc_hi2>:
+# IE64A-NEXT: addi zero, zero, 0x0
+# IE64A-NEXT: addi zero, zero, 0x0
+# IE64A-NEXT: 1258: auipc a0, 0x1
+# IE64A-NEXT: ld a0, 0xe8(a0)
+# IE64A-NEXT: add a0, a0, tp
+
#--- a.s
.macro load dst, src
.ifdef ELF32
@@ -202,6 +216,15 @@ a:
b:
.zero 1
+#--- a1.s
+## a.s without TLS definitions.
+.Ltlsdesc_hi2:
+ auipc a4, %tlsdesc_hi(c)
+ ld a5, %tlsdesc_load_lo(.Ltlsdesc_hi2)(a4)
+ addi a0, a4, %tlsdesc_add_lo(.Ltlsdesc_hi2)
+ jalr t0, 0(a5), %tlsdesc_call(.Ltlsdesc_hi2)
+ add a0, a0, tp
+
#--- c.s
.tbss
.globl c
|
Created using spr 1.3.5-bogner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, modulo what I think is a typo, and one comment that I wanted to be sure I understood correctly.
lld/test/ELF/riscv-tlsdesc.s
Outdated
@@ -164,6 +169,15 @@ | |||
# IE32-NEXT: lw a0, 0x80(a0) | |||
# IE32-NEXT: add a0, a0, tp | |||
|
|||
# IE64A-LABEL: 0000000000002340 R_RISCV_TLS_TPREL64 c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# IE64A-LABEL: 0000000000002340 R_RISCV_TLS_TPREL64 c | |
# IE64A-LABEL: 0000000000002340 R_RISCV_TLS_TPREL64 |
Typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The llvm-objdump -R output shows that R_RISCV_TLS_TPREL64
references c
. Added a header and a following IE64A-EMPTY:
to make this clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK. Makes sense.
lld/ELF/InputSection.cpp
Outdated
// `tls` may be null, the return value is ignored. | ||
if (s.type != STT_TLS) | ||
return 0; | ||
return s.getVA(0) + (tls ? tls->p_vaddr & (tls->p_align - 1) : 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make sure I follow the comment above correctly, we're adding 0
here in the case where tls
is null, because its getting ignored, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I should drop the tls
special case. Thanks for noticing the strange code here!
Created using spr 1.3.5-bogner
Created using spr 1.3.5-bogner
See the comment in handleTlsRelocation. For TLSDESC=>IE (the TLS symbol is defined in another DSO), R_RISCV_TLSDESC_{LOAD_LO12,ADD_LO12_I,CALL} referencing a non-preemptible label uses the `R_RELAX_TLS_GD_TO_LE` code path. If there is no TLS section, `getTlsTpOffset` will be called with null `Out::tlsPhdr`, leading to a null pointer dereference. Since the return value is used by `RISCV::relocateAlloc` and ignored there, just return 0. LoongArch TLSDESC doesn't use STT_NOTYPE labels. The `if (..) return 0;` is a no-op for LoongArch. This patch is a follow-up to llvm#79239 and fixes some comments. Pull Request: llvm#98569
See the comment in handleTlsRelocation. For TLSDESC=>IE (the TLS symbol
is defined in another DSO), R_RISCV_TLSDESC_{LOAD_LO12,ADD_LO12_I,CALL}
referencing a non-preemptible label uses the
R_RELAX_TLS_GD_TO_LE
codepath.
If there is no TLS section,
getTlsTpOffset
will be called with nullOut::tlsPhdr
, leading to a null pointer dereference. Since the returnvalue is used by
RISCV::relocateAlloc
and ignored there, just return0.
LoongArch TLSDESC doesn't use STT_NOTYPE labels. The
if (..) return 0;
is a no-op for LoongArch.
This patch is a follow-up to #79239 and fixes some comments.