Implement RegId for Mips and MSP430
#38
Conversation
There was a problem hiding this comment.
Hey, thanks for the PR!
There are a couple issues that need to be fixed before we can merge, but the core definitions seem fine.
I do have one big question - while I'm fairly confident that you've tested the Mips RegId (given that you contributed the initial Mips arch to gdbstub), have you actually tested the msp430 or Power PC definitions?
While it'd certainly be nice to close #29, the reason I opened the issue in the first place + implemented this somewhat janky-looking RegIdImpl approach was because I didn't feel comfortable adding definitions that haven't been tested-working in at least one project.
I've taken the same approach with every other architecture-specific piece of functionality in gdbstub. Case in point: the only arch I've actually added myself was armv4t - every other arch has been a community contribution!
While I appreciate the effort, if the msp430 and Power PC RegIds haven't actually been tested, then I'd want to omit them from this PR, and focus on just the Mips RegIds. I don't mind leaving #29 open until someone with more msp430 / Power PC experience comes along to fill in the gaps, since the issue doesn't really affect the broader codebase / end user experience all that much.
src/arch/mips/reg/mips.rs
Outdated
| @@ -22,6 +22,8 @@ pub struct MipsCoreRegs<U> { | |||
| pub cp0: MipsCp0Regs<U>, | |||
| /// FPU registers | |||
| pub fpu: MipsFpuRegs<U>, | |||
| /// DSP registers | |||
| pub dsp: MipsDspRegs<U>, | |||
There was a problem hiding this comment.
Ah, darn, adding this field will be a semver breaking change :'(
Unfortunately, this is a pretty glaring problem I've run into before, and is tracked under #12. In this case, since the DPS registers are optional, there really aught to be some way to specifying whether or not the DSP registers should be used + toggling the corresponding target.xml feature when active (see https://sourceware.org/gdb/current/onlinedocs/gdb/MIPS-Features.html#MIPS-Features)
At some point I really should sit down and hammer out a proper fix for #12, though I don't know when that'll be exactly. As such, the following workaround will have to suffice:
You'll need to create a new regs struct + associated Arch impls:
struct MipsCoreRegsWithDps<U> {
core: MipsCoreRegs<U>,
dsp: MipsDspRegs<U>
}
enum MipsWithDsp {}
impl Arch for MipsWithDsp {
type Usize = u32;
type Registers = reg::MipsCoreRegsWithDps<u32>;
type RegId = RegIdImpl;
type RegId = reg::id::MipsRegId;
fn target_description_xml() -> Option<&'static str> {
Some(r#"<target version="1.0"><architecture>mips</architecture><feature name="org.gnu.gdb.mips.dsp"></feature></target>"#)
}
}
// similarly for Mips64WithDspFeel free to pick a different/better suffix than WithDps.
So yeah, not pretty, but that's just how the code is structured right now unfortunately
That said, these changes would not require a semver breaking change, as they would only add new types without adding any old ones. As such, I'd be able to publish gdbstub 0.4.3 with these definitions included, which I'm sure you'd like 😄
There was a problem hiding this comment.
(leaving this comment unresolved so that I can easily find it in the future, if I ever need to reference it again)
| // ensure bytes.chunks_exact(ptrsize) won't panic | ||
| if bytes.len() % ptrsize != 0 { | ||
| // Ensure bytes contains enough data for all 72 registers | ||
| if bytes.len() < ptrsize * 72 { |
There was a problem hiding this comment.
These two checks aren't equivalent.
For example, if bytes.len() == ptrsize * 72 + 1, then chunks_exact will panic.
You can leave this condition in, but do not remove the other one.
| // Ensure bytes contains enough data for all 79 registers of target-width | ||
| // and the dspctl register which is always 4 bytes | ||
| let ptrsize = core::mem::size_of::<U>(); | ||
| if bytes.len() < (ptrsize * 79) + 4 { |
There was a problem hiding this comment.
Same comment - this check is not sufficient to ensure that chunks_exact won't panic.
You can leave it in, but you still need the other check as well.
There was a problem hiding this comment.
The issue I'm seeing with if bytes.len() % ptrsize != 0 { is when U is a u64 there will be 79 registers of size 64 and one register of size 32 (dspctl), thus this check will fail. It looks like chunks_exact actually doesn't panic when there are extra elements, it should still split the bytes up until the end with a possible remainder of 4 bytes.
There was a problem hiding this comment.
Ahh, yep, you're right.
Well, in any case, I decided to see if the optimizer was smart enough to get rid of any panics, and yep, sure enough it seems to have done the trick:
So yeah, this check can be left as-is.
|
I've separated the MIPS DSP struct from the core registers, which made me realize interestingly enough that |
starfleetcadet75
changed the title
RegId for Mips, MSP430, and PPCRegId for Mips and MSP430
This PR addresses the missing arch implementations (MIPS32/64, MSP430, and PPC) for
RegIdmentioned in issue #29. It also adds the MIPS DSP register struct.