[PATCH] D156716: [AArch64][PAC] Check authenticated LR value during tail call

Fri Aug 4 08:42:06 PDT 2023

atrosinenko added a subscriber: olista01.
atrosinenko added a comment.

Updated the patch.

================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:261-265
+cl::opt<bool> CheckAuthenticatedLRByLoad(
+    "aarch64-check-authenticated-lr-by-load", cl::Hidden, cl::init(true),
+    cl::desc("When performing a tail call with authenticated LR, "
+             "use a load instruction to check the LR"));
+
----------------
kristof.beyls wrote:
> My understanding is that using a load instruction to check the LR can only be done when code segments are not "execute-only".
> 
> The commit comment on https://github.com/llvm/llvm-project/commit/a932cd409b861582902211690b497cafc774bee6 suggests that at least LLD assumes that code generated for AArch64 targets is always compatible with execute-only segments. Therefore, I think that defaulting to checking-by-loading-lr is probably the wrong thing to do. It seems to me it would be better to default the other way and only allow checking-by-loading-lr when there is a guarantee that the target environment will not enforce "execute-only" code.
Updated. Though, I wonder if using the long snippet by default can introduce noticeable performance regression or not.

Meanwhile, is it permitted to remove a dead load in the machine pipeline (and should I mark it as volatile somehow).

================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1979
+  default:
+    return MBB;
+  }
----------------
kristof.beyls wrote:
> This is just nitpicking:
> 
> Would it be useful to have an isTailCallReturn function somewhere, and insert an
> ```
> assert(!isTailCallReturn(TI->getOpcode())
> ```
> here? Given how a range of tail call pseudo opcodes have been added recently, it might be likely that a few more could get added in the future, in which case this switch statement needs to be adapted.
> I'm just always very cautious when doing a switch on a set of specific opcodes as opcodes tend to evolve over time and such switch statements might be hard to maintain correctly. That's why I tend to prefer having an assert in the default statement that hopefully catches when that happens.
> 
> FWIW, https://github.com/llvm/llvm-project/blob/2df05cd01c17f3ef720e554dc7cde43df27e5224/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp#L275 already has a computation of an "IsTailCall" in this file (albeit that it doesn't consider TCRETURNriALL to be a tail call - is that an indication of an instance of the issue I described above?)).
Added an `AArch64InstrInfo::isTailCallReturnInst(MachineInstr &)` function and redirected there the existing code in `getArgumentStackToRestore()`. As far as I understand, the new `TCRETURNriALL` is only used by machine outliner and it seems to be used interchangeably with the existing `TCRETURNdi` instruction in https://github.com/llvm/llvm-project/blob/feafc2df43545e61a0ba67253284ecbabfd2ba09/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp#L8076C24-L8076C24.

cc: @olista01

================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1987
+  //
+  // Turn it into:
+  //
----------------
kristof.beyls wrote:
> I think this comment would be easier to read if instead of just saying "Turn it into:", it would also clearly indicate the intended effect. For example:
> 
> "To avoid generating a signing oracle, generate a code sequence that explicitly checks if the authentication passed or failed, as follows."
> 
> With just having written those extra few words now, I think that this extra checking may not be needed if the target core implements FEAT_FPAC (and maybe FEAT_FPACCOMBINE?). If so, maybe a FIXME here would be good to not generate the extra checking code sequences when FEAT_FPAC is implemented by the targeted core?
Fixed.

There exist separate review items D156784 and D156785 on taking FEAT_FPAC into account. I wonder if the particular CPU models should mention `+fpac` as supported or should it be only requested explicitly by the user - unlike many other subtarget features, FPAC doesn't make executable code explicitly fail on an unsupported CPU but silently makes it a bit less secure. So, the case "I have CPU X that implements all the instructions that are supported by Y (but not FEAT_FPAC)" may technically be unnoticed.

================
Comment at: llvm/test/CodeGen/AArch64/sign-return-address-tailcall.ll:11
+;
+; LDR-NEXT:       [[AUT]]
+; LDR-NEXT:       .cfi_negate_ra_state
----------------
kristof.beyls wrote:
> nitpick: I think I'd prefer `[[AUTIASP]]` rather than `[[AUT]]` as the macro name as that makes it clearer on first read exactly which authenticate operation is expected here. I do like the use of the macro though to hide away the difference between `hint #29` and `autiasp`.
Fixed

================
Comment at: llvm/test/CodeGen/AArch64/sign-return-address-tailcall.ll:19
+; XPAC-NEXT:      mov x16, x30
+; XPAC-NEXT:      [[XPAC]]
+; XPAC-NEXT:      cmp x16, x30
----------------
kristof.beyls wrote:
> nitpick: similarly, I think I'd prefer `[[XPACLRI]]`
Fixed

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156716/new/

https://reviews.llvm.org/D156716