[AArch64][PAC][GISel] Add missing clobbering info to LOADgotAUTH #157433

atrosinenko · 2025-09-08T11:55:59Z

When LOADgotAUTH is selected by GlobalISel, the existing MachineInstr is updated in-place instead of constructing a fresh instance by calling MIB.buildInstr(). This way, implicit-def operands have to be added manually for machine passes to take them into account.

This patch fixes miscompilation possibility observed when compiling with GlobalISel enabled or at -O0 optimization level.

atrosinenko · 2025-09-08T12:11:03Z

Here is an example of incorrect code generation:

extern unsigned long pac_field_mask;
static void f() {}

unsigned long get_non_signed_function_pointer(void) {
  return ((unsigned long)f) & ~pac_field_mask;
}

Compiled at -O0 optimization level with clang -target aarch64-linux-pauthtest -march=v8.3-a -Xclang -fptrauth-elf-got -S -o- example.c:

get_non_signed_function_pointer:        // @get_non_signed_function_pointer
	.cfi_startproc
// %bb.0:                               // %entry
                                        // implicit-def: $x16
                                        // implicit-def: $x17
	adrp	x16, f
	add	x16, x16, :lo12:f
	paciza	x16
	adrp	x17, :got_auth:pac_field_mask
	add	x17, x17, :got_auth_lo12:pac_field_mask
	ldr	x16, [x17]
	autda	x16, x17
	mov	x17, x16
	xpacd	x17
	cmp	x16, x17
	b.eq	.Lauth_success_0
	brk	#0xc472
.Lauth_success_0:
	mov	x8, x16
	ldr	x8, [x8]
	bic	x0, x16, x8
	ret

With this patch applied

get_non_signed_function_pointer:        // @get_non_signed_function_pointer
	.cfi_startproc
// %bb.0:                               // %entry
	sub	sp, sp, #16
	.cfi_def_cfa_offset 16
                                        // implicit-def: $x16
                                        // implicit-def: $x17
	adrp	x16, f
	add	x16, x16, :lo12:f
	paciza	x16
	str	x16, [sp, #8]                   // 8-byte Folded Spill
	adrp	x17, :got_auth:pac_field_mask
	add	x17, x17, :got_auth_lo12:pac_field_mask
	ldr	x16, [x17]
	autda	x16, x17
	mov	x17, x16
	xpacd	x17
	cmp	x16, x17
	b.eq	.Lauth_success_0
	brk	#0xc472
.Lauth_success_0:
	mov	x8, x16
	ldr	x16, [sp, #8]                   // 8-byte Folded Reload
	ldr	x8, [x8]
	bic	x0, x16, x8
	add	sp, sp, #16
	.cfi_def_cfa_offset 0
	ret

Compiled with -fglobal-isel with clang -target aarch64-linux-pauthtest -march=v8.3-a -Xclang -fptrauth-elf-got -S -o- example.c -O1 -fglobal-isel:

get_non_signed_function_pointer:        // @get_non_signed_function_pointer
	.cfi_startproc
// %bb.0:                               // %entry
	adrp	x16, f
	add	x16, x16, :lo12:f
	paciza	x16
	adrp	x17, :got_auth:pac_field_mask
	add	x17, x17, :got_auth_lo12:pac_field_mask
	ldr	x16, [x17]
	autda	x16, x17
	mov	x17, x16
	xpacd	x17
	cmp	x16, x17
	b.eq	.Lauth_success_0
	brk	#0xc472
.Lauth_success_0:
	mov	x8, x16
	ldr	x8, [x8]
	bic	x0, x16, x8
	ret

With this patch applied

get_non_signed_function_pointer:        // @get_non_signed_function_pointer
	.cfi_startproc
// %bb.0:                               // %entry
	adrp	x16, f
	add	x16, x16, :lo12:f
	paciza	x16
	mov	x8, x16
	adrp	x17, :got_auth:pac_field_mask
	add	x17, x17, :got_auth_lo12:pac_field_mask
	ldr	x16, [x17]
	autda	x16, x17
	mov	x17, x16
	xpacd	x17
	cmp	x16, x17
	b.eq	.Lauth_success_0
	brk	#0xc472
.Lauth_success_0:
	mov	x9, x16
	ldr	x9, [x9]
	bic	x0, x8, x9
	ret

llvmbot · 2025-09-08T12:11:59Z

@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-backend-aarch64

Author: Anatoly Trosinenko (atrosinenko)

Changes

When LOADgotAUTH is selected by GlobalISel, the existing MachineInstr is updated in-place instead of constructing a fresh instance by calling MIB.buildInstr(). This way, implicit-def operands have to be added manually for register allocator to take them into account.

This patch fixes miscompilation possibility observed when compiling with GlobalISel enabled or at -O0 optimization level.

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

2 Files Affected:

(modified) llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp (+8-3)
(added) llvm/test/CodeGen/AArch64/GlobalISel/ptrauth-elf-got.mir (+23)

diff --git a/llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp b/llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
index 5748556d07285..b1843d92e2a93 100644
--- a/llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
+++ b/llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
@@ -2914,10 +2914,15 @@ bool AArch64InstructionSelector::select(MachineInstr &I) {
     }
 
     if (OpFlags & AArch64II::MO_GOT) {
-      I.setDesc(TII.get(MF.getInfo<AArch64FunctionInfo>()->hasELFSignedGOT()
-                            ? AArch64::LOADgotAUTH
-                            : AArch64::LOADgot));
+      bool GOTIsSigned = MF.getInfo<AArch64FunctionInfo>()->hasELFSignedGOT();
+      I.setDesc(TII.get(GOTIsSigned ? AArch64::LOADgotAUTH : AArch64::LOADgot));
       I.getOperand(1).setTargetFlags(OpFlags);
+      if (GOTIsSigned) {
+        MachineInstrBuilder MIB(MF, I);
+        MIB.addDef(AArch64::X16, RegState::Implicit);
+        MIB.addDef(AArch64::X17, RegState::Implicit);
+        MIB.addDef(AArch64::NZCV, RegState::Implicit);
+      }
     } else if (TM.getCodeModel() == CodeModel::Large &&
                !TM.isPositionIndependent()) {
       // Materialize the global using movz/movk instructions.
diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/ptrauth-elf-got.mir b/llvm/test/CodeGen/AArch64/GlobalISel/ptrauth-elf-got.mir
new file mode 100644
index 0000000000000..faf2cb8221ec7
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/ptrauth-elf-got.mir
@@ -0,0 +1,23 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 4
+# RUN: llc -O0 -mtriple=aarch64-linux-gnu -relocation-model=pic -run-pass=instruction-select -global-isel-abort=1 -verify-machineinstrs %s -o - | FileCheck %s
+
+--- |
+  @var_got = external global i8
+  define ptr @loadgotauth_implicit_defs() { ret ptr null }
+
+  !llvm.module.flags = !{!0}
+  !0 = !{i32 8, !"ptrauth-elf-got", i32 1}
+...
+
+---
+name:            loadgotauth_implicit_defs
+legalized:       true
+regBankSelected: true
+body:             |
+  bb.0:
+    ; CHECK-LABEL: name: loadgotauth_implicit_defs
+    ; CHECK: [[LOADgotAUTH:%[0-9]+]]:gpr64common = LOADgotAUTH target-flags(aarch64-got) @var_got, implicit-def $x16, implicit-def $x17, implicit-def $nzcv
+    ; CHECK-NEXT: $x0 = COPY [[LOADgotAUTH]]
+    %0:gpr(p0) = G_GLOBAL_VALUE @var_got
+    $x0 = COPY %0(p0)
+...

arsenm · 2025-09-08T15:20:44Z

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

      I.getOperand(1).setTargetFlags(OpFlags);
+      if (GOTIsSigned) {
+        MachineInstrBuilder MIB(MF, I);
+        MIB.addDef(AArch64::X16, RegState::Implicit);


Use addImplicitDefUseOperands, or build a fresh instruction instead of hardcoding this list of registers?

Updated in 79230eb, thank you!

kovdan01

LGTM when objections from other reviewers are resolved

kovdan01 · 2025-09-08T16:07:09Z

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

-      I.setDesc(TII.get(MF.getInfo<AArch64FunctionInfo>()->hasELFSignedGOT()
-                            ? AArch64::LOADgotAUTH
-                            : AArch64::LOADgot));
+      bool GOTIsSigned = MF.getInfo<AArch64FunctionInfo>()->hasELFSignedGOT();


Nit: maybe IsGOTSigned would look better (having flag identifiers starting with 'is', 'has', ... seems to be a common convention).

This is completely optional and feel free to ignore.

Sounds reasonable, thanks!

atrosinenko · 2025-09-08T20:10:46Z

Restarted Build and Test Linux job as LLDB test failures seem unrelated to this PR.

UPD:
Restarting the failed job did not help.
Restarting all jobs did not help.
According to the downloadable logs, there are two failed (unit?) tests:

lldb-api :: functionalities/asan/TestMemoryHistory.py
lldb-api :: functionalities/asan/TestReportData.py

with two failed test cases in each:

TestReportData.AsanTestReportDataCase.test_dwarf
TestReportData.AsanTestReportDataCase.test_dwo
TestMemoryHistory.MemoryHistoryTestCase.test_compiler_rt_asan_dwarf
TestMemoryHistory.MemoryHistoryTestCase.test_compiler_rt_asan_dwo

Furthermore, both Attempt 1 and Attempt 3 mention the same commit hash in its "Fetching the repository" build step:

  /usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=2 origin +967dbe9ad59019d2162af8d2ed22a3d77260e731:refs/remotes/pull/157433/merge
  From https://github.com/llvm/llvm-project
   * [new ref]             967dbe9ad59019d2162af8d2ed22a3d77260e731 -> pull/157433/merge

I will try rebasing and re-pushing my branch to trigger a completely fresh re-run of the tests (assuming it may be the mainline commit that fails those tests).

When LOADgotAUTH is selected by GlobalISel, the existing MachineInstr is updated in-place instead of constructing a fresh instance by calling MIB.buildInstr(). This way, implicit-def operands have to be added manually for register allocator to take them into account. This patch fixes miscompilation possibility observed when compiling with GlobalISel enabled or at -O0 optimization level.

atrosinenko · 2025-09-09T09:48:12Z

It finally helped to push the rebased branch to trigger a completely fresh run of the tests.

arsenm · 2025-09-09T10:08:30Z

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

+      bool IsGOTSigned = MF.getInfo<AArch64FunctionInfo>()->hasELFSignedGOT();
+      I.setDesc(TII.get(IsGOTSigned ? AArch64::LOADgotAUTH : AArch64::LOADgot));
      I.getOperand(1).setTargetFlags(OpFlags);
+      if (IsGOTSigned)


Just do it unconditionally

Updated in 4ef4947.

atrosinenko · 2025-09-22T13:46:49Z

Ping.

atrosinenko · 2025-09-25T09:07:45Z

/cherry-pick cff5a43

llvmbot · 2025-09-25T09:14:12Z

/pull-request #160668

…m#157433) When LOADgotAUTH is selected by GlobalISel, the existing MachineInstr is updated in-place instead of constructing a fresh instance by calling MIB.buildInstr(). This way, implicit-def operands have to be added manually for machine passes to take them into account. This patch fixes miscompilation possibility observed when compiling with GlobalISel enabled or at -O0 optimization level.

atrosinenko requested review from asl, ahmedbougacha, pcc, ojhunt, kbeyls and kovdan01 September 8, 2025 11:55

atrosinenko marked this pull request as ready for review September 8, 2025 12:11

llvmbot added backend:AArch64 llvm:globalisel labels Sep 8, 2025

arsenm reviewed Sep 8, 2025

View reviewed changes

kovdan01 reviewed Sep 8, 2025

View reviewed changes

atrosinenko added 2 commits September 9, 2025 12:19

Address review comments

cafdddb

atrosinenko force-pushed the users/atrosinenko/gisel-loadgotauth branch from 79230eb to cafdddb Compare September 9, 2025 09:19

arsenm reviewed Sep 9, 2025

View reviewed changes

Call addImplicitDefUseOperands unconditionally

4ef4947

aemerson approved these changes Sep 22, 2025

View reviewed changes

atrosinenko merged commit cff5a43 into main Sep 23, 2025
9 checks passed

atrosinenko deleted the users/atrosinenko/gisel-loadgotauth branch September 23, 2025 14:56

atrosinenko added this to the LLVM 21.x Release milestone Sep 25, 2025

github-project-automation bot added this to LLVM Release Status Sep 25, 2025

github-project-automation bot moved this to Needs Triage in LLVM Release Status Sep 25, 2025

llvmbot moved this from Needs Triage to Done in LLVM Release Status Sep 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[AArch64][PAC][GISel] Add missing clobbering info to LOADgotAUTH #157433

[AArch64][PAC][GISel] Add missing clobbering info to LOADgotAUTH #157433

Uh oh!

atrosinenko commented Sep 8, 2025 •

edited

Loading

Uh oh!

atrosinenko commented Sep 8, 2025

Uh oh!

llvmbot commented Sep 8, 2025 •

edited

Loading

Uh oh!

arsenm Sep 8, 2025

Uh oh!

atrosinenko Sep 8, 2025

Uh oh!

kovdan01 left a comment

Uh oh!

kovdan01 Sep 8, 2025

Uh oh!

atrosinenko Sep 8, 2025

Uh oh!

atrosinenko commented Sep 8, 2025 •

edited

Loading

Uh oh!

atrosinenko commented Sep 9, 2025

Uh oh!

arsenm Sep 9, 2025

Uh oh!

atrosinenko Sep 15, 2025

Uh oh!

atrosinenko commented Sep 22, 2025

Uh oh!

Uh oh!

atrosinenko commented Sep 25, 2025

Uh oh!

llvmbot commented Sep 25, 2025

Uh oh!

Uh oh!

[AArch64][PAC][GISel] Add missing clobbering info to LOADgotAUTH #157433

[AArch64][PAC][GISel] Add missing clobbering info to LOADgotAUTH #157433

Uh oh!

Conversation

atrosinenko commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atrosinenko commented Sep 8, 2025

Uh oh!

llvmbot commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsenm Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

atrosinenko Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

kovdan01 left a comment

Choose a reason for hiding this comment

Uh oh!

kovdan01 Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

atrosinenko Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

atrosinenko commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atrosinenko commented Sep 9, 2025

Uh oh!

arsenm Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

atrosinenko Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

atrosinenko commented Sep 22, 2025

Uh oh!

Uh oh!

atrosinenko commented Sep 25, 2025

Uh oh!

llvmbot commented Sep 25, 2025

Uh oh!

Uh oh!

atrosinenko commented Sep 8, 2025 •

edited

Loading

llvmbot commented Sep 8, 2025 •

edited

Loading

atrosinenko commented Sep 8, 2025 •

edited

Loading