Breakpoints can be set, not only listed - #127
Conversation
`breakpoints()` already read the engine's whole table through eleven getters. Nothing wrote any of them: the only write path was the `execute` text hatch, and the one public type over the write side -- `Breakpoint` -- had no caller, sat on the v1 interface where the read path uses v2, offered no setter but `set_offset_expression`, and panicked in three of its four methods. `set_breakpoint`/`set_breakpoint_bounded` take a `BreakpointSpec` and answer with the breakpoint as the *engine* holds it, read back through the same getters `breakpoints()` uses rather than echoed from the spec. `remove_breakpoint` and `enable_breakpoint` take an id, which is what `bc`/`be`/`bd` take and the one identity that cannot dangle, so there is no public handle type at all and `ScopedBreakpoint` is left as the only wrapper over a raw breakpoint object -- one answer to who removes a breakpoint and when, where there were two that disagreed. Four things were measured first, on dbgeng 10.0.29547.1002, and each one shaped the API. `examples/breakpoint_probe.rs` is the record and re-runs all of it. A symbolic location resolves *eagerly*, so it can block: 2445 ms for a cold `KERNELBASE!CreateFileW` over `srv*` against an empty store, against 151 ms warm, 0 ms for an address and 0 ms to defer when the module is absent. `SetInterrupt` reaches it, so `set_breakpoint_bounded` is a real bound rather than a promise -- but a break is *silent*, returning `Ok` with a breakpoint and abandoning the symbol load, leaving the module on export symbols for the rest of the session. So the result carries `cut_short`. Written the obvious way, as `Result<(), DbgEngError>`, this change would have quietly traded away both the bound and the evidence of one. The engine does not deduplicate breakpoints at an address; `bp` and `bu` do, by resolving and then removing whatever is there. Keyed by the resolved address, so a deferred expression duplicates freely -- `bp nosuchmod!Sym` three times leaves three where `bp ntdll!NtCreateFile` three times leaves one. `OnExisting::Replace` is that behaviour as a value, reporting the ids it took as `BreakpointSet::replaced`; `Add` is the default, because a primitive should not destroy what the caller did not name. This crate and its caller had both recorded the deduplication as the engine's, which made the choice invisible. A duplicate is not a double stop: two breakpoints at one address stop the target once and activate *both*, so each one's command runs and removing one by id leaves the address armed by the other. That is what makes the choice matter to a caller installing a logging breakpoint rather than merely untidy. And a breakpoint is born disabled *and* at address zero, which is why `BreakpointSpec::enabled` defaults to true and flags are applied last, after the command: an enabled breakpoint whose command has not landed yet stops the target instead of running it -- on a live kernel, a halted machine where the caller asked for a log line. `BreakpointInfo::expression`'s documentation described only what `bp` does. A location set through `SetOffsetExpression` keeps its expression beside a resolved address, so `None` there is not the universal case for a live breakpoint; `deferred` is the field that answers whether one has an address yet. Refs #126 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WhhF5x9fE4bdd1jiKBvhNa
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: c5f06f99ae
ℹ️ 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".
| if let BreakpointAt::Address(address) = self.at | ||
| && !address.is_multiple_of(u64::from(watch.size)) | ||
| { |
There was a problem hiding this comment.
When a data breakpoint uses BreakpointAt::Expression, this check skips alignment even though new_breakpoint subsequently resolves the expression and makes the resulting address available. An expression such as nt!Foo+1 with an 8-byte watch therefore succeeds and leaves an invalid enabled breakpoint that DbgEng rejects only on the next resume—the exact delayed failure this validation is intended to prevent. Recheck the resolved offset before applying SetDataParameters.
Useful? React with 👍 / 👎.
| match unsafe { breakpoint.breakpoint.GetOffset() } { | ||
| Ok(address) if address != DEBUG_INVALID_OFFSET => { | ||
| self.remove_breakpoints_at(address, id)? | ||
| } |
There was a problem hiding this comment.
With OnExisting::Replace, the existing breakpoints are removed here before the new command, pass count, thread, data parameters, and flags are applied. If any later setter rejects the spec—for example an invalid thread or pass count—the guard removes the new breakpoint and the call returns Err, but the previous working breakpoints have already been destroyed. Configure the disabled replacement first, then remove the old entries immediately before arming it.
Useful? React with 👍 / 👎.
…nt is certain Two review findings on the same seam, both real, both accepted. A data breakpoint's alignment was judged only where the caller supplied an address. `BreakpointSpec::validated` cannot do better -- an expression has no address until the engine evaluates it -- but `set_breakpoint_bounded` can, and did not: `ba` on `nt!Foo+1` with an eight-byte watch was accepted and then refused at the next resume, which is precisely the delayed failure the check exists to prevent. The alignment is now judged again on the resolved offset, between the location step and everything after it. Measured: an eight-byte watch on `ntdll!NtCreateFile+1` is refused naming `0x7ffc9b880e11`. A *deferred* data breakpoint keeps the gap and cannot close it -- the engine resolves it on a later module load with nothing of this crate's on the stack to see the result -- and `DataWatch::size` says so rather than implying the rule is total. And `OnExisting::Replace` removed the existing breakpoints before the new one's command, pass count, thread and data parameters were applied. Any of those can fail, and the failure removes the new breakpoint and returns `Err` -- so a caller whose thread id the engine refused lost the breakpoints they already had *and* got an error. The removal moves to between the last setter and the arming, which is the only window where it is not destructive: everything that can fail has already succeeded, and the address is still not armed twice. It stays after the location, since a resolved address remains the only thing there is to key on, and a deferred location still replaces nothing. Both were placement, not logic, so nothing about what `Replace` means has moved: the probe still reports `replaced=[0, 1, 2]` for three collapsed breakpoints and an empty `replaced` for three deferred ones. Refs #126 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WhhF5x9fE4bdd1jiKBvhNa
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48a4124731
ℹ️ 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".
| let Some(watch) = self.data else { | ||
| return Ok(()); | ||
| }; | ||
| if !matches!(watch.size, 1 | 2 | 4 | 8) { |
There was a problem hiding this comment.
When DataAccess::Execute is paired with size 2, 4, or 8 on x86/x64, this access-independent check accepts the spec even though hardware execute breakpoints require a one-byte length. SetDataParameters can leave that invalid combination installed until the next resume rejects the debug register, recreating the delayed failure this validation is meant to prevent; require size == 1 for execute watches.
Useful? React with 👍 / 👎.
| .filter(|id| *id != DEBUG_ANY_ID); | ||
| Ok(BreakpointInfo { | ||
| id, | ||
| kind, |
There was a problem hiding this comment.
When this record describes a data breakpoint, the readback keeps only BreakpointKind::Data and never calls GetDataParameters, so neither breakpoints() nor the new BreakpointSet::breakpoint can report the watched size or whether it is a read, write, execute, or I/O watch. This makes the promised engine readback insufficient to verify or faithfully enumerate the newly supported data breakpoints; populate an optional DataWatch from the engine for data kinds.
Useful? React with 👍 / 👎.
Second review round, two findings, one built and one measured and declined. `breakpoint_info` read ten of a breakpoint's fields and never called `GetDataParameters`, so a data breakpoint read back as `kind: Data` with nothing about the region it watched. That was tolerable while nothing could *set* one; it is not now, because `BreakpointSet::breakpoint` is sold as the engine's own answer and it could not confirm the half of a spec most worth confirming. `BreakpointInfo::data` closes it, asked only of a data breakpoint since a code one has no watched region and the engine owes no meaningful answer for one. `DataAccess::Other` keeps bits this build cannot name rather than folding them into a plausible neighbour, exactly as `BreakpointKind::Other` does, and a unit test pins the round trip in both directions -- a mapping that is merely *a* function each way would let a breakpoint read back as something it is not. The other finding was that an execute watch must be one byte on x86/x64 -- a DR7 slot with `R/W=00` carries `LEN=00` -- and that accepting size 2, 4 or 8 recreates the delayed failure the alignment check exists to prevent. The fact is right and the consequence is not: `SetDataParameters` refuses those sizes **synchronously** with `E_INVALIDARG`, and `ba e2`/`e4`/`e8` fail the same way, against a size 1 that is accepted (measured on dbgeng 10.0.29547.1002). Nothing is installed and no resume is involved, so the engine already reports it against the call that caused it -- which is the whole property `validated` exists to provide. Restating it there would put an x86/x64 rule in front of an engine that answers per target, which is the reason that function already declines to judge an 8-byte watch it cannot know the pointer width for. The measurement is recorded beside that rule so the next round has it. Refs #126 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WhhF5x9fE4bdd1jiKBvhNa
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a27e52055
ℹ️ 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".
| source, | ||
| } | ||
| })?; | ||
| self.remove_breakpoints_at(address, id)? |
There was a problem hiding this comment.
With OnExisting::Replace, an HRESULT failure from the subsequent AddFlags call returns Err and drops the new breakpoint, but this call has already permanently removed every old breakpoint at the address. The fresh code moves removal past the parameter setters, but the fallible arming step remains after it, so the earlier state-loss issue still occurs on this error path; arm the replacement before deleting the old entries, or preserve/restore them if arming fails.
Useful? React with 👍 / 👎.
…ceeds Third review finding on one mechanism, which by this repo's own rule is the signal to delete the choice generating them rather than move it a third time. `OnExisting::Replace` removed the caller's breakpoints in the middle of building the new one, and every round found a fallible step still behind it: first the command, pass count, thread and data parameters, then -- after those moved -- the `AddFlags` that arms it. Each fix was locally correct and none of them could be the last, because there is no position in the middle that has the property wanted. "Nothing is destroyed unless the replacement is certain" is a statement about the *end* of the sequence, so the removal goes there: after every step that can fail. What that gives up is the original reason for the placement -- that the address is never armed twice -- and it is worth less than it sounded. The window is now between `AddFlags` and the removal, and the engine is not pumping in it: nothing in this call resumes the target and a `DebugEngine` drives one engine from one thread, so the target cannot execute between those two statements. An unobservable double-arm against a caller permanently losing breakpoints is not a close trade. The removal is also best-effort now, because at that point the mutation this call exists for has happened and been armed: raising a cleanup failure would report a breakpoint that is set as a call that failed, which is the shape a caller retries -- the same rule the openers follow for a post-commit step. `replaced` therefore reports what was actually taken, and one the engine would not give up stays visible in `breakpoints()` rather than being claimed as gone. Behaviour is unchanged where it succeeds: the probe still reports `replaced=[0, 1, 2]` for three collapsed breakpoints and an empty `replaced` for three deferred ones. Refs #126 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WhhF5x9fE4bdd1jiKBvhNa
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 208bbaf143
ℹ️ 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".
| let Some(watch) = self.data else { | ||
| return Ok(()); | ||
| }; | ||
| if !matches!(watch.size, 1 | 2 | 4 | 8) { |
There was a problem hiding this comment.
On an x86 target, this accepts an 8-byte data watch even though the architecture has no corresponding debug-register length; the breakpoint is therefore returned as successfully armed and the engine rejects it only when the caller next resumes execution. Because validation is invoked from set_breakpoint_bounded, the engine is available there to inspect the target architecture and reject size 8 before creating the breakpoint, preserving the API's stated guarantee that invalid watch sizes are reported at the set operation.
Useful? React with 👍 / 👎.
1 participant
Closes the write half of #126.
breakpoints()already reads the engine's whole table through eleven getters. Nothing wrote any ofthem: the only write path was the
executetext hatch, and the one public type over the write side—
Breakpoint— had no caller insrc/orexamples/, sat on the v1IDebugBreakpointwhere theread path uses v2, offered no setter but
set_offset_expression, and panicked in three of its fourmethods.
The API
set_breakpoint/set_breakpoint_boundedtake aBreakpointSpec— location asBreakpointAt::Addressor::Expression, plus an optional command, match thread, pass count,one-shot flag, and a
DataWatchthat makes it a data breakpoint (ba) — and answer with aBreakpointSetcarrying the new breakpoint as the engine holds it, read back through the samegetters
breakpoints()uses rather than echoed from the spec.remove_breakpointandenable_breakpointtake an id, which is whatbc/be/bdtake and the one identity thatcannot dangle.
So there is no public handle type at all, and
ScopedBreakpoint(private, removes on drop unlesskeep) is left as the only wrapper over a raw breakpoint object — one answer to who removes abreakpoint and when, where #126 notes there were two that disagreed.
The kind is not a field:
dataisSomeexactly when the breakpoint is a processor breakpoint, sothe kind and its parameters cannot disagree.
Four things measured first, each of which shaped the API
On dbgeng 10.0.29547.1002, x64, Windows 11 26200.
examples/breakpoint_probe.rsis the record andre-runs all of it.
1. A symbolic location resolves eagerly, so it can block — #126's open question, and the answer
is its second outcome.
KERNELBASE!CreateFileW(srv*, empty store)nosuchmod!Sym(module absent → defers)SetInterruptreaches it, soset_breakpoint_boundedis a real bound rather than a promise. But abreak is silent: it returns
Okwith a breakpoint, and abandons the symbol load, leaving themodule on export symbols for the rest of the session. Hence
BreakpointSet::cut_short. Written theobvious way —
Result<(), DbgEngError>, which is what the type this replaces had — the change wouldhave quietly traded away both the bound and the evidence of one, which is the regression #126 was
opened worrying about, arriving through the return type rather than through the API.
2. The engine does not deduplicate;
bpandbudo. Both resolve and then remove whatever isalready at that address, printing
breakpoint N redefined. Keyed by the resolved address — bysymbol, by literal address and by
symbol+0alike — so a deferred expression duplicates freely:bp ntdll!NtCreateFile×3bp nosuchmod!Sym×3bpreplaced0, 1, 2)OnExisting::Replaceis that behaviour as a value, reporting the ids it took asBreakpointSet::replaced.Addis the default, because a primitive should not destroy what thecaller did not name.
This one is a correction: both this crate and its caller had recorded the deduplication as the
engine's, in four places, which made the choice invisible — a caller migrating off
bpwouldsilently start producing duplicates.
3. A duplicate is not a double stop. Two breakpoints at one address stop the target once and
activate both — the probe's
duplicate-costarm shows both command strings running on one stop— and removing one by id leaves the address armed by the other. That is what makes the choice matter
to a caller installing a logging breakpoint rather than merely untidy.
4. A breakpoint is born disabled and at address zero (documented, and what the deleted type
shipped). So
BreakpointSpec::enableddefaults totrue, and flags are applied last, after thecommand: an enabled breakpoint whose command has not landed yet stops the target instead of running
it — on a live kernel, a halted machine where the caller asked for a log line.
Why this is worth having downstream
windbg-mcp'sset_breakpointrunsbp <expression>as text and pays three times: the operand hasto be screened for
;and"because either is an injection, the id has to be recovered by diffingblbefore and after (with a documented degraded mode when the "before" read fails), and the wholething has to be bounded because there is a command in it.
The command is the sharpest case, and it is
ioctl_trace's: it hand-buildsbp <dispatch> ".printf \"IOCTL %08x …\", …; gc"with the escaping done by hand in a format string.The probe's
commandarm sets exactly that string as a parameter and reads it back throughGetCommand:Also fixed
BreakpointInfo::expression's doc described only whatbpdoes. A location set throughSetOffsetExpressionkeeps its expression beside a resolved address, soNonethere is notthe universal case for a live breakpoint;
deferredis the field that answers whether one has anaddress yet.
breakpoints()reads throughGetBreakpointByIndex2, putting the whole breakpoint path onIDebugBreakpoint2instead of mixing interface versions.DataWatch's size and alignment are refused byBreakpointSpec::validatedbefore a breakpointexists. The engine takes a bad pair at the set and rejects it at the resume, so leaving it to
the engine reports the mistake against a
gothat did nothing wrong.DEBUG_BREAKPOINT_DEFERREDis deliberately not settable — the docs are explicit that it "cannot bemodified by any client" — so it is read back and never sent.
ADDER_ONLYandGO_ONLYare left out:one worker owns one engine client per session downstream, so a private breakpoint buys nothing.
No new
windowsfeature: every setter is already onIDebugBreakpoint2in the pinned 0.62.2.Commands run
cargo fmt --all -- --check— cleancargo clippy --all-targets— no new warnings (three pre-existingchunks_exactones remain)cargo test— 153 passed (5 new unit tests), 4 doctestscargo +nightly miri test— 135 passed, 0 failed, run by hand becausemiri.ymldoes not runon pull requests and this touches
unsafecargo run --example breakpoint_probe -- allagainst a real engine — every arm aboveOne flake seen, and I could not attribute it to this change.
test_a_mixed_session_comes_apart_by_where_each_process_came_fromfailed once in ~30 runs, assertinga two-process session that held only the attached one — the launched process had not appeared yet. It
did not reproduce in 12 full-suite runs on this branch or 12 on
main, and this diff touches nothingin the launch, attach or teardown path. Flagging it rather than claiming it proven pre-existing.
Note for the reviewer
examples/breakpoint_probe.rsneeds the engine DLLs intarget/debug/examples/, nottarget/debug/— an example loads from its own directory. Without that it gets System32'sdbgeng.dll, which with nosymsrv.dllcannot fetch a PDB and with nomsdia140.dllcannot parseone; nothing errors, exported symbols resolve from the export table and everything else defers, so a
cold fetch "takes" 3 ms and every arm measures the wrong engine. That is how the first run of the
example appeared to disprove the measurement it exists to record. The header says how to spot it.
🤖 Generated with Claude Code
https://claude.ai/code/session_01WhhF5x9fE4bdd1jiKBvhNa