diff --git a/examples/armv4t/emu.rs b/examples/armv4t/emu.rs index e8f3305..c1c905d 100644 --- a/examples/armv4t/emu.rs +++ b/examples/armv4t/emu.rs @@ -2,6 +2,7 @@ use armv4t_emu::{reg, Cpu, ExampleMem, Memory, Mode}; use crate::mem_sniffer::{AccessKind, MemSniffer}; use crate::DynResult; +use gdbstub::common::Pid; const HLE_RETURN_ADDR: u32 = 0x12345678; @@ -35,6 +36,8 @@ pub struct Emu { pub(crate) watchpoints: Vec, pub(crate) breakpoints: Vec, pub(crate) files: Vec>, + + pub(crate) reported_pid: Pid, } impl Emu { @@ -85,6 +88,8 @@ impl Emu { watchpoints: Vec::new(), breakpoints: Vec::new(), files: Vec::new(), + + reported_pid: Pid::new(1).unwrap(), }) } diff --git a/examples/armv4t/gdb/extended_mode.rs b/examples/armv4t/gdb/extended_mode.rs index 4b30657..61fe209 100644 --- a/examples/armv4t/gdb/extended_mode.rs +++ b/examples/armv4t/gdb/extended_mode.rs @@ -31,8 +31,11 @@ impl target::ext::extended_mode::ExtendedMode for Emu { } fn attach(&mut self, pid: Pid) -> TargetResult<(), Self> { - eprintln!("GDB tried to attach to a process with PID {}", pid); - Err(().into()) // non-specific failure + eprintln!("GDB attached to a process with PID {}", pid); + // stub implementation: just report the same code, but running under a + // different pid. + self.reported_pid = pid; + Ok(()) } fn run(&mut self, filename: Option<&[u8]>, args: Args<'_, '_>) -> TargetResult { @@ -96,6 +99,13 @@ 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 { @@ -158,3 +168,9 @@ 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/lib.rs b/src/lib.rs index d71756b..59b6a1d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -341,7 +341,8 @@ macro_rules! unwrap { /// (Internal) The fake Tid that's used when running in single-threaded mode. const SINGLE_THREAD_TID: common::Tid = unwrap!(common::Tid::new(1)); -/// (Internal) The fake Pid reported to GDB when running in multi-threaded mode. +/// (Internal) The fake Pid reported to GDB when the target hasn't opted into +/// reporting a custom Pid itself. const FAKE_PID: common::Pid = unwrap!(common::Pid::new(1)); pub(crate) mod is_valid_tid { diff --git a/src/protocol/commands.rs b/src/protocol/commands.rs index 8b44eb3..5cf6d0f 100644 --- a/src/protocol/commands.rs +++ b/src/protocol/commands.rs @@ -114,7 +114,7 @@ macro_rules! commands { fn support_lldb_register_info(&mut self) -> Option<()> { use crate::arch::Arch; - if self.use_lldb_register_info() + if self.use_lldb_register_info() && (T::Arch::lldb_register_info(usize::max_value()).is_some() || self.support_lldb_register_info_override().is_some()) { @@ -122,7 +122,7 @@ macro_rules! commands { } else { None } - } + } fn support_resume(&mut self) -> Option<()> { self.base_ops().resume_ops().map(drop) @@ -228,7 +228,6 @@ commands! { "M" => _m_upcase::M<'a>, "qAttached" => _qAttached::qAttached, "qfThreadInfo" => _qfThreadInfo::qfThreadInfo, - "qC" => _qC::qC, "QStartNoAckMode" => _QStartNoAckMode::QStartNoAckMode, "qsThreadInfo" => _qsThreadInfo::qsThreadInfo, "qSupported" => _qSupported::qSupported<'a>, @@ -257,6 +256,7 @@ commands! { extended_mode use 'a { "!" => exclamation_mark::ExclamationMark, + "qC" => _qC::qC, "QDisableRandomization" => _QDisableRandomization::QDisableRandomization, "QEnvironmentHexEncoded" => _QEnvironmentHexEncoded::QEnvironmentHexEncoded<'a>, "QEnvironmentReset" => _QEnvironmentReset::QEnvironmentReset, diff --git a/src/stub/core_impl/base.rs b/src/stub/core_impl/base.rs index d5fe3b2..dbe9d64 100644 --- a/src/stub/core_impl/base.rs +++ b/src/stub/core_impl/base.rs @@ -34,17 +34,26 @@ impl GdbStubImpl { Ok(tid) } - pub(crate) fn get_sane_current_pid( + pub(crate) fn get_current_pid( &mut self, target: &mut T, ) -> Result> { - match target.base_ops() { - BaseOps::SingleThread(_) => Ok(FAKE_PID), - BaseOps::MultiThread(ops) => ops.active_pid().map_err(Error::TargetError), + 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) } } - #[inline(always)] + // Used by `?` and `vAttach` to return a "reasonable" stop reason. + // + // This is a bit of an implementation wart, since this is really something + // the user ought to be able to customize. + // + // Works fine for now though... pub(crate) fn report_reasonable_stop_reason( &mut self, res: &mut ResponseWriter<'_, C>, @@ -58,7 +67,7 @@ impl GdbStubImpl { pid: self .features .multiprocess() - .then_some(SpecificIdKind::WithId(self.get_sane_current_pid(target)?)), + .then_some(SpecificIdKind::WithId(self.get_current_pid(target)?)), tid: SpecificIdKind::WithId(tid), })?; } else { @@ -190,9 +199,11 @@ impl GdbStubImpl { } // -------------------- "Core" Functionality -------------------- // - // TODO: Improve the '?' response based on last-sent stop reason. - // this will be particularly relevant when working on non-stop mode. - Base::QuestionMark(_) => self.report_reasonable_stop_reason(res, target)?, + Base::QuestionMark(_) => { + // TODO: Improve the '?' response. + // this will be particularly relevant when working on non-stop mode. + self.report_reasonable_stop_reason(res, target)? + } Base::qAttached(cmd) => { let is_attached = match target.support_extended_mode() { // when _not_ running in extended mode, just report that we're attaching to an @@ -349,62 +360,9 @@ impl GdbStubImpl { } HandlerStatus::NeedsOk } - Base::qC(_) => { - res.write_str("QC")?; - let pid = self.get_sane_current_pid(target)?; - - match target.base_ops() { - BaseOps::SingleThread(_) => res.write_specific_thread_id(SpecificThreadId { - pid: self - .features - .multiprocess() - .then_some(SpecificIdKind::WithId(pid)), - tid: SpecificIdKind::WithId(SINGLE_THREAD_TID), - })?, - BaseOps::MultiThread(ops) => { - if self.current_mem_tid == SINGLE_THREAD_TID { - let mut err: Result<_, Error> = Ok(()); - let mut first = true; - ops.list_active_threads(&mut |tid| { - // TODO: replace this with a try block (once stabilized) - let e = (|| { - if !first { - return Ok(()); - } - first = false; - res.write_specific_thread_id(SpecificThreadId { - pid: self - .features - .multiprocess() - .then_some(SpecificIdKind::WithId(pid)), - tid: SpecificIdKind::WithId(tid), - })?; - Ok(()) - })(); - - if let Err(e) = e { - err = Err(e) - } - }) - .map_err(Error::TargetError)?; - err? - } else { - res.write_specific_thread_id(SpecificThreadId { - pid: self - .features - .multiprocess() - .then_some(SpecificIdKind::WithId(pid)), - tid: SpecificIdKind::WithId(self.current_mem_tid), - })?; - } - } - } - - HandlerStatus::Handled - } Base::qfThreadInfo(_) => { res.write_str("m")?; - let pid = self.get_sane_current_pid(target)?; + let pid = self.get_current_pid(target)?; 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 5863890..fd569c2 100644 --- a/src/stub/core_impl/extended_mode.rs +++ b/src/stub/core_impl/extended_mode.rs @@ -1,6 +1,11 @@ use super::prelude::*; use crate::protocol::commands::ext::ExtendedMode; +use crate::protocol::SpecificIdKind; +use crate::protocol::SpecificThreadId; +use crate::target::ext::base::BaseOps; +use crate::SINGLE_THREAD_TID; + impl GdbStubImpl { pub(crate) fn handle_extended_mode( &mut self, @@ -25,9 +30,58 @@ impl GdbStubImpl { HandlerStatus::Handled } ExtendedMode::vAttach(cmd) => { + if ops.support_current_active_pid().is_none() { + return Err(Error::MissingCurrentActivePidImpl); + } + ops.attach(cmd.pid).handle_error()?; 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(); + + res.write_str("QC")?; + let pid = ops.current_active_pid().map_err(Error::TargetError)?; + let tid = match target.base_ops() { + BaseOps::SingleThread(_) => SINGLE_THREAD_TID, + BaseOps::MultiThread(ops) => { + // HACK: gdbstub should avoid using a sentinel value here... + if self.current_mem_tid == SINGLE_THREAD_TID { + let mut err: Result<_, Error> = Ok(()); + let mut first_tid = None; + ops.list_active_threads(&mut |tid| { + // TODO: replace this with a try block (once stabilized) + let e = (|| { + if first_tid.is_some() { + return Ok(()); + } + first_tid = Some(tid); + Ok(()) + })(); + + if let Err(e) = e { + err = Err(e) + } + }) + .map_err(Error::TargetError)?; + err?; + first_tid.unwrap_or(SINGLE_THREAD_TID) + } else { + self.current_mem_tid + } + } + }; + + res.write_specific_thread_id(SpecificThreadId { + pid: self + .features + .multiprocess() + .then_some(SpecificIdKind::WithId(pid)), + tid: SpecificIdKind::WithId(tid), + })?; + + HandlerStatus::Handled + } ExtendedMode::vRun(cmd) => { use crate::target::ext::extended_mode::Args; @@ -35,10 +89,7 @@ impl GdbStubImpl { .run(cmd.filename, Args::new(&mut cmd.args.into_iter())) .handle_error()?; - // This is a reasonable response, as the `run` handler must - // spawn the process in a stopped state. - res.write_str("S05")?; - HandlerStatus::Handled + self.report_reasonable_stop_reason(res, target)? } // --------- ASLR --------- // ExtendedMode::QDisableRandomization(cmd) if ops.support_configure_aslr().is_some() => { @@ -76,6 +127,7 @@ impl GdbStubImpl { ops.cfg_startup_with_shell(cmd.value).handle_error()?; HandlerStatus::NeedsOk } + _ => HandlerStatus::Handled, }; diff --git a/src/stub/core_impl/resume.rs b/src/stub/core_impl/resume.rs index 74a90d3..e829aff 100644 --- a/src/stub/core_impl/resume.rs +++ b/src/stub/core_impl/resume.rs @@ -272,7 +272,7 @@ impl GdbStubImpl { pid: self .features .multiprocess() - .then_some(SpecificIdKind::WithId(self.get_sane_current_pid(target)?)), + .then_some(SpecificIdKind::WithId(self.get_current_pid(target)?)), tid: SpecificIdKind::WithId(tid), })?; res.write_str(";")?; diff --git a/src/stub/error.rs b/src/stub/error.rs index 57908a9..a1c07fe 100644 --- a/src/stub/error.rs +++ b/src/stub/error.rs @@ -61,6 +61,16 @@ pub enum GdbStubError { /// [`Target::guard_rail_single_step_gdb_behavior`]: /// crate::target::Target::guard_rail_single_step_gdb_behavior SingleStepGdbBehavior(SingleStepGdbBehavior), + /// GDB client attempted to attach to a new process, but the target has not + /// implemented [`ExtendedMode::support_current_active_pid`]. + /// + /// [`ExtendedMode::support_current_active_pid`]: + /// crate::target::ext::extended_mode::ExtendedMode::support_current_active_pid + // 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, // Internal - A non-fatal error occurred (with errno-style error code) // @@ -119,6 +129,7 @@ where )?; write!(f, "See `Target::guard_rail_single_step_gdb_behavior` 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`"), NonFatalError(_) => write!(f, "Internal non-fatal error. End users 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 3a43d11..693030e 100644 --- a/src/target/ext/base/multithread.rs +++ b/src/target/ext/base/multithread.rs @@ -2,9 +2,8 @@ use crate::arch::Arch; use crate::common::Signal; -use crate::common::{Pid, Tid}; +use crate::common::Tid; use crate::target::{Target, TargetResult}; -use crate::FAKE_PID; /// Base required debugging operations for multi threaded targets. pub trait MultiThreadBase: Target { @@ -111,26 +110,6 @@ pub trait MultiThreadBase: Target { ) -> Option> { None } - - /// Override the reported PID when running in multithread mode (default: 1) - /// - /// 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()`]. - /// - /// This function is only useful if your target implements multiple - /// processes. - /// - /// [`attach()`]: - /// crate::target::ext::extended_mode::ExtendedMode::attach - #[inline(always)] - fn active_pid(&mut self) -> Result { - Ok(FAKE_PID) - } } /// Target extension - support for resuming multi threaded targets. diff --git a/src/target/ext/extended_mode.rs b/src/target/ext/extended_mode.rs index 817bec0..15fe16c 100644 --- a/src/target/ext/extended_mode.rs +++ b/src/target/ext/extended_mode.rs @@ -1,18 +1,6 @@ //! Enables [Extended Mode](https://sourceware.org/gdb/current/onlinedocs/gdb/Connecting.html) //! functionality when connecting using `target extended-remote`, such as //! spawning new processes and/or attaching to existing processes. -//! -//! # Disclaimer -//! -//! While this API has been end-to-end tested and confirmed working with a "toy" -//! target implementation (see the included `armv4t` example), it has _not_ been -//! "battle-tested" with a fully-featured extended-mode capable target. -//! -//! If you end up using this API to implement an extended-mode capable target, -//! _please_ file an issue on the repo detailing any bugs / usability issues you -//! may encountered while implementing this API! If everything happens to Just -//! Work as expected, nonetheless file an issue so that this disclaimer can be -//! removed in future releases! use crate::common::*; use crate::target::{Target, TargetResult}; @@ -92,6 +80,14 @@ 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). @@ -171,6 +167,13 @@ 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); @@ -265,3 +268,24 @@ 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);