Invalidate emitter state caches where they can go stale - #22
Closed
dougchansan wants to merge 2 commits into
Closed
Conversation
emitFPAvailable checks MSR[FP] once and caches the answer for the rest of
the region, so later floating-point instructions skip the check. That is
sound only while MSR cannot change underneath it, and noteStateWrite does
not clear the cache on an MSR write.
So a region shaped like
fadd f1, f2, f3 ; checks MSR[FP], caches "available"
mtmsr r0 ; clears MSR[FP]
fadd f4, f5, f6 ; skips the check, executes anyway
runs the second fadd with floating point disabled and raises no
FP-unavailable exception. The guest's lazy FPU context switching is built
on receiving that exception, so a thread that should have had its FPU
context saved silently does not.
noteStateWrite already invalidates the related caches for FPR, PS1, FPSCR
and HID2 writes; MSR was the missing case. The C backend does not have
this problem because it emits a check per instruction rather than caching
one per region.
Found while investigating an unrelated title. It is rare in practice: on
Pokemon Colosseum it changes exactly one of 4890 emitted objects
(chunk_3686_text1_801CFBE0.o), because an mtmsr and a floating-point
instruction have to land in the same region for it to bite. I do not have
a title whose visible behaviour changes because of it, so this is offered
as a correctness fix rather than a fix for a reported symptom.
The call-resume path reloads each used state slot from CPUState by hand but leaves the emitter's compile-time conclusions in place. Those conclusions are about state the callee is free to change: fp_available_checked_ whether MSR[FP] was already checked known_state_ which slots hold a known constant psq_direct_proven_ whether paired-single addressing was proven psq_indexed_proven_ the FP representation tracking known_state_ is the sharpest: stateValue() returns the cached constant in preference to loading memory, so if the emitter proved a slot held a constant before the call and the callee overwrites it, the caller keeps using the stale constant even though the reload put the correct value in memory. reloadUsedState() does the same per-slot reload and drops the caches, and the fallback resume path in control_flow.cpp already uses it for exactly this reason. Rare in practice, like the MSR case: on Pokemon Colosseum the two fixes together change one of 4890 emitted objects. Offered as a correctness fix rather than a fix for an observed symptom.
This was referenced Aug 23, 2026
Contributor
Author
|
Both fixes are in upstream |
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.
emitFPAvailablechecksMSR[FP]once and caches the answer for the rest of the region, so floating-point instructions after the first one skip the check. That is sound only whileMSRcannot change underneath it — andnoteStateWritedoes not clear the cache on anMSRwrite.So a region shaped like this:
runs the second
faddwith floating point disabled and raises no FP-unavailable exception. The guest's lazy FPU context switching is built on receiving that exception, so a thread that should have had its FPU context saved silently does not.noteStateWritealready invalidates the related caches forFPR,PS1,FPSCRandHID2writes —MSRwas the missing case:The C backend does not have this problem because it emits
ppc_fp_available_inlineper instruction rather than caching one answer per region.Honesty about impact
I found this while investigating something else, and it is rare in practice. An
mtmsrand a floating-point instruction have to land in the same region for it to bite. On Pokémon Colosseum it changes exactly one of 4890 emitted objects (chunk_3686_text1_801CFBE0.o), verified by hashing every object before and after.I do not have a title whose visible behaviour changes because of it. It is offered as a correctness fix, not as a fix for a reported symptom — the code is wrong in a way that could produce silent FPU state corruption on any title where the pattern occurs, and it is cheaper to close than to diagnose later.
Second commit: the same class of bug on the call-resume path
While checking whether the MSR case had siblings, the native call-resume path turned out to have the same shape. It reloads each used state slot from
CPUStateby hand, but leaves the emitter's compile-time conclusions intact — and the callee is free to invalidate every one of them:fp_available_checked_MSR[FP]was already checked this regionknown_state_psq_direct_proven_/psq_indexed_proven_known_state_is the sharpest of these, becausestateValue()returns the cached constant in preference to loading memory. If the emitter proved a slot held a constant before the call and the callee overwrites it, the caller keeps using the stale constant — even though the reload put the correct value in memory.reloadUsedState()performs the same per-slot reload and drops the caches, and the fallback resume path incontrol_flow.cppalready uses it for exactly this reason. This just makes the native-call path consistent with it.Same honesty as above: the two fixes together change one of 4890 emitted objects on Pokémon Colosseum. Both are correctness fixes without an observed symptom behind them.