[RISCV] Add support for TLSDESC #307

quic-akaryaki · 2025-08-06T19:59:50Z

No description provided.

lib/Target/GNULDBackend.cpp

quic-akaryaki · 2025-08-08T20:25:31Z

Looks like the build failed because upstream LLVM does not emit R_RISCV_ALIGN.

Signed-off-by: Alexey Karyakin <akaryaki@quicinc.com>

Add '.yaml' to config.suffixes and fix tests that had non-test yaml files outside of their Inputs directory. Signed-off-by: Alexey Karyakin <akaryaki@quicinc.com>

The RISC-V psABI is silent on where R_RISCV_RELAX has to be in the relocation table relative to the affected relocation, as long as it is "at the same position". It is also not clear where exactly `.reloc` assembly directive must place the relocation. Therefore, sort all the RISC-V relocations. They were already sorted, but not always. Signed-off-by: Alexey Karyakin <akaryaki@quicinc.com>

quic-seaswara · 2025-08-14T16:18:31Z

lib/Target/RISCV/RISCVLDBackend.cpp

+  // adjacent to the relocation they affect. The psABI is not clear if
+  // R_RISCV_RELAX must be adjacent, but using `.reloc` in assembly may generate
+  // those that are not. Note that because of this, this function must not have
+  // earlier non-error exists.


nit : exits

quic-seaswara · 2025-08-14T16:19:14Z

test/AArch64/standalone/BSS_none_bss/BSS_none_bss.yaml

@@ -1,3 +1,11 @@
+# RUN: %yaml2obj %s -o %t.o
+# RUN: %link %linkopts -march aarch64 -T %p/t.t %t.o -o %t.out
+# RUN: llvm-objdump -s %t.out | %filecheck %s


Sure (it was the original code).

quic-seaswara · 2025-08-14T16:25:30Z

lib/Target/RISCV/RISCVRelocator.cpp

+    ELFObjectFile *Obj = llvm::dyn_cast<ELFObjectFile>(&pInputFile);
+
+    // Common relocation processing for both local and global symbols.
+    switch (pReloc.type()) {


probably making this to be a function and calling this from local and global reloc would make it easy since its a common pattern that we follow across targets.

Actually I wanted to merge local/global functions because there is a lot duplication between them. We have had bugs in one of the branches because it's written a little different than the other.

quic-seaswara · 2025-08-14T16:35:11Z

lib/Target/RISCV/RISCVRelocator.cpp

+        // Non-preemptible symbols in executables will be optimized or relaxed,
+        // no GOT needed.
+        if (!m_Target.isSymbolPreemptible(*rsym))
+          return;


It looks like we dont do anything for hidden symbols / protected symbols that may get resolved as HIDDEN/PROTECTED across modules ?

cat > 1.c << !
__thread int tls_var = 0;
int foo() { return tls_var; }
!

cat > 2.c << !
extern __thread int tls_var;
int bar() { return tls_var; }
!

clang -c -target riscv32 1.c -fPIC -mtls-dialect=desc -fvisibility=hidden
clang -c -target riscv32 2.c -fPIC -mtls-dialect=desc
riscv-ld 1.o 2.o

They should not work differently because the code does not check for it. If there is a bug in isSymbolPreemptible(), then the symbol will be wrongly preemptible or not but TLS DESC code will still work correctly.

quic-seaswara · 2025-08-14T16:37:03Z

test/lld/ELF/riscv-tlsdesc-relax.test

+RUN: %llvm-mc -filetype=obj --defsym PAD=0 -mattr=+c,+relax %p/Inputs/riscv-tlsdesc-relax/a.s -o %t.a.o
+RUN: %llvm-mc -filetype=obj --defsym PAD=5000 -mattr=+c,+relax %p/Inputs/riscv-tlsdesc-relax/a.s -o %t.aa.o
+RUN: %llvm-mc -filetype=obj -mattr=+c,+relax %p/Inputs/riscv-tlsdesc-relax/c.s -o %t.c.o
+RUN: %link %linkopts -shared %t.c.o -o %t.c.so


May be useful to add tests using 'C' code to make it easy to debug ?

The test check for particular instruction at particular offsets which C compiler will not guarantee. Also, some tests have interleave patterns that compiler probably can never generate.

quic-seaswara · 2025-08-14T16:38:13Z

@lenary please review

quic-seaswara · 2025-08-15T13:14:15Z

I would like to see tests using 'C' code as well. You can still keep the asm test.

I would like to see more detailed testing mentioned below :

Basic TLSDESC tests without relaxation (Executable, Shared library, PIE, static linking, symbol visibility tests)
- Probably we want to have a flag --no-tlsdesc-relax
TLSDESC tests with relaxation for the cases above
Please add what relaxations is supported right now, and what is punted for later
- It is not clear from the commit message on what TLS relaxationare included in this patch
Any addition to map file for TLS relaxations will be useful
Test cases with mno-relax as needed
Documentation should follow as a seperate subtask (to know what the linker did for our users)

Please create subtasks as needed and totally I am ok to see the limited support that we add for 21.0 but my focus is to build this feature with more clarity.

cat > 1.c << \!
__attribute__((visibility("hidden"))) __thread int tls_var = 10;
int foo() { return tls_var; }
!

cat > 2.c << \!
extern __thread int tls_var;
int bar() { return tls_var; }
!

clang -c -target riscv32 1.c -fPIC -mtls-dialect=desc
clang -c -target riscv32 2.c -fPIC
# case 1
ld.eld -march riscv32 1.o 2.o -shared -o 1.so
llvm-readelf --dyn-syms 1.so | grep tls_var && echo BADIMAGE
llvm-readelf -r 1.so | grep tls_var && echo BADIMAGE
# case 2 (partial link)
ld.eld -march riscv32 1.o 2.o -r -o r.o
ld.eld -march riscv32 r.o -shared -o 1.so
llvm-readelf --dyn-syms 1.so | grep tls_var && echo BADIMAGE
llvm-readelf -r 1.so | grep tls_var && echo BADIMAGE
# case 3 (no relax)
clang -c -target riscv32 1.c -fPIC -mtls-dialect=desc -mno-relax
clang -c -target riscv32 2.c -fPIC -mno-relax
ld.eld -march riscv32 1.o 2.o -shared -o 1.so
llvm-readelf --dyn-syms 1.so | grep tls_var && echo BADIMAGE
llvm-readelf -r 1.so | grep tls_var && echo BADIMAGE

quic-akaryaki · 2025-08-15T20:48:56Z

I would like to see tests using 'C' code as well. You can still keep the asm test.

I would like to see more detailed testing mentioned below :

Basic TLSDESC tests without relaxation (Executable, Shared library, PIE, static linking, symbol visibility tests)

Probably we want to have a flag --no-tlsdesc-relax

TLSDESC tests with relaxation for the cases above

Please add what relaxations is supported right now, and what is punted for later

It is not clear from the commit message on what TLS relaxationare included in this patch

Any addition to map file for TLS relaxations will be useful

Test cases with mno-relax as needed

Documentation should follow as a seperate subtask (to know what the linker did for our users)

Please create subtasks as needed and totally I am ok to see the limited support that we add for 21.0 but my focus is to build this feature with more clarity.
cat > 1.c << \!
__attribute__((visibility("hidden"))) __thread int tls_var = 10;
int foo() { return tls_var; }
!

cat > 2.c << \!
extern __thread int tls_var;
int bar() { return tls_var; }
!

clang -c -target riscv32 1.c -fPIC -mtls-dialect=desc
clang -c -target riscv32 2.c -fPIC
# case 1
ld.eld -march riscv32 1.o 2.o -shared -o 1.so
llvm-readelf --dyn-syms 1.so | grep tls_var && echo BADIMAGE
llvm-readelf -r 1.so | grep tls_var && echo BADIMAGE
# case 2 (partial link)
ld.eld -march riscv32 1.o 2.o -r -o r.o
ld.eld -march riscv32 r.o -shared -o 1.so
llvm-readelf --dyn-syms 1.so | grep tls_var && echo BADIMAGE
llvm-readelf -r 1.so | grep tls_var && echo BADIMAGE
# case 3 (no relax)
clang -c -target riscv32 1.c -fPIC -mtls-dialect=desc -mno-relax
clang -c -target riscv32 2.c -fPIC -mno-relax
ld.eld -march riscv32 1.o 2.o -shared -o 1.so
llvm-readelf --dyn-syms 1.so | grep tls_var && echo BADIMAGE
llvm-readelf -r 1.so | grep tls_var && echo BADIMAGE

Just to clarify, this patch has full support for TLSDESC, including all 4 types of static relocations, linker optimizations (replacing instructions with nops) and relaxations (removing instructions). Of course, it may have bugs, which is the case with existing code, which could be fixed before the 21 releases. Since new requirements come 1 day before the 21.x feature deadline, it won't go into the release.

Also, please clarify:
-- if --no-tlsdesc-relax is needed
-- which addition to map file for TLS relaxations will be useful

Thank you

quic-seaswara · 2025-08-15T22:15:14Z

Just to clarify, this patch has full support for TLSDESC, including all 4 types of static relocations, linker optimizations (replacing instructions with nops) and relaxations (removing instructions). Of course, it may have bugs, which is the case with existing code, which could be fixed before the 21 releases. Since new requirements come 1 day before the 21.x feature deadline, it won't go into the release.

Yes, sorry!.

My point was to make TLSDESC without relaxation work before relaxations are added.

Also cases such as TLSDESC with symbol visibility is something users typically do. We should look into those cases to make sure that we have tests that show functionality.

Also, please clarify: -- if --no-tlsdesc-relax is needed -- which addition to map file for TLS relaxations will be useful

I think it will be useful to have --no-tlsdesc-relax like --no-gp-relax or --no-relax-c to have a workaround if any.

Map file can be annotated to point out what relaxations were applied.

quic-akaryaki requested review from lenary and quic-seaswara August 6, 2025 19:59

quic-akaryaki force-pushed the riscv-apply-reloc-nfc branch 2 times, most recently from 5108c07 to f371760 Compare August 7, 2025 18:49

Base automatically changed from riscv-apply-reloc-nfc to main August 8, 2025 14:36

quic-akaryaki force-pushed the riscv-tlsdesc branch from 8573ebd to ae80fed Compare August 8, 2025 14:40

partaror reviewed Aug 8, 2025

View reviewed changes

lib/Target/GNULDBackend.cpp Show resolved Hide resolved

lib/Target/GNULDBackend.cpp Outdated Show resolved Hide resolved

quic-akaryaki force-pushed the riscv-tlsdesc branch from ae80fed to e6d8711 Compare August 8, 2025 20:30

[RISCV] Add support for TLSDESC

b165c71

Signed-off-by: Alexey Karyakin <akaryaki@quicinc.com>

quic-akaryaki force-pushed the riscv-tlsdesc branch from e6d8711 to 6112a1c Compare August 13, 2025 23:33

quic-akaryaki requested a review from partaror August 13, 2025 23:35

NFC: Support tests written in the yaml language

ed813f4

Add '.yaml' to config.suffixes and fix tests that had non-test yaml files outside of their Inputs directory. Signed-off-by: Alexey Karyakin <akaryaki@quicinc.com>

quic-akaryaki force-pushed the riscv-tlsdesc branch from 6112a1c to bde6e34 Compare August 14, 2025 00:22

quic-akaryaki force-pushed the riscv-tlsdesc branch from bde6e34 to 04957ed Compare August 14, 2025 01:07

quic-akaryaki marked this pull request as ready for review August 14, 2025 01:15

quic-seaswara reviewed Aug 14, 2025

View reviewed changes

[RISCV] Add support for TLSDESC #307

Are you sure you want to change the base?

[RISCV] Add support for TLSDESC #307

Uh oh!

Conversation

quic-akaryaki commented Aug 6, 2025

Uh oh!

Uh oh!

Uh oh!

quic-akaryaki commented Aug 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quic-seaswara commented Aug 14, 2025

Uh oh!

quic-seaswara commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

quic-akaryaki commented Aug 15, 2025

Uh oh!

quic-seaswara commented Aug 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

quic-seaswara commented Aug 15, 2025 •

edited

Loading