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

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a comment.

Thank you for the update!
I haven't looked at Arch64PointerAuth.cpp and the regression test in detail yet.
Most of the other parts of this patch looks fine to me, apart from the 2 nitpicks and the one probably bigger issue with getInstSizeInBytes() - see inline comments.



================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:8252
+  if (isTailCallReturnInst(*RepeatedSequenceLocs[0].back()))
+    SequenceSize += NumBytesToCheckLRInTCEpilogue;
+
----------------
I had to spend quite a bit of time to understand the logic here.
I think, if possible, it would be easier to understand the logic if SequenceSize was updated much closer to where it is initially computed on line 8133.

Ideally, getInstSizeInBytes(MI) would return the correct number of bytes.

Actually, looking at the documentation of getInstSizeInBytes(MI), at https://github.com/llvm/llvm-project/blob/c4e2fcff788025415b523486efdbdac4f2b08c1e/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp#L83, it states that getInstSizeInBytes is guaranteed to return the maximum number of bytes.
So on a tail call return that might produce lots of bytes, it should return the maximum number.

If it does not produce the maximum number, I seem to remember that that can lead to hard-to-debug compilation failures where the code size estimation for computing whether a constant island is within range could go wrong. (I might be misremembering, it could be something different than a constant island going out of range).

So, I think that getInstSizeInBytes(MI) needs to be adapted so it returns the correct maximum size in bytes for a tail call. When that is implemented, the change on this line in this patch may no longer be needed?

Assuming I remember all of the above correctly, it may be good to add a regression test that verifies that getInstSizeInBytes is calculated correctly for large tail calls. https://reviews.llvm.org/D22870 is a patch that fixed a similar issue on another instruction. The test case that was added there could serve as inspiration.


================
Comment at: llvm/lib/Target/AArch64/AArch64PointerAuth.h:88
+      clEnumValN(AArch64PAuth::AuthCheckMethod::XPACHint, "xpac-hint",         \
+                 "Compare with the result of XPACLRI (pre-v8.3 compatible)")
+
----------------
nit pick - feel free to disagree. IIUC, all methods are pre-v8.3 compatible. Is it then worthwhile to call pre-v8.3 compatibility out in the help text? I think I'd remove the "(pre-v8.3 compatible)" part.


================
Comment at: llvm/lib/Target/AArch64/AArch64PointerAuth.h:94
+/// instructions, check the value using Method just before the instruction
+/// pointed to by MBBI. If the check succeedes, execution proceeds to the
+/// instruction pointed to by MBBI, otherwise a CPU exception is generated.
----------------
s/succeedes/succeeds/?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156716