Conversation
The tick basis was actually sound - CoreTiming::GetTicks() is continuous across Advance(), so "ticks unchanged" really does mean "no instruction retired since", which is exactly the window the suppression needs. The plumbing around it was the problem: - ExecRegBreakpoint() applied the skip only to the pause, so stepping off a log+pause register breakpoint printed it again and counted a second hit. The check now sits at the top of ExecBreakPoint(), ExecMemCheck(), ExecOpMemCheck() and ExecRegBreakpoint() instead of being repeated at seven call sites across the interpreter and four JIT frontends, where one of them had it wrong and another checked a different address than the rest. - Address 0 doubled as "nothing to skip" (ClearSkipFirst() existed but was dead code; the JITs cleared by calling SetSkipFirst(0)), so a breakpoint at 0 would have been permanently suppressed. There's an explicit valid flag now, and ClearSkipFirst() is what clears it. - The marker was set from five places and never cleared when execution stopped, so one could outlive the resume that armed it. Core_Break() clears it now, and the two WebSocket subscribers that set it immediately before asking for a step - which sets it again itself - no longer do. - SetSkipFirst() now only arms when some breakpoint machinery actually exists, so a stale marker can't sit around waiting to swallow a breakpoint added later. CheckSkipFirst() returning an address (compared against pc by each caller) is replaced by ShouldSkipBreakpoint(addr), which compares against both addr and currentMIPS->pc - under a JIT those differ, and only some callers knew that. Covered by the Breakpoints unit test, including that a suppressed breakpoint neither logs nor counts a hit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
Set two breakpoints four bytes apart, both logging, run into the first, then press Next: the second one never logs, however many times you step. Reproduced on both the interpreter and the JIT. The skip-first mechanism was doing two different jobs with one marker. Every resume and every step recorded the address it started from, and any breakpoint check at that address was suppressed outright. That's right for the breakpoint you're parked on - you have to be able to get off it - but stepping *onto* an address is not the same as having reported the breakpoint there, and the next step suppressed it before it ever logged. Split into the two things that were being conflated: - resumedFrom_ is where the current run or step started. It only drops the pause, not the log or the hit count. It still covers the temporary breakpoint, which is what makes "run to here" work when you're already on that address. - reported_ is the breakpoint we already logged and counted. Reporting stops the CPU before the instruction runs, so the resume that follows arrives at the same pending execution and must not report it twice. Both are (address, tick count) pairs, which identify one pending execution of one instruction: ticks only move when the CPU retires an instruction, so the marker stops matching as soon as it runs, and a breakpoint in a loop still fires every iteration. reported_ can't be armed where the report happens, though. Under a JIT that's inside a compiled block whose cycles are already accounted for, so the tick count there isn't the settled one we see on the way back in - arming it there double-logged the breakpoint under -j. So the report just records the address, and NotifyResumingFrom() turns it into a real marker once the CPU has stopped. That also has to be idempotent: a step-over arms its temporary breakpoint and then calls Core_Resume(), which notifies a second time. MemCheck::Action() no longer pauses by itself - the caller decides, the same way ExecBreakPoint() already did, so all three breakpoint kinds share the handling. Verified on both backends: two adjacent breakpoints now log once each while stepping (was one log total), stepping off a breakpoint still doesn't re-log it (was two under -j), step-over still skips the call and logs a breakpoint at the address it lands on, and a breakpoint in a loop reports once per iteration. pspautotests 314/314. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
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.
This fixes a bunch of minor issues when stepping around breakpoints that log, but I'm not yet completely convinced by Claude's solution here.