[PATCH] D81400: [AArch64] Introduce AArch64SLSHardeningPass, implementing hardening of RET and BR instructions.

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls marked 3 inline comments as done.
kristof.beyls added a comment.

Thanks for the feedback!

The pre-commit test failures in the newly added tests seem to have been caused by the parent patch D81399 <https://reviews.llvm.org/D81399> not having been committed before.
That has been committed since.
So if the pre-commit test infrastructure also runs tests on patch updates, it will hopefully show the tests passing now.



================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.h:388
 
 static inline bool isIndirectBranchOpcode(int Opc) {
+  switch (Opc) {
----------------
ostannard wrote:
> There's a `isIndirectBranch` flag on instructions in the tablegen, so is this needed?
When I change the one previous use of isIndirectBranchOpcode in AArch64InstrInfo::analyzeBranch to use isIndirectBranch, I see 2 tests starting to fail:
  LLVM :: CodeGen/AArch64/callbr-asm-obj-file.ll
  LLVM :: CodeGen/AArch64/callbr-asm-label.ll

The change in behaviour seems to be caused by the INLINEASM_BR being an IndirectBranch, but not recognized by isIndirectBranchOpcode as such.
Maybe this indicates a missed optimization (and makes the case for not having isIndirectBranchOpcode and its duplication).
But given it's a change in behaviour, I think it's best to do a change for this in a separate patch.

Also, analyzeBranch uses IsIndirectBranchOpcode, isCondBranchOpcode, isUncondBranchOpcode together, so maybe it's more consistent for it to keep on using a set of interfaces all based on opcodes; not sure.

Anyway, while I agree it would be better not to repeat the same information as the isIndirectBranch on instructions in tablegen, with the above, it seems best to tackle this seeming duplication as a separate patch.

I've added a test that does contain/produce INLINEASM_BR.


================
Comment at: llvm/lib/Target/AArch64/AArch64SLSHardening.cpp:106
+  MachineBasicBlock::iterator NextMBBI;
+  for (; MBBI != E; MBBI = NextMBBI) {
+    MachineInstr &MI = *MBBI;
----------------
ostannard wrote:
> Since we only care about instructions which are terminators, could this me made more efficient by just scanning the terminators?
Good point, and thanks for pointing me to getFirstTerminator().


================
Comment at: llvm/test/CodeGen/AArch64/speculation-hardening-sls.ll:22
+  ret i32 %retval.0
+; CHECK-LABEL: double_return:
+; CHECK:       {{ret$}}
----------------
ostannard wrote:
> I assume that the return instruction is being split into two in the backend somewhere, but it would be clearer to just have two returns in the IR. If this is testing for a bug triggered when that optimisation happening, then this could do with a comment explaining that.
Fair point, I'll make the change.


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

https://reviews.llvm.org/D81400