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

Mon Oct 2 09:34:25 PDT 2023

atrosinenko added a comment.

Updated the patch, thank you.

================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:8252
+  if (isTailCallReturnInst(*RepeatedSequenceLocs[0].back()))
+    SequenceSize += NumBytesToCheckLRInTCEpilogue;
+
----------------
kristof.beyls wrote:
> 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.
On one hand, updating `getInstSizeInBytes(MI)` should fix not only the computation of `SequenceSize` but other possible callers of getInstSizeInBytes as well. On the other hand, inserting checker code before TCRETURN* instructions seems like just yet another code transformation (and TCRETURN* instructions are actually four bytes long after `AArch64PointerAuth` pass processes the function).

Moreover, in `getInstSizeInBytes(MI)`, there is handling for several special cases and a comment stating
```
  // FIXME: We currently only handle pseudoinstructions that don't get expanded
  //        before the assembly printer.
```

Thus, I would rather keep updating `SequenceSize` ad-hoc, but move the update right after `NumBytesToCheckLRInTCEpilogue` is computed.

================
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)")
+
----------------
kristof.beyls wrote:
> 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.
There will probably be non v8.2-compatible checkers in the future - for example, if we want to check an *arbitrary* register using XPAC. Though, I agree that it is better to drop the "(pre-v8.3 compatible)" part here: it would be better to add more comprehensible comment like "(requires FEAT_PAUTH)" to those checkers, I think.

================
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.
----------------
kristof.beyls wrote:
> s/succeedes/succeeds/?
Fixed.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156716