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

[Anatoly Trosinenko via Phabricator via llvm-commits]

atrosinenko added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64PointerAuth.cpp:147
+
+class AddressCheckPseudoSourceValue : public PseudoSourceValue {
+public:
----------------
kristof.beyls wrote:
> 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.
> ```
Added your comment, thank you.


================
Comment at: llvm/lib/Target/AArch64/AArch64PointerAuth.cpp:160
+MachineMemOperand *createCheckMemOperand(MachineFunction &MF) {
+  static AddressCheckPseudoSourceValue CheckPSV(MF.getTarget());
+  MachinePointerInfo PointerInfo(&CheckPSV);
----------------
kristof.beyls wrote:
> 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.
I used the same approach as it is implemented in Hexagon backend: [HexagonInstrInfo.cpp](https://github.com/llvm/llvm-project/blob/d0663005a8dbdf12a568c7479b05e74dc14eef28/llvm/lib/Target/Hexagon/HexagonInstrInfo.cpp#L1457). For now TargetMachine is only used by `PseudoSourceValue` constructor to call `TM.getAddressSpaceForPseudoSourceKind(Kind)`. I agree that using static instance looks suspicious, but I guess allocating new instance from heap on each check is quite wasteful.


================
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?
----------------
kristof.beyls wrote:
> The convention in the LLVM code base is to use "FIXME" rather than "TODO".
> 
> 
Updated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156716