Mega-batch of 0.8 breaking changes - #198
Merged
Merged
Conversation
Owner
Author
|
@jonathanzetier I'm going to merge this big bundle of changes into the That said - I strongly believe that these changes (especially introduction of the Please feel free to share any thoughts you have on these changes! I'm fairly certain that these changes are the right call, but if you spot anything that you think I might've blundered on, please let me know! |
This was referenced May 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is def the sort of thing that should've been multiple PRs, but I got a bit carried away... While I initially intended to split this up, the
TidandStopReasonchanges proved to be deeply intertwined at the state-machine layer, making a single cohesive PR the most practical approach.TL;DR of changes:
Tidassociated type toTarget.StopReasonenums with a fluent builder API.Closes #195
Unblocks #51
Closes #185 (which I ended up implementing in this PR)
Updated
Targetto include a thread-id associated typeA new
type Tid: IsValidTidassociated type has been added to theTargettrait, and is now used throughout various IDETs, and internal/externalgdbstubAPIs.Several things nudged me towards making this change:
gdbstubhas accumulated a couple non-BaseIDETs that plumb thread-id values to the user'sTargetimplementation (namely:ThreadExtraInfoandWasm). These IDETs currently hard-codecrate::common::Tidin their APIs. The jump to multi-process support will require updating their function signatures, and single-threaded targets that otherwise don't need to care aboutTidvalues currently end up havinggdbstub'sSINGLE_THREAD_TIDconstant leaked to them.Targetimplementations to decide to swap between single-thread vs. multi-thread ops at runtime, in practice, a target picks what kind of target it is at compile time and sticks to it. Returning a non-()TID on a single-threaded target could confusegdbstubinternally. This change makesTidhandling significantly more consistent and type-safe across the board.gdbstub's implementation has been dramatically simplified and tangibly improved by havingT::Tideverywhere. I've been able to refactor a surprisingly large amount of Tid conversion code. The newfrom_fully_qualified_tidadds strong type-level guard rails for validating that the GDB client is sending over thread-id values that theTargetis able to operate on.TidviaTargetsignificantly reduces the amount of generics therun_blockingandGdbStubStateMachinefamily of APIs have to deal with. We no longer need an extra top-levelTid: IsValidTidbound on a bunch of those APIs.Removed
StopReasonenums, switched to function-based stop reason reportingThe
enum-based API was workable so long as stop reasons remained fixed-length... but as we've implemented more of the GDB RSP, the stop reason API hasn't kept up with the increasingly dynamic data that folks want to report.Two notable examples: reporting inline register values, and stop reasons with embedded strings. While we were able to squeeze in support for reporting inline register values via #189, things were getting a bit hacky, and would only get hackier as the GDB RSP added more metadata to stop reply packets (e.g: like how they added
coreto theTreply packet).This new approach rethinks stop reason reporting from the ground up, moving away from the enum-based approach that was present since the earliest days of
gdbstub, and swaps over to a more flexible, extensible, and safer fluent builder approach to reporting stop reasons.A huge DX win here is compile-time safety for IDETs. The new builder methods (like
.swbreak()) have trait bounds that will cause a compile-time failure if you try to use them without actually implementing the corresponding IDET on your target!So, for example: in the context of the
run_blockinginfrastructure:A major benefit under-the-hood is that there's no longer any point in time where a stop reason exists as a data structure in memory / needs to get "passed around" the codebase. This is a nice little binary size win! Albeit with one consequence...
Rework Ctrl-C interrupt stop-reason reporting
The ctrl-c interrupt APIs now require the implementor to stash the fact the interrupt occurred somewhere on/around their
Targetimplementation, and report the effects of the interrupt as a downstream stop reason the next time they enter theRunningstate.This is a direct consequence of the shift to function-based stop reason reporting, as there is no longer an in-memory representation of a stop reason that can be stashed via the
deferred_stop_reasoninfrastructure in the state machine.While this means the "fast path" is gone, this is actually a solid architectural win. Forcing targets to stash the interrupt and handle it asynchronously in their own execution loops enforces a more robust architecture and prevents weird state desyncs between GDB and the target's actual execution state. In practice, the vast majority of target implementations already maintain some form of internal "pending interrupt" flag or event queue, so setting a simple boolean flag is a trivial adjustment.
Internal Cleanup
InternalError(and by extensionGdbStubError) enum has been revamped. Catch-all errors likeTargetMismatchhave been replaced with highly specific variants likeUnexpectedIntegerSize,UnexpectedReg, andUnexpectedThreadId. TheDisplayimplementation clearly delineates between "Client" errors and "Target/Implementation" errors.ResponseWriterState: TheResponseWriterhas been decoupled slightly by extracting its state into aResponseWriterStatestruct. This allows the state to be passed around without needing a continuous mutable borrow over the underlying connection, which heavily facilitates the newStopReasonReporterbuilder pattern.ResponseWriterState, collapsed twobools into a bitflag (for a tiny bit of memory gainz)Here is a draft for that section that matches the tone of the rest of the PR description. You could slot this in right before the "Internal Cleanup" section:
Minor API terminology tweaks (
tid->thread_id)While going through and updating the various
Targetextension traits to use the newSelf::Tidassociated type, I took the opportunity to clean up some of the parameter naming in the API docs.Across the board, trait methods that previously accepted a
tid: Tidparameter (like inThreadExtraInfo,Wasm,SingleRegisterAccess, etc.) have been updated to usethread_id: Self::Tid.This is a purely cosmetic change for implementors, but as we enter the world of multi-process support, being more intentional about
tid,pid, andthread-idwill be important to maintain grokkability ofgdbstub's large API surface.Quick Migration Checklist
What's surprising is that, even through there's a lot of churn in
gdbstubitself, the actual end-user APIs that folks interact with aren't changing all that much from0.7. I'm running out of steam this weekend to write a comprehensive transition guide, but it shouldn't be too painful to move over!At a high level, this is all that'll really be needed:
type Tid = ();(ortype Tid = gdbstub::common::Tid;) to existingTargetimpls.