Invalidate per-PC code caches when a savestate replaces RAM - #80
Closed
mstan wants to merge 1 commit into
Closed
Conversation
Several consumers cache a classification of the instruction at a given PC and key that cache on g_dirty_ram_code_gen, per the contract stated where the counter is defined (runtime/src/memory.c): "Consumers that cache per-PC classifications of RAM instructions compare against this and re-derive after any code change - a cached kind must never survive an overlay reload." Today that counter is bumped from exactly two places, both on the instrumented store path: a page's clean->dirty transition, and dirty_ram_mark_executable_range(). A savestate restore reaches neither. BS_SEC_RAM applies a raw memcpy over all 2 MB, so code identity at any address can change completely while no page makes a clean->dirty transition and no executable range is marked. Every cache keyed on the generation therefore keeps serving verdicts derived from the PREVIOUS contents. The overlay loader's negative "no native owner" cache is the damaging one, because overlay_loader_dispatch() consults it before rediscovery, so one stale entry can pin an address to the wrong implementation for the rest of the run. Note the same restore already invalidates psx_kernel_bless_note_range(), so the path is aware it must invalidate downstream caches - it only ever did one of them. Fix: add dirty_ram_invalidate_code_caches() and call it after the BS_SEC_RAM memcpy. dirty_ram_set_bitmap_words() (the BS_SEC_DIRTY apply path) calls it too, since a wholesale bitmap replacement changes which addresses are RAM-resident code by the same argument. Bumping the generation is sufficient by construction: each cache stores the generation it was derived under and re-derives on mismatch, so this fixes every consumer at once instead of patching them individually. Scope, stated honestly: this closes a real latent correctness gap in the load path and is worth having on its own. It is NOT a fix for the separate, still-open bug where loading a gameplay state into a process that never entered that stage leaves the player unable to move - that was measured and the overlay cache is demonstrably NOT its cause (the dirty bitmap is identical between a working and a broken instance, and covers only the kernel window, so the player's code is static-dispatched in both). No behaviour change for a process that never loads a state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merged
Owner
Author
|
Closing as superseded: current master already includes the savestate/RAM replacement cache-invalidation behavior through the subsequently integrated runtime work. The final post-merge framework suite (36/36) and four-title regeneration/build regression passed. |
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.
Problem
g_dirty_ram_code_genexists so that consumers caching a per-PC classification of a RAM instruction can notice when the code at that address changed. The contract is stated where the counter is defined inruntime/src/memory.c:Today it is bumped from exactly two places, both on the instrumented store path:
dirty_ram_mark_page)dirty_ram_mark_executable_range()A savestate restore reaches neither.
BS_SEC_RAMapplies a rawmemcpyover all 2 MB, so code identity at any address can change completely while no page makes a clean→dirty transition and no executable range is marked. Every cache keyed on the generation therefore keeps serving verdicts derived from the previous RAM contents.The overlay loader's negative "no native owner" cache is the damaging one, because
overlay_loader_dispatch()consults it before rediscovery — so a single stale entry can pin an address to the wrong implementation for the remainder of the run.Worth noting: the same restore path already calls
psx_kernel_bless_note_range(), so it is aware it must invalidate downstream caches. It only ever did one of them.Fix
dirty_ram_invalidate_code_caches()(bumps the generation) and call it right after theBS_SEC_RAMmemcpy.dirty_ram_set_bitmap_words()too — theBS_SEC_DIRTYapply path — since replacing the dirty bitmap wholesale changes which addresses are RAM-resident code by the same argument.Bumping the generation is sufficient by construction: each cache stores the generation it was derived under and re-derives on mismatch, so this fixes every consumer at once rather than patching them one at a time.
+41 lines, 2 files, no behaviour change for a process that never loads a savestate.
Scope — stated honestly
This closes a real latent correctness gap in the load path and is worth having on its own merits.
It is not a fix for a separate, still-open bug I hit while working nearby: loading an in-stage gameplay state into a process that never itself entered that stage leaves the player unable to move. I measured that one and the overlay cache is demonstrably not its cause — the dirty bitmap is byte-identical between a working and a broken instance, and with 4 KB pages it covers only the kernel window, so the player's code is static-dispatched in both. That investigation continues separately; this commit is being sent on its own because it stands alone.
Testing
gcc -fsyntax-onlyof both translation units againstmaster+ this commit, using the real build's defines/includes: clean.