Skip to content
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

Merged

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Jul 12, 2024

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 #79239 and fixes some comments.

Created using spr 1.3.5-bogner
@llvmbot
Copy link
Member

llvmbot commented Jul 12, 2024

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-elf

Author: Fangrui Song (MaskRay)

Changes

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 ignored by RISCV::relocateAlloc, just return 0.


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

3 Files Affected:

  • (modified) lld/ELF/Arch/RISCV.cpp (+2-2)
  • (modified) lld/ELF/InputSection.cpp (+6-1)
  • (modified) lld/test/ELF/riscv-tlsdesc.s (+23)
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
Copy link
Contributor

@ilovepi ilovepi left a 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.

@@ -164,6 +169,15 @@
# IE32-NEXT: lw a0, 0x80(a0)
# IE32-NEXT: add a0, a0, tp

# IE64A-LABEL: 0000000000002340 R_RISCV_TLS_TPREL64 c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# IE64A-LABEL: 0000000000002340 R_RISCV_TLS_TPREL64 c
# IE64A-LABEL: 0000000000002340 R_RISCV_TLS_TPREL64

Typo?

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. Makes sense.

// `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);
Copy link
Contributor

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?

Copy link
Member Author

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!

MaskRay added 2 commits July 11, 2024 17:51
Created using spr 1.3.5-bogner
Created using spr 1.3.5-bogner
@MaskRay MaskRay changed the title [ELF] Fix TLSDESC=>IE when there is no TLS section [ELF,RISCV] Fix TLSDESC=>IE when there is no TLS section Jul 12, 2024
@MaskRay MaskRay merged commit cdd29f5 into main Jul 12, 2024
4 of 6 checks passed
@MaskRay MaskRay deleted the users/MaskRay/spr/elf-fix-tlsdescie-when-there-is-no-tls-section branch July 12, 2024 00:59
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants