remove SingleStepGdbBehavior guard rail

closes #132
daniel5151 · Apr 29, 2023 · 9ff95b4 · 9ff95b4
1 parent dfdb1d3
commit 9ff95b4
Show file tree

Hide file tree

Showing 14 changed files with 23 additions and 269 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,14 @@ All notable changes to this project will be documented in this file.
 
 This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+# 0.7.0 (UNRELEASED)
+
+#### Breaking API Changes
+
+- `Target` APIs:
+  - `SingleThreadBase`/`MultiThreadBase`
+    - `read_addrs` now returns a `usize` instead of a `()`, allowing implementations to report cases where only a subset of memory could be read.
+
 # 0.6.6
 
 #### New Features

diff --git a/examples/armv4t/gdb/mod.rs b/examples/armv4t/gdb/mod.rs
@@ -392,7 +392,7 @@ mod custom_arch {
     use core::num::NonZeroUsize;
 
     use gdbstub::arch::lldb::{Encoding, Format, Generic, Register, RegisterInfo};
-    use gdbstub::arch::{Arch, RegId, Registers, SingleStepGdbBehavior};
+    use gdbstub::arch::{Arch, RegId, Registers};
 
     use gdbstub_arch::arm::reg::id::ArmCoreRegId;
     use gdbstub_arch::arm::reg::ArmCoreRegs;
@@ -601,13 +601,5 @@ mod custom_arch {
                 }
             }
         }
-        // armv4t supports optional single stepping.
-        //
-        // notably, x86 is an example of an arch that does _not_ support
-        // optional single stepping.
-        #[inline(always)]
-        fn single_step_gdb_behavior() -> SingleStepGdbBehavior {
-            SingleStepGdbBehavior::Optional
-        }
     }
 }
diff --git a/gdbstub_arch/CHANGELOG.md b/gdbstub_arch/CHANGELOG.md
@@ -2,6 +2,12 @@ All notable changes to this project will be documented in this file.
 
 This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+# 0.3.0 (UNRELEASED)
+
+#### Breaking Arch Changes
+
+- Entirely removed `SingleStepGdbBehavior` APIs
+
 # 0.2.4
 
 - Add support for AArch64 [\#109](https://github.com/daniel5151/gdbstub/pull/109) ([ptosi](https://github.com/ptosi))

diff --git a/gdbstub_arch/src/aarch64/mod.rs b/gdbstub_arch/src/aarch64/mod.rs
@@ -7,7 +7,7 @@
 //! *Note*: the target XML currently advertises all system registers to the GDB
 //! client.
 
-use gdbstub::arch::{Arch, SingleStepGdbBehavior};
+use gdbstub::arch::Arch;
 
 pub mod reg;
 
@@ -32,9 +32,4 @@ impl Arch for AArch64 {
 
         Some(DESCRIPTION_XML)
     }
-
-    #[inline(always)]
-    fn single_step_gdb_behavior() -> SingleStepGdbBehavior {
-        SingleStepGdbBehavior::Required
-    }
 }
diff --git a/gdbstub_arch/src/arm/mod.rs b/gdbstub_arch/src/arm/mod.rs
@@ -1,6 +1,6 @@
 //! Implementations for various ARM architectures.
 
-use gdbstub::arch::{Arch, SingleStepGdbBehavior};
+use gdbstub::arch::{Arch};
 
 pub mod reg;
 
@@ -42,9 +42,4 @@ impl Arch for Armv4t {
     fn target_description_xml() -> Option<&'static str> {
         Some(r#"<target version="1.0"><architecture>armv4t</architecture></target>"#)
     }
-
-    #[inline(always)]
-    fn single_step_gdb_behavior() -> SingleStepGdbBehavior {
-        SingleStepGdbBehavior::Optional
-    }
 }
diff --git a/gdbstub_arch/src/mips/mod.rs b/gdbstub_arch/src/mips/mod.rs
@@ -1,6 +1,6 @@
 //! Implementations for the MIPS architecture.
 
-use gdbstub::arch::{Arch, SingleStepGdbBehavior};
+use gdbstub::arch::{Arch};
 
 pub mod reg;
 
@@ -73,11 +73,6 @@ impl Arch for Mips {
     fn target_description_xml() -> Option<&'static str> {
         Some(r#"<target version="1.0"><architecture>mips</architecture></target>"#)
     }
-
-    #[inline(always)]
-    fn single_step_gdb_behavior() -> SingleStepGdbBehavior {
-        SingleStepGdbBehavior::Ignored
-    }
 }
 
 impl Arch for MipsWithDsp {
@@ -91,11 +86,6 @@ impl Arch for MipsWithDsp {
             r#"<target version="1.0"><architecture>mips</architecture><feature name="org.gnu.gdb.mips.dsp"></feature></target>"#,
         )
     }
-
-    #[inline(always)]
-    fn single_step_gdb_behavior() -> SingleStepGdbBehavior {
-        SingleStepGdbBehavior::Ignored
-    }
 }
 
 #[allow(deprecated)]
@@ -108,11 +98,6 @@ impl Arch for Mips64 {
     fn target_description_xml() -> Option<&'static str> {
         None
     }
-
-    #[inline(always)]
-    fn single_step_gdb_behavior() -> SingleStepGdbBehavior {
-        SingleStepGdbBehavior::Ignored
-    }
 }
 
 #[allow(deprecated)]
@@ -125,9 +110,4 @@ impl Arch for Mips64WithDsp {
     fn target_description_xml() -> Option<&'static str> {
         None
     }
-
-    #[inline(always)]
-    fn single_step_gdb_behavior() -> SingleStepGdbBehavior {
-        SingleStepGdbBehavior::Ignored
-    }
 }
diff --git a/gdbstub_arch/src/msp430/mod.rs b/gdbstub_arch/src/msp430/mod.rs
@@ -1,6 +1,6 @@
 //! Implementations for the TI-MSP430 family of MCUs.
 
-use gdbstub::arch::{Arch, SingleStepGdbBehavior};
+use gdbstub::arch::{Arch};
 
 pub mod reg;
 
@@ -16,11 +16,6 @@ impl Arch for Msp430 {
     fn target_description_xml() -> Option<&'static str> {
         Some(r#"<target version="1.0"><architecture>msp430</architecture></target>"#)
     }
-
-    #[inline(always)]
-    fn single_step_gdb_behavior() -> SingleStepGdbBehavior {
-        SingleStepGdbBehavior::Required
-    }
 }
 
 /// Implements `Arch` for 20-bit TI-MSP430 MCUs (CPUX).
@@ -35,9 +30,4 @@ impl Arch for Msp430X {
     fn target_description_xml() -> Option<&'static str> {
         Some(r#"<target version="1.0"><architecture>msp430x</architecture></target>"#)
     }
-
-    #[inline(always)]
-    fn single_step_gdb_behavior() -> SingleStepGdbBehavior {
-        SingleStepGdbBehavior::Required
-    }
 }
diff --git a/gdbstub_arch/src/ppc/mod.rs b/gdbstub_arch/src/ppc/mod.rs
@@ -1,6 +1,6 @@
 //! Implementations for various PowerPC architectures.
 
-use gdbstub::arch::{Arch, RegId, SingleStepGdbBehavior};
+use gdbstub::arch::{Arch, RegId};
 
 pub mod reg;
 
@@ -24,9 +24,4 @@ impl<RegIdImpl: RegId> Arch for PowerPcAltivec32<RegIdImpl> {
             r#"<target version="1.0"><architecture>powerpc:common</architecture><feature name="org.gnu.gdb.power.core"></feature><feature name="org.gnu.gdb.power.fpu"></feature><feature name="org.gnu.gdb.power.altivec"></feature></target>"#,
         )
     }
-
-    #[inline(always)]
-    fn single_step_gdb_behavior() -> SingleStepGdbBehavior {
-        SingleStepGdbBehavior::Required
-    }
 }
diff --git a/gdbstub_arch/src/riscv/mod.rs b/gdbstub_arch/src/riscv/mod.rs
@@ -2,7 +2,7 @@
 //!
 //! *Note*: currently only supports integer versions of the ISA.
 
-use gdbstub::arch::{Arch, SingleStepGdbBehavior};
+use gdbstub::arch::{Arch};
 
 pub mod reg;
 
@@ -21,11 +21,6 @@ impl Arch for Riscv32 {
     fn target_description_xml() -> Option<&'static str> {
         Some(r#"<target version="1.0"><architecture>riscv:rv32</architecture></target>"#)
     }
-
-    #[inline(always)]
-    fn single_step_gdb_behavior() -> SingleStepGdbBehavior {
-        SingleStepGdbBehavior::Required
-    }
 }
 
 impl Arch for Riscv64 {
@@ -37,9 +32,4 @@ impl Arch for Riscv64 {
     fn target_description_xml() -> Option<&'static str> {
         Some(r#"<target version="1.0"><architecture>riscv:rv64</architecture></target>"#)
     }
-
-    #[inline(always)]
-    fn single_step_gdb_behavior() -> SingleStepGdbBehavior {
-        SingleStepGdbBehavior::Required
-    }
 }
diff --git a/gdbstub_arch/src/x86/mod.rs b/gdbstub_arch/src/x86/mod.rs
@@ -1,6 +1,6 @@
 //! Implementations for various x86 architectures.
 
-use gdbstub::arch::{Arch, SingleStepGdbBehavior};
+use gdbstub::arch::Arch;
 
 pub mod reg;
 
@@ -19,11 +19,6 @@ impl Arch for X86_64_SSE {
             r#"<target version="1.0"><architecture>i386:x86-64</architecture><feature name="org.gnu.gdb.i386.sse"></feature></target>"#,
         )
     }
-
-    #[inline(always)]
-    fn single_step_gdb_behavior() -> SingleStepGdbBehavior {
-        SingleStepGdbBehavior::Required
-    }
 }
 
 /// Implements `Arch` for 32-bit x86 + SSE Extensions.
@@ -41,9 +36,4 @@ impl Arch for X86_SSE {
             r#"<target version="1.0"><architecture>i386:intel</architecture><feature name="org.gnu.gdb.i386.sse"></feature></target>"#,
         )
     }
-
-    #[inline(always)]
-    fn single_step_gdb_behavior() -> SingleStepGdbBehavior {
-        SingleStepGdbBehavior::Required
-    }
 }
diff --git a/src/arch.rs b/src/arch.rs
@@ -187,102 +187,6 @@ pub trait Arch {
         let _ = reg_id;
         None
     }
-
-    /// Encode how the mainline GDB client handles target support for
-    /// single-step on this particular architecture.
-    ///
-    /// # Context
-    ///
-    /// According to the spec, supporting single step _should_ be quite
-    /// straightforward:
-    ///
-    /// - The GDB client sends a `vCont?` packet to enumerate supported
-    ///   resumption modes
-    /// - If the target supports single-step, it responds with the `s;S`
-    ///   capability as part of the response, omitting it if it is not
-    ///   supported.
-    /// - Later, when the user attempts to `stepi`, the GDB client sends a `s`
-    ///   resumption reason if it is supported, falling back to setting a
-    ///   temporary breakpoint + continue to "emulate" the single step.
-    ///
-    /// Unfortunately, the reality is that the mainline GDB client does _not_ do
-    /// this on all architectures...
-    ///
-    /// - On certain architectures (e.g: x86), GDB will _unconditionally_ assume
-    ///   single-step support, regardless whether or not the target reports
-    ///   supports it.
-    /// - On certain architectures (e.g: MIPS), GDB will _never_ use single-step
-    ///   support, even in the target has explicitly reported support for it.
-    ///
-    /// This is a bug, and has been reported at
-    /// <https://sourceware.org/bugzilla/show_bug.cgi?id=28440>.
-    ///
-    /// For a easy repro of this behavior, also see
-    /// <https://github.com/daniel5151/gdb-optional-step-bug>.
-    ///
-    /// # Implications
-    ///
-    /// Unfortunately, even if these idiosyncratic behaviors get fixed in the
-    /// mainline GDB client, it will be quite a while until the typical
-    /// user's distro-provided GDB client includes this bugfix.
-    ///
-    /// As such, `gdbstub` has opted to include this method as a "guard rail" to
-    /// preemptively detect cases of this idiosyncratic behavior, and throw a
-    /// pre-init error that informs the user of the potential issues they may
-    /// run into.
-    ///
-    /// # Writing a proper implementation
-    ///
-    /// To check whether or not a particular architecture exhibits this
-    /// behavior, an implementation should temporarily override this method to
-    /// return [`SingleStepGdbBehavior::Optional`], toggle target support for
-    /// single-step on/off, and observe the behavior of the GDB client after
-    /// invoking `stepi`.
-    ///
-    /// If single-stepping was **disabled**, yet the client nonetheless sent a
-    /// `vCont` packet with a `s` resume action, then this architecture
-    /// _does not_ support optional single stepping, and this method should
-    /// return [`SingleStepGdbBehavior::Required`].
-    ///
-    /// If single-stepping was **disabled**, and the client attempted to set a
-    /// temporary breakpoint (using the `z` packet), and then sent a `vCont`
-    /// packet with a `c` resume action, then this architecture _does_
-    /// support optional single stepping, and this method should return
-    /// [`SingleStepGdbBehavior::Optional`].
-    ///
-    /// If single-stepping was **enabled**, yet the client did _not_ send a
-    /// `vCont` packet with a `s` resume action, then this architecture
-    /// _ignores_ single stepping entirely, and this method should return
-    /// [`SingleStepGdbBehavior::Ignored`].
-    fn single_step_gdb_behavior() -> SingleStepGdbBehavior;
-}
-
-/// Encodes how the mainline GDB client handles target support for single-step
-/// on a particular architecture.
-///
-/// See [Arch::single_step_gdb_behavior] for details.
-#[non_exhaustive]
-#[derive(Debug, Clone, Copy)]
-pub enum SingleStepGdbBehavior {
-    /// GDB will use single-stepping if available, falling back to using
-    /// a temporary breakpoint + continue if unsupported.
-    ///
-    /// e.g: ARM
-    Optional,
-    /// GDB will unconditionally send single-step packets, _requiring_ the
-    /// target to handle these requests.
-    ///
-    /// e.g: x86/x64
-    Required,
-    /// GDB will never use single-stepping, regardless if it's supported by the
-    /// stub. It will always use a temporary breakpoint + continue.
-    ///
-    /// e.g: MIPS
-    Ignored,
-    /// Unknown behavior - no one has tested this platform yet. If possible,
-    /// please conduct a test + upstream your findings to `gdbstub_arch`.
-    #[doc(hidden)]
-    Unknown,
 }
 
 /// LLDB-specific types supporting [`Arch::lldb_register_info`] and

diff --git a/src/stub/error.rs b/src/stub/error.rs
@@ -1,6 +1,5 @@
 use core::fmt::{self, Debug, Display};
 
-use crate::arch::SingleStepGdbBehavior;
 use crate::protocol::{PacketParseError, ResponseWriterError};
 use crate::util::managed_vec::CapacityError;
 
@@ -51,16 +50,6 @@ pub enum GdbStubError<T, C> {
     /// [`Target::guard_rail_implicit_sw_breakpoints`]:
     /// crate::target::Target::guard_rail_implicit_sw_breakpoints
     ImplicitSwBreakpoints,
-    /// The target has not indicated support for optional single stepping. See
-    /// [`Target::guard_rail_single_step_gdb_behavior`] for more information.
-    ///
-    /// If you encountered this error while using an `Arch` implementation
-    /// defined in `gdbstub_arch` and believe this is incorrect, please file an
-    /// issue at <https://github.com/daniel5151/gdbstub/issues>.
-    ///
-    /// [`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`].
     ///
@@ -115,20 +104,6 @@ where
             NoActiveThreads => write!(f, "Target didn't report any active threads when there should have been at least one running."),
 
             ImplicitSwBreakpoints => write!(f, "Warning: The target has not opted into using implicit software breakpoints. See `Target::guard_rail_implicit_sw_breakpoints` for more information."),
-            SingleStepGdbBehavior(behavior) => {
-                use crate::arch::SingleStepGdbBehavior;
-                write!(
-                    f,
-                    "Warning: Mismatch between the targets' single-step support and arch-level single-step behavior: {} ",
-                    match behavior {
-                        SingleStepGdbBehavior::Optional => "", // unreachable, since optional single step will not result in an error
-                        SingleStepGdbBehavior::Required => "GDB requires single-step support on this arch.",
-                        SingleStepGdbBehavior::Ignored => "GDB ignores single-step support on this arch, yet the target has implemented support for it.",
-                        SingleStepGdbBehavior::Unknown => "This arch's single-step behavior hasn't been tested yet: please conduct a test + upstream your findings!",
-                    }
-                )?;
-                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!"),