[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/AArch64PointerAuth.cpp:160
+MachineMemOperand *createCheckMemOperand(MachineFunction &MF) {
+  static AddressCheckPseudoSourceValue CheckPSV(MF.getTarget());
+  MachinePointerInfo PointerInfo(&CheckPSV);
----------------
atrosinenko wrote:
> 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.
I guess this would be allocating new instances on the stack rather than the heap?
Anyway, I think it would be better to not rely on a static variable where we know this is likely to trigger a hard-to-track-down bug at some point in the future, for some use cases.

If the cost would be too high, I guess it might be possible to "cache" CheckPSV once per MachineFunction being processed? That may require finding an object that is constructed once per MachineFunction and put that cached CheckPSV in there. Not sure if there is such an obvious object at the moment and if so, how much refactoring would be needed to make this possible.

Anyway, I guess this doesn't get called that often (only once per tail call) so presumably it isn't worthwhile to make the code a lot more complicated for the small (would it even be measurable?) compile time gain.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156716