Rework scheduler locking

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;
             }
 
@@ -207,8 +207,7 @@ impl Emu {
         // 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

@@ -25,7 +25,8 @@ impl<'a> ParseCommand<'a> for vCont<'a> {
         let body = buf.into_body();
         match body as &[u8] {
             b"?" => Some(vCont::Query),
-            _ => Some(vCont::Actions(Actions::new_from_buf(body))),
+            [b';', rest @ ..] => Some(vCont::Actions(Actions::new_from_buf(rest))),
+            _ => None
         }
     }
 }
@@ -52,7 +53,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 +68,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 +114,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 +199,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!"),