Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 50 additions & 1 deletion src/stub/state_machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
use crate::stub::error::GdbStubError;
use crate::stub::error::InternalError;
use crate::stub::stop_reason::IntoStopReason;
use crate::stub::BaseStopReason;
use crate::target::Target;
use managed::ManagedSlice;

Expand Down Expand Up @@ -251,12 +252,60 @@
impl<'a, T: Target, C: Connection> GdbStubStateMachineInner<'a, state::Running, T, C> {
/// Report a target stop reason back to GDB.
pub fn report_stop(
self,
target: &mut T,
reason: impl IntoStopReason<T>,
) -> Result<GdbStubStateMachine<'a, T, C>, GdbStubError<T::Error, C::Error>> {
self.report_stop_impl(target, reason, None)
}

/// Report a target stop reason back to GDB, including expedited
/// register values in the stop reply T-packet.
///
/// The iterator yields `(register_number, value_bytes)` pairs that
/// are written as expedition registers in the T-packet. Values
/// should be in target byte order (typically little-endian).
///
/// This may be useful to use, rather than [`report_stop`], when

Check failure on line 269 in src/stub/state_machine.rs

View workflow job for this annotation

GitHub Actions / clippy + tests + docs

unresolved link to `report_stop`
/// we want to provide register values immediately to, for
/// example, avoid a round-trip, or work around a quirk/bug in a
/// debugger that does not otherwise request new register values.
pub fn report_stop_with_regs(
self,
target: &mut T,
reason: impl IntoStopReason<T>,
regs: &mut dyn Iterator<Item = (u32, &[u8])>,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should use the Target Arch RegId associated type (see https://docs.rs/gdbstub/0.7.6/gdbstub/arch/trait.Arch.html#associatedtype.RegId), as opposed to a bare u32

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this would require a breaking API change: RegId has from_raw_id, but not to_raw_id (nor is it constrained as a subtrait of any numeric trait or equivalent) -- in other words gdbstub currently requires arch implementers to parse from raw integers to registers, but not to provide raw integers for registers. (The ordering is implicit in the gdb_serialize/gdb_deserialize impls provided by the arch.)

I'm happy to add to_raw_id to RegId and then use it here if you'd prefer! Otherwise this is why I had to use a raw u32...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh fooey.

Yeah, we def need a to_raw_id method on RegId to pull this off...

On the bright side, if we add to_raw_id with a default stub impl (i.e: returning None), we can avoid making this a breaking change.

Lets go ahead and add to_raw_id, with some docs on the new method + on report_stop_with_regs commenting on how Arch implementations only need to implement the method if they want to use the report_stop_with_regs API.

For in-tree validation, please update the armv4t example to use report_stop_with_regs (e.g: to send back the PC, or whatever), and update the ArmCoreRegId type) with a non-trivial impl of the new method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, and added docs on both referring to each other (report_stop_with_regs re: the need for to_raw_id, and to_raw_id re: needed only if report_stop_with_regs used).

I wasn't able to add this to the in-tree example as-is because the API is not reflected up to the run_blocking API (this method here lives on the inner state machine, which I use directly). I'm not sure how you'd like to do so without carrying either an allocation (for a list of regs to send) or a &mut dyn callback or iterator (to pull regs to send) in BlockingEventLoopEvent, both of which seem like breaking changes -- requiring an allocator or adding a lifetime parameter respectively.

(A little more meta: given the above I'm kind of running out of steam on upstreaming-energy here -- if this can't go as-is because it needs more API surface to be included that I won't use in my use-case, I may just carry it as a patch in a local fork of gdbstub in Wasmtime. Sorry, just need to limit how much time I spend in side-adventures)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yep, that's right. No worries - this implementation seems pretty straightforward, and I'll trust that it's working fine with your use-case.

(and yep, totally understand! I really appreciate the willingness to iterate here. I think we're just wrapping up with a few nits here, but the various PRs should land shortly)

) -> Result<GdbStubStateMachine<'a, T, C>, GdbStubError<T::Error, C::Error>> {
self.report_stop_impl(target, reason, Some(regs))
}

/// Shared implementation for the
/// `report_stop`/`report_stop_with_regs` API. Takes an `Option`
/// around the `&mut dyn Iterator` to avoid making a dynamic
/// vtable dispatch in the common `report_stop` case.
fn report_stop_impl(
mut self,
target: &mut T,
reason: impl IntoStopReason<T>,
regs: Option<&mut dyn Iterator<Item = (u32, &[u8])>>,
) -> Result<GdbStubStateMachine<'a, T, C>, GdbStubError<T::Error, C::Error>> {
let reason: BaseStopReason<_, _> = reason.into();
let is_t_packet = reason.is_t_packet();

let mut res = ResponseWriter::new(&mut self.i.conn, target.use_rle());
let event = self.i.inner.finish_exec(&mut res, target, reason.into())?;
let event = self.i.inner.finish_exec(&mut res, target, reason)?;

if let Some(regs) = regs {
if is_t_packet {
for (reg_id, value) in regs {
res.write_num(reg_id).map_err(InternalError::from)?;
res.write_str(":").map_err(InternalError::from)?;
res.write_hex_buf(value).map_err(InternalError::from)?;
res.write_str(";").map_err(InternalError::from)?;
}
}
}

res.flush().map_err(InternalError::from)?;

Ok(match event {
Expand Down
22 changes: 22 additions & 0 deletions src/stub/stop_reason.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,28 @@ pub enum BaseStopReason<Tid, U> {
VForkDone(Tid),
}

impl<Tid, U> BaseStopReason<Tid, U> {
/// Does this stop reason respond with a `T` packet?
pub(crate) fn is_t_packet(&self) -> bool {
match self {
Self::SignalWithThread { .. } => true,
Self::SwBreak(_) => true,
Self::HwBreak(_) => true,
Self::Watch { .. } => true,
Self::ReplayLog { .. } => true,
Self::CatchSyscall { .. } => true,
Self::Library(_) => true,
Self::Fork { .. } => true,
Self::VFork { .. } => true,
Self::VForkDone(_) => true,
Self::DoneStep => false,
Self::Signal(_) => false,
Self::Exited(_) => false,
Self::Terminated(_) => false,
}
}
}

/// A stop reason for a single threaded target.
///
/// Threads are identified using the unit type `()` (as there is only a single
Expand Down
Loading