Repository navigation
Conversation
mstan
left a comment
There was a problem hiding this comment.
P1: the legacy extension fallback breaks supported title save-state compatibility.
In snapshot_check_guest, counting state_save_extra and requiring its current size to equal the incoming extension rejects older layouts that existing state_load_extra callbacks deliberately accept. The local released Mega Man X implementation has version-dependent v1-v16+ extension sizes, and Super Mario World accepts legacy SMWS/foreign chunks; neither registers state_validate_extra or snapshot_preflight. Their existing snapshots would start failing after an engine update. MMX's save callback also calls MmxCoopCapture, so invoking it while checking a rejected input does not satisfy the new purity assumption.
ROM-free reproduction: in this PR's transaction fixture, leave the already-supported one-byte load_variable path installed but omit the newly added validator, as existing consumers do. A valid RtlLoadSnapshotFromMemory(snapshot, core+1) is rejected (reproduced locally). Documentation of a new required callback does not migrate those consumers.
Please preserve the existing loader contract or include coordinated validators and compatibility tests for supported older title chunks before merging. Do not merely relax the size check and claim transactional protection: arbitrary legacy callbacks still need a real validation strategy.
The rest of the five-PR bundle passed 826 Python tests (2 skips), 109 Rust tests, and production C interpreter/bridge, PPU, audio-delivery, DMA snapshot, APU-port and DSP trace-gate checks. This is a consumer compatibility blocker despite those green generic tests. No new cross-title gameplay qualification is claimed.
Withdrawing the backward-compatibility merge requirement. The owner explicitly confirmed that older save-state compatibility is never required, across engine/game/serializer/backend revisions. Breaking or rejecting old snapshots is acceptable and requires no migration work. Same-build save/load, rewind/rollback and safe rejection still matter; native game saves are a separate contract. This supersedes that part of my prior review. The updated PR also removes the save-callback purity problem. This dismissal does not constitute approval of the newly revised head; current CI/code review remain separate.
5027379 to
64de342
Compare
Truncated guest snapshots could mutate the running machine before load failure was reported. File and memory loads now validate the complete guest prefix with the production serializer and private CPU/DMA/Snes containers before restoring live guest state.
Title extensions keep their existing opaque loader contract. The framework does not call a save callback to infer accepted extension sizes. Optional
snapshot_preflightruns outside the APU lock for titles that need extension rejection before live writes. Arbitrary latestate_load_extraeffects are not transactional.Rebased onto current main
68896b6, preserving the DSP-1 implementation identity check. The extracted regression fixture now explicitly includes<limits.h>andsnapshot_identity.h, repairing the failing compile on Linux and after the rebase.Validation of head
64de342:cf79444with updated Share HiROM contracts and evidence while preserving execution banks #199 and Settle the owed DRAM refresh tax before the first interpreted step after a compiled body #200–Remove the unreachablertl_apu_pace_check#202: 856 Python tests passed, 2 skipped, no exclusions; production dispatch and common_rtl/common_cpu_infra syntax checks pass.All three GitHub workflows (native analyzer, self-contained CLI and Lua) are green on this head, including Linux/Windows/macOS analyzer jobs.
Input remains capped at 16 MiB and must remain stable during the synchronous call. Current-build save/load and invalid-input safety are the contract; backward save-state compatibility is not a release requirement. No new cross-title gameplay or arbitrary title-callback transaction claim is made.