-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[RISCV][lld] Set the type of TLSDESC relocation's referenced local symbol to STT_NOTYPE #85817
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
[RISCV][lld] Set the type of TLSDESC relocation's referenced local symbol to STT_NOTYPE #85817
Conversation
Created using spr 1.3.4
Created using spr 1.3.4 [skip ci]
@llvm/pr-subscribers-backend-risc-v @llvm/pr-subscribers-lld-elf Author: Paul Kirth (ilovepi) ChangesWhen adding fixups for RISCV_TLSDESC_ADD_LO and RISCV_TLSDESC_LOAD_LO, This patch stops adding such fixups and avoid setting the STT_TLS on Full diff: https://github.com/llvm/llvm-project/pull/85817.diff 4 Files Affected:
diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index 619fbaf5dc5452..241a5b82d35f4a 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -1280,7 +1280,6 @@ static unsigned handleTlsRelocation(RelType type, Symbol &sym,
if (config->emachine == EM_MIPS)
return handleMipsTlsRelocation(type, sym, c, offset, addend, expr);
bool isRISCV = config->emachine == EM_RISCV;
-
if (oneof<R_AARCH64_TLSDESC_PAGE, R_TLSDESC, R_TLSDESC_CALL, R_TLSDESC_PC,
R_TLSDESC_GOTPLT>(expr) &&
config->shared) {
@@ -1480,7 +1479,13 @@ template <class ELFT, class RelTy> void RelocationScanner::scanOne(RelTy *&i) {
// Process TLS relocations, including TLS optimizations. Note that
// R_TPREL and R_TPREL_NEG relocations are resolved in processAux.
- if (sym.isTls()) {
+ if (sym.isTls() ||
+ // These RISCV TLSDESC relocations reference a local symbol that won't be
+ // a TLS symbol, but we need to process them in handleTlsRelocation the
+ // same as other TLS relocations.
+ (config->emachine == EM_RISCV &&
+ (type == R_RISCV_TLSDESC_CALL || type == R_RISCV_TLSDESC_LOAD_LO12 ||
+ type == R_RISCV_TLSDESC_ADD_LO12))) {
if (unsigned processed =
handleTlsRelocation(type, sym, *sec, offset, addend, expr)) {
i += processed - 1;
diff --git a/lld/test/ELF/riscv-tlsdesc-relax.s b/lld/test/ELF/riscv-tlsdesc-relax.s
index fb24317e6535ca..5718d4175be113 100644
--- a/lld/test/ELF/riscv-tlsdesc-relax.s
+++ b/lld/test/ELF/riscv-tlsdesc-relax.s
@@ -33,12 +33,14 @@
# GD64-NEXT: c.add a0, tp
# GD64-NEXT: jal {{.*}} <foo>
## &.got[c]-. = 0x20c0+8 - 0x1020 = 0x10a8
+# GD64-LABEL: <.Ltlsdesc_hi1>:
# GD64-NEXT: 1020: auipc a4, 0x1
# GD64-NEXT: ld a5, 0xa8(a4)
# GD64-NEXT: addi a0, a4, 0xa8
# GD64-NEXT: jalr t0, 0x0(a5)
# GD64-NEXT: c.add a0, tp
## &.got[c]-. = 0x20c0+8 - 0x1032 = 0x1096
+# GD64-LABEL: <.Ltlsdesc_hi2>:
# GD64-NEXT: 1032: auipc a6, 0x1
# GD64-NEXT: ld a7, 0x96(a6)
# GD64-NEXT: addi a0, a6, 0x96
@@ -64,6 +66,7 @@
# LE64-NEXT: jal {{.*}} <foo>
# LE64-NEXT: R_RISCV_JAL foo
# LE64-NEXT: R_RISCV_RELAX *ABS*
+# LE64-LABEL: <.Ltlsdesc_hi1>:
# LE64-NEXT: addi a0, zero, 0x7ff
# LE64-NEXT: R_RISCV_TLSDESC_HI20 b
# LE64-NEXT: R_RISCV_RELAX *ABS*
@@ -71,6 +74,7 @@
# LE64-NEXT: R_RISCV_TLSDESC_ADD_LO12 .Ltlsdesc_hi1
# LE64-NEXT: R_RISCV_TLSDESC_CALL .Ltlsdesc_hi1
# LE64-NEXT: c.add a0, tp
+# LE64-LABEL: <.Ltlsdesc_hi2>:
# LE64-NEXT: addi zero, zero, 0x0
# LE64-NEXT: R_RISCV_TLSDESC_HI20 b
# LE64-NEXT: addi zero, zero, 0x0
@@ -93,9 +97,11 @@
# LE64A-NEXT: addi a0, a0, -0x479
# LE64A-NEXT: c.add a0, tp
# LE64A-NEXT: jal {{.*}} <foo>
+# LE64A-LABEL: <.Ltlsdesc_hi1>:
# LE64A-NEXT: lui a0, 0x2
# LE64A-NEXT: addi a0, a0, -0x479
# LE64A-NEXT: c.add a0, tp
+# LE64A-LABEL: <.Ltlsdesc_hi2>:
# LE64A-NEXT: addi zero, zero, 0x0
# LE64A-NEXT: addi zero, zero, 0x0
# LE64A-NEXT: lui a0, 0x2
@@ -115,10 +121,12 @@
# IE64-NEXT: c.add a0, tp
# IE64-NEXT: jal {{.*}} <foo>
## &.got[c]-. = 0x120e0+8 - 0x11018 = 0x10d0
+# IE64-LABEL: <.Ltlsdesc_hi1>:
# IE64-NEXT: 11018: auipc a0, 0x1
# IE64-NEXT: ld a0, 0xd0(a0)
# IE64-NEXT: c.add a0, tp
## &.got[c]-. = 0x120e0+8 - 0x1102a = 0x10be
+# IE64-LABEL: <.Ltlsdesc_hi2>:
# IE64-NEXT: addi zero, zero, 0x0
# IE64-NEXT: addi zero, zero, 0x0
# IE64-NEXT: 1102a: auipc a0, 0x1
diff --git a/lld/test/ELF/riscv-tlsdesc.s b/lld/test/ELF/riscv-tlsdesc.s
index a893d098344d0a..b181f0e9a17a48 100644
--- a/lld/test/ELF/riscv-tlsdesc.s
+++ b/lld/test/ELF/riscv-tlsdesc.s
@@ -30,10 +30,10 @@
# 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: ld.lld -shared -soname=libd.64.so -o libd.64.so d.64.o 2>&1 | FileCheck %s --check-prefix=BADTLSLABEL --allow-empty
# 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
+# RUN: ld.lld -shared -soname=libd.32.so -o libd.32.so d.32.o 2>&1 | FileCheck %s --check-prefix=BADTLSLABEL --allow-empty
# GD64-RELA: .rela.dyn {
# GD64-RELA-NEXT: 0x2408 R_RISCV_TLSDESC - 0x7FF
@@ -74,14 +74,14 @@
# GD64-NEXT: add a0, a0, tp
## &.got[b]-. = 0x23e0+40 - 0x12f4 = 0x1114
-# GD64-NEXT: 12f4: auipc a2, 0x1
+# GD64: 12f4: auipc a2, 0x1
# GD64-NEXT: ld a3, 0x114(a2)
# GD64-NEXT: addi a0, a2, 0x114
# GD64-NEXT: jalr t0, 0x0(a3)
# GD64-NEXT: add a0, a0, tp
## &.got[c]-. = 0x23e0+24 - 0x1308 = 0x10f0
-# GD64-NEXT: 1308: auipc a4, 0x1
+# GD64: 1308: auipc a4, 0x1
# GD64-NEXT: ld a5, 0xf0(a4)
# GD64-NEXT: addi a0, a4, 0xf0
# GD64-NEXT: jalr t0, 0x0(a5)
@@ -89,7 +89,7 @@
# NOREL: no relocations
-# LE64-LABEL: <.text>:
+# LE64-LABEL: <.Ltlsdesc_hi0>:
## st_value(a) = 8
# LE64-NEXT: addi zero, zero, 0x0
# LE64-NEXT: addi zero, zero, 0x0
@@ -97,12 +97,14 @@
# LE64-NEXT: addi a0, zero, 0x8
# LE64-NEXT: add a0, a0, tp
## st_value(b) = 2047
+# LE64-LABEL: <.Ltlsdesc_hi1>:
# LE64-NEXT: addi zero, zero, 0x0
# LE64-NEXT: addi zero, zero, 0x0
# LE64-NEXT: addi zero, zero, 0x0
# LE64-NEXT: addi a0, zero, 0x7ff
# LE64-NEXT: add a0, a0, tp
## st_value(c) = 2048
+# LE64-LABEL: <.Ltlsdesc_hi2>:
# LE64-NEXT: addi zero, zero, 0x0
# LE64-NEXT: addi zero, zero, 0x0
# LE64-NEXT: lui a0, 0x1
@@ -116,18 +118,20 @@
# IE64: .got 00000010 00000000000123a8
## a and b are optimized to use LE. c is optimized to IE.
-# IE64-LABEL: <.text>:
+# IE64-LABEL: <.Ltlsdesc_hi0>:
# IE64-NEXT: addi zero, zero, 0x0
# IE64-NEXT: addi zero, zero, 0x0
# IE64-NEXT: addi zero, zero, 0x0
# IE64-NEXT: addi a0, zero, 0x8
# IE64-NEXT: add a0, a0, tp
+# IE64-LABEL: <.Ltlsdesc_hi1>:
# IE64-NEXT: addi zero, zero, 0x0
# IE64-NEXT: addi zero, zero, 0x0
# IE64-NEXT: addi zero, zero, 0x0
# IE64-NEXT: addi a0, zero, 0x7ff
# IE64-NEXT: add a0, a0, tp
## &.got[c]-. = 0x123a8+8 - 0x112b8 = 0x10f8
+# IE64-LABEL: <.Ltlsdesc_hi2>:
# IE64-NEXT: addi zero, zero, 0x0
# IE64-NEXT: addi zero, zero, 0x0
# IE64-NEXT: 112b8: auipc a0, 0x1
@@ -136,7 +140,7 @@
# IE32: .got 00000008 00012248
-# IE32-LABEL: <.text>:
+# IE32-LABEL: <.Ltlsdesc_hi0>:
## st_value(a) = 8
# IE32-NEXT: addi zero, zero, 0x0
# IE32-NEXT: addi zero, zero, 0x0
@@ -144,20 +148,22 @@
# IE32-NEXT: addi a0, zero, 0x8
# IE32-NEXT: add a0, a0, tp
## st_value(b) = 2047
+# IE32-LABEL: <.Ltlsdesc_hi1>:
# IE32-NEXT: addi zero, zero, 0x0
# IE32-NEXT: addi zero, zero, 0x0
# IE32-NEXT: addi zero, zero, 0x0
# IE32-NEXT: addi a0, zero, 0x7ff
# IE32-NEXT: add a0, a0, tp
## &.got[c]-. = 0x12248+4 - 0x111cc = 0x1080
+# IE32-LABEL: <.Ltlsdesc_hi2>:
# IE32-NEXT: addi zero, zero, 0x0
# IE32-NEXT: addi zero, zero, 0x0
# IE32-NEXT: 111cc: auipc a0, 0x1
# 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
+# At one point the local TLSDESC labels would be marked STT_TLS, so make sure we don't regress
+# BADTLSLABEL-NOT: error: d.{{.*}}.o has an STT_TLS symbol but doesn't have an SHF_TLS section
#--- a.s
.macro load dst, src
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp
index 254a9a4bc0ef0b..b8e0f3a867f402 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp
@@ -207,8 +207,6 @@ void RISCVMCExpr::fixELFSymbolsInTLSFixups(MCAssembler &Asm) const {
case VK_RISCV_TLS_GOT_HI:
case VK_RISCV_TLS_GD_HI:
case VK_RISCV_TLSDESC_HI:
- case VK_RISCV_TLSDESC_ADD_LO:
- case VK_RISCV_TLSDESC_LOAD_LO:
break;
}
|
Created using spr 1.3.4
Created using spr 1.3.4 [skip ci]
Created using spr 1.3.4
Created using spr 1.3.4 [skip ci]
Created using spr 1.3.4
Created using spr 1.3.4 [skip ci]
Created using spr 1.3.4 [skip ci]
Created using spr 1.3.4 [skip ci]
lld/test/ELF/riscv-tlsdesc.s
Outdated
@@ -30,10 +30,10 @@ | |||
# 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: ld.lld -shared -soname=d.64.so -o d.64.so d.64.o 2>&1 | FileCheck %s --check-prefix=BADTLSLABEL --allow-empty |
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.
--allow-empty is an anti-pattern. Avoid.
Replace 2>&1 | ...
with --fatal-warnings
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.
Oh, I wasn't aware. Thanks. Do you think we should update the FileCheck.rst to note that other options/patterns are preferred ... or maybe in the TestingGuide.rst?
lld/test/ELF/riscv-tlsdesc.s
Outdated
## 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 | ||
# At one point the local TLSDESC labels would be marked STT_TLS, so make sure we don't regress | ||
# BADTLSLABEL-NOT: error: d.{{.*}}.o has an STT_TLS symbol but doesn't have an SHF_TLS section |
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.
Delete the NOT pattern. If the diagnostic changes, the NOT pattern may likely not get updated.
#
=> ## Before #85817
lld/ELF/Relocations.cpp
Outdated
@@ -1480,7 +1480,13 @@ template <class ELFT, class RelTy> void RelocationScanner::scanOne(RelTy *&i) { | |||
|
|||
// Process TLS relocations, including TLS optimizations. Note that | |||
// R_TPREL and R_TPREL_NEG relocations are resolved in processAux. | |||
if (sym.isTls()) { | |||
if (sym.isTls() || |
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.
Attach this diff
- if (sym.isTls() ||
- // These RISCV TLSDESC relocations reference a local symbol that won't be
- // a TLS symbol, but we need to process them in handleTlsRelocation the
- // same as other TLS relocations.
- (config->emachine == EM_RISCV &&
- (type == R_RISCV_TLSDESC_CALL || type == R_RISCV_TLSDESC_LOAD_LO12 ||
- type == R_RISCV_TLSDESC_ADD_LO12))) {
+ //
+ // Some RISCV TLSDESC relocations reference a local NOTYPE symbol, but we
+ // need to process them in handleTlsRelocation.
+ if (sym.isTls() || oneof<R_TLSDESC_PC, R_TLSDESC_CALL>(expr)) {
The title is not descriptive. This is primarily a RISC-V side MC issue. Perhaps
|
Created using spr 1.3.4
…mbol to STT_NOTYPE When adding fixups for RISCV_TLSDESC_ADD_LO and RISCV_TLSDESC_LOAD_LO, the local label added for RISCV TLSDESC relocations have STT_TLS set, which is incorrect. Instead, these labels should have `STT_NOTYPE`. This patch stops adding such fixups and avoid setting the STT_TLS on these symbols. Failing to do so can cause LLD to emit an error `has an STT_TLS symbol but doesn't have an SHF_TLS section`. We additionally, adjust how LLD services these relocations to avoid errors with incompatible relocation and symbol types. Reviewers: topperc, MaskRay Reviewed By: MaskRay Pull Request: llvm#85817
Just realized that I don't think we backported this fix. /cherry-pick #85817 |
Error: Command failed due to missing milestone. |
/cherry-pick #85817 |
Failed to cherry-pick: #85817 https://github.com/llvm/llvm-project/actions/runs/8944899494 Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request |
…mbol to STT_NOTYPE When adding fixups for RISCV_TLSDESC_ADD_LO and RISCV_TLSDESC_LOAD_LO, the local label added for RISCV TLSDESC relocations have STT_TLS set, which is incorrect. Instead, these labels should have `STT_NOTYPE`. This patch stops adding such fixups and avoid setting the STT_TLS on these symbols. Failing to do so can cause LLD to emit an error `has an STT_TLS symbol but doesn't have an SHF_TLS section`. We additionally, adjust how LLD services these relocations to avoid errors with incompatible relocation and symbol types. Reviewers: topperc, MaskRay Reviewed By: MaskRay Pull Request: llvm#85817
Failed to cherry-pick: f6f474c, https://github.com/llvm/llvm-project/actions/runs/9024562989 Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request |
…mbol to STT_NOTYPE When adding fixups for RISCV_TLSDESC_ADD_LO and RISCV_TLSDESC_LOAD_LO, the local label added for RISCV TLSDESC relocations have STT_TLS set, which is incorrect. Instead, these labels should have `STT_NOTYPE`. This patch stops adding such fixups and avoid setting the STT_TLS on these symbols. Failing to do so can cause LLD to emit an error `has an STT_TLS symbol but doesn't have an SHF_TLS section`. We additionally, adjust how LLD services these relocations to avoid errors with incompatible relocation and symbol types. Reviewers: topperc, MaskRay Reviewed By: MaskRay Pull Request: llvm#85817 (cherry picked from commit dfe4ca9)
/pull-request #91678 |
…mbol to STT_NOTYPE When adding fixups for RISCV_TLSDESC_ADD_LO and RISCV_TLSDESC_LOAD_LO, the local label added for RISCV TLSDESC relocations have STT_TLS set, which is incorrect. Instead, these labels should have `STT_NOTYPE`. This patch stops adding such fixups and avoid setting the STT_TLS on these symbols. Failing to do so can cause LLD to emit an error `has an STT_TLS symbol but doesn't have an SHF_TLS section`. We additionally, adjust how LLD services these relocations to avoid errors with incompatible relocation and symbol types. Reviewers: topperc, MaskRay Reviewed By: MaskRay Pull Request: llvm#85817 (cherry picked from commit dfe4ca9)
…mbol to STT_NOTYPE When adding fixups for RISCV_TLSDESC_ADD_LO and RISCV_TLSDESC_LOAD_LO, the local label added for RISCV TLSDESC relocations have STT_TLS set, which is incorrect. Instead, these labels should have `STT_NOTYPE`. This patch stops adding such fixups and avoid setting the STT_TLS on these symbols. Failing to do so can cause LLD to emit an error `has an STT_TLS symbol but doesn't have an SHF_TLS section`. We additionally, adjust how LLD services these relocations to avoid errors with incompatible relocation and symbol types. Reviewers: topperc, MaskRay Reviewed By: MaskRay Pull Request: llvm#85817 (cherry picked from commit dfe4ca9)
When adding fixups for RISCV_TLSDESC_ADD_LO and RISCV_TLSDESC_LOAD_LO,
the local label added for RISCV TLSDESC relocations have STT_TLS set,
which is incorrect. Instead, these labels should have
STT_NOTYPE
.This patch stops adding such fixups and avoid setting the STT_TLS on
these symbols. Failing to do so can cause LLD to emit an error
has an STT_TLS symbol but doesn't have an SHF_TLS section
. We additionally,adjust how LLD services these relocations to avoid errors with
incompatible relocation and symbol types.