finally clean up multiprocess extension support

Ughh, it took a long while, but I've finally come up with an API for
multiprocess support that I'm happy with, _and_ fixed some icky hacks
and bad implementation choices in the core gdbstub implementation.

Some interesting things that come out of this (chonker of a) commit:

- Turns out the `qC` packet is totally useless, so I've gone ahead and
removed it entirely. The less code the better.
- I've finally fixed the incredibly confusing naming scheme surrounding
the thread-id structures. The new names should be much more sane (and
have less overlap)
- I've massively improved the `resume` method for multithreaded targets.
The key insight is that returning a tuple of `(Tid, StopReason)` wasn't
exactly correct, as there are certain stop reasons that are _not_
associated with any Tid (e.g: GDB interrupt), and it effectively forced
the target to "flip a coin" as to which Tid it reported. This old system
wreaked havoc when dealing with breakpoints in multithreaded systems,
but with the new `ThreadStopReason` API, i've managed to clean up the
library internals to keep the GDB nice and happy, and removed the burden
of making arbitrary decisions from the Targer implementation.
- vCont works now! I'm a dummy, and never actually realized that my
vCont implementation was totally busted. I've gone ahead and fixed it,
so it should be working now.

.gitignore

@@ -1,3 +1,7 @@
 /target
 **/*.rs.bk
 Cargo.lock
+
+**/.gdb_history
+# GDB will core dump if the target is implemented incorrectly (which it often is during debugging)
+**/core

examples/armv4t/gdb.rs

@@ -3,7 +3,7 @@ use core::convert::TryInto;
 use armv4t_emu::{reg, Memory};
 use gdbstub::arch;
 use gdbstub::arch::arm::reg::ArmCoreRegId;
-use gdbstub::target::base::{ResumeAction, StopReason};
+use gdbstub::target::base::singlethread::{ResumeAction, SingleThreadOps, StopReason};
 use gdbstub::target::ext::breakpoint::WatchKind;
 use gdbstub::target::{base, ext, Target};
 
@@ -38,7 +38,7 @@ impl Target for Emu {
     }
 }
 
-impl base::SingleThread for Emu {
+impl SingleThreadOps for Emu {
     fn resume(
         &mut self,
         action: ResumeAction,

examples/armv4t_multicore/emu.rs

@@ -1,3 +1,16 @@
+//! ------------------------------------------------------------------------ !//
+//! ------------------------------ DISCLAIMER ------------------------------ !//
+//! ------------------------------------------------------------------------ !//
+//!
+//! This code is absolutely awful, and completely slapped together for the sake
+//! of example. The watchpoint implementation is particularly awful.
+//!
+//! While it technically "gets the job done" and provides a simple multicore
+//! system that can be debugged, it would really merit a re-write, since it's
+//! not a good example of "proper Rust coding practices"
+
+use std::collections::HashMap;
+
 use armv4t_emu::{reg, Cpu, ExampleMem, Memory, Mode};
 
 use crate::mem_sniffer::{AccessKind, MemSniffer};
@@ -26,7 +39,13 @@ pub struct Emu {
     pub(crate) mem: ExampleMem,
 
     pub(crate) watchpoints: Vec<u32>,
+    /// (read, write)
+    pub(crate) watchpoint_kind: HashMap<u32, (bool, bool)>,
     pub(crate) breakpoints: Vec<u32>,
+
+    // GDB seems to get gets very confused if two threads are executing the exact same code at the
+    // exact same time. Maybe this is a bug with `gdbstub`?
+    stall_cop_cycles: usize,
 }
 
 impl Emu {
@@ -70,14 +89,20 @@ impl Emu {
             cop,
             mem,
             watchpoints: Vec::new(),
+            watchpoint_kind: HashMap::new(),
             breakpoints: Vec::new(),
+            stall_cop_cycles: 24,
         })
     }
 
     pub fn step_core(&mut self, id: CpuId) -> Option<Event> {
         let cpu = match id {
-            CpuId::Cpu => &mut self.cpu,
+            CpuId::Cop if self.stall_cop_cycles != 0 => {
+                self.stall_cop_cycles -= 1;
+                return None;
+            }
             CpuId::Cop => &mut self.cop,
+            CpuId::Cpu => &mut self.cpu,
         };
 
         // set up magic memory location
@@ -97,27 +122,49 @@ impl Emu {
         cpu.step(&mut sniffer);
         let pc = cpu.reg_get(Mode::User, reg::PC);
 
-        if let Some(access) = hit_watchpoint {
-            let fixup = if cpu.thumb_mode() { 2 } else { 4 };
-            cpu.reg_set(Mode::User, reg::PC, pc - fixup);
-
-            return Some(match access.kind {
-                AccessKind::Read => Event::WatchRead(access.addr),
-                AccessKind::Write => Event::WatchWrite(access.addr),
-            });
-        }
-
-        if self.breakpoints.contains(&pc) {
-            return Some(Event::Break);
-        }
-
         if pc == HLE_RETURN_ADDR {
             match id {
                 CpuId::Cpu => return Some(Event::Halted),
                 CpuId::Cop => return Some(Event::Halted),
             }
         }
 
+        if let Some(access) = hit_watchpoint {
+            // NOTE: this isn't a particularly elegant way to do watchpoints! This works
+            // fine for some example code, but don't use this as inspiration in your own
+            // emulator!
+            match access.kind {
+                AccessKind::Read => {
+                    if *self
+                        .watchpoint_kind
+                        .get(&access.addr)
+                        .map(|(r, _w)| r)
+                        .unwrap_or(&false)
+                    {
+                        let fixup = if cpu.thumb_mode() { 2 } else { 4 };
+                        cpu.reg_set(Mode::User, reg::PC, pc - fixup);
+                        return Some(Event::WatchRead(access.addr));
+                    }
+                }
+                AccessKind::Write => {
+                    if *self
+                        .watchpoint_kind
+                        .get(&access.addr)
+                        .map(|(_r, w)| w)
+                        .unwrap_or(&false)
+                    {
+                        let fixup = if cpu.thumb_mode() { 2 } else { 4 };
+                        cpu.reg_set(Mode::User, reg::PC, pc - fixup);
+                        return Some(Event::WatchWrite(access.addr));
+                    }
+                }
+            }
+        }
+
+        if self.breakpoints.contains(&pc) {
+            return Some(Event::Break);
+        }
+
         None
     }
 

examples/armv4t_multicore/gdb.rs

@@ -1,21 +1,26 @@
 use armv4t_emu::{reg, Memory};
 use gdbstub::arch;
-use gdbstub::target::base::{Actions, ResumeAction, StopReason, Tid};
+use gdbstub::target::base::multithread::{
+    Actions, MultiThreadOps, ResumeAction, ThreadStopReason, Tid,
+};
 use gdbstub::target::ext::breakpoint::WatchKind;
 use gdbstub::target::ext::monitor::{outputln, ConsoleOutput};
 use gdbstub::target::{base, ext, Target};
 
 use crate::emu::{CpuId, Emu, Event};
 
-fn event_to_stopreason(e: Event) -> StopReason<u32> {
+fn event_to_stopreason(e: Event, id: CpuId) -> ThreadStopReason<u32> {
+    let tid = cpuid_to_tid(id);
     match e {
-        Event::Halted => StopReason::Halted,
-        Event::Break => StopReason::HwBreak,
-        Event::WatchWrite(addr) => StopReason::Watch {
+        Event::Halted => ThreadStopReason::Halted,
+        Event::Break => ThreadStopReason::SwBreak(tid),
+        Event::WatchWrite(addr) => ThreadStopReason::Watch {
+            tid,
             kind: WatchKind::Write,
             addr,
         },
-        Event::WatchRead(addr) => StopReason::Watch {
+        Event::WatchRead(addr) => ThreadStopReason::Watch {
+            tid,
             kind: WatchKind::Read,
             addr,
         },
@@ -58,49 +63,41 @@ impl Target for Emu {
     }
 }
 
-impl base::MultiThread for Emu {
+impl MultiThreadOps for Emu {
     fn resume(
         &mut self,
         actions: Actions,
         check_gdb_interrupt: &mut dyn FnMut() -> bool,
-    ) -> Result<(Tid, StopReason<u32>), Self::Error> {
-        // in this emulator, we ignore the Tid associated with the action, and only care
-        // if GDB requests execution to start / stop. Each core runs in lock-step.
+    ) -> Result<ThreadStopReason<u32>, Self::Error> {
+        // in this emulator, each core runs in lock-step, so we can ignore the
+        // TidSelector associated with each action, and only care if GDB
+        // requests execution to start / stop.
         //
         // In general, the behavior of multi-threaded systems during debugging is
         // determined by the system scheduler. On certain systems, this behavior can be
         // configured using the GDB command `set scheduler-locking _mode_`, but at the
-        // moment, `gdbstub` doesn't plumb-through that option.
+        // moment, `gdbstub` doesn't plumb-through that configuration command.
 
+        // FIXME: properly handle multiple actions...
         let actions = actions.collect::<Vec<_>>();
-        if actions.len() != 1 {
-            // AFAIK, this will never happen on such a simple system. Plus, it's just an
-            // example, cut me some slack!
-            return Err("too lazy to implement support for more than one action :P");
-        }
-        let (tid_selector, action) = actions[0];
-
-        let tid = match tid_selector {
-            base::TidSelector::WithID(id) => id,
-            _ => cpuid_to_tid(CpuId::Cpu), // ...
-        };
+        let (_, action) = actions[0];
 
         match action {
             ResumeAction::Step => match self.step() {
-                Some((event, id)) => Ok((cpuid_to_tid(id), event_to_stopreason(event))),
-                None => Ok((tid, StopReason::DoneStep)),
+                Some((event, id)) => Ok(event_to_stopreason(event, id)),
+                None => Ok(ThreadStopReason::DoneStep),
             },
             ResumeAction::Continue => {
                 let mut cycles: usize = 0;
                 loop {
                     // check for GDB interrupt every 1024 instructions
                     if cycles % 1024 == 0 && check_gdb_interrupt() {
-                        return Ok((tid, StopReason::GdbInterrupt));
+                        return Ok(ThreadStopReason::GdbInterrupt);
                     }
                     cycles += 1;
 
                     if let Some((event, id)) = self.step() {
-                        return Ok((cpuid_to_tid(id), event_to_stopreason(event)));
+                        return Ok(event_to_stopreason(event, id));
                     };
                 }
             }
@@ -205,27 +202,34 @@ impl ext::breakpoint::SwBreakpoint for Emu {
 
 impl ext::breakpoint::HwWatchpoint for Emu {
     fn add_hw_watchpoint(&mut self, addr: u32, kind: WatchKind) -> Result<bool, &'static str> {
+        self.watchpoints.push(addr);
+
+        let entry = self.watchpoint_kind.entry(addr).or_insert((false, false));
         match kind {
-            WatchKind::Write => self.watchpoints.push(addr),
-            WatchKind::Read => self.watchpoints.push(addr),
-            WatchKind::ReadWrite => self.watchpoints.push(addr),
+            WatchKind::Write => entry.1 = true,
+            WatchKind::Read => entry.0 = true,
+            WatchKind::ReadWrite => entry.0 = true, // arbitrary
         };
 
         Ok(true)
     }
 
     fn remove_hw_watchpoint(&mut self, addr: u32, kind: WatchKind) -> Result<bool, &'static str> {
-        let pos = match self.watchpoints.iter().position(|x| *x == addr) {
-            None => return Ok(false),
-            Some(pos) => pos,
-        };
-
+        let entry = self.watchpoint_kind.entry(addr).or_insert((false, false));
         match kind {
-            WatchKind::Write => self.watchpoints.remove(pos),
-            WatchKind::Read => self.watchpoints.remove(pos),
-            WatchKind::ReadWrite => self.watchpoints.remove(pos),
+            WatchKind::Write => entry.1 = false,
+            WatchKind::Read => entry.0 = false,
+            WatchKind::ReadWrite => entry.0 = false, // arbitrary
         };
 
+        if !self.watchpoint_kind.contains_key(&addr) {
+            let pos = match self.watchpoints.iter().position(|x| *x == addr) {
+                None => return Ok(false),
+                Some(pos) => pos,
+            };
+            self.watchpoints.remove(pos);
+        }
+
         Ok(true)
     }
 }

examples/armv4t_multicore/mem_sniffer.rs

@@ -1,10 +1,12 @@
 use armv4t_emu::Memory;
 
+#[derive(Debug)]
 pub enum AccessKind {
     Read,
     Write,
 }
 
+#[derive(Debug)]
 pub struct Access {
     pub kind: AccessKind,
     pub addr: u32,

src/gdbstub_impl/error.rs

@@ -21,9 +21,10 @@ pub enum GdbStubError<T, C> {
     PacketUnexpected,
     /// Target threw a fatal error.
     TargetError(T),
-    /// Target doesn't implement `set_current_thread`, but reported multiple
-    /// threads.
-    MissingSetCurrentTid,
+    /// Target didn't report any active threads.
+    NoActiveThreads,
+    /// Resuming with a signal is not implemented yet. Consider opening a PR?
+    ResumeWithSignalUnimplemented,
 }
 
 impl<T, C> From<ResponseWriterError<C>> for GdbStubError<T, C> {
@@ -53,7 +54,8 @@ where
             PacketParse => write!(f, "Could not parse the packet into a valid command."),
             PacketUnexpected => write!(f, "Client sent an unexpected packet."),
             TargetError(e) => write!(f, "Target threw a fatal error: {:?}", e),
-            MissingSetCurrentTid => write!(f, "Target doesn't implement `set_current_thread`, but reported multiple threads."),
+            NoActiveThreads => write!(f, "Target didn't report any active threads."),
+            ResumeWithSignalUnimplemented => write!(f, "Resuming with a signal is not implemented yet. Consider opening a PR?")
         }
     }
 }