Skip to content

[PAC][lld][AArch64][ELF] Support signed TLSDESC #113817

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 13 commits into from
Jan 22, 2025

Conversation

kovdan01
Copy link
Contributor

@kovdan01 kovdan01 commented Oct 27, 2024

Depends on #120010

Support R_AARCH64_AUTH_TLSDESC_ADR_PAGE21, R_AARCH64_AUTH_TLSDESC_LD64_LO12
and R_AARCH64_AUTH_TLSDESC_LD64_LO12 static relocations and
R_AARCH64_AUTH_TLSDESC dynamic relocation. IE/LE optimization is not
currently supported for AUTH TLSDESC.

Copy link
Contributor Author

kovdan01 commented Oct 27, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

@llvmbot
Copy link
Member

llvmbot commented Oct 28, 2024

@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-lld

Author: Daniil Kovalev (kovdan01)

Changes

Depends on #113813

Support R_AARCH64_AUTH_TLSDESC_ADR_PAGE21, R_AARCH64_AUTH_TLSDESC_LD64_LO12
and R_AARCH64_AUTH_TLSDESC_LD64_LO12 static relocations and
R_AARCH64_AUTH_TLSDESC dynamic relocation. IE/LE optimization is not
currently supported for AUTH TLSDESC.


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

7 Files Affected:

  • (modified) lld/ELF/Arch/AArch64.cpp (+8)
  • (modified) lld/ELF/InputSection.cpp (+2)
  • (modified) lld/ELF/Relocations.cpp (+36-2)
  • (modified) lld/ELF/Relocations.h (+4)
  • (modified) lld/ELF/Symbols.h (+1)
  • (modified) lld/ELF/SyntheticSections.cpp (+5)
  • (added) lld/test/ELF/aarch64-tlsdesc-pauth.s (+134)
diff --git a/lld/ELF/Arch/AArch64.cpp b/lld/ELF/Arch/AArch64.cpp
index 86f509f3fd78a7..8ad466bf49878b 100644
--- a/lld/ELF/Arch/AArch64.cpp
+++ b/lld/ELF/Arch/AArch64.cpp
@@ -157,9 +157,14 @@ RelExpr AArch64::getRelExpr(RelType type, const Symbol &s,
     return R_AARCH64_AUTH;
   case R_AARCH64_TLSDESC_ADR_PAGE21:
     return R_AARCH64_TLSDESC_PAGE;
+  case R_AARCH64_AUTH_TLSDESC_ADR_PAGE21:
+    return R_AARCH64_AUTH_TLSDESC_PAGE;
   case R_AARCH64_TLSDESC_LD64_LO12:
   case R_AARCH64_TLSDESC_ADD_LO12:
     return R_TLSDESC;
+  case R_AARCH64_AUTH_TLSDESC_LD64_LO12:
+  case R_AARCH64_AUTH_TLSDESC_ADD_LO12:
+    return RelExpr::R_AARCH64_AUTH_TLSDESC;
   case R_AARCH64_TLSDESC_CALL:
     return R_TLSDESC_CALL;
   case R_AARCH64_TLSLE_ADD_TPREL_HI12:
@@ -543,6 +548,7 @@ void AArch64::relocate(uint8_t *loc, const Relocation &rel,
   case R_AARCH64_ADR_PREL_PG_HI21:
   case R_AARCH64_TLSIE_ADR_GOTTPREL_PAGE21:
   case R_AARCH64_TLSDESC_ADR_PAGE21:
+  case R_AARCH64_AUTH_TLSDESC_ADR_PAGE21:
     checkInt(ctx, loc, val, 33, rel);
     [[fallthrough]];
   case R_AARCH64_ADR_PREL_PG_HI21_NC:
@@ -593,6 +599,7 @@ void AArch64::relocate(uint8_t *loc, const Relocation &rel,
   case R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC:
   case R_AARCH64_TLSLE_LDST64_TPREL_LO12_NC:
   case R_AARCH64_TLSDESC_LD64_LO12:
+  case R_AARCH64_AUTH_TLSDESC_LD64_LO12:
     checkAlignment(ctx, loc, val, 8, rel);
     write32Imm12(loc, getBits(val, 3, 11));
     break;
@@ -667,6 +674,7 @@ void AArch64::relocate(uint8_t *loc, const Relocation &rel,
     break;
   case R_AARCH64_TLSLE_ADD_TPREL_LO12_NC:
   case R_AARCH64_TLSDESC_ADD_LO12:
+  case R_AARCH64_AUTH_TLSDESC_ADD_LO12:
     write32Imm12(loc, val);
     break;
   case R_AARCH64_TLSDESC:
diff --git a/lld/ELF/InputSection.cpp b/lld/ELF/InputSection.cpp
index ccc7cf8c6e2de9..b3303c59a3b4a5 100644
--- a/lld/ELF/InputSection.cpp
+++ b/lld/ELF/InputSection.cpp
@@ -935,12 +935,14 @@ uint64_t InputSectionBase::getRelocTargetVA(Ctx &ctx, const Relocation &r,
   case R_SIZE:
     return r.sym->getSize() + a;
   case R_TLSDESC:
+  case RelExpr::R_AARCH64_AUTH_TLSDESC:
     return ctx.in.got->getTlsDescAddr(*r.sym) + a;
   case R_TLSDESC_PC:
     return ctx.in.got->getTlsDescAddr(*r.sym) + a - p;
   case R_TLSDESC_GOTPLT:
     return ctx.in.got->getTlsDescAddr(*r.sym) + a - ctx.in.gotPlt->getVA();
   case R_AARCH64_TLSDESC_PAGE:
+  case R_AARCH64_AUTH_TLSDESC_PAGE:
     return getAArch64Page(ctx.in.got->getTlsDescAddr(*r.sym) + a) -
            getAArch64Page(p);
   case R_LOONGARCH_TLSDESC_PAGE_PC:
diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index 2d3815e58b5f67..966088aca77669 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -1355,6 +1355,36 @@ unsigned RelocationScanner::handleTlsRelocation(RelExpr expr, RelType type,
     return 1;
   }
 
+  auto fatalBothAuthAndNonAuth = [&sym]() {
+    fatal("both AUTH and non-AUTH TLSDESC entries for '" + sym.getName() +
+          "' requested, but only one type of TLSDESC entry per symbol is "
+          "supported");
+  };
+
+  // Do not optimize signed TLSDESC as described in pauthabielf64 to LE/IE.
+  // https://github.com/ARM-software/abi-aa/blob/main/pauthabielf64/pauthabielf64.rst#general-restrictions
+  // > PAUTHELF64 only supports the descriptor based TLS (TLSDESC).
+  if (oneof<R_AARCH64_AUTH_TLSDESC_PAGE, RelExpr::R_AARCH64_AUTH_TLSDESC>(
+          expr)) {
+    assert(ctx.arg.emachine == EM_AARCH64);
+    if (!sym.hasFlag(NEEDS_TLSDESC))
+      sym.setFlags(NEEDS_TLSDESC | NEEDS_TLSDESC_AUTH);
+    else if (!sym.hasFlag(NEEDS_TLSDESC_AUTH))
+      fatalBothAuthAndNonAuth();
+    sec->addReloc({expr, type, offset, addend, &sym});
+    return 1;
+  }
+
+  if (sym.hasFlag(NEEDS_TLSDESC_AUTH)) {
+    assert(ctx.arg.emachine == EM_AARCH64);
+    // TLSDESC_CALL hint relocation probably should not be emitted by compiler
+    // with signed TLSDESC enabled since it does not give any value, but leave a
+    // check against that just in case someone uses it.
+    if (expr != R_TLSDESC_CALL)
+      fatalBothAuthAndNonAuth();
+    return 1;
+  }
+
   bool isRISCV = ctx.arg.emachine == EM_RISCV;
 
   if (oneof<R_AARCH64_TLSDESC_PAGE, R_TLSDESC, R_TLSDESC_CALL, R_TLSDESC_PC,
@@ -1855,9 +1885,13 @@ void elf::postScanRelocations(Ctx &ctx) {
 
     if (flags & NEEDS_TLSDESC) {
       got->addTlsDescEntry(sym);
+      RelType tlsDescRel = ctx.target->tlsDescRel;
+      if (flags & NEEDS_TLSDESC_AUTH) {
+        assert(ctx.arg.emachine == EM_AARCH64);
+        tlsDescRel = ELF::R_AARCH64_AUTH_TLSDESC;
+      }
       ctx.mainPart->relaDyn->addAddendOnlyRelocIfNonPreemptible(
-          ctx.target->tlsDescRel, *got, got->getTlsDescOffset(sym), sym,
-          ctx.target->tlsDescRel);
+          tlsDescRel, *got, got->getTlsDescOffset(sym), sym, tlsDescRel);
     }
     if (flags & NEEDS_TLSGD) {
       got->addDynTlsEntry(sym);
diff --git a/lld/ELF/Relocations.h b/lld/ELF/Relocations.h
index 20d88de402ac18..2fe05068948aa9 100644
--- a/lld/ELF/Relocations.h
+++ b/lld/ELF/Relocations.h
@@ -92,6 +92,10 @@ enum RelExpr {
   R_AARCH64_PAGE_PC,
   R_AARCH64_RELAX_TLS_GD_TO_IE_PAGE_PC,
   R_AARCH64_TLSDESC_PAGE,
+  R_AARCH64_AUTH_TLSDESC_PAGE,
+  // TODO: maybe it's better to rename this expression
+  // to avoid name conflict with dynamic reloc
+  R_AARCH64_AUTH_TLSDESC,
   R_AARCH64_AUTH,
   R_ARM_PCA,
   R_ARM_SBREL,
diff --git a/lld/ELF/Symbols.h b/lld/ELF/Symbols.h
index 470bae23832d7d..326e82ad6cccbf 100644
--- a/lld/ELF/Symbols.h
+++ b/lld/ELF/Symbols.h
@@ -55,6 +55,7 @@ enum {
   NEEDS_GOT_DTPREL = 1 << 7,
   NEEDS_TLSIE = 1 << 8,
   NEEDS_GOT_AUTH = 1 << 9,
+  NEEDS_TLSDESC_AUTH = 1 << 10,
 };
 
 // The base class for real symbol classes.
diff --git a/lld/ELF/SyntheticSections.cpp b/lld/ELF/SyntheticSections.cpp
index 0cf45e51bc58a8..f3cf6b5aebe1de 100644
--- a/lld/ELF/SyntheticSections.cpp
+++ b/lld/ELF/SyntheticSections.cpp
@@ -672,6 +672,11 @@ bool GotSection::addTlsDescEntry(const Symbol &sym) {
   assert(sym.auxIdx == ctx.symAux.size() - 1);
   ctx.symAux.back().tlsDescIdx = numEntries;
   numEntries += 2;
+  if (sym.hasFlag(NEEDS_TLSDESC_AUTH)) {
+    assert(ctx.arg.emachine == EM_AARCH64);
+    authEntries.push_back({(numEntries - 2) * ctx.arg.wordsize, true});
+    authEntries.push_back({(numEntries - 1) * ctx.arg.wordsize, false});
+  }
   return true;
 }
 
diff --git a/lld/test/ELF/aarch64-tlsdesc-pauth.s b/lld/test/ELF/aarch64-tlsdesc-pauth.s
new file mode 100644
index 00000000000000..36505f652b0c52
--- /dev/null
+++ b/lld/test/ELF/aarch64-tlsdesc-pauth.s
@@ -0,0 +1,134 @@
+// REQUIRES: aarch64
+// RUN: rm -rf %t && split-file %s %t && cd %t
+
+//--- a.s
+
+.section .tbss,"awT",@nobits
+.global a
+a:
+.xword 0
+
+//--- ok.s
+
+// RUN: llvm-mc -filetype=obj -triple=aarch64-pc-linux -mattr=+pauth ok.s -o ok.o
+// RUN: ld.lld -shared ok.o -o ok.so
+// RUN: llvm-objdump --no-print-imm-hex -d --no-show-raw-insn ok.so | \
+// RUN:   FileCheck -DP=20 -DA=896 -DB=912 -DC=928 %s
+// RUN: llvm-readobj -r -x .got ok.so | FileCheck --check-prefix=REL \
+// RUN:   -DP1=20 -DA1=380 -DB1=390 -DC1=3A0 -DP2=020 -DA2=380 -DB2=390 -DC2=3a0 %s
+
+// RUN: llvm-mc -filetype=obj -triple=aarch64-pc-linux -mattr=+pauth a.s -o a.so.o
+// RUN: ld.lld -shared a.so.o -soname=so -o a.so
+// RUN: ld.lld ok.o a.so -o ok
+// RUN: llvm-objdump --no-print-imm-hex -d --no-show-raw-insn ok | \
+// RUN:   FileCheck -DP=220 -DA=936 -DB=952 -DC=968 %s
+// RUN: llvm-readobj -r -x .got ok | FileCheck --check-prefix=REL \
+// RUN:   -DP1=220 -DA1=3A8 -DB1=3B8 -DC1=3C8 -DP2=220 -DA2=3a8 -DB2=3b8 -DC2=3c8 %s
+
+        .text
+        adrp    x0, :tlsdesc_auth:a
+        ldr     x16, [x0, :tlsdesc_auth_lo12:a]
+        add     x0, x0, :tlsdesc_auth_lo12:a
+        .tlsdesccall a
+        blraa   x16, x0
+
+// CHECK:      adrp    x0, 0x[[P]]000
+// CHECK-NEXT: ldr     x16, [x0, #[[A]]]
+// CHECK-NEXT: add     x0, x0, #[[A]]
+// CHECK-NEXT: blraa   x16, x0
+
+// Create relocation against local TLS symbols where linker should
+// create target specific dynamic TLSDESC relocation where addend is
+// the symbol VMA in tls block.
+
+        adrp    x0, :tlsdesc_auth:local1
+        ldr     x16, [x0, :tlsdesc_auth_lo12:local1]
+        add     x0, x0, :tlsdesc_auth_lo12:local1
+        .tlsdesccall local1
+        blraa   x16, x0
+
+// CHECK:      adrp    x0, 0x[[P]]000
+// CHECK-NEXT: ldr     x16, [x0, #[[B]]]
+// CHECK-NEXT: add     x0, x0, #[[B]]
+// CHECK-NEXT: blraa   x16, x0
+
+        adrp    x0, :tlsdesc_auth:local2
+        ldr     x16, [x0, :tlsdesc_auth_lo12:local2]
+        add     x0, x0, :tlsdesc_auth_lo12:local2
+        .tlsdesccall local2
+        blraa   x16, x0
+
+// CHECK:      adrp    x0, 0x[[P]]000
+// CHECK-NEXT: ldr     x16, [x0, #[[C]]]
+// CHECK-NEXT: add     x0, x0, #[[C]]
+// CHECK-NEXT: blraa   x16, x0
+
+        .section .tbss,"awT",@nobits
+        .type   local1,@object
+        .p2align 2
+local1:
+        .word   0
+        .size   local1, 4
+
+        .type   local2,@object
+        .p2align 3
+local2:
+        .xword  0
+        .size   local2, 8
+
+
+// R_AARCH64_AUTH_TLSDESC - 0x0 -> start of tls block
+// R_AARCH64_AUTH_TLSDESC - 0x8 -> align (sizeof (local1), 8)
+
+// REL:      Relocations [
+// REL-NEXT:   Section (5) .rela.dyn {
+// REL-NEXT:     0x[[P1]][[B1]] R_AARCH64_AUTH_TLSDESC - 0x0
+// REL-NEXT:     0x[[P1]][[C1]] R_AARCH64_AUTH_TLSDESC - 0x8
+// REL-NEXT:     0x[[P1]][[A1]] R_AARCH64_AUTH_TLSDESC a 0x0
+// REL-NEXT:   }
+// REL-NEXT: ]
+
+// REL:      Hex dump of section '.got':
+// REL-NEXT: 0x00[[P2]][[A2]] 00000000 00000080 00000000 000000a0
+// REL-NEXT: 0x00[[P2]][[B2]] 00000000 00000080 00000000 000000a0
+// REL-NEXT: 0x00[[P2]][[C2]] 00000000 00000080 00000000 000000a0
+//                                           ^^
+//                                           0b10000000 bit 63 address diversity = true, bits 61..60 key = IA
+//                                                             ^^
+//                                                             0b10100000 bit 63 address diversity = true, bits 61..60 key = DA
+
+//--- err1.s
+
+// RUN: llvm-mc -filetype=obj -triple=aarch64-pc-linux -mattr=+pauth err1.s -o err1.o
+// RUN: not ld.lld -shared err1.o -o err1.so 2>&1 | FileCheck --check-prefix=ERR1 %s
+// ERR1: error: both AUTH and non-AUTH TLSDESC entries for 'a' requested, but only one type of TLSDESC entry per symbol is supported
+        .text
+        adrp    x0, :tlsdesc_auth:a
+        ldr     x16, [x0, :tlsdesc_auth_lo12:a]
+        add     x0, x0, :tlsdesc_auth_lo12:a
+        .tlsdesccall a
+        blraa   x16, x0
+
+        adrp    x0, :tlsdesc:a
+        ldr     x1, [x0, :tlsdesc_lo12:a]
+        add     x0, x0, :tlsdesc_lo12:a
+        .tlsdesccall a
+        blr     x1
+
+//--- err2.s
+
+// RUN: llvm-mc -filetype=obj -triple=aarch64-pc-linux -mattr=+pauth err2.s -o err2.o
+// RUN: not ld.lld -shared err2.o -o err2.so 2>&1 | FileCheck --check-prefix=ERR2 %s
+// ERR2: error: both AUTH and non-AUTH TLSDESC entries for 'a' requested, but only one type of TLSDESC entry per symbol is supported
+        .text
+        adrp    x0, :tlsdesc:a
+        ldr     x1, [x0, :tlsdesc_lo12:a]
+        add     x0, x0, :tlsdesc_lo12:a
+        .tlsdesccall a
+        blr     x1
+
+        adrp    x0, :tlsdesc_auth:a
+        ldr     x16, [x0, :tlsdesc_auth_lo12:a]
+        add     x0, x0, :tlsdesc_auth_lo12:a
+        .tlsdesccall a
+        blraa   x16, x0

@kovdan01 kovdan01 force-pushed the users/kovdan01/pauth-signed-got-lld branch from 0d23449 to 38d5936 Compare November 1, 2024 10:33
@kovdan01 kovdan01 force-pushed the users/kovdan01/pauth-signed-tlsdesc-lld branch from 48a2a0d to e3837fa Compare November 1, 2024 10:34
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.

I think this is basically good from my perspective, but I'd like one of the longtime LLD maintainers, and maybe someone more experienced w/ PAC to chime in before landing. Maybe @smithp35, @MaskRay, or @kbeyls have some thoughts?

@smithp35
Copy link
Collaborator

smithp35 commented Nov 4, 2024

Just got back from vacation today. I plan to create a PR for the ABI tomorrow. Will take a look at this patch tomorrow.

Copy link
Collaborator

@smithp35 smithp35 left a comment

Choose a reason for hiding this comment

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

A few small suggestions from me.

@@ -1352,6 +1352,36 @@ unsigned RelocationScanner::handleTlsRelocation(RelExpr expr, RelType type,
return 1;
}

auto fatalBothAuthAndNonAuth = [&sym]() {
fatal("both AUTH and non-AUTH TLSDESC entries for '" + sym.getName() +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add getLocation to the error message so that a user can find the source of at least one of the relocations?

I also recommend not using fatal but use error so that a user can get diagnostics like the map file out of the link.

Copy link
Member

@MaskRay MaskRay Nov 9, 2024

Choose a reason for hiding this comment

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

We now use Fatal(ctx) << .... Unlike fatal, Fatal executes exit but is not noreturn.

Fatal should generally be avoided in favor of Err. You will need to make sure lld doesn't crash when it keeps execution, though the output is not required to be functional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now use Fatal(ctx) << .... Unlike fatal, Fatal executes exit but is not noreturn.

Fatal should generally be avoided in favor of Err. You will need to make sure lld doesn't crash when it keeps execution, though the output is not required to be functional.

Thanks! Switched from fatal to Err(ctx). After emitting an error, we return the number of processed relocations (which is 1), and do not add any entries to GOT synthetic section. So, I don't expect lld to crash, and it should be OK to use Err instead of Fatal here. See 105213d

Can you add getLocation to the error message so that a user can find the source of at least one of the relocations?

Added that, thanks! See 105213d

R_AARCH64_AUTH_TLSDESC_PAGE,
// TODO: maybe it's better to rename this expression
// to avoid name conflict with dynamic reloc
R_AARCH64_AUTH_TLSDESC,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be R_AARCH64_AUTH_TLSDESC_STATIC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's fine for me, but it would be glad to see @MaskRay 's opinion on rel exprs naming conventions :)

Copy link
Member

Choose a reason for hiding this comment

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

Apologies for the slow response.
I propose that we rename these RelExpr enumerators to REL_*
#118424

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies for the slow response. I propose that we rename these RelExpr enumerators to REL_* #118424

Thanks @MaskRay! Updated #113815, #113816, #113817 to follow this naming convention

// CHECK-NEXT: add x0, x0, #[[A]]
// CHECK-NEXT: blraa x16, x0

// Create relocation against local TLS symbols where linker should
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think LLD has a convention to put an additional comment character for lines that are not RUN or FileCheck comments. For example:
/// Create relocation ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for bringing attention to this, forgot to follow the convention here. Fixed in 4b78eae

@kovdan01 kovdan01 force-pushed the users/kovdan01/pauth-signed-got-lld branch from 38d5936 to 42ca2d9 Compare November 10, 2024 17:43
@kovdan01 kovdan01 force-pushed the users/kovdan01/pauth-signed-tlsdesc-lld branch 2 times, most recently from 105213d to dce37de Compare November 10, 2024 19:01
@kovdan01 kovdan01 force-pushed the users/kovdan01/pauth-signed-got-lld branch from 1621897 to bf7d8a9 Compare November 18, 2024 05:33
@kovdan01 kovdan01 force-pushed the users/kovdan01/pauth-signed-tlsdesc-lld branch from dce37de to ecff903 Compare November 18, 2024 12:03
@kovdan01 kovdan01 force-pushed the users/kovdan01/pauth-signed-got-lld branch from bf7d8a9 to fd0c868 Compare November 25, 2024 00:40
@kovdan01 kovdan01 force-pushed the users/kovdan01/pauth-signed-tlsdesc-lld branch from ecff903 to b14271d Compare November 25, 2024 00:40
Base automatically changed from users/kovdan01/pauth-signed-got-lld to main December 17, 2024 07:23
@kovdan01 kovdan01 force-pushed the users/kovdan01/pauth-signed-tlsdesc-lld branch from 5d1c993 to 30b5529 Compare December 17, 2024 08:04
@kovdan01
Copy link
Contributor Author

@MaskRay Would be glad to see your feedback on this. It would be nice to get this merged before llvm20 release.

@kovdan01
Copy link
Contributor Author

kovdan01 commented Jan 6, 2025

@MaskRay would be glad to see your feedback on this

@kovdan01 kovdan01 force-pushed the users/kovdan01/pauth-signed-tlsdesc-lld branch from 30b5529 to d0dc91b Compare January 12, 2025 20:45
@kovdan01
Copy link
Contributor Author

@MaskRay Would be glad to see your feedback on this. It would be nice to get this merged before llvm20 release.

@@ -1847,10 +1861,21 @@ void elf::postScanRelocations(Ctx &ctx) {
GotSection *got = ctx.in.got.get();

if (flags & NEEDS_TLSDESC) {
if ((flags & NEEDS_TLSDESC_AUTH) && (flags & NEEDS_TLSDESC_NONAUTH)) {
auto diag = Err(ctx);
diag << "both AUTH and non-AUTH TLSDESC entries for '" << sym.getName()
Copy link
Member

Choose a reason for hiding this comment

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

Delete diag. Use Err(ctx) << ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted, thanks, see d67040d

@@ -1328,6 +1328,20 @@ unsigned RelocationScanner::handleTlsRelocation(RelExpr expr, RelType type,
return 1;
}

// Do not optimize signed TLSDESC (as described in pauthabielf64 to LE/IE).
Copy link
Member

Choose a reason for hiding this comment

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

I am concerned that AArch64 AUTH has quite intrusive changes to this function. It seems that we should make this entirely target-specific now.

Copy link
Contributor Author

@kovdan01 kovdan01 Jan 13, 2025

Choose a reason for hiding this comment

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

Please let me know if d67040d addresses your concern or if smth else should be done.

I've moved these two if statements to handleAArch64PAuthTlsRelocation helper which is called in case of ctx.arg.emachine == EM_AARCH64 at the beginning of handleTlsRelocation. Also, added a check against AArch64 when adding NEEDS_TLSDESC_NONAUTH flag (to ensure that we do not change the behavior for other targets; this check is not necessary right now though).

This still changes handleTlsRelocation and I'm not sure if it addresses your comment, but, it looks like that if we try to create a completely separate handler for AArch64, it would require code duplication inside that handler and also we'll have an additional if statement against AArch64 when calling handleTlsRelocation from scanOne, and this looks undesirable as well.

@kovdan01 kovdan01 requested a review from MaskRay January 13, 2025 05:41
@kovdan01
Copy link
Contributor Author

@MaskRay Please let me know if d67040d addresses your comments

@kovdan01
Copy link
Contributor Author

@MaskRay Please let me know your thoughts on this

@@ -1337,7 +1363,9 @@ unsigned RelocationScanner::handleTlsRelocation(RelExpr expr, RelType type,
// set NEEDS_TLSDESC on the label.
if (expr != R_TLSDESC_CALL) {
if (!isRISCV || type == R_RISCV_TLSDESC_HI20)
Copy link
Member

Choose a reason for hiding this comment

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

if (aarch64) ...;
else if (!isRISCV || type == ...) ...;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 5f2ee82, thanks

@kovdan01 kovdan01 force-pushed the users/kovdan01/pauth-signed-tlsdesc-lld branch from d67040d to 5f2ee82 Compare January 22, 2025 07:20
Copy link
Contributor Author

kovdan01 commented Jan 22, 2025

Merge activity

  • Jan 22, 4:15 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Jan 22, 4:18 AM EST: A user merged this pull request with Graphite.

@kovdan01 kovdan01 merged commit 9178708 into main Jan 22, 2025
8 checks passed
@kovdan01 kovdan01 deleted the users/kovdan01/pauth-signed-tlsdesc-lld branch January 22, 2025 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants