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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
