[PATCH] D56717: [SLH] AArch64: correctly pick temporary register to mask SP

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls marked 2 inline comments as done.
kristof.beyls added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64SpeculationHardening.cpp:232
+                                                  unsigned PreferredReg) const {
+  // This function will only be called on either terminator instructions or on
+  // call instructions. The implementation makes use of that knowledge.
----------------
olista01 wrote:
> I think this requires a return instruction, not just any terminator, because terminators could have other values live across them. I'm also not sure that it's correct for calls, which could have registers live across them, without using them directly.
> 
> Maybe we could use the RegScavenger here, which looks like it can also handle some more difficult cases by spilling a register to the stack?
Yeah, you're right probably better to reuse RegScavenger than trying to reinvent the wheel here.
I am not fully sure if introducing new loads and stores in this pass which aims to protect loads and stores is a good idea, will try to think about that.
Even if we can't find a free register without introducing spill/fills, in that case, we could still use the "emergency" ISB/DSB barrier to block speculation entirely in the affected basic block and not need to mask values in that basic block.
I'll experiment with such an approach.


================
Comment at: llvm/lib/Target/AArch64/AArch64SpeculationHardening.cpp:252
+  }
+  llvm_unreachable("The nature of call instructions and the ABI must result in "
+                   "finding at least one register above.");
----------------
olista01 wrote:
> This can be hit by this code:
> 
>   typedef int (*fptr_t)(void);
>   int foo(fptr_t f) {
>     register fptr_t f2 asm("x30") = f;
>     asm("" : "+r"(f2));
>     return f2() + 1;
>   }
> 
> This uses inline assembly to force the function pointer into x30, but it is an allocatable register so I think it is possible for this to happen "naturally".
Thanks for that example. I didn't manage to come up with an example myself.
My understanding is that the llvm_unreachable is reached because the ABI rules on the indirect call are not modelled very accurately.
Some of the registers will be clobbered by the call, and could be used as temporary register, but it seems this isn't modelled on the BLR mir instruction.
Anyway, as I said above - probably I'll need to implement generation of the ISB/DSB full speculation barrier in such a rare case.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56717