diff --git a/docs/transition_guide.md b/docs/transition_guide.md index 3ae106cb..0f7c0385 100644 --- a/docs/transition_guide.md +++ b/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. diff --git a/example_no_std/src/gdb.rs b/example_no_std/src/gdb.rs index f724fac4..de105cc5 100644 --- a/example_no_std/src/gdb.rs +++ b/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, _signal: Option, ) -> Result<(), Self::Error> { print_str("> set_resume_action_continue"); diff --git a/examples/armv4t_multicore/emu.rs b/examples/armv4t_multicore/emu.rs index 7e8595fc..794ee9a0 100644 --- a/examples/armv4t_multicore/emu.rs +++ b/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, pub(crate) watchpoints: Vec, @@ -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 { diff --git a/examples/armv4t_multicore/gdb.rs b/examples/armv4t_multicore/gdb.rs index e6fb556c..f436e4b7 100644 --- a/examples/armv4t_multicore/gdb.rs +++ b/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, signal: Option, ) -> 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> { - 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 { diff --git a/src/protocol/commands/_vCont.rs b/src/protocol/commands/_vCont.rs index d0cb4ced..801f7713 100644 --- a/src/protocol/commands/_vCont.rs +++ b/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>> + '_ { + pub fn iter(&self) -> impl DoubleEndedIterator>> + '_ { 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>> + '_ { - self.0.split(|b| *b == b';').skip(1).map(|act| { + fn iter(&self) -> impl DoubleEndedIterator>> + '_ { + // `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 DoubleEndedIterator for EitherIter +where + A: DoubleEndedIterator, + B: DoubleEndedIterator, +{ + #[inline(always)] + fn next_back(&mut self) -> Option { + match self { + EitherIter::A(a) => a.next_back(), + EitherIter::B(b) => b.next_back(), + } + } +} diff --git a/src/stub/core_impl/resume.rs b/src/stub/core_impl/resume.rs index f04b529c..f76793cc 100644 --- a/src/stub/core_impl/resume.rs +++ b/src/stub/core_impl/resume.rs @@ -89,6 +89,12 @@ impl GdbStubImpl { ) -> Result<(), 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 GdbStubImpl { ) -> Result<(), 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 GdbStubImpl { _ => 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 GdbStubImpl { } } - 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) } diff --git a/src/stub/error.rs b/src/stub/error.rs index b331c503..3cf9c9fe 100644 --- a/src/stub/error.rs +++ b/src/stub/error.rs @@ -43,7 +43,6 @@ pub(crate) enum InternalError { 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!"), diff --git a/src/target/ext/base/multithread.rs b/src/target/ext/base/multithread.rs index 640986bd..7031437c 100644 --- a/src/target/ext/base/multithread.rs +++ b/src/target/ext/base/multithread.rs @@ -51,9 +51,9 @@ pub trait MultiThreadBase: Target { /// to indicate that memory starting at `start_addr + n` cannot be /// accessed. /// - /// Implemenations may also return an appropriate non-fatal error if the + /// Implementations may also return an appropriate non-fatal error if the /// requested address range could not be accessed (e.g: due to MMU - /// protection, unhanded page fault, etc...). + /// protection, unhandled page fault, etc...). /// /// Implementations must guarantee that the returned number is less than or /// equal `data.len()`. @@ -67,7 +67,7 @@ pub trait MultiThreadBase: Target { /// Write bytes to the specified address range. /// /// If the requested address range could not be accessed (e.g: due to - /// MMU protection, unhanded page fault, etc...), an appropriate non-fatal + /// MMU protection, unhandled page fault, etc...), an appropriate non-fatal /// error should be returned. fn write_addrs( &mut self, @@ -132,21 +132,21 @@ pub trait MultiThreadResume: Target { /// Upon returning from the `resume` method, the target being debugged /// should be configured to run according to whatever resume actions the /// GDB client had specified using any of the `set_resume_action_XXX` - /// methods. - /// - /// # Default Resume Behavior - /// - /// By default, any thread that wasn't explicitly resumed by a - /// `set_resume_action_XXX` method should be resumed as though it was - /// resumed with `set_resume_action_continue`. - /// - /// **However**, if [`support_scheduler_locking`] is implemented and - /// [`set_resume_action_scheduler_lock`] has been called for the current - /// resume cycle, this default changes: **unmentioned threads must remain - /// stopped.** - /// - /// [`support_scheduler_locking`]: Self::support_scheduler_locking - /// [`set_resume_action_scheduler_lock`]: MultiThreadSchedulerLocking::set_resume_action_scheduler_lock + /// methods. Any thread that wasn't explicitly specified in a + /// `set_resume_action_XXX` method should remain in the same state. + /// Currently, because `gdbstub` only supports all-stop mode, the only state + /// threads will be in while processing `resume` is stopped, so any threads + /// not specified in `set_resume_action_XXX` should remain stopped. + /// + /// If a thread is specified in multiple `set_resume_action_XXX` methods, + /// **the last call** takes precedence. A common situation when this happens + /// is if a `set_resume_action_XXX` method specifies all threads, and then + /// a following call to `set_resume_action_XXX` method specifies one + /// specific thread. The second call to the specific thread takes + /// precedence. + /// + /// Implementations may assume that at least one `set_resume_action_XXX` was + /// called between `clear_resume_actions` and `resume`. /// /// # Protocol Extensions /// @@ -158,20 +158,26 @@ pub trait MultiThreadResume: Target { /// ----------------------------|------------------------------ /// Optimized [Single Stepping] | See [`support_single_step()`] /// Optimized [Range Stepping] | See [`support_range_step()`] - /// [Scheduler Locking] | See [`support_scheduler_locking()`] /// "Stop" | Used in "Non-Stop" mode \* /// /// \* "Non-Stop" mode is currently unimplemented in `gdbstub` /// /// [Single stepping]: https://sourceware.org/gdb/current/onlinedocs/gdb/Continuing-and-Stepping.html#index-stepi /// [Range Stepping]: https://sourceware.org/gdb/current/onlinedocs/gdb/Continuing-and-Stepping.html#range-stepping - /// [Scheduler Locking]: https://sourceware.org/gdb/current/onlinedocs/gdb#index-scheduler-locking-mode /// [`support_single_step()`]: Self::support_single_step /// [`support_range_step()`]: Self::support_range_step - /// [`support_scheduler_locking()`]: Self::support_scheduler_locking /// /// # Additional Considerations /// + /// ### Scheduler locking + /// + /// The GDB client may send a resume message that only continues one thread + /// and keeps all other threads in a halted state. Not all OS schedulers are + /// able to handle this, so `resume` should return an error if it's ever + /// asked to do something that the stub is unable to support. More details + /// can be found in the `set scheduler-locking mode` section of the + /// [GDB documentation](https://sourceware.org/gdb/current/onlinedocs/gdb#index-scheduler-locking-mode) + /// /// ### Adjusting PC after a breakpoint is hit /// /// The [GDB remote serial protocol documentation](https://sourceware.org/gdb/current/onlinedocs/gdb/Stop-Reply-Packets.html#swbreak-stop-reason) @@ -202,6 +208,8 @@ pub trait MultiThreadResume: Target { /// Continue the specified thread. /// + /// If no thread is specified, continue all threads. + /// /// See the [`resume`](Self::resume) docs for information on when this is /// called. /// @@ -209,7 +217,7 @@ pub trait MultiThreadResume: Target { /// target. fn set_resume_action_continue( &mut self, - tid: Tid, + tid: Option, signal: Option, ) -> Result<(), Self::Error>; @@ -248,14 +256,6 @@ pub trait MultiThreadResume: Target { ) -> Option> { None } - - /// Support for [scheduler locking]. - /// - /// [scheduler locking]: https://sourceware.org/gdb/current/onlinedocs/gdb#index-scheduler-locking-mode - #[inline(always)] - fn support_scheduler_locking(&mut self) -> Option> { - None - } } define_ext!(MultiThreadResumeOps, MultiThreadResume); @@ -313,26 +313,3 @@ pub trait MultiThreadRangeStepping: Target + MultiThreadResume { } define_ext!(MultiThreadRangeSteppingOps, MultiThreadRangeStepping); - -/// Target Extension - support for GDB's "Scheduler Locking" mode. -/// See [`MultiThreadResume::support_scheduler_locking`]. -pub trait MultiThreadSchedulerLocking: Target + MultiThreadResume { - /// Configure the target to enable "Scheduler Locking" for the upcoming - /// resume. - /// - /// This method is invoked when the GDB client expects only a specific set - /// of threads to run, while all other threads remain frozen. This behavior - /// is typically toggled in GDB using the `set scheduler-locking on` - /// command. - /// - /// When this method is called, the implementation must ensure that any - /// threads not explicitly resumed via previous `set_resume_action_...` - /// calls **remain stopped**. - /// - /// This prevents any "implicit continue" behavior for unmentioned threads, - /// satisfying GDB's expectation that only the designated threads will - /// advance during the next resume. - fn set_resume_action_scheduler_lock(&mut self) -> Result<(), Self::Error>; -} - -define_ext!(MultiThreadSchedulerLockingOps, MultiThreadSchedulerLocking);