[PATCH] D84414: [RISCV] Support Shadow Call Stack

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 requested changes to this revision.
jrtc27 added a comment.
This revision now requires changes to proceed.

This is currently incompatible with the save/restore libcalls, and I don't think there's any way to avoid that (the restore libcall both loads ra and jumps to it). We should ensure combining them give an error.



================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:36-37
+
+  // Do not save RA to SCS if it's not saved to regular stack, i.e.
+  // RA is not subject to overwritten.
+  std::vector<CalleeSavedInfo> &CSI = MF.getFrameInfo().getCalleeSavedInfo();
----------------



================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:43
+
+  // Get shadow call stack pointer register.
+  Register SCSPReg = RISCVABI::getSCSPReg();
----------------
Pointless comment; remove


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:46
+
+  // Emit an error message and bail out.
+  if (!STI.isRegisterReservedByUser(SCSPReg)) {
----------------
Pointless comment; remove


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:53
+
+  DebugLoc DL = MI != MBB.end() ? MI->getDebugLoc() : DebugLoc();
+
----------------
This should be passed in as an argument IMO (same for the epilogue) given the standard prologue/epilogue code already has a DebugLoc lying around.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:59-60
+  // Store return address to shadow call stack
+  // sw   ra, 0(s2)
+  // addi s2, s2, 4
+  BuildMI(MBB, MI, DL, TII->get(IsRV64 ? RISCV::SD : RISCV::SW))
----------------



================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:60
+  // sw   ra, 0(s2)
+  // addi s2, s2, 4
+  BuildMI(MBB, MI, DL, TII->get(IsRV64 ? RISCV::SD : RISCV::SW))
----------------
Is it intended that the shadow call stack grows *up* unlike the normal stack?


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:79-80
+
+  // Do not restore RA from SCS if it's not saved to regular stack, i.e.
+  // RA is not subject to overwritten.
+  std::vector<CalleeSavedInfo> &CSI = MF.getFrameInfo().getCalleeSavedInfo();
----------------
No need to repeat ourselves.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:102-103
+  // Load return address from shadow call stack
+  // addi s2, s2, -4
+  // lw   ra, 0(s2)
+  BuildMI(MBB, MI, DL, TII->get(RISCV::ADDI))
----------------
Although in fact you have both a bug and a minor performance issue with this, and it should be:

```
  // l[w|d] ra, [-4|-8](s2)
  // addi   s2, s2, -[4|8]
```

Then there's no read-after-write dependency chain, which is better for out-of-order cores.

The bug is that, currently, you risk a signal handler clobbering your SCS slot in between the two instructions, since you deallocate the frame before you read from it. Will be rare in practice, but a possibility.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84414