[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/AArch64InstrInfo.cpp:8252
+  if (isTailCallReturnInst(*RepeatedSequenceLocs[0].back()))
+    SequenceSize += NumBytesToCheckLRInTCEpilogue;
+
----------------
atrosinenko wrote:
> 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.
Thanks for pointing to that FIXME - indeed, it seems like as long as the expansion happens early enough, there should be no issue.



================
Comment at: llvm/lib/Target/AArch64/AArch64PointerAuth.cpp:147
+
+class AddressCheckPseudoSourceValue : public PseudoSourceValue {
+public:
----------------
I think this class could use a comment.
Would something like the following be correct?

```
// AddressCheckPseudoSourceValue represents the memory access inserted by the
// the AuthCheckMethod::DummyLoad method to trigger a run-time error if the
// authentication of a pointer failed.
```


================
Comment at: llvm/lib/Target/AArch64/AArch64PointerAuth.cpp:160
+MachineMemOperand *createCheckMemOperand(MachineFunction &MF) {
+  static AddressCheckPseudoSourceValue CheckPSV(MF.getTarget());
+  MachinePointerInfo PointerInfo(&CheckPSV);
----------------
Seeing this is a static variable made me think: would it be possible in a single run of any program using the LLVM libraries (such as clang, flang, rust, opt, ...) for MF.getTarget() to potentially return a different result sometimes within the same execution of the that program.
If that could occur, then some from some executions of this function, the `CheckPSV` variable could be initialized with the wrong `TargetMachine` reference....

I cannot immediately think of an example, but I am also not sure.
I think it would be more prudent to not have this as a static variable.


================
Comment at: llvm/lib/Target/AArch64/AArch64PointerAuth.cpp:191
+        .addMemOperand(createCheckMemOperand(MF))
+        .setMIFlags(MachineInstr::FrameDestroy);
+    return MBB;
----------------
I'm wondering why any/all of the machineinsts created in this function need to have the FrameDestroy flag set? Do you know?


================
Comment at: llvm/lib/Target/AArch64/AArch64PointerAuth.cpp:350
 
+  // TODO Do we need to emit any PAuth-related epilogue code at all
+  //      when SCS is enabled?
----------------
The convention in the LLVM code base is to use "FIXME" rather than "TODO".




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156716