diff --git a/docs/transition_guide.md b/docs/transition_guide.md index 0f7c0385..e3e2715b 100644 --- a/docs/transition_guide.md +++ b/docs/transition_guide.md @@ -14,6 +14,10 @@ Previously, if a thread had no `resume_set_action_XXX` methods called on it, the 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. +#### The `CurrentActivePid` trait no longer exists + +The current PID is now tracked in the stub itself, so targets no longer need to implement this. No changes are needed for the `attach` 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/examples/armv4t/gdb/extended_mode.rs b/examples/armv4t/gdb/extended_mode.rs index 31116f92..20861dac 100644 --- a/examples/armv4t/gdb/extended_mode.rs +++ b/examples/armv4t/gdb/extended_mode.rs @@ -100,13 +100,6 @@ impl target::ext::extended_mode::ExtendedMode for Emu { ) -> Option> { Some(self) } - - #[inline(always)] - fn support_current_active_pid( - &mut self, - ) -> Option> { - Some(self) - } } impl target::ext::extended_mode::ConfigureAslr for Emu { @@ -169,9 +162,3 @@ impl target::ext::extended_mode::ConfigureWorkingDir for Emu { Ok(()) } } - -impl target::ext::extended_mode::CurrentActivePid for Emu { - fn current_active_pid(&mut self) -> Result { - Ok(self.reported_pid) - } -} diff --git a/src/stub/core_impl.rs b/src/stub/core_impl.rs index 125671c7..24f31679 100644 --- a/src/stub/core_impl.rs +++ b/src/stub/core_impl.rs @@ -1,4 +1,5 @@ use crate::common::IsValidTid; +use crate::common::Pid; use crate::common::Signal; use crate::conn::Connection; use crate::protocol::commands::Command; @@ -7,6 +8,7 @@ use crate::protocol::ResponseWriter; use crate::protocol::SpecificIdKind; use crate::stub::error::InternalError; use crate::target::Target; +use crate::FAKE_PID; use crate::SINGLE_THREAD_TID; use core::marker::PhantomData; @@ -105,6 +107,11 @@ pub(crate) struct GdbStubImpl { _target: PhantomData, _connection: PhantomData, + // The most recently attached PID + current_active_pid: Pid, + + // The TID (and when multiprocess support added, PID) used for memory/register + // operations current_mem_tid: T::Tid, current_resume_tid: SpecificIdKind, features: ProtocolFeatures, @@ -123,14 +130,16 @@ impl GdbStubImpl { _target: PhantomData, _connection: PhantomData, - // NOTE: `current_mem_tid` and `current_resume_tid` are never queried prior to being set - // by the GDB client (via the 'H' packet), so it's fine to use dummy values here. + // NOTE: `current_active_pid`, `current_mem_tid` and `current_resume_tid` are never + // queried prior to being set by the GDB client (via the 'H' packet), so + // it's fine to use dummy values here. // // The alternative would be to use `Option`, and while this would be more "correct", it // would introduce a _lot_ of noisy and heavy error handling logic all over the place. // // Plus, even if the GDB client is acting strangely and doesn't overwrite these values, // the target will simply return a non-fatal error, which is totally fine. + current_active_pid: FAKE_PID, current_mem_tid: T::Tid::sentinel(), // FUTURE: this should get switched over to T::Tid as well current_resume_tid: SpecificIdKind::WithId(SINGLE_THREAD_TID), diff --git a/src/stub/core_impl/base.rs b/src/stub/core_impl/base.rs index ce688cc9..3820bdda 100644 --- a/src/stub/core_impl/base.rs +++ b/src/stub/core_impl/base.rs @@ -2,7 +2,6 @@ use super::prelude::*; use super::DisconnectReason; use crate::arch::Arch; use crate::arch::Registers; -use crate::common::Pid; use crate::common::Tid; use crate::protocol::commands::ext::Base; use crate::protocol::IdKind; @@ -10,7 +9,6 @@ use crate::protocol::SpecificIdKind; use crate::protocol::SpecificThreadId; use crate::target::ext::base::BaseOps; use crate::target::ext::base::ResumeOps; -use crate::FAKE_PID; use crate::SINGLE_THREAD_TID; impl GdbStubImpl { @@ -38,20 +36,6 @@ impl GdbStubImpl { Ok(tid) } - pub(crate) fn get_current_pid( - &mut self, - target: &mut T, - ) -> Result> { - if let Some(ops) = target - .support_extended_mode() - .and_then(|ops| ops.support_current_active_pid()) - { - ops.current_active_pid().map_err(Error::TargetError) - } else { - Ok(FAKE_PID) - } - } - // Used by `?` and `vAttach` to return a "reasonable" stop reason. // // This is a bit of an implementation wart, since this is really something @@ -71,7 +55,7 @@ impl GdbStubImpl { pid: self .features .multiprocess() - .then_some(SpecificIdKind::WithId(self.get_current_pid(target)?)), + .then_some(SpecificIdKind::WithId(self.current_active_pid)), tid: SpecificIdKind::WithId(tid), })?; } else { @@ -402,7 +386,7 @@ impl GdbStubImpl { } Base::qfThreadInfo(_) => { res.write_str("m")?; - let pid = self.get_current_pid(target)?; + let pid = self.current_active_pid; match target.base_ops() { BaseOps::SingleThread(_) => res.write_specific_thread_id(SpecificThreadId { diff --git a/src/stub/core_impl/extended_mode.rs b/src/stub/core_impl/extended_mode.rs index 49121abf..08a57515 100644 --- a/src/stub/core_impl/extended_mode.rs +++ b/src/stub/core_impl/extended_mode.rs @@ -3,6 +3,7 @@ use crate::protocol::commands::ext::ExtendedMode; use crate::protocol::SpecificIdKind; use crate::protocol::SpecificThreadId; use crate::target::ext::base::BaseOps; +use crate::FAKE_PID; impl GdbStubImpl { pub(crate) fn handle_extended_mode( @@ -28,18 +29,16 @@ impl GdbStubImpl { HandlerStatus::Handled } ExtendedMode::vAttach(cmd) => { - if ops.support_current_active_pid().is_none() { - return Err(Error::MissingCurrentActivePidImpl); + if let Err(e) = ops.attach(cmd.pid).handle_error() { + self.current_active_pid = FAKE_PID; + return Err(e); } - - ops.attach(cmd.pid).handle_error()?; + self.current_active_pid = cmd.pid; self.report_reasonable_stop_reason(res, target)? } - ExtendedMode::qC(_cmd) if ops.support_current_active_pid().is_some() => { - let ops = ops.support_current_active_pid().unwrap(); - + ExtendedMode::qC(_cmd) => { res.write_str("QC")?; - let pid = ops.current_active_pid().map_err(Error::TargetError)?; + let pid = self.current_active_pid; let tid = match target.base_ops() { BaseOps::SingleThread(_) => T::Tid::sentinel(), BaseOps::MultiThread(ops) => { diff --git a/src/stub/core_impl/resume.rs b/src/stub/core_impl/resume.rs index 22d0cda1..32ee4025 100644 --- a/src/stub/core_impl/resume.rs +++ b/src/stub/core_impl/resume.rs @@ -293,7 +293,6 @@ impl GdbStubImpl { fn write_stop_common( &mut self, res: &mut ResponseWriter<'_, C>, - target: &mut T, tid: Option, signal: Signal, ) -> Result<(), Error> { @@ -309,7 +308,7 @@ impl GdbStubImpl { pid: self .features .multiprocess() - .then_some(SpecificIdKind::WithId(self.get_current_pid(target)?)), + .then_some(SpecificIdKind::WithId(self.current_active_pid)), tid: SpecificIdKind::WithId(tid.into_fully_qualified_tid()), })?; res.write_str(";")?; @@ -365,11 +364,10 @@ impl GdbStubImpl { pub(crate) fn finish_signal_with_thread( &mut self, res: &mut ResponseWriter<'_, C>, - target: &mut T, tid: T::Tid, sig: Signal, ) -> Result<(), Error> { - self.write_stop_common(res, target, Some(tid), sig)?; + self.write_stop_common(res, Some(tid), sig)?; Ok(()) } @@ -390,7 +388,7 @@ impl GdbStubImpl { crate::__dead_code_marker!("sw_breakpoint", "stop_reason"); - self.write_stop_common(res, target, Some(tid), Signal::SIGTRAP)?; + self.write_stop_common(res, Some(tid), Signal::SIGTRAP)?; res.write_str("swbreak:;")?; Ok(()) } @@ -412,7 +410,7 @@ impl GdbStubImpl { crate::__dead_code_marker!("hw_breakpoint", "stop_reason"); - self.write_stop_common(res, target, Some(tid), Signal::SIGTRAP)?; + self.write_stop_common(res, Some(tid), Signal::SIGTRAP)?; res.write_str("hwbreak:;")?; Ok(()) } @@ -436,7 +434,7 @@ impl GdbStubImpl { crate::__dead_code_marker!("hw_watchpoint", "stop_reason"); - self.write_stop_common(res, target, Some(tid), Signal::SIGTRAP)?; + self.write_stop_common(res, Some(tid), Signal::SIGTRAP)?; res.write_str(match kind { WatchKind::Write => "watch:", @@ -479,7 +477,7 @@ impl GdbStubImpl { crate::__dead_code_marker!("reverse_exec", "stop_reason"); - self.write_stop_common(res, target, tid, Signal::SIGTRAP)?; + self.write_stop_common(res, tid, Signal::SIGTRAP)?; res.write_str("replaylog:")?; res.write_str(match pos { @@ -505,7 +503,7 @@ impl GdbStubImpl { crate::__dead_code_marker!("catch_syscall", "stop_reason"); - self.write_stop_common(res, target, tid, Signal::SIGTRAP)?; + self.write_stop_common(res, tid, Signal::SIGTRAP)?; res.write_str(match position { CatchSyscallPosition::Entry => "syscall_entry:", @@ -520,10 +518,9 @@ impl GdbStubImpl { pub(crate) fn finish_library( &mut self, res: &mut ResponseWriter<'_, C>, - target: &mut T, tid: T::Tid, ) -> Result<(), Error> { - self.write_stop_common(res, target, Some(tid), Signal::SIGTRAP)?; + self.write_stop_common(res, Some(tid), Signal::SIGTRAP)?; res.write_str("library:;")?; Ok(()) } @@ -541,13 +538,13 @@ impl GdbStubImpl { } crate::__dead_code_marker!("fork_events", "stop_reason"); - self.write_stop_common(res, target, Some(cur_tid), Signal::SIGTRAP)?; + self.write_stop_common(res, Some(cur_tid), Signal::SIGTRAP)?; res.write_str("fork:")?; res.write_specific_thread_id(SpecificThreadId { pid: self .features .multiprocess() - .then_some(SpecificIdKind::WithId(self.get_current_pid(target)?)), + .then_some(SpecificIdKind::WithId(self.current_active_pid)), tid: SpecificIdKind::WithId(new_tid.into_fully_qualified_tid()), })?; res.write_str(";")?; @@ -567,13 +564,13 @@ impl GdbStubImpl { } crate::__dead_code_marker!("vfork_events", "stop_reason"); - self.write_stop_common(res, target, Some(cur_tid), Signal::SIGTRAP)?; + self.write_stop_common(res, Some(cur_tid), Signal::SIGTRAP)?; res.write_str("vfork:")?; res.write_specific_thread_id(SpecificThreadId { pid: self .features .multiprocess() - .then_some(SpecificIdKind::WithId(self.get_current_pid(target)?)), + .then_some(SpecificIdKind::WithId(self.current_active_pid)), tid: SpecificIdKind::WithId(new_tid.into_fully_qualified_tid()), })?; res.write_str(";")?; @@ -592,7 +589,7 @@ impl GdbStubImpl { } crate::__dead_code_marker!("vforkdone_events", "stop_reason"); - self.write_stop_common(res, target, Some(tid), Signal::SIGTRAP)?; + self.write_stop_common(res, Some(tid), Signal::SIGTRAP)?; res.write_str("vforkdone:;")?; Ok(()) } @@ -609,7 +606,7 @@ impl GdbStubImpl { } crate::__dead_code_marker!("vforkdone_events", "stop_reason"); - self.write_stop_common(res, target, None, Signal::SIGTRAP)?; + self.write_stop_common(res, None, Signal::SIGTRAP)?; res.write_str("exec:")?; res.write_hex_buf(path)?; res.write_str(";")?; diff --git a/src/stub/error.rs b/src/stub/error.rs index b7d0283e..4a73d81b 100644 --- a/src/stub/error.rs +++ b/src/stub/error.rs @@ -39,11 +39,6 @@ pub(crate) enum InternalError { // Errors indicative of a error in the user's `Target` implementation / `gdbstub` integration. ImplicitSwBreakpoints, - // DEVNOTE: this is a temporary workaround for something that can and should - // be caught at compile time via IDETs. That said, since i'm not sure when - // I'll find the time to cut a breaking release of gdbstub, I'd prefer to - // push out this feature as a non-breaking change now. - MissingCurrentActivePidImpl, MissingToRawId, TracepointUnsupportedSourceEnumeration, UnsupportedStopReason, @@ -88,7 +83,6 @@ impl InternalError { TargetError(_) | ImplicitSwBreakpoints - | MissingCurrentActivePidImpl | MissingToRawId | TracepointUnsupportedSourceEnumeration | UnsupportedStopReason => "Target", @@ -104,7 +98,6 @@ impl InternalError { ClientSentNack | Connection(_, _) | ImplicitSwBreakpoints - | MissingCurrentActivePidImpl | MissingToRawId | PacketBufferOverflow | PacketParse(_) @@ -208,7 +201,6 @@ where // Errors indicating an error in the user's `Target` implementation / `gdbstub` integration. ImplicitSwBreakpoints => write!(f, "The target has not opted into using implicit software breakpoints. See `Target::guard_rail_implicit_sw_breakpoints` for more information"), - 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`"), MissingToRawId => write!(f, "A RegId was used with an API that requires raw register IDs to be available (e.g. `StopReasonReporter::add_reg`) but returned `None` from `to_raw_id()`"), TracepointUnsupportedSourceEnumeration => write!(f, "The target doesn't support the gdbstub TracepointSource extension, but attempted to transition to enumerating tracepoint sources"), UnsupportedStopReason => write!(f, "{} {}", unsupported_stop_reason!(), SEE_DOCS_FOR_CONTEXT), diff --git a/src/stub/state_machine.rs b/src/stub/state_machine.rs index d9c586e0..96cd2700 100644 --- a/src/stub/state_machine.rs +++ b/src/stub/state_machine.rs @@ -490,7 +490,7 @@ where gdb.i .inner - .finish_signal_with_thread(&mut res, target, tid, signal)?; + .finish_signal_with_thread(&mut res, tid, signal)?; Ok(StopReasonReporter { target, @@ -681,7 +681,7 @@ where let mut res = ResponseWriter::from_state(&mut gdb.i.conn, res); - gdb.i.inner.finish_library(&mut res, target, tid)?; + gdb.i.inner.finish_library(&mut res, tid)?; Ok(StopReasonReporter { target, diff --git a/src/target/ext/extended_mode.rs b/src/target/ext/extended_mode.rs index 7315a504..f5bc75ee 100644 --- a/src/target/ext/extended_mode.rs +++ b/src/target/ext/extended_mode.rs @@ -81,14 +81,6 @@ pub trait ExtendedMode: Target { /// Attach to a new process with the specified PID. /// - /// Targets that wish to use `attach` are required to implement - /// [`CurrentActivePid`] (via `support_current_active_pid`), as the default - /// `gdbstub` behavior of always reporting a Pid of `1` will cause issues - /// when attaching to new processes. - /// - /// _Note:_ In the next API-breaking release of `gdbstub`, this coupling - /// will become a compile-time checked invariant. - /// /// In all-stop mode, all threads in the attached process are stopped; in /// non-stop mode, it may be attached without being stopped (if that is /// supported by the target). @@ -168,13 +160,6 @@ pub trait ExtendedMode: Target { fn support_configure_working_dir(&mut self) -> Option> { None } - - /// Support for reporting the current active Pid. Must be implemented in - /// order to use `attach`. - #[inline(always)] - fn support_current_active_pid(&mut self) -> Option> { - None - } } define_ext!(ExtendedModeOps, ExtendedMode); @@ -269,24 +254,3 @@ pub trait ConfigureWorkingDir: ExtendedMode { } define_ext!(ConfigureWorkingDirOps, ConfigureWorkingDir); - -/// Nested Target extension - Return the current active Pid. -pub trait CurrentActivePid: ExtendedMode { - /// Report the current active Pid. - /// - /// When implementing gdbstub on a platform that supports multiple - /// processes, the active PID needs to match the attached process. Failing - /// to do so will cause GDB to fail to attach to the target process. - /// - /// This should reflect the currently-debugged process which should be - /// updated when switching processes after calling - /// [`attach()`](ExtendedMode::attach). - /// - /// _Note:_ `gdbstub` doesn't yet support debugging multiple processes - /// _simultaneously_. If this is a feature you're interested in, please - /// leave a comment on this [tracking - /// issue](https://github.com/daniel5151/gdbstub/issues/124). - fn current_active_pid(&mut self) -> Result; -} - -define_ext!(CurrentActivePidOps, CurrentActivePid);