Skip to content

release/18.x: [llvm][lld] Pre-commit tests for RISCV TLSDESC symbols #91632

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

Merged
merged 1 commit into from
May 10, 2024

Conversation

llvmbot
Copy link
Member

@llvmbot llvmbot commented May 9, 2024

Backport f6f474c

Requested by: @tstellar

@llvmbot llvmbot added this to the LLVM 18.X Release milestone May 9, 2024
@llvmbot
Copy link
Member Author

llvmbot commented May 9, 2024

@MaskRay What do you think about merging this PR to the release branch?

@llvmbot
Copy link
Member Author

llvmbot commented May 9, 2024

@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-lld

Author: None (llvmbot)

Changes

Backport f6f474c

Requested by: @tstellar


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

2 Files Affected:

  • (modified) lld/test/ELF/riscv-tlsdesc.s (+25)
  • (added) llvm/test/CodeGen/RISCV/tlsdesc-symbol.ll (+24)
diff --git a/lld/test/ELF/riscv-tlsdesc.s b/lld/test/ELF/riscv-tlsdesc.s
index 1738f86256caa..c583e15cf30ce 100644
--- a/lld/test/ELF/riscv-tlsdesc.s
+++ b/lld/test/ELF/riscv-tlsdesc.s
@@ -29,6 +29,12 @@
 # RUN: ld.lld -e 0 -z now a.32.o c.32.so -o a.32.ie
 # RUN: llvm-objdump --no-show-raw-insn -M no-aliases -h -d a.32.ie | FileCheck %s --check-prefix=IE32
 
+# RUN: llvm-mc -triple=riscv64 -filetype=obj d.s -o d.64.o
+# RUN: not ld.lld -shared -soname=d.64.so -o d.64.so d.64.o 2>&1 | FileCheck %s --check-prefix=BADTLSLABEL
+
+# RUN: llvm-mc -triple=riscv32 -filetype=obj d.s -o d.32.o --defsym ELF32=1
+# RUN: not ld.lld -shared -soname=d.32.so -o d.32.so d.32.o 2>&1 | FileCheck %s --check-prefix=BADTLSLABEL
+
 # GD64-RELA:      .rela.dyn {
 # GD64-RELA-NEXT:   0x2408 R_RISCV_TLSDESC - 0x7FF
 # GD64-RELA-NEXT:   0x23E8 R_RISCV_TLSDESC a 0x0
@@ -150,6 +156,9 @@
 # IE32-NEXT:         lw      a0, 0x80(a0)
 # IE32-NEXT:         add     a0, a0, tp
 
+## FIXME This should not pass, but the code MC layer needs a fix to prevent this.
+# BADTLSLABEL: error: d.{{.*}}.o has an STT_TLS symbol but doesn't have an SHF_TLS section
+
 #--- a.s
 .macro load dst, src
 .ifdef ELF32
@@ -192,3 +201,19 @@ b:
 .tbss
 .globl c
 c: .zero 4
+
+#--- d.s
+.macro load dst, src
+.ifdef ELF32
+lw \dst, \src
+.else
+ld \dst, \src
+.endif
+.endm
+
+.Ltlsdesc_hi0:
+  auipc	a0, %tlsdesc_hi(foo)
+  load	a1, %tlsdesc_load_lo(.Ltlsdesc_hi0)(a0)
+  addi	a0, a0, %tlsdesc_add_lo(.Ltlsdesc_hi0)
+  jalr	t0, 0(a1), %tlsdesc_call(.Ltlsdesc_hi0)
+  add	a1, a0, tp
diff --git a/llvm/test/CodeGen/RISCV/tlsdesc-symbol.ll b/llvm/test/CodeGen/RISCV/tlsdesc-symbol.ll
new file mode 100644
index 0000000000000..23ba2ffb1ad76
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/tlsdesc-symbol.ll
@@ -0,0 +1,24 @@
+;; The test in this file do not appear in tls-models.ll because
+;; they are not auto-generated.
+; RUN: llc -mtriple=riscv64 -relocation-model=pic -enable-tlsdesc < %s \
+; RUN:     | llvm-mc -triple=riscv64 -filetype=obj -o - \
+; RUN:     | llvm-readelf --symbols - \
+; RUN:     | FileCheck %s
+
+; RUN: llc -mtriple=riscv32 -relocation-model=pic -enable-tlsdesc < %s \
+; RUN:     | llvm-mc -triple=riscv32 -filetype=obj -o - \
+; RUN:     | llvm-readelf --symbols - \
+; RUN:     | FileCheck %s
+
+; Check that TLS symbols are lowered correctly based on the specified
+; model. Make sure they're external to avoid them all being optimised to Local
+; Exec for the executable.
+
+@unspecified = external thread_local global i32
+
+define ptr @f1() nounwind {
+entry:
+  ret ptr @unspecified
+  ; CHECK: Symbol table '.symtab' contains 7 entries:
+  ; CHECK: TLS {{.*}} unspecified
+}

@MaskRay
Copy link
Member

MaskRay commented May 9, 2024

LGTM

I suggested the commits in an Android toolchain cherrypick as well.

Currently, we mistakenly mark the local labels used in RISC-V TLSDESC as
TLS symbols, when they should not be. This patch adds tests with the
current incorrect behavior, and subsequent patches will address the
issue.

Reviewers: MaskRay, topperc

Reviewed By: MaskRay

Pull Request: llvm#85816

(cherry picked from commit f6f474c)
@tstellar tstellar merged commit 5c77c1d into llvm:release/18.x May 10, 2024
10 of 11 checks passed
@tstellar
Copy link
Collaborator

@MaskRay (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants