Observe execution instead of sampling it - #14
Merged
Merged
Conversation
wait({ pc }) compared registers[15] once per runFrame(). A frame is ~280,896
cycles, so the comparison saw only whatever the CPU was doing at the boundary.
Measured on a real game over 300 frames: exactly ONE distinct PC is ever
observable there — the BIOS wait loop the game idles in — against 1,624 distinct
PCs that actually execute. Every other address timed out, which a caller reads as
"never reached". A function running seven times a frame reported as never reached.
- ArmCpu.addExecWatchpoint(address, cb) fires from the instruction step and
returns a disposer. It is a registry rather than a hook because setDebugHooks is
a single slot that one owner replaces wholesale, and gba-browser already owns
it; an analysis tool must not evict a debugger's hooks to watch a PC.
- wait({ pc }) uses it, and accepts a symbol name as well as an address, so the
timeout message can name what did not run.
- watchExecution(target) is the execution counterpart to watchMemory: exact
`count`, `hits` carrying the caller's lr and source location, and `dropped`.
- watchMemory reports `dropped`. Capping silently left hits.length === maxHits
meaning either "that is all" or "that is the first few" — on one measured watch,
50 recorded and 26,744 dropped with nothing to say so.
The obvious objection does not hold: a per-instruction dispatch measured 0.92x
against the unhooked run over 120 frames, i.e. inside the noise. The dispatch is
gated on a flag so the no-watchpoint path stays a single boolean test.
Three things measurement corrected:
- I had cited AcknowledgeInterrupt as a function that "runs every frame". The
instruction counter says it executes ZERO times in this savestate, so the
original example was wrong; call_r3 (420 executions in 60 frames) replaces it.
- The first test loop used `b .-4` encoded 0xE7FD, which targets BASE+2 and never
re-executes BASE. The watchpoint fired once and the test failed for the right
reason. Correct encoding is 0xE7FC.
- watchExecution read thumb state from the optional `cpuCpsr` hook, which only
HeadlessRuntime wires, so a directly-constructed engine reported ARM for
everything. It reads the CPU now.
Ablations: removing the dispatch fails 5 arm-emulator and 4 gba-emulator tests;
removing the drop counter fails the watchMemory cap test. The first attempt at
the gba-emulator half proved nothing — that package builds against arm-emulator's
dist, so the ablation has to be rebuilt before it is visible.
141 tests in arm-emulator (was 135), 74 in gba-emulator (was 67).
… address
Follow-up to the review of `pc: number | string`. The union itself is sound — both
arms denote one instruction — but it invited an asymmetry, and looking for that
turned up two more of the same shape.
- A numeric code address kept bit 0, while a symbol had it cleared. On ARM that
bit is a state marker, never part of an instruction address, and a Thumb
function POINTER carries it set — which is exactly what read32 returns from a
callback table. Measured: watchExecution(0x804fb8d) counted 0 where
watchExecution(0x804fb8c) and watchExecution('call_r3') both counted 420, for
one function. That is the false negative this whole branch exists to remove,
reintroduced through the arm I forgot to normalize.
- watchMemory took only a number. The script context is untyped JS, so a symbol
name was accepted, coerced by `>>> 0` to address 0, and watched nothing — zero
hits, no error. It now takes a symbol, and an unknown one throws.
- watchSymbol sized its watch from st_size, which is null for a linker-placed
global. In a decomp that is EVERY data global, so watchSymbol('gBgInfo')
watched the first byte of a 112-byte array and reported the rest as never
written. Both paths now use the object's extent, so one rule sizes them.
watchSymbol is now watchMemory with a name, which is all it ever was.
One test could not be written: nothing in test-projects has a RAM symbol whose
extent comes only from the DWARF, so the dwarf-sourced default length is covered
only by debug-info's own symbolExtent tests. I dropped the test I had written for
it rather than assert it against a ROM symbol whose writes the bus discards —
which is why it failed in the first place.
Ablations: unmasking the Thumb bit fails the pointer test; defaulting a symbol
watch back to 1 byte fails both the watchMemory and watchSymbol tests.
78 tests in gba-emulator (was 74).
`pc` named the field after the one value it does not take. This API already
defines the program counter as pipeline-ahead of the instruction it is executing —
WatchHit carries `pc` and `instructionAddress` as separate fields for exactly that
reason — while the wait wants an instruction address. Passing a hit's `pc` watches
the NEXT instruction, and measurement shows both failure shapes:
call_r3 entry=420 entry+2= 0 <- reads as "never ran"
MP2KPlayerMain entry=240 entry+2=240 <- plausible, identical, wrong instruction
The second is the worse one: nothing distinguishes it from a correct answer.
`execution` was chosen over `instruction`, `execute`, `breakpoint` and keeping `pc`:
- it maps one concept to one word across the API — watchExecution records the
event, wait({ execution }) waits for it — so learning either teaches the other;
- it is a noun, like its siblings `frames`, `memory` and `pixel`;
- it carries no register connotation, so the hit.pc mistake cannot be spelled;
- `breakpoint` has the strongest prior from GDB and mGBA but the wrong model:
everywhere else that word halts execution, and this resumes past it;
- `instruction` matches instructionAddress and was the close runner-up, but it
ambiguously names an opcode, so `instruction: 0x4770` could read as "wait for a
bx lr".
`wait({ pc })` still works and logs a deprecation notice, so no script breaks.
Ablations: dropping the alias branch and dropping the notice each fail the
deprecation test. 82 tests in gba-emulator (was 78).
- wait({ pc }) is removed rather than deprecated. The rename is the breaking
change; carrying both spellings would keep the misleading one in the API and in
every example an agent might copy.
- Removing it exposed that wait() fell out of the function when no branch
matched, so an unknown key waited for nothing and continued as though the
condition had been met. Scripts run as untyped JS, so TypeScript does not catch
a stale `pc` there. It now throws and names the conditions it accepts.
- The three changesets are cut to what changed.
- docs/scripting.md keeps the behaviour and drops the argument for it: no
deprecation note, no explanation of why an unaligned read refuses or what an
out-of-bounds subscript would have addressed.
81 tests in gba-emulator; all gates exit 0.
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.
Item 2 of the analysis-surface audit: telling "not observed" apart from "not happening".
wait({ pc })could not see executionIt compared
registers[15]once perrunFrame(). A frame is ~280,896 cycles, so the comparison only ever saw whatever the CPU was doing at the boundary.Measured on a real game with an instruction-level counter over 300 frames:
So for the busiest named function in that window:
A caller reads that as never reached. Absence is the answer that gets written into a docstring or used to justify deleting a branch, and no build gate checks it.
After: found in 23 ms.
The change
ArmCpu.addExecWatchpoint(address, cb)— fires from the instruction step, returns a disposer. A registry rather than a hook becausesetDebugHooksis a single slot one owner replaces wholesale, andgba-browseralready owns it — an analysis tool must not evict a debugger's hooks to watch a PC.wait({ pc })uses it, and accepts a symbol name as well as an address, so the timeout can name what did not run:wait({ pc }) timed out after 30 frames waiting for "AcknowledgeInterrupt" (0x8001144).watchExecution(target, options?)— the execution counterpart towatchMemory. Exactcount,hitscarrying the caller'slrand source location,dropped,stop().watchMemoryreportsdropped. Capping silently lefthits.length === maxHitsmeaning either "that is all of them" or "that is the first few". On one measured watch: 50 recorded, 26,744 dropped, nothing in the handle saying so.The obvious objection does not hold. A per-instruction dispatch measured 0.92x against the unhooked run over 120 frames — inside the noise. The dispatch is gated on a flag, so the no-watchpoint path is a single boolean test.
Verification
Against the ROM:
wait({ pc })findscall_r3by symbol and by address, still times out on a function that genuinely never runs, andwatchExecutioncounts exactly 420 — matching the independent instruction-level ground truth.Ablations: removing the dispatch fails 5
arm-emulatorand 4gba-emulatortests; removing the drop counter fails thewatchMemorycap test.141 tests in
arm-emulator(was 135), 74 ingba-emulator(was 67).check-types,lint,format:check,buildandtestall exit 0.Three things measurement corrected
AcknowledgeInterruptas a function that "runs every frame" when briefing this item. The counter says it executes zero times in this savestate — the example was wrong, andcall_r3replaces it.b .-4as0xE7FD, which targetsBASE+2and never re-executesBASE. The watchpoint fired once and the test failed for the right reason; the correct encoding is0xE7FC.watchExecutionread Thumb state from the optionalcpuCpsrhook, which onlyHeadlessRuntimewires, so a directly-constructed engine reported ARM for everything.One process note: the first
gba-emulatorablation proved nothing, because that package builds againstarm-emulator'sdist— the ablation has to be rebuilt before it is visible. Re-run with a build in between, it bites.