[PATCH] D156716: [AArch64][PAC] Check authenticated LR value during tail call
Kristof Beyls via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 4 01:25:42 PDT 2023
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
More information about the llvm-commits
mailing list