Thank you for the review. I read through the comments - they are already addressed except maybe for a few FPAC-related notes, so I added a FIXME to checkAuthenticatedLR function and left a note in the discussion of D156784.

Added FIXME mentioning FEAT_FPAC to AArch64PointerAuth::checkAuthenticatedLR.

As discussed in D156716, it is not clear if I have to add FeatureFPAC to every relevant CPU. Maybe it is worth conservatively assuming that this feature should only be enabled manually by the user as a precaution against "I have CPU core X but it is not listed, so let's use cpu=Y because X supports all the instructions supported by Y (but not FEAT_FPAC)" - that would not cause any obvious run-time crashes under normal operation, but would make the code less secure.

Moved AddressCheckPseudoSourceValue to AArch64SubtargetInfo.

Address the review comments.

Addressed the comments so far.

Updated the patch, thank you.

Updated the patch:

Here is a summary of the contents of this patch:

• implemented a standalone llvm::AArch64PAuth::checkAuthenticatedRegister utility function to emit one of a number of checks in case a pointer is AUT'ed and not immediately used for memory access
  • placed this function into a sub-namespace instead of making it a static class member, so I don't have to put otherwise irrelevant AArch64PointerAuth class definition to header file
  • note that the checks that are inserted by checkAuthenticatedRegister function are not specific to tail calls (but some of the checks may have restrictions - such as requiring AuthenticatedReg == LR because XPACLRI is encoded as HINT while generic XPAC* instructions require FEAT_PAUTH)
• hooked it to AArch64PointerAuth class via checkAuthenticatedLR method dedicated to hardening tail calls
• in machine outliner, update the costs computed by AArch64InstrInfo::getOutliningCandidateInfo method on a best-effort basis:
  • in MachineOutlinerTailCall outlining mode, we need to insert checks in each caller of OUTLINED_FUNCTION
  • in MachineOutlinerThunk mode, at most a single extra check is inserted in OUTLINED_FUNCTION itself
  • other modes do not introduce new tail calls, so let's just try to account for the checks that would possibly be inserted later into the original candidates by the AArch64PointerAuth pass (including in the two aboves modes)
• factored out isTailCallReturnInst and needsShadowCallStackPrologueEpilogue utility functions

Rebased and updated the patch a bit more:

• in getOutliningCandidateInfo, adjusted SequenceSize variable before its first use
• updated code comments
• skipped checking LR if Scadow Call Stack is enabled: the LR value just before TCRETURN* instruction is anyway not the one produced by AUT* (and we cannot check right after AUT* because we cannot be sure which register is usable as a temporary)

Updated after the D159357 was changed.

Addressed the review comments, rebased.

In D159357#4638040, @vhscampos wrote:

I don't understand the motivation here. This moves PAuth's instruction insertion later down the pipeline, but why? The handling in the machine outliner isn't made any simpler IMO.

• Updated the patch based on the changes from D159357
• Made the check opt-in for subtargets, "none" by default, with command line option to override
• Fixed use after free in createCheckMemOperand function (the same way it is done in Hexagon)

Reworked the patch

• replaced "use a fast checker or not" boolean argument with multiple choices
• created a separate source file and put checkAuthenticatedRegister() function there as it is likely to be later used by other parts of the codegen
• exposed a tail-call-specific AArch64FrameLowering::checkAuthenticatedLR() similarly to signLR() and authenticateLR() so it can be used by outliner callbacks later
• marked a dummy load instruction as volatile similar to Hexagon::PS_crash from HexagonInstrInfo.cpp
• added the third implementation of authenticated address checker, similar to this code. Of course, more checkers can be implemented, but I think it would be better to leave this patch as a common implementation + a few "proofs of concept" and add more checkers via later patches, if needed.

Refactoring.

Looks like adding FeatureFPAC may require changes in more places - need to investigate whether I have to add something like AEK_FPAC constant, etc.

Rebased onto current main branch, will upload a few fixes shortly.

Rebased onto current main branch (after D156428). Further simplified the code by introducing the mergeOutliningCandidateAttributes() callback.

Need to think a bit more if it is correct to just copy the PAuth-related function attributes in mergeOutliningCandidateAttributes - this would make it possible to pass less boolean arguments to signLR / authenticateLR.

Updated the patch.

Address the review comments.

This patch is inspired by the commit https://github.com/ahmedbougacha/llvm-project/commit/58cf59b84ca4e7930a640480fd5ad1ea194864f5 (and uses the same immediate operand for BRK instruction) but adds the checks during epilogue insertion instead of asm printing.

https://github.com/ahmedbougacha/llvm-project/commit/7924c7d75ae0015a9fd9786a580b10b2190bccc6

@atrosinenko do you want to try out that patch, see if it helps?

After debugging a bit more, I suspect that the actual reason for emitting XPACI LR is not the instruction selection phase but register allocation.

Agree, updated the patch. Added comments in a few places where CHECK-NOT were more descriptive (spicifically, not emitting BTI before PACI(A|B)SP and not using RETAA with shadow call stack).

While most features of https://github.com/apple/llvm-project/commit/7924c7d75ae0015a9fd9786a580b10b2190bccc6 are already implemented in mainline, @llvm.returnaddress still tends to be lowered like

xpaci x30
mov dest, x30

instead of

mov dest, x30
xpaci dest

This patch forces the return address to be copied out of LR first, if FEAT_PAuth is avaialble (at the instruction selection phase).

Sorry for a hung patch. After getting the approval I had doubts that I interpret the corner cases of LLVM IR correctly. Thus, I postponed the patch these days to not subtly break the stable targets while fixing one issue on MSP430.

Here is the rebased branch: https://github.com/access-softek/llvm-project/tree/rebased-ptrauth. Some tests might fail as it is work-in-progress.

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

Could this patch be landed? (I can commit it on your behalf if appropriate)

The idea of introducing SizeOfInt to TargetLibraryInfo looks quite reasonable to me, although I am not too familiar with this part of target description.

In D86547#2284095, @MaskRay wrote:

The (aWidth - 1) - clzsi(a) change is correct, but why is the ctz change?

See rG553833958fd as an example.

Ping.

Ping.

Thank you very much for review!

Update after the latest comments.

Re-upload after amending parent diff + add minor clarification.

Clarify rounding-related part of function.

In D86453#2241702, @zatrazz wrote:

Hi atrosinenko, do you think this patch need any more change on the testing side?
The fp_lib.h/int_lib.h change would most likely require in a more complex without
much gain in organization imho.

Re-upload after amending parent diff.

Add more clarifications, fix explanation for "why it is enough to adjust only once in case of overflow".

Re-upload after changing parent diff.

Add some other explanations.

This update is expected to be completely NFC w.r.t. code behavior and significantly clarify the proof up to the end of half-width iterations.

No-change re-upload: rebase onto current master branch.

No-change re-upload: rebase onto current master branch.

Pinging @echristo in case there is some trivial way to allocate a calling convention ID (but probably there is not, unless GCC allocated one already). As I understand, without this ID the debugger may show some misleading output for a limited number of LibCalls. On the other hand, it will not help either, unless the debugger knows this ID.

• Rebase onto current master branch
• Add a dummy switch case with FIXME, as suggested by @aaron.ballman
• Applied a couple of style fixes proposed by linter

Uploaded because it was already approved and no other review comments received since then. Anyway, please feel free to request followup changes if needed.

Uploaded because it was already approved and no other review comments received since then. Anyway, please feel free to request followup changes if needed.

Replaced this by D86546: [compiler-rt][builtins] Use explicitly-sized integer types for LibCalls and D86547: [compiler-rt][builtins] Use c[tl]zsi macro instead of __builtin_c[tl]z. These two patches include everything except for factoring out src_rep_t_clz() and rep_clz() to clzdi() for now.

Uploaded, thank you!