rework vAttach + custom pid reporting (#133)

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<u32>,
     pub(crate) breakpoints: Vec<u32>,
     pub(crate) files: Vec<Option<std::fs::File>>,
+
+    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(),
         })
     }
 

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<Pid, Self> {
@@ -96,6 +99,13 @@ impl target::ext::extended_mode::ExtendedMode for Emu {
     ) -> Option<target::ext::extended_mode::ConfigureWorkingDirOps<'_, Self>> {
         Some(self)
     }
+
+    #[inline(always)]
+    fn support_current_active_pid(
+        &mut self,
+    ) -> Option<target::ext::extended_mode::CurrentActivePidOps<'_, Self>> {
+        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<Pid, Self::Error> {
+        Ok(self.reported_pid)
+    }
+}

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 {

src/protocol/commands.rs

@@ -114,15 +114,15 @@ 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())
                         {
                             Some(())
                         } 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,

src/stub/core_impl/base.rs

@@ -34,17 +34,26 @@ impl<T: Target, C: Connection> GdbStubImpl<T, C> {
         Ok(tid)
     }
 
-    pub(crate) fn get_sane_current_pid(
+    pub(crate) fn get_current_pid(
         &mut self,
         target: &mut T,
     ) -> Result<Pid, Error<T::Error, C::Error>> {
-        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<T: Target, C: Connection> GdbStubImpl<T, C> {
                 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<T: Target, C: Connection> GdbStubImpl<T, C> {
             }
 
             // -------------------- "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<T: Target, C: Connection> GdbStubImpl<T, C> {
                 }
                 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<T::Error, C::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 {

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<T: Target, C: Connection> GdbStubImpl<T, C> {
     pub(crate) fn handle_extended_mode(
         &mut self,
@@ -25,20 +30,66 @@ impl<T: Target, C: Connection> GdbStubImpl<T, C> {
                 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<T::Error, C::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;
 
                 let _pid = ops
                     .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<T: Target, C: Connection> GdbStubImpl<T, C> {
                 ops.cfg_startup_with_shell(cmd.value).handle_error()?;
                 HandlerStatus::NeedsOk
             }
+
             _ => HandlerStatus::Handled,
         };
 

src/stub/core_impl/resume.rs

@@ -272,7 +272,7 @@ impl<T: Target, C: Connection> GdbStubImpl<T, C> {
                 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(";")?;

src/stub/error.rs

@@ -61,6 +61,16 @@ pub enum GdbStubError<T, C> {
     /// [`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!"),
         }

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<crate::target::ext::thread_extra_info::ThreadExtraInfoOps<'_, Self>> {
         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<Pid, Self::Error> {
-        Ok(FAKE_PID)
-    }
 }
 
 /// Target extension - support for resuming multi threaded targets.