Add c18n support on RISC-V #785
Conversation
Stop treating the trusted stack pointer as a register because it is unnecessary and some architectures like RISC-V do not in fact store it in a dedicated register.
| #ifdef _LIBUNWIND_HAS_CHERI_LIB_C18N | ||
| // Preserve the argument in a callee-saved register instead of pushing it onto | ||
| // the stack because stack unwinding will switch the stack. | ||
| cmv cs0, ca0 |
There was a problem hiding this comment.
cs0 is the frame pointer; is that a problem? Even if not, probably more friendly to use a different register here?
libunwind/src/Registers.hpp
Outdated
| @@ -4554,10 +4552,21 @@ class _LIBUNWIND_HIDDEN Registers_riscv { | |||
| void setSP(reg_t value) { _registers[2] = value; } | |||
| reg_t getIP() const { return _registers[0]; } | |||
| void setIP(reg_t value) { _registers[0] = value; } | |||
| reg_t getFP() const { return _registers[8]; } | |||
| void setFP(reg_t value) { _registers[8] = value; } | |||
There was a problem hiding this comment.
Eh, I would argue there's no need to encode FP, it's just a saved register, and may not be being used as a frame pointer. Then you don't need to extend each Registers_arch with knowledge about the FP.
There was a problem hiding this comment.
The trusted stack frame treats fp as a special register and does not store it in the array of callee-saved registers. So without the setter, I'd need some other code that does the equivalent thing, which complicates unwindIfAtBoundary.
I suppose we can gate the getter/setter by _LIBUNWIND_HAS_CHERI_LIB_C18N?
There was a problem hiding this comment.
I mean yes, part of my point is that the interface should really have put fp in regs. But what's wrong with setRegister(UNW_ARCH_[XC]N, state.fp) for now until we change the interface to be more sensible? You already need to list the UNW_ARCH_[XC]N for all the other callee-saved registers.
There was a problem hiding this comment.
That works for me. The reason fp is separately stored in the trusted frame is so that the trusted frame would 'look like' a normal frame. When a special flag is passed to RTLD, the trampoline will not clear the tag of fp when entering a callee compartment, allowing anyone to follow fp to cross compartment boundaries and trace the whole call stack.
This should be useful when using pmcstat, but we have never actually tried it...
libunwind/src/CompartmentInfo.hpp
Outdated
| #ifdef _LIBUNWIND_TARGET_AARCH64 | ||
| reg_num = UNW_ARM64_C19 + i; | ||
| #elif defined(_LIBUNWIND_TARGET_RISCV) | ||
| static constexpr int callee_saved[] = { |
There was a problem hiding this comment.
Might be better to hoist this out, define an equivalent array for Morello, and static assert that it's the same length as state.regs.
libunwind/src/UnwindRegistersSave.S
Outdated
| @@ -1273,6 +1273,19 @@ DEFINE_LIBUNWIND_FUNCTION(__unw_getcontext) | |||
| .endr | |||
| # endif | |||
|
|
|||
| #ifdef _LIBUNWIND_HAS_CHERI_LIB_C18N | |||
| // Store the trusted stack pointer | |||
| caddi csp, csp, -32 | |||
There was a problem hiding this comment.
AFAICT these two caddis are the only reason this code is RVY-specific. It'd be nice if it worked for Xcheri; we accept addi as an alias for cincoffset(imm) these days. But I see the current RVY spec is stupid and says it should be add not addi, and so that's what Codasip's LLVM implements. I'll go fix the spec, and someone should then bug them to update their assembler. But then you should be able to write assembly that works for both.
libunwind/src/UnwindRegistersSave.S
Outdated
| // Store the trusted stack pointer | ||
| caddi csp, csp, -32 | ||
| sc ca0, 0(csp) | ||
| sc cra, 16(csp) |
There was a problem hiding this comment.
Can we use __SIZEOF_CHERI_CAPABILITY__ here (and for the frame sizes)? We don't have an RV32 platform for this but there's no reason not to make it work when it's this easy (and you've already used __SIZEOF_CHERI_CAPABILITY__ for the final store, and for the restore path).
| // Use 240 for ECSP (executive stack pointer). ECSP is not a real DWARF | ||
| // register, but we need it to implement c18n-aware unwinding. We pick 240 | ||
| // because it is far enough away from the range of reserved registers on Arm. | ||
| UNW_ARM64_ECSP = 240, |
There was a problem hiding this comment.
I guess in an earlier iteration of the patch we really did need it to be a DWARF register?..
This extends libunwind's c18n support to RV64Y.
Although c18n for RV64Y hasn't officially landed yet, this libunwind implementation does not depend on any implementation detail on CheriBSD's side because it merely uses APIs that the existing Morello implementation relies on. And before CheriBSD sets
_LIBUNWIND_HAS_CHERI_LIB_C18Nfor RV64Y, none of the unwinding logic would be compiled in.