[llvm][lld] Pre-commit tests for RISCV TLSDESC symbols #85816

ilovepi · 2024-03-19T16:42:31Z

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.

Created using spr 1.3.4

llvmbot · 2024-03-19T16:43:00Z

@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-lld

Author: Paul Kirth (ilovepi)

Changes

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

2 Files Affected:

(modified) lld/test/ELF/riscv-tlsdesc.s (+37)
(added) llvm/test/CodeGen/RISCV/tlsdesc-symbol.ll (+22)

diff --git a/lld/test/ELF/riscv-tlsdesc.s b/lld/test/ELF/riscv-tlsdesc.s
index 1738f86256caa6..a893d098344d0a 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 --position-independent -filetype=obj d.s -o d.64.o
+# RUN: not ld.lld -shared -soname=libd.64.so -o libd.64.so d.64.o 2>&1 | FileCheck %s --check-prefix=BADTLSLABEL
+
+# RUN: llvm-mc -triple=riscv32 --position-independent -filetype=obj d.s -o d.32.o
+# RUN: not ld.lld -shared -soname=libd.32.so -o libd.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,31 @@ b:
 .tbss
 .globl c
 c: .zero 4
+
+#--- d.s
+  .text
+  .attribute	4, 16
+  .attribute	5, "rv64i2p1_m2p0_a2p1_c2p0"
+  .globl	bar
+  .p2align	1
+  .type	bar,@function
+bar:
+  addi	sp, sp, -16
+.Lpcrel_hi0:
+  auipc	a0, %got_pcrel_hi(_ZTH3foo)
+  ld	a0, %pcrel_lo(.Lpcrel_hi0)(a0)
+  beqz	a0, .Ltlsdesc_hi0
+  call	_ZTH3foo
+.Ltlsdesc_hi0:
+  auipc	a0, %tlsdesc_hi(foo)
+  ld	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
+  lw	a0, 0(a1)
+  addiw	a0, a0, 1
+  sw	a0, 0(a1)
+  addi	sp, sp, 16
+  ret
+
+.weak _ZTH3foo
diff --git a/llvm/test/CodeGen/RISCV/tlsdesc-symbol.ll b/llvm/test/CodeGen/RISCV/tlsdesc-symbol.ll
new file mode 100644
index 00000000000000..4fb88b33aae516
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/tlsdesc-symbol.ll
@@ -0,0 +1,22 @@
+; RUN: llc -mtriple=riscv64 -relocation-model=pic -enable-tlsdesc < %s \
+; RUN:     | llvm-mc -triple=riscv64 -filetype=obj -o %t.o --position-independent \
+; RUN:     | llvm-readelf --symbols %t.o \
+; RUN:     | FileCheck %s
+
+; RUN: llc -mtriple=riscv32 -relocation-model=pic -enable-tlsdesc < %s \
+; RUN:     | llvm-mc -triple=riscv32 -filetype=obj -o %t.o --position-independent \
+; RUN:     | llvm-readelf --symbols %t.o \
+; 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 · 2024-03-19T16:49:55Z

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 --position-independent -filetype=obj d.s -o d.64.o
+# RUN: not ld.lld -shared -soname=libd.64.so -o libd.64.so d.64.o 2>&1 | FileCheck %s --check-prefix=BADTLSLABEL


we don't use lib*.so in tests. Just d.64.so

MaskRay · 2024-03-19T16:57:18Z

lld/test/ELF/riscv-tlsdesc.s

+  auipc	a0, %got_pcrel_hi(_ZTH3foo)
+  ld	a0, %pcrel_lo(.Lpcrel_hi0)(a0)
+  beqz	a0, .Ltlsdesc_hi0
+  call	_ZTH3foo


remove unneeded instructions from the test

Thanks. The new version should be much smaller, but let me know if I should reduce it more.

MaskRay · 2024-03-19T16:58:19Z

llvm/test/CodeGen/RISCV/tlsdesc-symbol.ll

@@ -0,0 +1,22 @@
+; RUN: llc -mtriple=riscv64 -relocation-model=pic -enable-tlsdesc < %s \
+; RUN:     | llvm-mc -triple=riscv64 -filetype=obj -o %t.o --position-independent \


If -o %t.o, the pipe should not be used

I don't think a new file is needed. You can additionally add llvm-readelf -s lines to tls-models.ll

I used a separate file since the other checks were autogenerated.

Well, I used a new file since that one is autogenerated, and my understanding was that its better not to mix the autogenerated tests w/ hand written ones. That said, I guess its fine to make it autogenerated?

I think the latest version now avoids the output file. Thanks for catching that.

Just to confirm, we're OK w/ the extra test file? or shall I add manual tests to tls-model.ll?

Created using spr 1.3.4

MaskRay · 2024-03-19T22:54:35Z

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 --position-independent -filetype=obj d.s -o d.64.o


Delete --position-independent, which is only relevant for mips.

Thanks for pointing that out.

Created using spr 1.3.4

MaskRay · 2024-03-19T23:25:29Z

llvm/test/CodeGen/RISCV/tlsdesc-symbol.ll

@@ -0,0 +1,22 @@
+; RUN: llc -mtriple=riscv64 -relocation-model=pic -enable-tlsdesc < %s \


Yes, this extra file is file.

Best to reference tls-models.ll

Created using spr 1.3.4

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

ilovepi · 2024-05-03T23:10:26Z

\cherry-pick #85816

tstellar · 2024-05-09T17:43:21Z

/cherry-pick f6f474c

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)

llvmbot · 2024-05-09T17:48:45Z

/pull-request #91632

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)

[𝘀𝗽𝗿] initial version

6a944f7

Created using spr 1.3.4

llvmbot added lld lld:ELF labels Mar 19, 2024

ilovepi changed the title ~~[llvm] Pre-commit test for RISCV TLSDESC symbols~~ [llvm][lld] Pre-commit tests for RISCV TLSDESC symbols Mar 19, 2024

ilovepi requested review from MaskRay and topperc March 19, 2024 16:47

MaskRay reviewed Mar 19, 2024

View reviewed changes

Address comments, update names, and use stdout

daf3ea0

Created using spr 1.3.4

MaskRay reviewed Mar 19, 2024

View reviewed changes

MaskRay approved these changes Mar 19, 2024

View reviewed changes

Remove --position-independent

2ea78dc

Created using spr 1.3.4

MaskRay reviewed Mar 19, 2024

View reviewed changes

Reference tls-model.ll in related test

6ad93ed

Created using spr 1.3.4

ilovepi merged commit f6f474c into main Mar 20, 2024

ilovepi deleted the users/ilovepi/spr/llvm-pre-commit-test-for-riscv-tlsdesc-symbols branch March 20, 2024 20:39

ilovepi added this to the LLVM 18.X Release milestone May 3, 2024

ilovepi mentioned this pull request May 7, 2024

[RISCV][lld] Set the type of TLSDESC relocation's referenced local symbol to STT_NOTYPE #85817

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[llvm][lld] Pre-commit tests for RISCV TLSDESC symbols #85816

[llvm][lld] Pre-commit tests for RISCV TLSDESC symbols #85816

ilovepi commented Mar 19, 2024 •

edited

Loading

llvmbot commented Mar 19, 2024 •

edited

Loading

MaskRay Mar 19, 2024

ilovepi Mar 19, 2024

MaskRay Mar 19, 2024

ilovepi Mar 19, 2024

MaskRay Mar 19, 2024

MaskRay Mar 19, 2024

ilovepi Mar 19, 2024

ilovepi Mar 19, 2024

ilovepi Mar 19, 2024

ilovepi Mar 19, 2024

MaskRay Mar 19, 2024

ilovepi Mar 19, 2024

MaskRay Mar 19, 2024

ilovepi commented May 3, 2024

tstellar commented May 9, 2024

llvmbot commented May 9, 2024

		@@ -0,0 +1,22 @@
		; RUN: llc -mtriple=riscv64 -relocation-model=pic -enable-tlsdesc < %s \
		; RUN: \| llvm-mc -triple=riscv64 -filetype=obj -o %t.o --position-independent \

[llvm][lld] Pre-commit tests for RISCV TLSDESC symbols #85816

[llvm][lld] Pre-commit tests for RISCV TLSDESC symbols #85816

Conversation

ilovepi commented Mar 19, 2024 • edited Loading

llvmbot commented Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilovepi commented May 3, 2024

tstellar commented May 9, 2024

llvmbot commented May 9, 2024

ilovepi commented Mar 19, 2024 •

edited

Loading

llvmbot commented Mar 19, 2024 •

edited

Loading