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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


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



================
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))
----------------
zzheng wrote:
> jrtc27 wrote:
> > Is it intended that the shadow call stack grows *up* unlike the normal stack?
> No. Which direction the SCS grows on is trivial.
> 
> The memory area hosting SCS is independent of the regular stack; and it's provided by the runtime.
> mmap/malloc returns the low address of newly mapped/allocated area. Making the SCS growing down requires the runtime to return upper bound of the SCS. On AArch64, the SCS grows up as well.
Ok. Wasn't saying there was anything wrong with it, was just something that jumped out at me. Having it grow up makes more sense (the only real advantage to the normal stack growing down these days is that doing aligned allocations is slightly cheaper).


================
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();
----------------
jrtc27 wrote:
> No need to repeat ourselves.
This still holds.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84414