[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.

1. Please use local variables with meaningful names for `RISCV::Xn` rather than inlining them everywhere and making it harder at a glance to work out what's going on without knowing what the ABI register names are.

2. Please make a `RISCVABI::getSCSPReg` or similar function to avoid hard-coding X18 in a bunch of places.

3. I'm not convinced a callee-saved register makes any more sense than anything else. Whatever you pick, compatibility only works one way round. If foo uses an SCS and calls bar that doesn't, then being callee saved helps you, but if bar calls foo then (a) foo will try to dereference SCSP (to store then later load), likely faulting, perhaps instead "just" clobbering arbitrary memory (b) if foo manages to return back to bar without faulting then bar would expect the register to have been saved, but it hasn't, an ABI violation. If you use a caller-saved register instead then bar can call foo just fine, but foo can't call bar as it'll clobber its SCSP. Reserving any register like this is a fundamental ABI incompatibility, so picking x18 because it's callee-saved is meaningless, and picking it to avoid duplicating 6 lines of code (or fewer, if the existing 6 lines are refactored) isn't a good reason either. It's ultimately arbitrary, but it should be at least be based on some kind of valid reasoning if such reasoning exists. X18 may or may not be a right answer.

  (One might initially think that this incompatibility is fine if you're building an executable with SCSP that calls other libraries without SCSP, but that breaks down as soon as callbacks exist, as well as more niche things like symbol preemption.)



================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:33-37
+  std::vector<CalleeSavedInfo> &CSI = MF.getFrameInfo().getCalleeSavedInfo();
+  if (std::none_of(CSI.begin(), CSI.end(), [](CalleeSavedInfo &CSR) {
+        return CSR.getReg() == RISCV::X1;
+      }))
+    return;
----------------
```
    if (find(CSI, RISCV::X1) == CSI.end())
      return;
```
(using `llvm::find` as a convenient wrapper around `std::find`, ie shorthand for `std::find(CSI.begin(), CSI.end(), RISCV::X1)`). Though personally I'd prefer to see X1 come from `RI.getRARegister()` rather than be hard-coded; other functions in this file already hard-code it, but in our CHERI fork we need to replace RISCV::X1 with RISCV::C1 everywhere so have changed those. Having said that, CHERI renders a shadow call stack unnecessary, so I don't particularly care if it's broken there, personally. But I still think it's nicer code.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:54
+  // addi s2, s2, 4
+  BuildMI(MBB, MI, DL, TII->get(RISCV::SW))
+      .addReg(RISCV::X1)
----------------
This is wrong for RV64.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:69-73
+  std::vector<CalleeSavedInfo> &CSI = MF.getFrameInfo().getCalleeSavedInfo();
+  if (std::none_of(CSI.begin(), CSI.end(), [](CalleeSavedInfo &CSR) {
+        return CSR.getReg() == RISCV::X1;
+      }))
+    return;
----------------
As above.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:94
+      .addImm(-SlotSize);
+  BuildMI(MBB, MI, DL, TII->get(RISCV::LW))
+      .addReg(RISCV::X1, RegState::Define)
----------------
Also wrong for RV64.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84414