Lockstep: stop reporting a step-cap timeout as a control-flow divergence - #16
Open
KakarottoCake wants to merge 1 commit into
Open
Lockstep: stop reporting a step-cap timeout as a control-flow divergence#16KakarottoCake wants to merge 1 commit into
KakarottoCake wants to merge 1 commit into
Conversation
The shadow interpreter walks at most m_ls_step_cap instructions looking for the block's end PC. When it runs out, `reached` is false and the block is reported as CTRLFLOW -- but nothing has been compared at that point, so there is no evidence the module diverged at all. The verifier was reporting its own give-up condition as a fault in the code under test. At the old default of 512 this dominated the output. On a full retail GameCube title the same run reported 631 CTRLFLOW divergences at cap 512 and 22 at cap 20000, and the 609 that disappeared were all sitting exactly at the cap. The cap was also hiding real divergences, which is the worse half: any block longer than 512 steps never reaches the register comparison, so its mismatches are silently skipped. Raising the cap took FP divergences from 463 to 622 -- 159 real reports the old default concealed. So: raise the default to 20000, and count a cap timeout in its own counter (cap_hits, surfaced in the summary line) instead of emitting a divergence for it. STATICRECOMP_LOCKSTEP_STEPCAP still overrides it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
The problem
LockstepChecksingle-steps the shadow interpreter up tom_ls_step_capinstructions looking for the block's end PC. If it runs out of steps,
reachedstays false and the block is reported as a
CTRLFLOWdivergence.But at that point nothing has been compared. Running out of steps is the
verifier giving up, not evidence that the module went anywhere different. The
tool was reporting its own timeout as a fault in the code under test, and the
default cap of 512 is low enough that this dominated the output.
Concretely, on a full retail GameCube title, the same run:
Two separate problems show up in that table.
It manufactures divergences. The 609
CTRLFLOWreports that disappear wereall sitting exactly at the cap. They were never divergences, and no change to
the recompiler could ever have fixed them — I spent real time chasing them
before working out where they came from.
It also hides divergences, which is worse. Any block longer than the cap
never reaches the register comparison at all, so its mismatches are silently
skipped. Raising the cap surfaced 159 additional real FP divergences that
the old default had been concealing. A verifier that quietly stops checking is
a more expensive bug than one that over-reports.
The change
Three small edits, all in the lockstep verifier — no emulator behavior is
touched:
m_ls_step_capdefault512→20000.STATICRECOMP_LOCKSTEP_STEPCAPstill overrides it, so anyone relying on the old value can set it back.
m_ls_cap_hitscounter and returns,instead of formatting a
CTRLFLOWreport. This is placed immediately afterreachedis computed, alongside the existing early returns.cap_hits=added to the[lockstep] summary:line, so the give-up count isvisible rather than merely absent.
A cap timeout is now reported as what it is — the verifier declining to answer —
which is different information from both "clean" and "diverged".
I did not touch
LS_UNDERCHARGE_GRACE, the loop-header logic, or anything inthe comparison itself.
Testing
I should be straightforward about the limits here. This is exercised in a
downstream Dolphin fork running static-recomp modules for a retail GameCube
title, which is where the numbers above come from; the same three edits applied
to a tree of yours. I have not built this specific branch against upstream
mainlocally — a full build is a long job on this machine and I would ratherput the finding in front of you now than sit on it. The change is a counter, an
early return and a
printfargument, so I would expect CI to be the fast check.Happy to build it properly if you would like that before considering it.
If 20000 seems arbitrary — it is chosen to be comfortably above the longest
block observed in practice rather than tuned. If you would prefer the cap left
alone and only the reporting fixed, that is a one-line change and I am happy to
split it.
AI disclosure
Contributing.mdasks for this, so: this patch was written with AIassistance, and I would rather you close it than have it merged under a wrong
impression.
It came out of debugging a downstream static-recomp runtime, where the false
CTRLFLOWreports sent me down a long dead end before the cause became clear.The investigation, the measurements above and the patch were all AI-assisted.
I am aware of the rule against using LLMs for changes related to emulated
console behavior. I believe this one sits outside it — it changes only how the
differential verifier reports its own timeouts, and touches no decoding, no FP
semantics, no register or memory modelling, and nothing the guest can observe.
But that is your call to make, not mine, which is why the disclosure is up front
rather than in a footnote.
If the rule means no regardless, that is completely fair. The table above is the
part that matters and it is yours to use either way: the default step cap is
concealing real divergences, whoever fixes it.