Support RV32I with only F extension and RV32E#53
Conversation
nbdd0121
left a comment
nbdd0121
left a comment
There was a problem hiding this comment.
Thanks for the PR! Overall LGTM, some suggestions:
src/unwinder/arch/riscv32.rs
Outdated
| for i in 0..=15 { | ||
| fmt.field(RiscV::register_name(Register(i as _)).unwrap(), &self.gp[i]); | ||
| } | ||
| #[cfg(not(target_feature = "e"))] | ||
| for i in 16..=31 { |
There was a problem hiding this comment.
I think this can be written as for (i, gp) in self.gp.enumerate()?
There was a problem hiding this comment.
Updated to use enumerate.
src/unwinder/arch/riscv32.rs
Outdated
| fn index(&self, reg: Register) -> &usize { | ||
| match reg { | ||
| Register(0..=31) => &self.gp[reg.0 as usize], | ||
| Register(0..=15) => &self.gp[reg.0 as usize], |
There was a problem hiding this comment.
This can just be 0..=31 I think. The meaning of 16..=31 should still mean GP even in RV32? Turning a unimplemented!() panic into OOB panic is fine; the caller of C unwind API wouldn't pass in illegal register values anyway.
There was a problem hiding this comment.
Reverted this and similar changes.
src/unwinder/arch/riscv32.rs
Outdated
| sw x0, 0x00(sp) | ||
| sw ra, 0x04(sp) | ||
| sw t0, 0x08(sp) | ||
| sw gp, 0x0C(sp) | ||
| sw tp, 0x10(sp) | ||
| sw s0, 0x20(sp) | ||
| sw s1, 0x24(sp) |
There was a problem hiding this comment.
This part can be shared between E & I? IIUC the calling convention doesn't change the caller/callee-saved status of first 16 registers.
|
Thank you! |
2 participants
This supports the following builtin RISC-V RV32 targets. (This allows all the current builtin bare-metal RISC-V targets to be supported by this crate.)
As with my PR that added RV32 support (#14), I tested catch_unwind + backtrace using the semihosting crate's test suite (taiki-e/semihosting@7199732).