This went through so many rebases that the incremental diff is not very helpful ;) Let's start over on github: https://github.com/llvm/llvm-project/pull/65996

Added the relevant bits of the great documentation John wrote in clang/docs/PointerAuthentication.rst. I tried to trim it down to only the general principles and the intrinsics; some of the discussion around the security model is a little ahead of the rest of the changes, but is still seems fine with these initial changes.

We tried to address this problem by adding an XPACIuntied pseudo, only selecting that instead of the real instruction, and later (after regalloc, which is now unburdened by the tied nature of XPAC, like you're discussing) expanding it to add the mov if necessary:

Please don’t. I’ll send out updated patches.

In D132384#4405529, @atrosinenko wrote:

I am planning to land this patch the next week, if there will be no objections.

A comment inline, but LGTM otherwise, thanks!

This makes sense, thanks for checking! I'll have a closer look on my side and follow-up if needed

Sorry for the delay; LGTM, thanks!

To make sure I follow: would it make sense to have the legality check compare the ptrauth bits, and if they match, remove the bundle and promote? We'd have to pass the pointer (not the Function) around. And I don't see how we'd get a signed pointer in the SampleProfile case, so we wouldn't promote there anyway (without a much more interesting target check at least?) But it seems like the other users would generate a direct ptrauth-bundle call to ptrauth-signed target; InstCombine should handle that.

In D134658#3815804, @jroelofs wrote:

In D134658#3815787, @tschuett wrote:

Apple revealed:

#define CPUFAMILY_ARM_BLIZZARD_AVALANCHE 0xda33d83d

link?

I didn't see it in https://opensource.apple.com/source/xnu/xnu-7195.81.3/osfmk/mach/machine.h.auto.html

but in AArch64TargetParser.def, marking them v8.6 brings in support for the SM4 cryptographic hash and we don't actually have that

• add test for resign+auth -> auth

• further constrain GISel immediate blends to fail if wider than 16 bits

• Extract out XPACIuntied, leaving it for ptrauth-returns.
• Pull in key/opcode helper functions from later patch in the series.

Yep, committed c68b469e0788, thanks!

Hi @nikic, I may have hit an issue with this change: when the noop-bitcast result is already used in other (potentially already selected) blocks, we already assigned a vreg for the value, and it may be too late to change it when selecting this bitcast. Concretely, this repro fails for x86_64 with -verify-machineinstrs, because the second use of %tmp1 uses a never-defined vreg:

Sure, filed 54914 to cherry-pick into 14.0.2

D'oh, this fell through the cracks, sorry folks! I committed the fix I had in mind (cfa4fe7c5187). Long story short: this change exposed an old bug in the LOH pass: it wasn't honoring regmasks in bundled MIs, but since we used to split the bundle and emit the attached call separately, the (unbundled) latter was still visible to the pass. That's not true as of this change. I think for the sort of code that uses bundles on aarch64 it's usually selectors that get corrupted in a later msgSend, and that ends up exploding in dyld the way we're seeing in both cases.

Reading this again, I have a feeling defaulting to newest will come back to bite us (or, well, apple folks.) But we can reevaluate if we get to that. So, LGTM, thanks!

The apple-* bits LGTM, thanks! @pcc caught a couple more in rGa8a3a43792, I still need to go over those as well

Ah, further down, there's a different crash signature in bisect6.txt, in dyld strlen: that one does ring a bell. I assume it's handling a corrupt selector, but there's not much context to know for sure. I have a later change that may take care of that, hopefully that resolves your original crash as well!

In D118214#3393603, @phosek wrote:

@ab We started seeing crashes in Flutter Engine on iOS after the latest Clang roll. We did a bisect and found this change to be a culprit, see https://github.com/flutter/flutter/issues/99488 for more details. Would it be possible to take a look?

Thanks!

In D120455#3352886, @dexonsmith wrote:

I don't have a strong opinion; IMO -mllvm options shouldn't get used (except by compiler developers) since they're unstable, although I'm sure they get used a little anyway. IMO, if this makes it easier for the intended users (compiler developers) then there isn't a real downside to it.

@ab @t.p.northover @Gerolf , any thoughts from you?

In D119788#3331324, @keith wrote:

Thanks for the tips! I left the default as generic here, but I totally take your point on defaulting to the newest and I'm happy to update if you feel strongly, lmk what you think

I committed a modified description, happy to make more changes:

In D118214#3274486, @ahatanak wrote:

Are there any users who need the changes in autoupgrade? clang.arc.attachedcall bundles without operands are emitted only after ARC contract (ARC optimizer does't remove the operand), so it seems safe not to upgrade the IR.

Same, makes sense, thanks!

Quite the old TODO, nice ;) LGTM!

Makes sense, LGTM!

Thanks all for the reviews! 68854f4e572a

Hey folks! Any more comments?