Discussion: atrosinenko/pauth-blend-removal #1

atrosinenko · 2025-11-24T15:18:06Z

Here is a list of open questions, in case corresponding comments disappear as a result of rebase:

link "Not directly related to this PR, but might be a subject for change: it looks like that PointerAuthenticationMode::SignAndStrip is not used at all..."
link Whether CodeGenModule::getPointerAuthOtherDiscriminator should return unsigned or uint16_t (the same question applies to its callees).
link Non-trivial computation of constant modifier.
- should probably be fixed separately, updated FIXME with more details
link 1, link 2 Switching ConstantPtrAuth to an arbitrary tuple of i64 parameters.
link Add negative test cases checking that unsupported ptrauth bundles are rejected by the backend
link Overall test formatting
link Fix for TableGen should probably be sent beforehand

kovdan01 · 2025-11-24T15:25:50Z

llvm/test/CodeGen/RISCV/rv32zknd-intrinsic-autoupgrade.ll

Could you please explain why do we need this change?

Sorry, this was merely a note to myself, I will file an issue / implement a fix and drop this change.

kovdan01 · 2025-11-24T15:27:47Z

llvm/test/Verifier/operand-bundles.ll

Could you please explain why the behavior has changed?

IIRC this test file uses an old form of "clang.arc.attachedcall"() operand bundle - this is probably unintentional. This test turned out to be affected by a change to the logic of auto-upgrading operand bundles.

kovdan01 · 2025-11-24T15:31:27Z

llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp

    return Error::success();
  }

+  if (VTy.getMachineValueType() == MVT::Untyped)


Could you please explain why do we need this condition and which particular test case triggers it?

Hmm, this is a fix for a rather obscure bug in TableGen that prevented me from defining ptrauth_bundle SDNode. I probably have to send this fix beforehand - maybe this is not even a bug, but an intended behavior - I would have to fix my SDNode definitions then.

(link added to the description)

kovdan01 · 2025-11-24T15:34:36Z

llvm/test/Verifier/ptrauth-operand-bundles.ll

-; CHECK-NEXT: call void %arg2() [ "ptrauth"(i64 42, i64 120) ]
-  call void %arg2() [ "ptrauth"(i64 42, i64 120) ]
+; CHECK-NOT: call void %ok()
+  call void %ok() [ "ptrauth"(i32 42, i64 120) ]   ; OK


The first operand here and on the line below is i32 - is this considered OK? BTW, on the line 31 we have a similar situation with i32, but it's treated as an error there. Should we do the same here?

"ptrauth"(i32, i64) is a correct old-style form of ptrauth bundles (because it has exactly two operands), which is auto-upgraded when the *.ll file is being read.

kovdan01 · 2025-11-24T15:35:38Z

llvm/test/Verifier/ptrauth-operand-bundles.ll

+; CHECK-NEXT: call void %arg.ptr() [ "ptrauth"(i64 42, i64 120, i32 123) ]
+  call void %arg.ptr() [ "ptrauth"(i64 42, i64 120, i32 123) ]
+
+; Note that for compatibility reasons the first operand (originally, "the key ID")


Nit: when we have a major test update, it might be worth using ;; prefix for comments which are actual comments and not FileCheck directives.

(link added to the description)

Are there any pre-existing discussion I could look up to not to introduce yet another almost correct code style? :)

kovdan01 · 2025-11-24T16:11:28Z

clang/lib/CodeGen/ItaniumCXXABI.cpp

+    auto *NonVirtualDiscriminator =
+        Builder.getInt64(AuthInfo.getIntDiscriminator());
+    assert(!AuthInfo.getAddrDiscriminator());
+    // FIXME This does not involve call to @llvm.ptrauth.blend(), but such


Could you please explain what do you particularly mean by "such usage of constant modifier is unsafe"?

I assume that choosing the discriminator conditionally may be susceptible to unintentional spilling of intermediate values to the stack.

(link added to the description)

I investigated this a bit more and updated the FIXME. ItaniumCXXABI::EmitLoadOfMemberFunctionPointer returns a CGCallee result carrying both the (potentially signed) pointer to a function together with the schema to authenticate at the call site. Furthermore, EmitLoadOfMemberFunctionPointer is a virtual function. It is thus hardly possible to safely emit two separate authentications with hardcoded integer constants, as it would make things even worse by exposing a completely unprotected function pointer instead of the discriminator value. A better option would probably be re-signing the pointer to the same non-zero discriminator on the "FnVirtual" code path, but this requires further investigation not to introduce some other vulnerability.

kovdan01 · 2025-11-24T16:18:20Z

clang/lib/CodeGen/CGCall.cpp

+      callee, args, allBundles, name);
  call->setCallingConv(getRuntimeCC());

+  // FIXME Attach "convergencectrl" bundle right away instead of re-creating


Could you please refer me to the tests which trigger this convergence-related condition?

I tried adding an assertion that the result of cast<llvm::CallInst>(addConvergenceControlToken(call)) equals to call (which should fail on the condition mentioned in this FIXME), here is the list of tests:

Clang :: CodeGenHLSL/GlobalDestructors.hlsl Clang :: CodeGenHLSL/builtins/GroupMemoryBarrierWithGroupSync.hlsl Clang :: CodeGenHLSL/builtins/WaveActiveAllTrue.hlsl Clang :: CodeGenHLSL/builtins/WaveActiveAnyTrue.hlsl Clang :: CodeGenHLSL/builtins/WaveActiveCountBits.hlsl Clang :: CodeGenHLSL/builtins/WaveActiveMax.hlsl Clang :: CodeGenHLSL/builtins/WaveActiveMin.hlsl Clang :: CodeGenHLSL/builtins/WaveActiveSum.hlsl Clang :: CodeGenHLSL/builtins/WaveReadLaneAt.hlsl Clang :: CodeGenHLSL/builtins/wave_get_lane_count.hlsl Clang :: CodeGenHLSL/builtins/wave_get_lane_index_do_while.hlsl Clang :: CodeGenHLSL/builtins/wave_get_lane_index_simple.hlsl Clang :: CodeGenHLSL/builtins/wave_get_lane_index_subcall.hlsl Clang :: CodeGenHLSL/builtins/wave_is_first_lane.hlsl Clang :: CodeGenHLSL/convergence/global_array.hlsl Clang :: CodeGenHLSL/resources/RWBuffer-constructor-opt.hlsl Clang :: CodeGenHLSL/resources/RWBuffer-imageformat.hlsl Clang :: CodeGenHLSL/resources/StructuredBuffers-constructors.hlsl Clang :: CodeGenHLSL/resources/StructuredBuffers-elementtype.hlsl Clang :: CodeGenHLSL/resources/StructuredBuffers-subscripts.hlsl Clang :: CodeGenHLSL/resources/TypedBuffers-elementtype.hlsl Clang :: CodeGenHLSL/resources/TypedBuffers-subscript.hlsl Clang :: CodeGenHLSL/resources/default_cbuffer.hlsl Clang :: CodeGenHLSL/resources/res-array-global-multi-dim.hlsl Clang :: CodeGenHLSL/resources/res-array-global-unbounded.hlsl Clang :: CodeGenHLSL/resources/res-array-global.hlsl Clang :: CodeGenHLSL/vk-features/vk.spec-constant.hlsl Clang :: CodeGenHLSL/vk-input-builtin.hlsl Clang :: CodeGenHLSL/vk_binding_attr.hlsl

kovdan01 · 2025-11-24T16:24:39Z

clang/test/Sema/ptrauth-blend.c

Do we expect existing tests like clang/test/Sema/ptrauth-intrinsics-macro.c now become failing with the same Standalone blend builtin is not supported error?

Hmm, I thought I have updated all the tests as needed... I do expect that some code that was previously supported may be rejected now, but the in-tree tests are not expected to fail.

kovdan01 · 2025-11-24T16:25:37Z

clang/test/Sema/ptrauth-blend.c

@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios -S -verify -fptrauth-intrinsics %s -fexperimental-new-constant-interpreter
+
+typedef __UINTPTR_TYPE__ uintptr_t;


Nit: why do we need this typedef?

This typedef introduces a familiar platform-independent type name without the need to #include any headers.

kovdan01 · 2025-11-24T16:27:01Z

clang/test/Sema/ptrauth-blend.c

@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios -S -verify -fptrauth-intrinsics %s -fexperimental-new-constant-interpreter


Could you please explain which purpose does -fexperimental-new-constant-interpreter serve? I suppose that the error message should not be dependent on such flags and we should get the same result w/o the flag, shouldn't we?

I'm not very familiar with state-of-the-art frontend testing, so this option was probably copied from a similar test file :)

Dropped this option in 2c20897.

kovdan01 · 2025-12-01T16:33:13Z

llvm/lib/IR/Constants.cpp

                                            const DataLayout &DL) const {
-  // If the keys are different, there's no chance for this to be compatible.
-  if (getKey() != Key)
+  // FIXME: Generalize ptrauth constants to arbitrary schemas instead of


It looks like that it's crucial to resolve this FIXME before presenting these changes for public discussion, isn't it?

If supporting arbitrary BundleOperands array is too challenging for some reason, I suppose we should at least not assume that we have always use the exact same meaning of int/address discriminators. So as for me, it might be OK to limit BundleOperands.size() (while leaving FIXME to support arbitrary size in future), but it does not look OK to treat specific operands as specific discriminator values with their AArch64-specific meaning.

Please let me know your thoughts on this.

(link added to the description)

kovdan01 · 2025-12-01T16:38:23Z

llvm/lib/IR/Constants.cpp


 ConstantPtrAuth *ConstantPtrAuth::get(Constant *Ptr, ConstantInt *Key,
                                      ConstantInt *Disc, Constant *AddrDisc) {
+  // FIXME Should we simply enforce i64 instead?


Probably enforcing i64 is a good idea since it would make aarch64-specific ptrauth constants

BTW, is there a reason why ConstantPtrAuth is AArch64-specific or can we make it "generic" with arbitrary amount of i64 parameters as well? When we need to access that in AArch64-specific manner in code, we might just use some kind of wrapper which would ensure that we have expected amount of parameters in the constant and they fit expected width.

But I might be missing smth - would be glad if you could explain how you see the situation.

I would rather make ConstantPtrAuth target-independent as well, but this requires further investigation. One of my concerns is whether binary encoding of IR constants is generic enough to transparently switch from a fixed structure of fields to generic tuples of i64.

(link added to the description)

atrosinenko

Added 2c20897 and 5a97aa4 on top of the existing commits (i.e. fast-forward push). I prepared a new version with somewhat simplified history (keeping exactly the same final patch), I will send it in a few moments via a separate push to simplify the review.

atrosinenko · 2025-12-02T18:26:53Z

clang/lib/CodeGen/CGCall.cpp

+      callee, args, allBundles, name);
  call->setCallingConv(getRuntimeCC());

+  // FIXME Attach "convergencectrl" bundle right away instead of re-creating


I tried adding an assertion that the result of cast<llvm::CallInst>(addConvergenceControlToken(call)) equals to call (which should fail on the condition mentioned in this FIXME), here is the list of tests:

Clang :: CodeGenHLSL/GlobalDestructors.hlsl Clang :: CodeGenHLSL/builtins/GroupMemoryBarrierWithGroupSync.hlsl Clang :: CodeGenHLSL/builtins/WaveActiveAllTrue.hlsl Clang :: CodeGenHLSL/builtins/WaveActiveAnyTrue.hlsl Clang :: CodeGenHLSL/builtins/WaveActiveCountBits.hlsl Clang :: CodeGenHLSL/builtins/WaveActiveMax.hlsl Clang :: CodeGenHLSL/builtins/WaveActiveMin.hlsl Clang :: CodeGenHLSL/builtins/WaveActiveSum.hlsl Clang :: CodeGenHLSL/builtins/WaveReadLaneAt.hlsl Clang :: CodeGenHLSL/builtins/wave_get_lane_count.hlsl Clang :: CodeGenHLSL/builtins/wave_get_lane_index_do_while.hlsl Clang :: CodeGenHLSL/builtins/wave_get_lane_index_simple.hlsl Clang :: CodeGenHLSL/builtins/wave_get_lane_index_subcall.hlsl Clang :: CodeGenHLSL/builtins/wave_is_first_lane.hlsl Clang :: CodeGenHLSL/convergence/global_array.hlsl Clang :: CodeGenHLSL/resources/RWBuffer-constructor-opt.hlsl Clang :: CodeGenHLSL/resources/RWBuffer-imageformat.hlsl Clang :: CodeGenHLSL/resources/StructuredBuffers-constructors.hlsl Clang :: CodeGenHLSL/resources/StructuredBuffers-elementtype.hlsl Clang :: CodeGenHLSL/resources/StructuredBuffers-subscripts.hlsl Clang :: CodeGenHLSL/resources/TypedBuffers-elementtype.hlsl Clang :: CodeGenHLSL/resources/TypedBuffers-subscript.hlsl Clang :: CodeGenHLSL/resources/default_cbuffer.hlsl Clang :: CodeGenHLSL/resources/res-array-global-multi-dim.hlsl Clang :: CodeGenHLSL/resources/res-array-global-unbounded.hlsl Clang :: CodeGenHLSL/resources/res-array-global.hlsl Clang :: CodeGenHLSL/vk-features/vk.spec-constant.hlsl Clang :: CodeGenHLSL/vk-input-builtin.hlsl Clang :: CodeGenHLSL/vk_binding_attr.hlsl

atrosinenko · 2025-12-02T18:34:46Z

clang/lib/CodeGen/CGPointerAuthInfo.h

+  unsigned IsIsaPointer : 1;
+  unsigned AuthenticatesNullValues : 1;
+  unsigned Key : 2;
+  unsigned IntDiscriminator : 16;


Fixed IntDiscriminator crossing the byte boundary in 2c20897

atrosinenko · 2025-12-02T18:37:29Z

clang/lib/CodeGen/CGClass.cpp

      }
    } else {
      VTable = cast<llvm::Instruction>(EmitPointerAuthAuth(
          CGPointerAuthInfo(0, PointerAuthenticationMode::Strip, false, false,


(link added to the description)

atrosinenko · 2025-12-02T18:37:47Z

clang/lib/CodeGen/CGPointerAuth.cpp

 /// Given a pointer-authentication schema, return a concrete "other"
 /// discriminator for it.
-llvm::ConstantInt *CodeGenModule::getPointerAuthOtherDiscriminator(
+unsigned CodeGenModule::getPointerAuthOtherDiscriminator(


(link added to the description)

atrosinenko · 2025-12-02T18:38:15Z

clang/lib/CodeGen/ItaniumCXXABI.cpp

+    auto *NonVirtualDiscriminator =
+        Builder.getInt64(AuthInfo.getIntDiscriminator());
+    assert(!AuthInfo.getAddrDiscriminator());
+    // FIXME This does not involve call to @llvm.ptrauth.blend(), but such


(link added to the description)

atrosinenko · 2025-12-02T19:10:07Z

llvm/test/CodeGen/AArch64/ptrauth-bti-call.ll

 ; CHECK-NEXT:  braaz x16
-define i32 @test_tailcall_ia_0(ptr %arg0) #0 {
-  %tmp0 = tail call i32 %arg0() [ "ptrauth"(i32 0, i64 0) ]
+define i32 @test_tailcall_ia_0(i32 ()* %arg0) #0 {


Unintentionally rolled back the conversion to untyped pointers. Fixed in 2c20897.

atrosinenko · 2025-12-02T19:12:26Z

llvm/test/CodeGen/AArch64/ptrauth-call-rv-marker.ll


-declare ptr @foo0(i32)
-declare ptr @foo1()
+declare i8* @foo0(i32)


Fixed in 2c20897.

atrosinenko · 2025-12-02T19:20:10Z

llvm/test/Verifier/ptrauth-operand-bundles.ll

+  call void %ok() [ "ptrauth"(i32 42, i64 120) ]   ; OK
+  call void %ok() [ "ptrauth"(i32 42, i64 %arg.64) ] ; OK
+  call void %ok() [ "ptrauth"(i64 %arg.64, i64 123) ] ; OK
+  call void %ok() [ "ptrauth"(i64 %arg.64, i64 123, i64 %arg.64, i64 42) ] ; OK


(link added to the description)

atrosinenko · 2025-12-02T19:21:14Z

llvm/test/Verifier/ptrauth-operand-bundles.ll

+; CHECK-NEXT: call void %arg.ptr() [ "ptrauth"(i64 42, i64 120, i32 123) ]
+  call void %arg.ptr() [ "ptrauth"(i64 42, i64 120, i32 123) ]
+
+; Note that for compatibility reasons the first operand (originally, "the key ID")


(link added to the description)

atrosinenko · 2025-12-02T19:22:30Z

llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp

    return Error::success();
  }

+  if (VTy.getMachineValueType() == MVT::Untyped)


(link added to the description)

atrosinenko · 2025-12-03T10:21:45Z

@kovdan01 The above force-push reorganized the commit history without changing the file tree (i.e. output of git diff 5a97aa4 a2dc74c is empty). Please note that by this time I should have replied to every your comment at least once (at least by adding the comment link to the description of this PR) - if I missed any comment, please ping me back.

* Three-operand bundle always implies blend (OK) * Two-operand bundle has three modes: - immediate integer only (OK) - raw value (OK) - result of llvm.ptrauth.blend (compatibility, will be removed later) At ISel time this means PtrAuthInfo-style structs are converted from Key+Discr to homogenous vectors of `Register`s or `SDValue`s with the second operation mode added at the instruction legalization site.

Always use three-operand bundles for simplicity (except for ptrauth.strip). Change operand order from (key, addr, int) to (key, int, addr) to reflect operand order of ptrauth constants. More cleanup.

atrosinenko · 2025-12-03T17:57:37Z

Rebased onto current mainline. This branch turned out to conflict with a recently merged deactivation symbols implementation: llvm@6227eb9. By the way, that commit seems to implement ISD::DEACTIVATION_SYMBOL untyped SDNode - its implementation seems to solve mostly the same problems using a slightly different approach than the one used for ISD::PtrAuthBundle in this prototype. I wonder if it is possible to simplify the code by combining these ideas.

atrosinenko changed the title ~~Atrosinenko/pauth blend removal~~ atrosinenko/pauth-blend-removal Nov 24, 2025

atrosinenko changed the title ~~atrosinenko/pauth-blend-removal~~ Discussion: atrosinenko/pauth-blend-removal Nov 24, 2025

kovdan01 reviewed Nov 24, 2025

View reviewed changes

kovdan01 reviewed Dec 1, 2025

View reviewed changes

atrosinenko commented Dec 2, 2025

View reviewed changes

atrosinenko force-pushed the atrosinenko/pauth-blend-removal branch from 5a97aa4 to a2dc74c Compare December 2, 2025 19:56

atrosinenko added 23 commits December 3, 2025 13:22

Update the docs

53a6590

Support D-keys in auth call pseudos

9f7b13f

Switch ptrauth_strip to bundles

dd6dc25

--> deactivation-symbol Switch auth/sign/resign to bundles

280fe0f

More fixes for deactivation symbols after rebase

00b6349

Update the tests in preparation for dropping blend() calls

fe2adda

Do not emit blend() in frontend-generated code

4288098

Do not emit temporary blend intrinsic when processing builtins

ea798a7

libunwind: do not store the result of blend() to temporaries

af616a9

Normalize operands of ptrauth bundle

8847b2d

Fix ConstantPtrAuth::isKnownCompatibleWith

3aef7e3

Removing compatibility hacks from ISel

7b8adca

Update InstCombine tests

30aa963

Implement auto-upgrade

0b8df8c

Upgrade key operand of ptrauth bundle to i64

47b92c7

Upgrade various tests

1a26e4e

CGPointerAuthInfo: use null discriminator components by default

621dc28

Update Verifier and fix the test

d314ed4

Add negative test cases for auto-upgrading of ptrauth

7a1cc15

Check that unused llvm.ptrauth.blend is silently eliminated

0d0073d

Misc cleanup

b744611

WIP: [LLVM] Adjust ptrauth operand bundles

238e726

Always use three-operand bundles for simplicity (except for ptrauth.strip). Change operand order from (key, addr, int) to (key, int, addr) to reflect operand order of ptrauth constants. More cleanup.

atrosinenko added 4 commits December 3, 2025 20:26

LLVM: more cleanups

5160d99

WIP: Clang: misc cleanup

804a5b5

IMPORTANT FIX: add earlyclobber constraint to AUTxMxN

2020667

Add/update InstCombine test cases

f84201d

atrosinenko force-pushed the atrosinenko/pauth-blend-removal branch from a2dc74c to f84201d Compare December 3, 2025 17:43

Fix target-specific bundle validation and add tests

31f049f

atrosinenko force-pushed the atrosinenko/pauth-blend-removal branch from 260894f to 31f049f Compare December 3, 2025 20:22

Clarify the FIXME in EmitLoadOfMemberFunctionPointer

1715021

		@@ -0,0 +1,13 @@
		// RUN: %clang_cc1 -triple arm64-apple-ios -S -verify -fptrauth-intrinsics %s -fexperimental-new-constant-interpreter

		typedef __UINTPTR_TYPE__ uintptr_t;

Discussion: atrosinenko/pauth-blend-removal #1

Are you sure you want to change the base?

Discussion: atrosinenko/pauth-blend-removal #1

Uh oh!

Conversation

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

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

atrosinenko left a comment

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

atrosinenko commented Nov 24, 2025 •

edited

Loading

atrosinenko Dec 2, 2025 •

edited

Loading

atrosinenko Dec 2, 2025 •

edited

Loading

atrosinenko Dec 2, 2025 •

edited

Loading