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

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added inline comments.


================
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"));
+
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1979
+  default:
+    return MBB;
+  }
----------------
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?)).


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1987
+  //
+  // Turn it into:
+  //
----------------
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?


================
Comment at: llvm/test/CodeGen/AArch64/sign-return-address-tailcall.ll:11
+;
+; LDR-NEXT:       [[AUT]]
+; LDR-NEXT:       .cfi_negate_ra_state
----------------
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`.


================
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
----------------
nitpick: similarly, I think I'd prefer `[[XPACLRI]]`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156716