feat(gdb): add typed PS2 EE Workbench attach and PC preview - #29
Merged
Conversation
Signed-off-by: Krill <natew94@gmail.com>
Signed-off-by: Krill <natew94@gmail.com>
Signed-off-by: Krill <natew94@gmail.com>
Signed-off-by: Krill <natew94@gmail.com>
Signed-off-by: Krill <natew94@gmail.com>
Add one explicit, capacity-one, read-only "Read current EE PC" operation to the outbound RSP session. It is offered only when the connected target matched the exact canonical 109-register, 708-byte PS2 EE schema. The schema is enforced twice: the egui-side route gate refuses submission, and the remote-I/O worker re-derives the same decision from its own retained target description before touching the socket. Both enforcement points resolve "canonical EE schema" through one shared comparison so they cannot drift. The worker reads `g`, validates the length against the connected description, re-checks the exact canonical length, parses with the protocol crate's ps2_ee_gpacket_to_registers, and sends only the typed u32 program counter over the worker/UI channel. The raw packet and every other decoded register -- including the EE's 128-bit GPR, HI, and LO state -- stay worker-local and are dropped there. The UI renders a fixed-width 0xXXXXXXXX literal. This route is a strict narrowing of the existing read-register route: it issues the same `g` read and adds no capability. Routing policy is unchanged -- no polling, execution control, memory or instruction reads, disassembly, breakpoints, writes, export, persistence, or evidence publication. Scrubbing follows the existing session lifecycle: the preview clears on a connection attempt, disconnect, cancellation, worker or connection loss, and session replacement. Result staleness is factored into a pure classify_event so a completion that lost its race with socket shutdown, or one belonging to an already-cancelled request, never reaches presentation state. Tests cover canonical extraction against a literal packet offset, near-schema and foreign-target rejection, near-length packet rejection, the controller route gate, fixed-width formatting, lifecycle scrubbing, and stale/cancelled dispositions. Signed-off-by: Krill <natew94@gmail.com>
Record the new capacity-one, read-only EE program-counter operation: its exact-schema-only availability, the two enforcement points, the worker-local parse that lets only the typed u32 cross the channel, the fixed-width display, and the scrub and staleness rules it inherits. Also narrow the deferred-work item so the single typed read is not mistaken for general typed register decoding, polling, instruction reads, or disassembly. Signed-off-by: Krill <natew94@gmail.com>
The typed EE current-PC read retained the observed value in one place it could never be scrubbed. On success the controller built a notice embedding the formatted program counter, and Workbench forwards every remote notice into its activity log and, when attached, on to the companion console process. That log only appends and evicts its oldest entry at capacity, so it has no scrub at all, and the console is a separate process the session cannot reach. A value the route promises to drop on a connection attempt, disconnect, cancellation, worker or connection loss, and session replacement therefore outlived every one of them, and stayed attributable to an endpoint after that connection was gone. Make the success notice value-free. The completed read is still announced, but with a constant message identical for every observed program counter, so the log-facing payload encodes nothing about the value. The program counter now exists only as the typed u32 scalar in RemoteSessionView, which the existing clear_observations and clear_session scrubs own, and is rendered only by the session panel's explicit "Current EE PC" property row. The transition is extracted into a pure apply_program_counter so the retention invariant is unit-testable without a controller or a worker thread. The regression test applies the outcome across the 32-bit range and asserts that the whole log-facing payload -- level and message -- is identical for every value and carries no fixed-width literal, bare hexadecimal, decimal rendering, or hex prefix, while the view still stores the typed scalar and still answers to the scrub. The docs claimed the value was ephemeral UI state on the same terms as every other observation, which was untrue while the notice carried it. They now state that the property row is the value's only presentation and that the notice is deliberately value-free. No route, capability, schema gate, decode path, or worker/UI channel payload changes. Signed-off-by: Krill <natew94@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Krill <natew94@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a548c4168
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Propagates I/O errors. | ||
| pub fn query_supported(&mut self) -> io::Result<Vec<u8>> { | ||
| self.transact(b"qSupported:multiprocess+;swbreak+") | ||
| let reply = self.transact(b"qSupported:multiprocess+;swbreak+;qXfer:features:read+")?; |
There was a problem hiding this comment.
This sends qXfer:features:read+ as if it were a client feature, but the target-description negotiation that many stubs key off is the client xmlRegisters=... gdbfeature; qXfer:features:read+ is the stub feature returned in the response. With stubs that only advertise/serve target XML after seeing xmlRegisters (the same handshake GDB uses), Workbench reaches require_target_description and fails attach even though the target can provide the XML schema.
Useful? React with 👍 / 👎.
| self.view.last_error = None; | ||
| notices.push(RemoteSessionNotice { | ||
| level: RemoteNoticeLevel::Success, | ||
| message: format!("Read {} remote byte(s) at {address:#x}", bytes.len()), |
There was a problem hiding this comment.
When a memory read succeeds, this notice includes the guest address; WorkbenchApp::poll_remote_session forwards notices to self.log, which appends them to the activity ring and companion console. Unlike view.memory_summary, those log entries are not scrubbed on disconnect, cancellation, or connection loss, so reading a proprietary guest location leaves the address attributable after the remote session is cleared.
Useful? React with 👍 / 👎.
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
1 participant
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.
Summary
Safety and privacy
Verification
git diff --check origin/main...HEADcargo fmt --all -- --checkThe host is under a RAM safety stop, so Cargo build/test was intentionally left to CI.