Msp430 fixes (#62)

* Improve MSP430 support.

- Use the correct number of registers.
- Add support for CPUX variant (20-bit registers)

* Fix tests and cleanup comments

* Use fixed RegId type for msp430

Author: mchesser

gdbstub_arch/src/msp430/mod.rs

@@ -1,26 +1,33 @@
 //! Implementations for the TI-MSP430 family of MCUs.
 
 use gdbstub::arch::Arch;
-use gdbstub::arch::RegId;
 
 pub mod reg;
 
 /// Implements `Arch` for standard 16-bit TI-MSP430 MCUs.
-///
-/// Check out the [module level docs](gdbstub::arch#whats-with-regidimpl) for
-/// more info about the `RegIdImpl` type parameter.
-pub enum Msp430<RegIdImpl: RegId = reg::id::Msp430RegId> {
-    #[doc(hidden)]
-    _Marker(core::marker::PhantomData<RegIdImpl>),
-}
+pub struct Msp430 {}
 
-impl<RegIdImpl: RegId> Arch for Msp430<RegIdImpl> {
+impl Arch for Msp430 {
     type Usize = u16;
-    type Registers = reg::Msp430Regs;
-    type RegId = RegIdImpl;
+    type Registers = reg::Msp430Regs<u16>;
+    type RegId = reg::id::Msp430RegId<u16>;
     type BreakpointKind = usize;
 
     fn target_description_xml() -> Option<&'static str> {
         Some(r#"<target version="1.0"><architecture>msp430</architecture></target>"#)
     }
 }
+
+/// Implements `Arch` for 20-bit TI-MSP430 MCUs (CPUX).
+pub struct Msp430X {}
+
+impl Arch for Msp430X {
+    type Usize = u32;
+    type Registers = reg::Msp430Regs<u32>;
+    type RegId = reg::id::Msp430RegId<u32>;
+    type BreakpointKind = usize;
+
+    fn target_description_xml() -> Option<&'static str> {
+        Some(r#"<target version="1.0"><architecture>msp430x</architecture></target>"#)
+    }
+}

gdbstub_arch/src/msp430/reg/id.rs

@@ -6,7 +6,7 @@ use gdbstub::arch::RegId;
 /// The best file to reference is [msp430-tdep.c](https://github.com/bminor/binutils-gdb/blob/master/gdb/msp430-tdep.c).
 #[derive(Debug, Clone, Copy)]
 #[non_exhaustive]
-pub enum Msp430RegId {
+pub enum Msp430RegId<U> {
     /// Program Counter (R0)
     Pc,
     /// Stack Pointer (R1)
@@ -17,19 +17,33 @@ pub enum Msp430RegId {
     Cg,
     /// General Purpose Registers (R4-R15)
     Gpr(u8),
+    #[doc(hidden)]
+    _Size(U),
 }
 
-impl RegId for Msp430RegId {
+fn from_raw_id<U>(id: usize) -> Option<(Msp430RegId<U>, usize)> {
+    let reg = match id {
+        0 => Msp430RegId::Pc,
+        1 => Msp430RegId::Sp,
+        2 => Msp430RegId::Sr,
+        3 => Msp430RegId::Cg,
+        4..=15 => Msp430RegId::Gpr((id as u8) - 4),
+        _ => return None,
+    };
+
+    let ptrsize = core::mem::size_of::<U>();
+    Some((reg, ptrsize))
+}
+
+impl RegId for Msp430RegId<u16> {
     fn from_raw_id(id: usize) -> Option<(Self, usize)> {
-        let reg = match id {
-            0 => Self::Pc,
-            1 => Self::Sp,
-            2 => Self::Sr,
-            3 => Self::Cg,
-            4..=15 => Self::Gpr((id as u8) - 4),
-            _ => return None,
-        };
-        Some((reg, 2))
+        from_raw_id::<u16>(id)
+    }
+}
+
+impl RegId for Msp430RegId<u32> {
+    fn from_raw_id(id: usize) -> Option<(Self, usize)> {
+        from_raw_id::<u32>(id)
     }
 }
 
@@ -51,7 +65,7 @@ mod tests {
 
         // The `Msp430Regs` implementation does not increment the size for
         // the CG register since it will always be the constant zero.
-        serialized_data_len += 4;
+        serialized_data_len += RId::from_raw_id(3).unwrap().1;
 
         // Accumulate register sizes returned by `from_raw_id`.
         let mut i = 0;
@@ -66,6 +80,11 @@ mod tests {
 
     #[test]
     fn test_msp430() {
-        test::<crate::msp430::reg::Msp430Regs, crate::msp430::reg::id::Msp430RegId>()
+        test::<crate::msp430::reg::Msp430Regs<u16>, crate::msp430::reg::id::Msp430RegId<u16>>()
+    }
+
+    #[test]
+    fn test_msp430x() {
+        test::<crate::msp430::reg::Msp430Regs<u32>, crate::msp430::reg::id::Msp430RegId<u32>>()
     }
 }

gdbstub_arch/src/msp430/reg/msp430.rs

@@ -1,60 +1,74 @@
+use num_traits::PrimInt;
+
 use gdbstub::arch::Registers;
+use gdbstub::internal::LeBytes;
 
-/// 16-bit TI-MSP430 registers.
+/// TI-MSP430 registers.
+///
+/// The register width is set based on the `<U>` type. For 16-bit MSP430 CPUs
+/// this should be `u16` and for 20-bit MSP430 CPUs (CPUX) this should be `u32`.
 #[derive(Debug, Default, Clone, Eq, PartialEq)]
-pub struct Msp430Regs {
+pub struct Msp430Regs<U> {
     /// Program Counter (R0)
-    pub pc: u16,
+    pub pc: U,
     /// Stack Pointer (R1)
-    pub sp: u16,
+    pub sp: U,
     /// Status Register (R2)
-    pub sr: u16,
+    pub sr: U,
     /// General Purpose Registers (R4-R15)
-    pub r: [u16; 11],
+    pub r: [U; 12],
 }
 
-impl Registers for Msp430Regs {
-    type ProgramCounter = u16;
+impl<U> Registers for Msp430Regs<U>
+where
+    U: PrimInt + LeBytes + Default + core::fmt::Debug,
+{
+    type ProgramCounter = U;
 
     fn pc(&self) -> Self::ProgramCounter {
         self.pc
     }
 
     fn gdb_serialize(&self, mut write_byte: impl FnMut(Option<u8>)) {
-        macro_rules! write_bytes {
-            ($bytes:expr) => {
-                for b in $bytes {
-                    write_byte(Some(*b))
+        macro_rules! write_le_bytes {
+            ($value:expr) => {
+                let mut buf = [0; 4];
+                // infallible (register size a maximum of 32-bits)
+                let len = $value.to_le_bytes(&mut buf).unwrap();
+                let buf = &buf[..len];
+                for b in buf {
+                    write_byte(Some(*b));
                 }
             };
         }
 
-        write_bytes!(&self.pc.to_le_bytes());
-        write_bytes!(&self.sp.to_le_bytes());
-        write_bytes!(&self.sr.to_le_bytes());
-        (0..4).for_each(|_| write_byte(None)); // Constant Generator (CG/R3)
+        write_le_bytes!(&self.pc);
+        write_le_bytes!(&self.sp);
+        write_le_bytes!(&self.sr);
+        (0..core::mem::size_of::<U>()).for_each(|_| write_byte(None)); // Constant Generator (CG/R3)
         for reg in self.r.iter() {
-            write_bytes!(&reg.to_le_bytes());
+            write_le_bytes!(&reg);
         }
     }
 
     fn gdb_deserialize(&mut self, bytes: &[u8]) -> Result<(), ()> {
-        // ensure bytes.chunks_exact(2) won't panic
-        if bytes.len() % 2 != 0 {
+        let ptrsize = core::mem::size_of::<U>();
+
+        // Ensure bytes contains enough data for all 16 registers
+        if bytes.len() < ptrsize * 16 {
             return Err(());
         }
 
-        use core::convert::TryInto;
         let mut regs = bytes
-            .chunks_exact(2)
-            .map(|c| u16::from_le_bytes(c.try_into().unwrap()));
+            .chunks_exact(ptrsize)
+            .map(|c| U::from_le_bytes(c).unwrap());
 
         self.pc = regs.next().ok_or(())?;
         self.sp = regs.next().ok_or(())?;
         self.sr = regs.next().ok_or(())?;
 
         // Constant Generator (CG/R3) should always be 0
-        if regs.next().ok_or(())? != 0 {
+        if regs.next().ok_or(())? != U::zero() {
             return Err(());
         }