Rework scheduler locking

This lays down some groundwork for v0.8, where the primary new feature
will be multiprocess support, which will be similar to multithreaded
support.

As discussed on the multiprocess github support issue, there's
a simpler way to support the scheduler locking behavior that we plan to
use for multiprocess targets, and since we're breaking the API anyways
with v0.8, it's a decent time to implement it for multithreaded targets
as well.

Author: jonathanzetier

docs/transition_guide.md

@@ -6,6 +6,14 @@ This document does _not_ discuss any new features that might have been added bet
 
 > _Note:_ after reading through this doc, you may also find it helpful to refer to the in-tree `armv4t` and `armv4t_multicore` examples when transitioning between versions.
 
+## `0.7` -> `0.8`
+
+#### Changes to multi-threaded resume behavior
+
+Previously, if a thread had no `resume_set_action_XXX` methods called on it, the default was to assume `set_resume_action_continue` was called on it.  Now, if a thread has no `set_resume_action_XXX` methods called on it, it should remain stopped.
+
+This new behavior obsoletes the `MultiThreadSchedulerLocking` trait, which has been removed.  If a stub is unable to handle executing a single thread and keeping all others locked, it should return an error in the `resume` method.
+
 ## `0.6` -> `0.7`
 
 `0.7` is a fairly minimal "cleanup" release, landing a collection of small breaking changes that collectively improve various ergonomic issues in `gdbstub`'s API.

example_no_std/src/gdb.rs

@@ -149,7 +149,7 @@ impl MultiThreadResume for DummyTarget {
     #[inline(never)]
     fn set_resume_action_continue(
         &mut self,
-        _tid: Tid,
+        _tid: Option<Tid>,
         _signal: Option<Signal>,
     ) -> Result<(), Self::Error> {
         print_str("> set_resume_action_continue");

examples/armv4t_multicore/emu.rs

@@ -36,11 +36,10 @@ pub enum Event {
     WatchRead(u32),
 }
 
-#[derive(PartialEq)]
+#[derive(Debug, PartialEq)]
 pub enum ExecMode {
     Step,
     Continue,
-    Stop,
 }
 
 /// incredibly barebones armv4t-based emulator
@@ -49,6 +48,7 @@ pub struct Emu {
     pub(crate) cop: Cpu,
     pub(crate) mem: ExampleMem,
 
+    // If a CpuId is not in this, it is presumed "stopped".
     pub(crate) exec_mode: HashMap<CpuId, ExecMode>,
 
     pub(crate) watchpoints: Vec<u32>,
@@ -189,7 +189,7 @@ impl Emu {
         let mut evt = None;
 
         for id in [CpuId::Cpu, CpuId::Cop].iter().copied() {
-            if matches!(self.exec_mode.get(&id), Some(ExecMode::Stop)) {
+            if !self.exec_mode.contains_key(&id) {
                 continue;
             }
 
@@ -204,11 +204,18 @@ impl Emu {
     }
 
     pub fn run(&mut self, mut poll_incoming_data: impl FnMut() -> bool) -> RunEvent {
+        if self.exec_mode.is_empty() {
+            // This should never happen (gdbstub will always ensure at least one
+            // `set_resume_action_XXX` method is called), but in case it does, we explicitly
+            // log it and return the closest event that represents this.
+            eprintln!("Running while all threads are stopped; this should never happen!  Treating as 0 steps");
+            return RunEvent::Event(Event::DoneStep, CpuId::Cpu);
+        }
+
         // The underlying armv4t_multicore emulator cycles all cores in lock-step.
         //
         // Inside `self.step()`, we iterate through all cores and only invoke
-        // `step_core` if that core's `ExecMode` is not `Stop`.
-
+        // `step_core` if that core has an `ExecMode`.
         let should_single_step = self.exec_mode.values().any(|mode| mode == &ExecMode::Step);
 
         match should_single_step {

examples/armv4t_multicore/gdb.rs

@@ -160,6 +160,10 @@ impl MultiThreadResume for Emu {
     }
 
     fn clear_resume_actions(&mut self) -> Result<(), Self::Error> {
+        // We're in all-stop mode (since `gdbstub` doesn't support non-stop yet), so
+        // the fact that we're processing commands from the GDB client means that all
+        // threads have already stopped, which is represented by the thread being
+        // absent from the `exec_mode` map.
         self.exec_mode.clear();
         Ok(())
     }
@@ -173,25 +177,27 @@ impl MultiThreadResume for Emu {
 
     fn set_resume_action_continue(
         &mut self,
-        tid: Tid,
+        tid: Option<Tid>,
         signal: Option<Signal>,
     ) -> Result<(), Self::Error> {
         if signal.is_some() {
             return Err("no support for continuing with signal");
         }
 
-        self.exec_mode
-            .insert(tid_to_cpuid(tid)?, ExecMode::Continue);
+        match tid {
+            None => {
+                for id in [CpuId::Cpu, CpuId::Cop] {
+                    self.exec_mode.entry(id).or_insert(ExecMode::Continue);
+                }
+            }
+            Some(tid) => {
+                self.exec_mode
+                    .insert(tid_to_cpuid(tid)?, ExecMode::Continue);
+            }
+        }
 
         Ok(())
     }
-
-    #[inline(always)]
-    fn support_scheduler_locking(
-        &mut self,
-    ) -> Option<target::ext::base::multithread::MultiThreadSchedulerLockingOps<'_, Self>> {
-        Some(self)
-    }
 }
 
 impl target::ext::base::multithread::MultiThreadSingleStep for Emu {
@@ -301,15 +307,6 @@ impl target::ext::thread_extra_info::ThreadExtraInfo for Emu {
     }
 }
 
-impl target::ext::base::multithread::MultiThreadSchedulerLocking for Emu {
-    fn set_resume_action_scheduler_lock(&mut self) -> Result<(), Self::Error> {
-        for id in [CpuId::Cpu, CpuId::Cop] {
-            self.exec_mode.entry(id).or_insert(ExecMode::Stop);
-        }
-        Ok(())
-    }
-}
-
 /// Copy all bytes of `data` to `buf`.
 /// Return the size of data copied.
 pub fn copy_to_buf(data: &[u8], buf: &mut [u8]) -> usize {

src/protocol/commands/_vCont.rs

@@ -24,8 +24,13 @@ impl<'a> ParseCommand<'a> for vCont<'a> {
     fn from_packet(buf: PacketBuf<'a>) -> Option<Self> {
         let body = buf.into_body();
         match body as &[u8] {
+            // From https://sourceware.org/gdb/current/onlinedocs/gdb.html/Packets.html#vCont-packet:
+            // "Specifying no actions is an error."
+            b"" => None,
             b"?" => Some(vCont::Query),
-            _ => Some(vCont::Actions(Actions::new_from_buf(body))),
+            [b';', rest @ ..] => Some(vCont::Actions(Actions::new_from_buf(rest))),
+            // Anything that doesn't start with a semicolon is malformed
+            _ => None
         }
     }
 }
@@ -52,7 +57,7 @@ impl<'a> Actions<'a> {
         Actions::FixedCont(tid)
     }
 
-    pub fn iter(&self) -> impl Iterator<Item = Option<VContAction<'a>>> + '_ {
+    pub fn iter(&self) -> impl DoubleEndedIterator<Item = Option<VContAction<'a>>> + '_ {
         match self {
             Actions::Buf(x) => EitherIter::A(x.iter()),
             Actions::FixedStep(x) => EitherIter::B(core::iter::once(Some(VContAction {
@@ -67,12 +72,16 @@ impl<'a> Actions<'a> {
     }
 }
 
+// This does not include the leading semicolon of the first action.
 #[derive(Debug)]
 pub struct ActionsBuf<'a>(&'a [u8]);
 
 impl<'a> ActionsBuf<'a> {
-    fn iter(&self) -> impl Iterator<Item = Option<VContAction<'a>>> + '_ {
-        self.0.split(|b| *b == b';').skip(1).map(|act| {
+    fn iter(&self) -> impl DoubleEndedIterator<Item = Option<VContAction<'a>>> + '_ {
+        // `ActionsBuf` doesn't include the leading semicolon in the first
+        // action, so we don't need to worry about the first element of the
+        // split being empty.
+        self.0.split(|b| *b == b';').map(|act| {
             let mut s = act.split(|b| *b == b':');
             let kind = s.next()?;
             let thread = match s.next() {
@@ -109,7 +118,7 @@ impl<'a> ActionsBuf<'a> {
                     //
                     // As a workaround for these weird GDB clients, `gdbstub`
                     // takes the pragmatic approach of treating this request as
-                    // though it the client requested _all_ threads to be
+                    // though the client requested _all_ threads to be
                     // resumed.
                     //
                     // If this turns out to be wrong... `gdbstub` can explore a
@@ -194,3 +203,17 @@ where
         }
     }
 }
+
+impl<A, B, T> DoubleEndedIterator for EitherIter<A, B>
+where
+    A: DoubleEndedIterator<Item = T>,
+    B: DoubleEndedIterator<Item = T>,
+{
+    #[inline(always)]
+    fn next_back(&mut self) -> Option<T> {
+        match self {
+            EitherIter::A(a) => a.next_back(),
+            EitherIter::B(b) => b.next_back(),
+        }
+    }
+}

src/stub/core_impl/resume.rs

@@ -89,6 +89,12 @@ impl<T: Target, C: Connection> GdbStubImpl<T, C> {
     ) -> Result<(), Error<T::Error, C::Error>> {
         use crate::protocol::commands::_vCont::VContKind;
 
+        // In the single-threaded scenario, we don't reverse the actions like we
+        // do in the multi-threaded: there are only two scenarios we concern
+        // ourselves with: 1 action, or 2 actions where the second is a
+        // continue action (sometimes GDB sends a packet of the form
+        // `vCont;s:foo;c`, even in single-threaded scenarios). We ignore the
+        // continue action, since there aren't any other threads to continue.
         let mut actions = actions.iter();
         let first_action = actions
             .next()
@@ -166,14 +172,16 @@ impl<T: Target, C: Connection> GdbStubImpl<T, C> {
     ) -> Result<(), Error<T::Error, C::Error>> {
         ops.clear_resume_actions().map_err(Error::TargetError)?;
 
-        // Track whether the packet contains a wildcard/default continue action
-        // (e.g., `c` or `c:-1`).
+        // NOTE: We iterate through these actions in reverse order, which corresponds to
+        // a right-to-left ordering of the actions specified in the vCont packet. This
+        // is intentionally the opposite of the left-to-right order specified by
+        // the vCont packet documentation.
         //
-        // Presence of this action implies "Scheduler Locking" is OFF.
-        // Absence implies "Scheduler Locking" is ON.
-        let mut has_wildcard_continue = false;
-
-        for action in actions.iter() {
+        // This is to simplify target implementations: each `set_resume_action_XXX`
+        // callback can overwrite the current state, instead of having to keep track of
+        // each thread specified by previous actions and making sure they don't get
+        // overwritten.
+        for action in actions.iter().rev() {
             use crate::protocol::commands::_vCont::VContKind;
 
             let action = action.ok_or(Error::PacketParse(
@@ -187,17 +195,15 @@ impl<T: Target, C: Connection> GdbStubImpl<T, C> {
                         _ => None,
                     };
 
-                    match action.thread.map(|thread| thread.tid) {
-                        // An action with no thread-id matches all threads
-                        None | Some(SpecificIdKind::All) => {
-                            // Target API contract specifies that the default
-                            // resume action for all threads is continue.
-                            has_wildcard_continue = true;
-                        }
-                        Some(SpecificIdKind::WithId(tid)) => ops
-                            .set_resume_action_continue(tid, signal)
-                            .map_err(Error::TargetError)?,
-                    }
+                    let tid = match action.thread.map(|thread| thread.tid) {
+                        // An action with no thread-id matches all threads, which is passed to
+                        // `set_resume_action_continue` as `None`.
+                        None | Some(SpecificIdKind::All) => None,
+                        Some(SpecificIdKind::WithId(tid)) => Some(tid),
+                    };
+
+                    ops.set_resume_action_continue(tid, signal)
+                        .map_err(Error::TargetError)?;
                 }
                 VContKind::Step | VContKind::StepWithSig(_)
                     if ops.support_single_step().is_some() =>
@@ -259,15 +265,6 @@ impl<T: Target, C: Connection> GdbStubImpl<T, C> {
             }
         }
 
-        if !has_wildcard_continue {
-            let Some(locking_ops) = ops.support_scheduler_locking() else {
-                return Err(Error::MissingMultiThreadSchedulerLocking);
-            };
-            locking_ops
-                .set_resume_action_scheduler_lock()
-                .map_err(Error::TargetError)?;
-        }
-
         ops.resume().map_err(Error::TargetError)
     }
 

src/stub/error.rs

@@ -43,7 +43,6 @@ pub(crate) enum InternalError<T, C> {
     MissingCurrentActivePidImpl,
     TracepointFeatureUnimplemented(u8),
     TracepointUnsupportedSourceEnumeration,
-    MissingMultiThreadSchedulerLocking,
     MissingToRawId,
 
     // Internal - A non-fatal error occurred (with errno-style error code)
@@ -149,7 +148,6 @@ where
             MissingCurrentActivePidImpl => write!(f, "GDB client attempted to attach to a new process, but the target has not implemented support for `ExtendedMode::support_current_active_pid`"),
             TracepointFeatureUnimplemented(feat) => write!(f, "GDB client sent us a tracepoint packet using feature {}, but `gdbstub` doesn't implement it. If this is something you require, please file an issue at https://github.com/daniel5151/gdbstub/issues", *feat as char),
             TracepointUnsupportedSourceEnumeration => write!(f, "The target doesn't support the gdbstub TracepointSource extension, but attempted to transition to enumerating tracepoint sources"),
-            MissingMultiThreadSchedulerLocking => write!(f, "GDB requested Scheduler Locking, but the Target does not implement the `MultiThreadSchedulerLocking` IDET"),
             MissingToRawId => write!(f, "A RegId was used with an API that requires raw register IDs to be available (e.g. `report_stop_with_regs`) but returned `None` from `to_raw_id()`"),
 
             NonFatalError(_) => write!(f, "Internal non-fatal error. You should never see this! Please file an issue if you do!"),