[PATCH] D81402: [AArch64] Extend AArch64SLSHardeningPass to harden BLR instructions.

[Oliver Stannard (Linaro) via Phabricator via llvm-commits]

ostannard added a comment.

A few nits, but given the time sensitivity of this the only blocking one is how this will work with PLTs or long-branch veneers,



================
Comment at: llvm/lib/Target/AArch64/AArch64SLSHardening.cpp:153
+    "__llvm_slsblr_thunk_x14", "__llvm_slsblr_thunk_x15",
+    "__llvm_slsblr_thunk_x16", "__llvm_slsblr_thunk_x17",
+    "__llvm_slsblr_thunk_x18", "__llvm_slsblr_thunk_x19",
----------------
Is it possible for the call to these thunks to go through a long-branch veneer, PLT, etc, which could clobber x16 or x17 before it gets used in the thunk? Maybe we could avoid generating indirect calls using those registers, but we have the exact opposite restriction for tail calls when using BTI (grep for TCRETURNriBTI).


================
Comment at: llvm/lib/Target/AArch64/AArch64SLSHardening.cpp:193
+void SLSBLRThunkInserter::populateThunk(MachineFunction &MF) {
+  Register ThunkReg;
+  // FIXME: How to better communicate Register number, rather than through
----------------
Style nit: Variable should be declared further down, when first set.


================
Comment at: llvm/lib/Target/AArch64/AArch64SLSHardening.cpp:208
+      MF.getSubtarget<AArch64Subtarget>().getInstrInfo();
+  // Grab the entry MBB and erase any other blocks. O0 codegen appears to
+  // generate two bbs for the entry block.
----------------
Is this something which could be fixed in ThunkInserter?


================
Comment at: llvm/lib/Target/AArch64/AArch64SLSHardening.cpp:232
+                                    MachineBasicBlock::iterator MBBI) const {
+  // Split the current basic block as follows:
+  // Before:
----------------
This comment is a bit misleading, we're not actually splitting the basic block (that's terminology used elsewhere in LLVM for replacing one basic block with two).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81402