diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index acd79709..ae1c7f32 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -310,6 +310,22 @@ jobs: # running a single test (`cargo metadata --all-features` exits 101). Plain # `cargo test` with explicit features stays the runner. - run: cargo test --workspace + # v2.3.7 — the default-feature run cannot see a `mapper-audio`-off defect. + # `rustynes-mappers` compiles two shapes: with the on-cart synthesizers + # (default) and without, and only the second is what the `no_std` + # cross-build and any downstream dependency-light consumer gets. CI linted + # that shape and never RAN it, so a VRC7 save-state version check that + # rejected every v2 blob on a no-audio build passed every gate — the + # default build takes the other branch and is correct. Two review bots + # caught it; no test could have, because no test ran in that + # configuration. + # + # Linux only, and only this crate: the shape is target-independent, and + # `rustynes-mappers` is the only crate whose behaviour is feature-gated + # this way. Cheap (one extra compile of a mid-sized crate), so it runs on + # every PR rather than only on a full run. + - if: runner.os == 'Linux' + run: cargo test -p rustynes-mappers --no-default-features test-roms: name: test (test-roms feature) diff --git a/CHANGELOG.md b/CHANGELOG.md index 283ed840..fa4ba5ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,101 @@ cycle-accurate core later replaced. ### Fixed +- **VRC7 save states now carry the FM synthesizer, so rewind no longer garbles + the music.** `Vrc7::save_state` wrote the *shadow* OPLL register bytes and + never the live synthesizer — not `opll`, not `opll_clock_counter`, not + `last_opll_sample` — and `load_state` never replayed them either. After a + rewind, a netplay rollback, or a TAS/save-state restore the FM voice therefore + resumed from whatever envelope and phase state it happened to be holding. + Banking, IRQ, mirroring and PRG-RAM had always round-tripped correctly; this + was audio-only, and only on mapper 85. Recorded as an open frontier in + `docs/accuracy-ledger.md` since v2.2.3, closed now. + + `rustynes_apu::Opll` gains a `snapshot` / `restore` pair carrying the register + shadow, the EG and LFO counters, the per-channel patch selection, all 18 + operator slots (phase accumulators, envelope state machines, feedback history) + and the per-channel outputs. The lookup tables and the chip's patch ROM are + deliberately not carried — they are constants of construction, and restoring + them would be restoring a copy of the binary into itself. The chip type rides + along only as a tag, so a YM2413 blob restored into a VRC7 is rejected rather + than silently reinterpreting every slot patch against the wrong instrument set. + + The VRC7 mapper section is now **v2**, appending that blob after the VRAM. + It is additive: a v1 blob still loads and leaves the synthesizer exactly where + the old build left it, so an old save is no worse than it always was rather + than newly silent. A build without `mapper-audio` has no synthesizer to + describe, so it still writes v1 and validates-then-ignores a v2 tail — which + preserves the cross-feature save portability ADR 0004 promises, and is why the + version byte is build-dependent rather than unconditionally 2. + + The repair a reader will think of first was rejected on the merits: replaying + the register shadow through `Opll::write_reg` on load needs no new format, but + restarts every keyed-on channel's envelope at attack, so every rewind frame + would produce an audible transient. + + The regression net keys a note, advances 20,000 CPU cycles, saves, and then + compares 4,000 mixed samples from the source against 4,000 from a **fresh** + mapper restored from the blob — equal sample for sample. It is + mutation-checked: making the tail carry a *reset* synthesizer reproduces the + pre-fix failure exactly. `Opll` is also now registered in + `snapshot_schema_audit.rs`, the standing field-vs-schema audit, which had + never been able to see this surface — a save-state surface no audit can see is + precisely how a gap this size survives for four releases. + + Emulation output is unchanged (nothing on the synthesis path moved), and the + accuracy contract was verified rather than assumed: AccuracyCoin **141/141** + via the authoritative RAM decoder, nestest 0-diff. + + Review caught a defect in the fix itself, worth recording because of *why* no + test could have. The accept check read `version != 1 && version != + VRC7_SECTION_VERSION`, and that constant is **1** on a `mapper-audio`-off + build — so the condition collapsed to "v1 only" there and a no-audio build + **rejected** every v2 blob, the exact opposite of the portability the constant's + own doc comment claimed. What a build can *write* and what it must *accept* are + different sets, and only the first varies by feature; deriving one from the + other reads as tidy and silently couples them. The check now compares against + literals. + + The default build takes the other branch and was correct throughout, which is + why every gate stayed green: CI **linted** the `--no-default-features` shape + and never **ran** it. `cargo test -p rustynes-mappers --no-default-features` is + now a CI step, and the new regression test is mutation-checked in both + configurations — red on no-audio with the old condition, green on the default + build either way. + + A second review pass found two more, both in the new code and both of a kind + the tests as written could not see. **A hand-edited save state could crash the + emulator**: `commit_slot_update` indexes the TLL table as + `[block_fnum][tl][kl]` with dimensions `[128][64][4]`, and `restore` was + handing it raw bytes — a `tl` of 255 computes an index of 524,539 into a + 32,768-entry table. Every register field is now masked to its hardware width + at the parse boundary, which is what those fields physically are. The test that + proves it is careful about one thing: an all-`0xFF` blob is rejected by the + envelope-state tag check before any numeric field is read, so the naive hostile + input passes **by accident** and reports the emulator safe. The interesting + input is the one that satisfies every explicit check and is still nonsense. A + later review pass pushed back that the masking did not in fact cover every + field, and was right: replacing the single fixed payload with a deterministic + pseudo-random sweep found **three more panics** the fixed one could not, + including one it actively hid — with every byte `0xFF`, `update_requests` is + also all-ones, so the slot state was recomputed before the restored values + could be used. A blob that is maximally hostile in one dimension can be + harmless in another. The three: `eg_shift` used as a shift amount (`1u32 <<` + panics at 32), the operator feedback pair summed as two arbitrary `i32`s, and + `eg_rate_l` indexing a 4-entry table — the last being a field I had explicitly + traced as safe, using a broken grep whose empty output I read as proof. + + And **`load_state` was not atomic**. The v2 tail introduced a failure that can + occur *after* the core fields are assigned, which the v1 layout could not, so a + truncated tail returned `Err` with the banking, IRQ state and 2 KiB of VRAM + already overwritten — a mapper left in neither its old state nor its new one + while the caller reported failure and kept running. `Opll::restore` was already + atomic internally, which is exactly what made it easy to miss: the guarantee + existed one level down and was silently discarded one level up. It now parses + into a staged value before the first write. The truncation test asserted only + on the return value, which is why review found this and the test did not; it + now asserts the target is byte-identical afterwards. + - **Corrected a stale comment in `security.yml`.** It justified installing `cargo-audit` / `cargo-deny` as prebuilt binaries with "the repo pins rustc 1.96 **but** cargo-audit needs >= 1.88 to compile" — which argues against @@ -106,8 +201,9 @@ cycle-accurate core later replaced. - **CI jobs are bounded, so a hung job can no longer block a release.** No job in `ci.yml` carried a `timeout-minutes`, which means every one inherited GitHub's **six-hour** default. On the night of the v2.3.6 cut the `lint` job — - normally four minutes — hung on `main` (2026-08-17 21:11 UTC). Because `main` runs deliberately do not cancel each - other, the v2.3.6 release commit queued behind it and never started; GitHub + normally four minutes — hung on `main` (2026-08-17 21:11 UTC). Because `main` + runs deliberately do not cancel each other, the v2.3.6 release commit queued + behind it and never started; GitHub keeps only one pending run per concurrency group, so the commit between them was cancelled outright; `Auto Release` fired on *that* cancellation, saw a non-success conclusion, and correctly skipped. diff --git a/crates/rustynes-apu/src/lib.rs b/crates/rustynes-apu/src/lib.rs index d17d7ca5..157be09f 100644 --- a/crates/rustynes-apu/src/lib.rs +++ b/crates/rustynes-apu/src/lib.rs @@ -58,7 +58,10 @@ pub use frame_counter::{FrameCounter, FrameEvents, Mode as FrameCounterMode}; pub use length::{LENGTH_TABLE, LengthCounter}; pub use mixer::{FilterChain, FilterModel, Mixer, OnePole}; pub use noise::{NTSC_NOISE_PERIODS, Noise, PAL_NOISE_PERIODS}; -pub use opll::{ChipType as OpllChipType, Opll, Patch as OpllPatch}; +pub use opll::{ + ChipType as OpllChipType, OPLL_SNAPSHOT_LEN, OPLL_SNAPSHOT_VERSION, Opll, OpllStateError, + Patch as OpllPatch, +}; pub use pulse::Pulse; pub use snapshot::{APU_SNAPSHOT_VERSION, ApuSnapshotError}; pub use triangle::Triangle; diff --git a/crates/rustynes-apu/src/opll.rs b/crates/rustynes-apu/src/opll.rs index 16ceb8ed..47f1ff3e 100644 --- a/crates/rustynes-apu/src/opll.rs +++ b/crates/rustynes-apu/src/opll.rs @@ -1586,6 +1586,482 @@ impl Opll { } } +// --------------------------------------------------------------------------- +// Save-state surface (v2.3.7 — closes the `docs/accuracy-ledger.md` OPLL row) +// --------------------------------------------------------------------------- + +/// Errors returned by [`Opll::restore`]. +/// +/// Deliberately its own type rather than `ApuSnapshotError`: the OPLL blob does +/// not ride in the APU section of a save state. It rides in the **mapper** +/// section of whichever board carries the chip (VRC7 today), which is versioned +/// independently of `APU_SNAPSHOT_VERSION`. Sharing an error type would imply a +/// coupling between two schemas that must be free to move apart. +#[derive(Debug, thiserror::Error)] +#[non_exhaustive] +pub enum OpllStateError { + /// Blob is shorter than the schema declares. + #[error("OPLL snapshot truncated at offset {0}")] + Truncated(usize), + /// The blob's version byte is not understood by this build. + #[error("OPLL snapshot unsupported version {0}")] + UnsupportedVersion(u8), + /// The blob was written by a differently-configured chip (YM2413 state + /// restored into a VRC7 instance, or the reverse). The patch ROM differs + /// between them, so the slot patches would be reinterpreted against the + /// wrong instrument set. + #[error("OPLL snapshot chip type {got} does not match this instance ({want})")] + ChipTypeMismatch { + /// The tag read from the blob. + got: u8, + /// The tag this instance was constructed with. + want: u8, + }, + /// An envelope-generator state tag outside the six defined variants. + #[error("OPLL snapshot has invalid envelope-generator state tag {0}")] + InvalidEgState(u8), +} + +/// Schema version of the blob [`Opll::snapshot`] emits. +pub const OPLL_SNAPSHOT_VERSION: u8 = 1; + +/// Number of slots (operators) carried. 18 in the YM2413; the VRC7 uses the +/// first 12, but all 18 are serialized so the same blob describes either chip. +const SNAPSHOT_SLOTS: usize = 18; + +/// Serialized size of one [`Slot`], in bytes. Asserted against the writer by +/// `slot_serialized_size_matches_the_declared_constant` so the two cannot drift. +const SLOT_BYTES: usize = 62; + +/// Largest `eg_shift` the envelope generator can legitimately produce. +/// +/// `commit_slot_update` assigns `13 - eg_rate_h` with `eg_rate_h <= 13`, so the +/// value is always in `0..=13`. It matters on restore because `calc_envelope` +/// evaluates `1u32 << eg_shift`, which panics at 32 and above. +const EG_SHIFT_MAX: u32 = 13; + +/// Highest instrument number a channel can select: the `$3x` high nibble is four +/// bits, and index 0 is the user patch. +const MAX_PATCH_NUMBER: i32 = 15; + +/// Clamp a restored operator output into the range synthesis can actually +/// produce. See the call site for why the field is wider than its contents. +const fn clamp_i16(v: i32) -> i32 { + if v < i16::MIN as i32 { + i16::MIN as i32 + } else if v > i16::MAX as i32 { + i16::MAX as i32 + } else { + v + } +} + +/// Serialized size of one [`Patch`], in bytes (13 one-byte parameters). +const PATCH_BYTES: usize = 13; + +/// Total serialized size of an OPLL snapshot, in bytes. +/// +/// version(1) + chip_type(1) + adr(1) + reg(64) + test_flag(1) +/// + slot_key_status(4) + eg_counter(4) + pm_phase(4) + am_phase(4) +/// + lfo_am(1) + patch_number(9x4) + user patch pair(2x13) +/// + slot(18x62) + ch_out(14x2) + mix_out(2) +pub const OPLL_SNAPSHOT_LEN: usize = 1 + + 1 + + 1 + + 0x40 + + 1 + + 4 + + 4 + + 4 + + 4 + + 1 + + 9 * 4 + + 2 * PATCH_BYTES + + SNAPSHOT_SLOTS * SLOT_BYTES + + 14 * 2 + + 2; + +impl EgState { + /// Stable on-disk tag. Explicit rather than `as u8` so reordering the enum + /// cannot silently reinterpret existing save states. + const fn to_tag(self) -> u8 { + match self { + Self::Attack => 0, + Self::Decay => 1, + Self::Sustain => 2, + Self::Release => 3, + Self::Damp => 4, + Self::Unknown => 5, + } + } + + const fn from_tag(tag: u8) -> Result { + match tag { + 0 => Ok(Self::Attack), + 1 => Ok(Self::Decay), + 2 => Ok(Self::Sustain), + 3 => Ok(Self::Release), + 4 => Ok(Self::Damp), + 5 => Ok(Self::Unknown), + other => Err(OpllStateError::InvalidEgState(other)), + } + } +} + +impl ChipType { + /// Stable on-disk tag, for the same reason as [`EgState::to_tag`]. + const fn to_tag(self) -> u8 { + match self { + Self::Ym2413 => 0, + Self::Vrc7 => 1, + Self::Ymf281b => 2, + } + } +} + +/// Append-only byte writer. Local to this module because the OPLL schema is +/// versioned with the mapper section, not with the APU section (see +/// [`OpllStateError`]). +struct OpllW(Vec); + +impl OpllW { + fn u8(&mut self, v: u8) { + self.0.push(v); + } + fn u16(&mut self, v: u16) { + self.0.extend_from_slice(&v.to_le_bytes()); + } + fn u32(&mut self, v: u32) { + self.0.extend_from_slice(&v.to_le_bytes()); + } + fn i16(&mut self, v: i16) { + self.0.extend_from_slice(&v.to_le_bytes()); + } + fn i32(&mut self, v: i32) { + self.0.extend_from_slice(&v.to_le_bytes()); + } + fn patch(&mut self, p: &Patch) { + // Field order is load-bearing: `OpllR::patch` reads it back verbatim. + for b in [ + p.tl, p.fb, p.eg, p.ml, p.ar, p.dr, p.sl, p.rr, p.kr, p.kl, p.am, p.pm, p.ws, + ] { + self.0.push(b); + } + } +} + +/// Bounds-checked byte reader. Every read is length-checked before it happens, +/// so a truncated or hand-edited blob returns [`OpllStateError::Truncated`] +/// rather than panicking — this parses untrusted save-state bytes. +struct OpllR<'a> { + src: &'a [u8], + pos: usize, +} + +impl OpllR<'_> { + fn need(&self, n: usize) -> Result<(), OpllStateError> { + if self.src.len() - self.pos < n { + return Err(OpllStateError::Truncated(self.pos)); + } + Ok(()) + } + fn u8(&mut self) -> Result { + self.need(1)?; + let v = self.src[self.pos]; + self.pos += 1; + Ok(v) + } + fn u16(&mut self) -> Result { + self.need(2)?; + let v = u16::from_le_bytes([self.src[self.pos], self.src[self.pos + 1]]); + self.pos += 2; + Ok(v) + } + fn u32(&mut self) -> Result { + self.need(4)?; + let mut b = [0u8; 4]; + b.copy_from_slice(&self.src[self.pos..self.pos + 4]); + self.pos += 4; + Ok(u32::from_le_bytes(b)) + } + fn i16(&mut self) -> Result { + Ok(self.u16()? as i16) + } + fn i32(&mut self) -> Result { + Ok(self.u32()? as i32) + } + fn patch(&mut self) -> Result { + self.need(PATCH_BYTES)?; + // Destructured positionally rather than field-by-field so the order + // here is visibly the same list `OpllW::patch` writes; a reordering on + // one side is then a visible diff on the other, not a silent + // reinterpretation of thirteen interchangeable `u8`s. + let [tl, fb, eg, ml, ar, dr, sl, rr, kr, kl, am, pm, ws]: [u8; PATCH_BYTES] = self.src + [self.pos..self.pos + PATCH_BYTES] + .try_into() + .expect("slice length checked by `need` above"); + self.pos += PATCH_BYTES; + // MASK to each parameter's hardware width. These are register fields of + // fixed bit width, so a wider value does not describe a chip state that + // exists -- masking IS the parse, not a repair after it. + // + // Scope, precisely, because an earlier version of this comment claimed + // "every register field is masked" and that was an OVERCLAIM (review + // caught it): what is masked is everything that reaches a SUBSCRIPT -- + // the 13 patch parameters here, and `blk_fnum` / `fnum` / `blk` / + // `number` / `wave_table_idx` / `pg_keep` in the slot reader. The + // remaining restored fields are deliberately left alone because none of + // them can index anything: `eg_rate_h`/`eg_rate_l` are consumed by a + // `match` and recomputed as `(p_rate + rks_h2).min(15)`; `lfo_am` is + // overwritten every `update_ampm` from `AM_TABLE[idx % len]`; `rks`, + // `tll`, `type_flags`, `key_flag`, `sus_flag` and `test_flag` are only + // ever compared, shifted or added; and `lookup_exp_table` masks its own + // index to 8 bits. `opll_restore_survives_a_hostile_blob` sweeps + // pseudo-random payloads to keep that true rather than assumed. + // + // Load-bearing, not defensive tidiness: `commit_slot_update` indexes the + // TLL table as `[block_fnum][tl][kl]`, dimensions `[128][64][4]`. An + // unmasked `tl` of 255 computes an index of ~524k into a 32,768-entry + // table and PANICS. A save state is a file on disk -- untrusted input -- + // so a hand-edited one must not be able to crash the emulator. Pinned by + // `opll_restore_survives_a_hostile_blob`, which panicked before this. + Ok(Patch { + tl: tl & 0x3F, + fb: fb & 0x07, + eg: eg & 0x01, + ml: ml & 0x0F, + ar: ar & 0x0F, + dr: dr & 0x0F, + sl: sl & 0x0F, + rr: rr & 0x0F, + kr: kr & 0x01, + kl: kl & 0x03, + am: am & 0x01, + pm: pm & 0x01, + ws: ws & 0x01, + }) + } +} + +impl Opll { + /// Serialize the complete live synthesizer state. + /// + /// # Why this exists + /// + /// Until v2.3.7 the VRC7 mapper's save state carried only the *shadow* + /// register bytes and replayed nothing into the synthesizer, so after a + /// rewind, a netplay rollback, or a TAS restore the FM voice resumed from + /// whatever envelope and phase state it happened to hold — audible, and a + /// determinism gap in a project whose central claim is determinism. The + /// obvious format-free repair (replaying `regs` through + /// [`Opll::write_reg`]) is worse than the disease: it restarts every + /// keyed-on channel's envelope at attack, so every rewind frame produces a + /// transient. Carrying the state verbatim is the only repair that restores + /// the sound that was actually playing. + /// + /// # What is and is not carried + /// + /// Everything mutated during synthesis: the register shadow, the EG/LFO + /// counters, the per-channel patch selection, all 18 operator slots + /// (phase accumulators, envelope state machines, feedback history), the + /// per-channel outputs and the mix. The user patch pair (`patch_set[0..2]`, + /// writeable through registers `$00-$07`) is carried explicitly rather than + /// re-derived, so a restore cannot depend on `refresh_user_patch_pointers` + /// running in the right order. + /// + /// Deliberately NOT carried, because they are constants of construction and + /// restoring them would be restoring a copy of the binary into itself: + /// `waves` and `tll_rks` (pure lookup tables built in [`Opll::new`]) and + /// `patch_set[2..]` (the chip's patch ROM, fixed by `chip_type`). The chip + /// type itself IS carried, as a tag, purely so a mismatched restore is + /// rejected instead of silently reinterpreting slot patches against the + /// wrong instrument set. + /// + /// The blob is exactly [`OPLL_SNAPSHOT_LEN`] bytes and self-describes its + /// version in byte 0. + #[must_use] + pub fn snapshot(&self) -> Vec { + let mut w = OpllW(Vec::with_capacity(OPLL_SNAPSHOT_LEN)); + w.u8(OPLL_SNAPSHOT_VERSION); + w.u8(self.chip_type.to_tag()); + w.u8(self.adr); + w.0.extend_from_slice(&self.reg); + w.u8(self.test_flag); + w.u32(self.slot_key_status); + w.u32(self.eg_counter); + w.u32(self.pm_phase); + w.i32(self.am_phase); + w.u8(self.lfo_am); + for n in self.patch_number { + w.i32(n); + } + // The user patch (modulator + carrier), written through $00-$07. + for p in self.patch_set.iter().take(2) { + w.patch(p); + } + for s in &self.slot { + w.u8(s.number); + w.u8(s.type_flags); + w.patch(&s.patch); + w.i32(s.output[0]); + w.i32(s.output[1]); + w.u8(s.wave_table_idx); + w.u32(s.pg_phase); + w.u32(s.pg_out); + w.u8(s.pg_keep); + w.u16(s.blk_fnum); + w.u16(s.fnum); + w.u8(s.blk); + w.u8(s.eg_state.to_tag()); + w.i32(s.volume); + w.u8(s.key_flag); + w.u8(s.sus_flag); + w.u16(s.tll); + w.u8(s.rks); + w.u8(s.eg_rate_h); + w.u8(s.eg_rate_l); + w.u32(s.eg_shift); + w.u32(s.eg_out); + w.u32(s.update_requests); + } + for v in self.ch_out { + w.i16(v); + } + w.i16(self.mix_out); + debug_assert_eq!(w.0.len(), OPLL_SNAPSHOT_LEN, "OPLL snapshot length drift"); + w.0 + } + + /// Restore state previously produced by [`Opll::snapshot`]. + /// + /// Trailing bytes past the schema are ignored, so a future version may + /// append without breaking this reader — the same additive discipline the + /// PPU and APU sections use. + /// + /// # Errors + /// + /// [`OpllStateError::Truncated`] if the blob is shorter than the schema, + /// [`OpllStateError::UnsupportedVersion`] if byte 0 is not + /// [`OPLL_SNAPSHOT_VERSION`], [`OpllStateError::ChipTypeMismatch`] if the + /// blob describes a different chip, and + /// [`OpllStateError::InvalidEgState`] on a corrupt envelope-state tag. + pub fn restore(&mut self, data: &[u8]) -> Result<(), OpllStateError> { + let mut r = OpllR { src: data, pos: 0 }; + let version = r.u8()?; + if version != OPLL_SNAPSHOT_VERSION { + return Err(OpllStateError::UnsupportedVersion(version)); + } + let chip_tag = r.u8()?; + if chip_tag != self.chip_type.to_tag() { + return Err(OpllStateError::ChipTypeMismatch { + got: chip_tag, + want: self.chip_type.to_tag(), + }); + } + // Read the whole blob into locals BEFORE mutating `self`: a truncated + // or corrupt tail must leave the synthesizer on its previous state + // rather than half-overwritten, since the caller (a mapper's + // `load_state`) reports the error and keeps running. + let adr = r.u8()?; + r.need(0x40)?; + let mut reg = [0u8; 0x40]; + reg.copy_from_slice(&r.src[r.pos..r.pos + 0x40]); + r.pos += 0x40; + let test_flag = r.u8()? & 0x01; + let slot_key_status = r.u32()?; + let eg_counter = r.u32()?; + let pm_phase = r.u32()?; + let am_phase = r.i32()?; + let lfo_am = r.u8()?; + let mut patch_number = [0i32; 9]; + for n in &mut patch_number { + // CLAMPED to the instrument range even though it is not currently a + // subscript -- it is only ever compared to zero, and `set_patch` + // bounds-checks its own argument before touching `patch_set`. + // + // Clamped anyway, for consistency with every other field here: the + // legal domain is 0..=15 (a 4-bit `$3x` high nibble), so a wider + // value describes a chip state that cannot exist, and letting one + // through would leave the ONE field whose safety rests on "nothing + // indexes it today" rather than on its own width. Reviewers flagged + // it three times; that is a fair signal that the invariant was too + // subtle to be load-bearing. + *n = r.i32()?.clamp(0, MAX_PATCH_NUMBER); + } + let user_patch = [r.patch()?, r.patch()?]; + let mut slots = [Slot::default(); SNAPSHOT_SLOTS]; + for s in &mut slots { + // Masked for the same reason as the patch fields: `blk_fnum` feeds + // the TLL/RKS row index (`>> 5` into 128 rows, `>> 8` into 16), so an + // unmasked u16 indexes far past both tables. A legal `blk_fnum` is + // `(blk3 << 9) | fnum9`, i.e. at most 0x0FFF. + s.number = r.u8()? % SNAPSHOT_SLOTS as u8; + s.type_flags = r.u8()? & 0x03; + s.patch = r.patch()?; + // CLAMPED to i16: `calc_slot_mod` / `calc_slot_car` only ever store + // `i32::from(out)` with `out: i16`, and the feedback path evaluates + // `output[0] + output[1]`, which overflows on two arbitrary i32s. + // The field is `i32` for headroom in that sum, not because the + // values are ever wider than i16. + s.output = [clamp_i16(r.i32()?), clamp_i16(r.i32()?)]; + s.wave_table_idx = r.u8()? & 0x01; + s.pg_phase = r.u32()?; + s.pg_out = r.u32()?; + s.pg_keep = r.u8()? & 0x01; + s.blk_fnum = r.u16()? & 0x0FFF; + s.fnum = r.u16()? & 0x01FF; + s.blk = r.u8()? & 0x07; + s.eg_state = EgState::from_tag(r.u8()?)?; + s.volume = r.i32()?; + s.key_flag = r.u8()? & 0x01; + s.sus_flag = r.u8()? & 0x01; + s.tll = r.u16()?; + s.rks = r.u8()? & 0x0F; + s.eg_rate_h = r.u8()? & 0x0F; + // `eg_rate_l` INDEXES `EG_STEP_TABLES`, whose outer dimension is + // 4. Legal values are `rks & 3`. This one is why the whole sweep + // exists: I had claimed, after tracing by hand, that none of the + // flag fields reach a subscript -- and this one does. The trace + // was run with a broken grep whose empty output I read as proof. + s.eg_rate_l = r.u8()? & 0x03; + // CLAMPED, and this one is a shift amount rather than a + // subscript -- `calc_envelope` computes `1u32 << eg_shift`, which + // PANICS for any value >= 32. `commit_slot_update` only ever + // produces `13 - eg_rate_h` with `eg_rate_h <= 13`, so 13 is the + // real ceiling. Found by the randomized half of + // `opll_restore_survives_a_hostile_blob`, NOT by its fixed + // all-`0xFF` payload: with every byte 0xFF, `update_requests` is + // also all-ones, so `commit_slot_update` recomputed `eg_shift` + // before `calc_envelope` could use the restored one. The fixed blob + // was too hostile in one dimension to expose a bug in another. + s.eg_shift = r.u32()?.min(EG_SHIFT_MAX); + s.eg_out = r.u32()?; + s.update_requests = r.u32()?; + } + let mut ch_out = [0i16; 14]; + for v in &mut ch_out { + *v = r.i16()?; + } + let mix_out = r.i16()?; + + self.adr = adr; + self.reg = reg; + self.test_flag = test_flag; + self.slot_key_status = slot_key_status; + self.eg_counter = eg_counter; + self.pm_phase = pm_phase; + self.am_phase = am_phase; + self.lfo_am = lfo_am; + self.patch_number = patch_number; + self.patch_set[0] = user_patch[0]; + self.patch_set[1] = user_patch[1]; + self.slot = slots; + self.ch_out = ch_out; + self.mix_out = mix_out; + Ok(()) + } +} + // --------------------------------------------------------------------------- // Tests — verify the static tables match the C source byte-for-byte. // These tests run unconditionally (no feature gate) since the OPLL @@ -2222,4 +2698,225 @@ mod tests { assert_eq!(s.type_flags & 1, (i & 1) as u8, "slot {i} M/C bit"); } } + + // ----------------------------------------------------------------------- + // Save-state surface (v2.3.7) + // ----------------------------------------------------------------------- + + /// Key a note and run it well past the attack phase, so the snapshot under + /// test describes a genuinely mid-flight synthesizer rather than something + /// a `reset()` could coincidentally reproduce. + fn opll_mid_note() -> Opll { + let mut opll = Opll::new(ChipType::Vrc7); + opll.write_reg(0x30, 0x10); // channel 0: instrument 1, full volume + opll.write_reg(0x10, 0xAD); // F-number low + opll.write_reg(0x20, 0x15); // key on, block 2, F-number bit 8 + for _ in 0..600 { + let _ = opll.calc(); + } + opll + } + + #[test] + fn opll_snapshot_length_matches_the_declared_constant() { + // Pins SLOT_BYTES / PATCH_BYTES against the writer. A field added to + // `Slot` without extending the arithmetic fails here rather than + // silently shifting every subsequent field on restore. + assert_eq!( + Opll::new(ChipType::Vrc7).snapshot().len(), + OPLL_SNAPSHOT_LEN + ); + } + + #[test] + fn opll_snapshot_restore_reproduces_the_sample_stream_exactly() { + let mut source = opll_mid_note(); + let blob = source.snapshot(); + let expected: Vec = (0..2000).map(|_| source.calc()).collect(); + assert!( + expected.iter().any(|&s| s != 0), + "fixture is silent — the comparison would pass vacuously" + ); + + // Restore into a FRESH chip, not the one that produced the blob: this + // has to work from power-on state, which is the actual rewind case. + let mut restored = Opll::new(ChipType::Vrc7); + restored.restore(&blob).expect("round-trip must load"); + let got: Vec = (0..2000).map(|_| restored.calc()).collect(); + + assert_eq!( + got, expected, + "restored OPLL diverged from the source stream" + ); + } + + #[test] + fn opll_snapshot_is_stable_across_a_restore_cycle() { + // Byte-level idempotence: snapshot -> restore -> snapshot must be the + // same bytes. Catches a field that is written but not read back (which + // the stream test above can miss if the field happens not to affect + // the next 2000 samples). + let source = opll_mid_note(); + let first = source.snapshot(); + let mut restored = Opll::new(ChipType::Vrc7); + restored.restore(&first).unwrap(); + assert_eq!(restored.snapshot(), first); + } + + #[test] + fn opll_restore_rejects_a_blob_from_a_different_chip() { + // The patch ROM differs per chip type, so slot patches restored across + // types would be reinterpreted against the wrong instrument set — + // silently, since every field is otherwise structurally valid. + let blob = Opll::new(ChipType::Ym2413).snapshot(); + let mut vrc7 = Opll::new(ChipType::Vrc7); + let err = vrc7 + .restore(&blob) + .expect_err("chip mismatch must be rejected"); + assert!( + matches!(err, OpllStateError::ChipTypeMismatch { got: 0, want: 1 }), + "expected ChipTypeMismatch, got {err:?}" + ); + } + + #[test] + fn opll_restore_rejects_a_truncated_blob_without_mutating_state() { + let source = opll_mid_note(); + let blob = source.snapshot(); + let mut target = Opll::new(ChipType::Vrc7); + let before = target.snapshot(); + + let err = target + .restore(&blob[..blob.len() - 1]) + .expect_err("a truncated blob must be rejected"); + assert!( + matches!(err, OpllStateError::Truncated(_)), + "expected Truncated, got {err:?}" + ); + assert_eq!( + target.snapshot(), + before, + "a rejected restore left the synthesizer half-overwritten" + ); + } + + /// A save state is a file on disk. A hand-edited one must not be able to + /// crash the emulator. + /// + /// This FAILED before the parse-boundary masks: with every payload byte set + /// to `0xFF` and only the envelope-state tags made valid, `commit_slot_update` + /// computed a TLL index of 524,539 into a 32,768-entry table and panicked. + /// + /// Making the tags valid is the point of the test rather than an + /// inconvenience. An all-`0xFF` blob is rejected by `EgState::from_tag` + /// before any numeric field is touched, so the naive hostile input passes + /// *by accident* and reports the emulator safe. The interesting input is the + /// one that satisfies every explicit check and is still nonsense. + #[test] + fn opll_restore_survives_a_hostile_blob() { + let mut blob = Opll::new(ChipType::Vrc7).snapshot(); + for b in blob.iter_mut().skip(2) { + *b = 0xFF; + } + blob[0] = OPLL_SNAPSHOT_VERSION; + blob[1] = ChipType::Vrc7.to_tag(); + for i in 0..SNAPSHOT_SLOTS { + blob[EG_STATE_OFFSET + i * SLOT_BYTES] = EgState::Release.to_tag(); + } + + let mut opll = Opll::new(ChipType::Vrc7); + opll.restore(&blob) + .expect("a structurally valid blob must load"); + // Run synthesis: the panic was not in `restore`, it was in the first + // slot update the restored state provoked. + for _ in 0..4_000 { + let _ = opll.calc(); + } + + // One fixed payload proves one path. Sweep pseudo-random ones too, so + // the claim is "no hostile blob reaches a subscript" rather than "this + // particular blob did not" -- which is the difference review asked + // about. A tiny xorshift keeps it deterministic and dependency-free; a + // flaky fuzz test would be worse than none. + let mut state = 0x2545_F491_4F6C_DD1Du64; + let mut next = move || { + state ^= state << 13; + state ^= state >> 7; + state ^= state << 17; + state + }; + for round in 0..64 { + let mut b = Opll::new(ChipType::Vrc7).snapshot(); + for byte in b.iter_mut().skip(2) { + *byte = (next() & 0xFF) as u8; + } + b[0] = OPLL_SNAPSHOT_VERSION; + b[1] = ChipType::Vrc7.to_tag(); + for i in 0..SNAPSHOT_SLOTS { + // Keep the tag valid: an invalid one short-circuits the parse, + // and the round would then prove nothing. + b[EG_STATE_OFFSET + i * SLOT_BYTES] = (next() % 6) as u8; + } + let mut o = Opll::new(ChipType::Vrc7); + o.restore(&b) + .unwrap_or_else(|e| panic!("round {round}: valid-shaped blob rejected: {e}")); + for _ in 0..1_000 { + let _ = o.calc(); + } + + // Then drive the REGISTER PORT on the restored chip. `calc()` alone + // never exercises `write_reg`, so a restored field that is only + // consumed on a subsequent port write -- `adr` is the candidate + // review raised -- would sail past a synthesis-only sweep. Covering + // both is cheaper than arguing about which fields reach a subscript, + // and this session has shown my hand-tracing to be the less reliable + // instrument. + for _ in 0..64 { + o.write_reg((next() & 0xFF) as u8, (next() & 0xFF) as u8); + let _ = o.calc(); + let _ = o.read_reg((next() & 0xFF) as u8); + } + } + + // And a hostile blob must not be able to smuggle out-of-range register + // fields past the parse, which is what the masks are for. + for (i, s) in opll.slot.iter().enumerate() { + assert!(s.patch.tl <= 0x3F, "slot {i} tl out of range"); + assert!(s.patch.kl <= 0x03, "slot {i} kl out of range"); + assert!(s.blk_fnum <= 0x0FFF, "slot {i} blk_fnum out of range"); + assert!(usize::from(s.number) < SNAPSHOT_SLOTS, "slot {i} number"); + } + } + + #[test] + fn opll_restore_rejects_an_unknown_version() { + let mut blob = Opll::new(ChipType::Vrc7).snapshot(); + blob[0] = 99; + let mut target = Opll::new(ChipType::Vrc7); + assert!(matches!( + target.restore(&blob), + Err(OpllStateError::UnsupportedVersion(99)) + )); + } + + /// Byte offset of slot 0's `eg_state` tag within a snapshot blob. + /// + /// Header: `version(1) + chip_type(1) + adr(1) + reg(64) + test_flag(1) + + /// slot_key_status/eg_counter/pm_phase/am_phase(16) + lfo_am(1) + + /// patch_number(36) + user patch pair(26) = 147`. Then within slot 0: + /// `number(1) + type_flags(1) + patch(13) + output(8) + wave_table_idx(1) + + /// pg_phase(4) + pg_out(4) + pg_keep(1) + blk_fnum(2) + fnum(2) + blk(1) = 38`. + const EG_STATE_OFFSET: usize = 147 + 38; + + #[test] + fn opll_restore_rejects_an_invalid_envelope_state_tag() { + let mut blob = opll_mid_note().snapshot(); + assert!(blob[EG_STATE_OFFSET] <= 5, "offset does not point at a tag"); + blob[EG_STATE_OFFSET] = 6; + let mut target = Opll::new(ChipType::Vrc7); + assert!(matches!( + target.restore(&blob), + Err(OpllStateError::InvalidEgState(6)) + )); + } } diff --git a/crates/rustynes-mappers/src/m085_vrc7.rs b/crates/rustynes-mappers/src/m085_vrc7.rs index 2582c03a..ea5c97c4 100644 --- a/crates/rustynes-mappers/src/m085_vrc7.rs +++ b/crates/rustynes-mappers/src/m085_vrc7.rs @@ -44,6 +44,36 @@ const CHR_BANK_8K: usize = 0x2000; const NAMETABLE_SIZE: usize = 0x0400; const NAMETABLE_SIZE_U16: u16 = 0x0400; +/// Version byte this board writes in its mapper save-state section. +/// +/// **v1** (through v2.3.6) carried banking, IRQ, mirroring, PRG-RAM and the +/// *shadow* OPLL register bytes. **v2** (v2.3.7) appends the live synthesizer, +/// closing the save-state audio-continuity gap recorded in +/// `docs/accuracy-ledger.md`. `load_state` accepts both. +/// +/// A build without `mapper-audio` has no synthesizer to describe, so it writes +/// **v1** and, on load, validates a v2 tail's length and ignores its contents. +/// That keeps the cross-build property this crate's feature documentation +/// promises — an audio build's save still loads in a no-audio build, and a +/// no-audio build's save still loads everywhere. +/// +/// **This constant is the WRITE version only. Never key the accept set on it.** +/// `load_state` compares against the literals 1 and 2 for that reason: it once +/// compared against this constant, which is 1 here, so the condition collapsed +/// to "v1 only" and a no-audio build rejected v2 outright — the precise opposite +/// of the sentence above, which is what the code claimed to do while doing the +/// reverse. What a build can write and what it must accept are different sets, +/// and only the first varies by feature. Pinned by +/// `vrc7_load_state_accepts_a_v2_blob_on_every_build`. +#[cfg(feature = "mapper-audio")] +const VRC7_SECTION_VERSION: u8 = 2; +#[cfg(not(feature = "mapper-audio"))] +const VRC7_SECTION_VERSION: u8 = 1; + +/// Bytes the v2 tail adds after the VRAM: `opll_clock_counter` (2), +/// `last_opll_sample` (2), and the self-versioned OPLL blob. +const VRC7_V2_TAIL_LEN: usize = 2 + 2 + rustynes_apu::OPLL_SNAPSHOT_LEN; + fn nametable_offset(addr: u16, mirroring: Mirroring) -> usize { let table = (((addr - 0x2000) / NAMETABLE_SIZE_U16) & 0x03) as u8; let local = (addr as usize) & (NAMETABLE_SIZE - 1); @@ -545,10 +575,14 @@ impl Mapper for Vrc7 { // audio.regs[0..64] (64) // vram (2 KiB) // - // Per ADR-0003: the future v1.x commit that lands the OPLL state - // bumps version 1 → 2, appending the synthesizer's internal - // state (operator phases, envelope phases, key-on flags) at the - // tail. v1 blobs default-load the synthesizer to silent. + // v2 (v2.3.7) is that commit: it appends, after the VRAM, + // opll_clock_counter(2 le) + last_opll_sample(2 le) + // + opll blob (OPLL_SNAPSHOT_LEN bytes, self-versioned) + // closing the `docs/accuracy-ledger.md` row that recorded the FM + // voice resuming from arbitrary envelope + phase state after a + // rewind / rollback / TAS restore. `load_state` still accepts a v1 + // blob, which leaves the synthesizer wherever it was — the exact + // pre-v2.3.7 behaviour, so an old save is no worse than it was. // version(1) + prg(3) + chr(8) + mirroring(1) + prg_ram_enable(1) // + irq_latch(1) + irq_counter(1) + irq_enabled(1) // + irq_enable_after_ack(1) + irq_mode_scanline(1) @@ -556,8 +590,14 @@ impl Mapper for Vrc7 { // + audio addr_latch(1) + data_latch(1) + silenced(1) + regs(64) // = 1 + 3 + 8 + 1 + 1 + 5 + 5 + 67 = 91 let scalar_len = 1 + 3 + 8 + 1 + 1 + 10 + 3 + 64; + // The v2 tail only exists on a `mapper-audio` build, so only reserve for + // it there — a no-audio build would otherwise over-allocate ~1.3 KiB on + // every save for a tail it never writes. + #[cfg(feature = "mapper-audio")] + let mut out = Vec::with_capacity(scalar_len + self.vram.len() + VRC7_V2_TAIL_LEN); + #[cfg(not(feature = "mapper-audio"))] let mut out = Vec::with_capacity(scalar_len + self.vram.len()); - out.push(1u8); // version + out.push(VRC7_SECTION_VERSION); // version out.push(self.prg_0); out.push(self.prg_1); out.push(self.prg_2); @@ -576,6 +616,13 @@ impl Mapper for Vrc7 { out.push(u8::from(self.audio.silenced)); out.extend_from_slice(&self.audio.regs); out.extend_from_slice(&self.vram); + // --- v2 tail: the live synthesizer --- + #[cfg(feature = "mapper-audio")] + { + out.extend_from_slice(&self.opll_clock_counter.to_le_bytes()); + out.extend_from_slice(&self.last_opll_sample.to_le_bytes()); + out.extend_from_slice(&self.opll.snapshot()); + } out } @@ -595,9 +642,73 @@ impl Mapper for Vrc7 { }); } let version = data[0]; - if version != 1 { + // Both READABLE versions, spelled as literals — deliberately NOT + // `VRC7_SECTION_VERSION`, which is what this WRITES and is 1 on a + // no-audio build. Keying the accept set on the write version made the + // condition collapse to `version != 1` there, so a no-audio build + // REJECTED a v2 blob outright — the exact opposite of the + // validate-then-ignore portability ADR 0004 asks for, and of what the + // comment on `VRC7_SECTION_VERSION` claimed. What a build can write and + // what it must accept are different sets; only the first varies by + // feature. Caught in review by two independent bots. + if version != 1 && version != 2 { return Err(MapperError::UnsupportedVersion(version)); } + + // VALIDATE EVERYTHING BEFORE MUTATING ANYTHING. + // + // The v2 tail introduced a failure that can occur AFTER the core fields + // have been assigned, which the v1 layout could not: v1 validated its + // whole length and version up front, so once it started writing it could + // not fail. A truncated or corrupt v2 tail used to return `Err` with + // `prg_0`, `chr`, the IRQ state and 2 KiB of VRAM already overwritten -- + // a mapper left in a state that is neither the old one nor the new one, + // while the caller reports the load as failed and keeps running. + // + // `Opll::restore` was already atomic internally, which is exactly what + // made this easy to miss: the guarantee existed one level down and was + // silently discarded one level up. Parse into a temporary here, so this + // function has the same all-or-nothing property its own comments claim. + // Caught in review; the truncation test missed it because it asserted on + // the return value and never on the target. + #[cfg(feature = "mapper-audio")] + let staged_opll = if version >= 2 { + let tail = &data[core_expected..]; + if tail.len() < VRC7_V2_TAIL_LEN { + return Err(MapperError::Truncated { + expected: core_expected + VRC7_V2_TAIL_LEN, + got: data.len(), + }); + } + let mut opll = self.opll.clone(); + opll.restore(&tail[4..]) + .map_err(|e| MapperError::Invalid(format!("VRC7 OPLL state: {e}")))?; + Some(( + u16::from_le_bytes(tail[0..2].try_into().expect("length checked above")), + i16::from_le_bytes(tail[2..4].try_into().expect("length checked above")), + opll, + )) + } else { + None + }; + // A no-audio build has no synthesizer to stage into, but must still + // reject a truncated tail identically -- the same blob has to be + // accepted or refused the same way on every build. + #[cfg(not(feature = "mapper-audio"))] + // Written as an addition rather than `data.len() - core_expected < ..`: + // the subtraction cannot underflow TODAY (the length guard at the top of + // this function already proved `data.len() >= core_expected`), but it is + // one moved guard away from being able to, and an underflow here would + // wrap to a huge value and silently ACCEPT a truncated blob rather than + // panicking. Not worth leaving a correctness proof spread across two + // distant statements to save an addition. + if version >= 2 && data.len() < core_expected + VRC7_V2_TAIL_LEN { + return Err(MapperError::Truncated { + expected: core_expected + VRC7_V2_TAIL_LEN, + got: data.len(), + }); + } + self.prg_0 = data[1]; self.prg_1 = data[2]; self.prg_2 = data[3]; @@ -628,6 +739,21 @@ impl Mapper for Vrc7 { self.audio.silenced = data[26] != 0; self.audio.regs.copy_from_slice(&data[27..91]); self.vram.copy_from_slice(&data[91..91 + self.vram.len()]); + + // --- v2 tail: the live synthesizer --- + // + // A v1 blob stops here. That is deliberately NOT an error and NOT a + // reset: it is the pre-v2.3.7 behaviour, in which the synthesizer kept + // running from whatever state it held. An old save is therefore exactly + // as (in)accurate as it always was, rather than newly silent. + // Commit the already-validated synthesizer. Infallible by construction: + // every way this could fail was exercised above, before the first write. + #[cfg(feature = "mapper-audio")] + if let Some((counter, sample, opll)) = staged_opll { + self.opll_clock_counter = counter; + self.last_opll_sample = sample; + self.opll = opll; + } Ok(()) } } @@ -940,7 +1066,10 @@ mod tests { m.cpu_write(0x9010, 0x30); m.cpu_write(0x9030, 0x5F); let blob = m.save_state(); - assert_eq!(blob[0], 1u8, "VRC7 save-state version tag"); + // v1 through v2.3.6; v2 since v2.3.7, which appends the live OPLL. + // A build without `mapper-audio` has no synthesizer to describe and + // still writes v1 — see `VRC7_SECTION_VERSION`. + assert_eq!(blob[0], VRC7_SECTION_VERSION, "VRC7 save-state version tag"); let mut target = vrc7_default(); target.load_state(&blob).unwrap(); @@ -997,4 +1126,174 @@ mod tests { "feature-off path must remain silent (matches feature-on for VRC7 v0.9.x)" ); } + + // ----------------------------------------------------------------------- + // Save-state audio continuity (v2.3.7 — closes the accuracy-ledger row) + // ----------------------------------------------------------------------- + + /// Key a note on channel 0 with a real melodic patch, so the OPLL has + /// non-trivial envelope + phase state to carry. + #[cfg(feature = "mapper-audio")] + fn key_on_channel_0(m: &mut Vrc7) { + // $3x: high nibble = instrument (1 = the first Konami melodic patch), + // low nibble = attenuation (0 = loudest). + m.cpu_write(0x9010, 0x30); + m.cpu_write(0x9030, 0x10); + // $1x: F-number low 8 bits. + m.cpu_write(0x9010, 0x10); + m.cpu_write(0x9030, 0xAD); + // $2x: bit5 sustain, bit4 key-on, bits3-1 block, bit0 F-number bit 8. + m.cpu_write(0x9010, 0x20); + m.cpu_write(0x9030, 0x15); + } + + /// Run `n` CPU cycles and return every mixed sample, so two timelines can + /// be compared as a waveform rather than as a single instant. + #[cfg(feature = "mapper-audio")] + fn run_capture(m: &mut Vrc7, n: usize) -> Vec { + let mut out = Vec::with_capacity(n); + for _ in 0..n { + m.notify_cpu_cycle(); + out.push(m.mix_audio()); + } + out + } + + /// **The test the ledger row existed for.** A restored VRC7 must resume the + /// note that was playing, sample for sample. + /// + /// Before v2.3.7 the section carried only the shadow register bytes, so the + /// restored synthesizer started from its power-on state and this comparison + /// failed on the very first sample after the envelope diverged. Deleting + /// the v2 tail from `save_state` reproduces that failure — the mutation + /// check for this test. + #[cfg(feature = "mapper-audio")] + #[test] + fn vrc7_save_state_carries_the_live_opll_so_audio_resumes_identically() { + let mut source = vrc7_default(); + key_on_channel_0(&mut source); + // Advance far enough that the envelope is well past attack and the + // phase accumulators hold values no reset could coincidentally match. + let _ = run_capture(&mut source, 20_000); + + let blob = source.save_state(); + assert_eq!(blob[0], 2, "a mapper-audio build must write section v2"); + + let expected = run_capture(&mut source, 4_000); + assert!( + expected.iter().any(|&s| s != 0), + "fixture produced silence — the test would pass vacuously" + ); + + let mut restored = vrc7_default(); + restored.load_state(&blob).expect("v2 blob must load"); + let got = run_capture(&mut restored, 4_000); + + assert_eq!( + got, expected, + "the restored VRC7 did not resume the note that was playing: the OPLL \ + envelope + phase state is not surviving the save state" + ); + } + + /// The v2 tail is additive: a v1 blob (every save written before v2.3.7) + /// still loads. It leaves the synthesizer untouched, which is exactly the + /// pre-v2.3.7 behaviour — an old save is no worse than it always was. + #[test] + fn vrc7_load_state_still_accepts_a_v1_blob() { + let mut source = vrc7_default(); + source.cpu_write(0x8000, 5); + source.cpu_write(0x9010, 0x15); + source.cpu_write(0x9030, 0x77); + let blob = source.save_state(); + + // Synthesize the v1 form: version byte 1, and no tail past the VRAM. + let core_len = 91 + source.vram.len(); + let mut v1 = blob[..core_len].to_vec(); + v1[0] = 1; + + let mut target = vrc7_default(); + target.load_state(&v1).expect("a v1 blob must still load"); + assert_eq!(target.prg_0, 5, "v1 core fields must round-trip"); + assert_eq!(target.audio.regs[0x15], 0x77); + } + + /// **Every build must ACCEPT a v2 blob, including one that cannot write it.** + /// + /// Regression for a defect two review bots caught independently: the accept + /// check read `version != 1 && version != VRC7_SECTION_VERSION`, and + /// `VRC7_SECTION_VERSION` is 1 on a no-audio build — so the condition + /// collapsed to `version != 1` there and a v2 blob was rejected outright. + /// That is the exact opposite of the validate-then-ignore portability + /// ADR 0004 asks for, and the opposite of what the constant's own doc + /// comment claimed. + /// + /// The lesson, which is why this test exists rather than a one-line diff: + /// **what a build can WRITE and what it must ACCEPT are different sets, and + /// only the first varies by feature.** Deriving one from the other reads as + /// tidy and silently couples them. + #[test] + fn vrc7_load_state_accepts_a_v2_blob_on_every_build() { + let mut source = vrc7_default(); + source.cpu_write(0x8000, 5); + + // On a `mapper-audio` build `save_state` already emits v2. On a no-audio + // build it emits v1 with no tail, so synthesize the v2 shape: the load + // path validates that tail's LENGTH on every build and reads its + // CONTENTS only where there is a synthesizer, so zeros are correct here. + #[cfg(feature = "mapper-audio")] + let blob = source.save_state(); + #[cfg(not(feature = "mapper-audio"))] + let blob = { + let mut b = source.save_state(); + b[0] = 2; + b.resize(b.len() + VRC7_V2_TAIL_LEN, 0); + b + }; + assert_eq!(blob[0], 2, "the fixture must be a v2 blob"); + + let mut target = vrc7_default(); + target + .load_state(&blob) + .expect("a v2 blob must load on every build, whether or not it can write one"); + assert_eq!(target.prg_0, 5, "the core fields must still round-trip"); + } + + /// A v2 blob truncated inside its tail must be rejected, not partially + /// applied. This is untrusted input: a save state is a file on disk. + #[cfg(feature = "mapper-audio")] + #[test] + fn vrc7_load_state_rejects_a_truncated_v2_tail() { + let mut source = vrc7_default(); + key_on_channel_0(&mut source); + let _ = run_capture(&mut source, 500); + let blob = source.save_state(); + + // Give the target DIFFERENT state from the source, so a partial write + // is observable rather than coincidentally identical. + let mut target = vrc7_default(); + target.cpu_write(0x8000, 3); + target.cpu_write(0x9000, 6); + let pristine = target.save_state(); + + let err = target + .load_state(&blob[..blob.len() - 1]) + .expect_err("a truncated v2 tail must be rejected"); + assert!( + matches!(err, MapperError::Truncated { .. }), + "expected Truncated, got {err:?}" + ); + + // The half this test used to be missing. Returning `Err` is not enough: + // `load_state` assigned the core fields BEFORE validating the v2 tail, so + // a rejected load left the mapper neither in its old state nor the new + // one, while the caller reported failure and kept running. Asserting only + // on the return value cannot see that -- which is why review found it and + // this test did not. + assert_eq!( + target.save_state(), + pristine, + "a rejected load mutated the mapper: load_state is not atomic" + ); + } } diff --git a/crates/rustynes-test-harness/tests/snapshot_schema_audit.rs b/crates/rustynes-test-harness/tests/snapshot_schema_audit.rs index ab71c5fa..c83e2875 100644 --- a/crates/rustynes-test-harness/tests/snapshot_schema_audit.rs +++ b/crates/rustynes-test-harness/tests/snapshot_schema_audit.rs @@ -317,6 +317,42 @@ const CHIPS: &[Chip] = &[ ], known_gaps: &[], }, + // The OPLL is not a 2A03 chip, but it is a serialized synthesizer with the + // same failure mode, and it reached this audit the hard way: its state was + // NOT carried at all until v2.3.7, and nothing mechanical noticed for the + // whole life of the feature because the audit only knew about the three + // chips in the console. A save-state surface that no audit can see is + // exactly how the gap this closes was able to persist — so the new surface + // is registered here in the same change that creates it. + // + // Its blob rides in the *mapper* section of whichever board carries the + // chip (VRC7 today), which is why it has its own version byte and its own + // error type rather than APU_SNAPSHOT_VERSION's. + Chip { + label: "Opll", + struct_src: include_str!("../../rustynes-apu/src/opll.rs"), + struct_name: "Opll", + snapshot_src: include_str!("../../rustynes-apu/src/opll.rs"), + // `chip_type` and `patch_set` are absent from this list on purpose: + // both ARE written by the serializer, so the audit already accounts for + // them and an exclusion entry would be rejected as a false admission. + // Their subtlety is on the *read* side, and is documented at the writer + // — `chip_type` is emitted only as a tag that rejects a cross-chip + // restore and is never assigned from, and of `patch_set` only the two + // non-ROM entries (the user patch written through `$00-$07`) round-trip; + // the remaining 36 are the chip's patch ROM, fixed by `chip_type`. + derived_or_config: &[ + ( + "waves", + "derived: the 1024-entry sine / half-sine lookup tables, built in `Opll::new`", + ), + ( + "tll_rks", + "derived: the TLL + RKS lookup tables (~128 KiB), built in `Opll::new`", + ), + ], + known_gaps: &[], + }, ]; /// Extract the field names of `struct ` from Rust source. diff --git a/docs/accuracy-ledger.md b/docs/accuracy-ledger.md index fbc23877..a3ac0aab 100644 --- a/docs/accuracy-ledger.md +++ b/docs/accuracy-ledger.md @@ -53,7 +53,7 @@ disposition under the v2.1.0 "Fathom" accuracy-remediation line | PlayChoice-10 Z80 second-screen menu | Not modeled | — | **Out of scope** | | MMC1 software WRAM write-protect | **REMEDIATED (v2.2.3 A2).** MMC1 has two software PRG-RAM write-protect layers -- the `$E000` bit-4 disable common to every board, and SNROM's second layer where a CHR-**RAM** board's CHR register bit 4 is wired to the RAM's other enable. Neither was modelled: `$6000-$7FFF` was read and written unconditionally | Holy Mapperel `M1_*` WRAM nibble (`1000` SJROM = `$E000` layer; `5000` SNROM = both) | **Closed** -- both layers modelled; a disabled window reports `cpu_read_unmapped` so the read floats to open bus rather than returning stale RAM, and writes are discarded. The CHR-register layer is gated on `chr_is_ram` so a CHR-ROM board still treats those bits as CHR banking. Holy Mapperel's README calls this a game-compat hazard (FCEUX / PowerPak omit it), so it was validated before landing rather than assumed: commercial oracle **60/60** (incl. 7 battery-backed MMC1 saves -- Zelda, Metroid, Final Fantasy, Mega Man 2, Castlevania II, Ninja Gaiden, Kid Icarus) and the extended corpus **138/138**. Pinned by three new unit tests, incl. a negative control that a CHR-ROM board ignores the SNROM layer | | FME-7 open bus on RAM-selected-but-disabled `$6000-$7FFF` | **REMEDIATED (v2.2.3 A2).** FME-7 models the command-`$8` RAM-enable (bit 7) / RAM-select (bit 6) bits; the one unmodelled state was **selected but disabled** (bit 6 = 1, bit 7 = 0), which drives neither the RAM nor the ROM chip, so the databus floats. RustyNES fell through to the PRG-ROM bank and returned its tag byte `1`, failing Holy Mapperel's "read open bus" sub-check (requires `>= 3`) and setting `MAPTEST_WRAMEN` | Holy Mapperel `M69_*` WRAM nibble | **Closed** — routed through `Mapper::cpu_read_unmapped`, the trait's existing "not wired to mapper-resident memory" contract, so the bus preserves whatever value the open-bus latch already holds rather than clobbering it with the ROM tag byte — the disabled window now returns open bus, not a fixed constant. (In the Holy Mapperel `M69_*` run that latch value is observed as `$7F`, which is the test's observation, not a universal result.) `M69_*` detail goes `1000` -> `0000`. Verified by negative control (reverting flips the on-screen digit back `0` -> `1`) and against the commercial oracle, where the FME-7 titles are unaffected | -| VRC7 OPLL synthesizer state is not carried by the mapper save state | `Vrc7::save_state` (mapper section **v1**) writes the *shadow* register bytes (`audio.addr_latch`, `audio.data_latch`, `audio.silenced`, `audio.regs[0..64]`) but not the live `self.opll` synthesizer, `opll_clock_counter`, or `last_opll_sample`. `load_state` restores the shadow bytes and never replays them into the OPLL, so after a rewind / netplay rollback / TAS restore the FM voice resumes from whatever envelope + phase state it happened to hold. Banking, IRQ, mirroring and PRG-RAM all round-trip correctly; this is audio-only, and only on mapper 85 with `mapper-audio` on | **None exists** — no pass/fail ROM covers save-state audio continuity, and `rustynes_apu::Opll` exposes no serialization surface at all (no `save_state`/`load_state`; its envelope generators, phase accumulators and LFO are private), so there is nothing to serialize without new schema | **Frontier — documented, not closed** (found by the v2.2.3 CodeRabbit pass; pre-existing since the ADR-0006 VRC7 audio landing, *not* introduced by v2.2.3). Deliberately NOT patched in this cut: the only format-free partial fix — replaying `audio.regs` through `Opll::write_reg` on load — restarts every keyed-on channel's envelope at attack, an audible transient on every rewind frame, and there is no oracle to adjudicate whether that is better or worse than the status quo. A real fix needs an `Opll` snapshot surface plus a mapper section **v2** additive tail, with its own tests — a change of its own, not a release-cut drive-by | +| VRC7 OPLL synthesizer state carried by the mapper save state (v2.2.3 → **v2.3.7**) | **REMEDIATED (v2.3.7).** `Vrc7::save_state` wrote only the *shadow* register bytes (`audio.addr_latch`, `audio.data_latch`, `audio.silenced`, `audio.regs[0..64]`) and never the live `self.opll`, `opll_clock_counter`, or `last_opll_sample`; `load_state` never replayed them either, so after a rewind / netplay rollback / TAS restore the FM voice resumed from whatever envelope and phase state it happened to hold. `rustynes_apu::Opll` now exposes a `snapshot`/`restore` pair (`OPLL_SNAPSHOT_VERSION` 1, a fixed `OPLL_SNAPSHOT_LEN`) carrying the register shadow, the EG/LFO counters, the per-channel patch selection, all 18 operator slots, the user patch pair and the per-channel outputs. The lookup tables and the patch ROM are NOT carried — they are constants of construction — and `chip_type` rides only as a tag that rejects a cross-chip restore. The VRC7 mapper section is **v2**: the tail appends `opll_clock_counter`, `last_opll_sample` and that blob after the VRAM | `vrc7_save_state_carries_the_live_opll_so_audio_resumes_identically` (key a note, advance 20,000 cycles, save, then compare 4,000 mixed samples from the source against 4,000 from a **fresh** mapper restored from the blob — equal sample for sample), plus `Opll` round-trip / snapshot-idempotence / cross-chip-rejection / truncation / bad-tag tests, and `Opll` is now registered in `snapshot_schema_audit.rs` so a future field cannot be added without being classified | **CLOSED (v2.3.7)** — additive, so a v1 blob still loads and leaves the synthesizer exactly where the pre-v2.3.7 build left it (an old save is no worse than it was, not newly silent). A build without `mapper-audio` has no synthesizer to describe, so it still writes v1 and validates-then-ignores a v2 tail, preserving the cross-feature portability ADR 0004 promises. The rejected alternative is recorded because it is the one a reader will think of first: replaying `audio.regs` through `Opll::write_reg` needs no new format, but restarts every keyed-on channel at attack, so every rewind frame would produce an audible transient. Mutation-checked — making the tail carry a *reset* synthesizer reproduces the pre-fix failure | | 2A03 die-revision "unexpected DMA" extra read (`Cpu2A03Revision`, ADR 0033) | The DMC-halt-overlaps-OAM-halt "double-halt" extra parked-address re-read is revision-gated: `Rp2A03G` (default) performs it, `Rp2A03H` omits it. On this engine the gate fires (~75× in a synthetic DMC+OAM+`$2007` probe) but is a no-op — the parked address during a DMC+OAM overlap is always the post-`$4014` instruction fetch, never a side-effect register — so `Rp2A03H` is byte-identical to `Rp2A03G` on every oracle | **None exists** — no public reference (Mesen2/ares/BizHawk/TriCNES/fceux/nestopia/GeraNES/higan) branches DMA behavior on 2A03 die stepping, and no test ROM captures it; the five `dmc_dma_during_read4` ROMs + both `sprdma_and_dmc_dma` ROMs all `Pass` on the default and are the verified floor | **Frontier — documented, not closed** (v2.1.7, ADR 0033). Config surface + mechanism-correct gate shipped **default-off / byte-identical**; the `Rp2A03H` direction is an unverified hypothesis; the H≡G equality is pinned by `cpu_2a03_revision::rp2a03h_matches_rp2a03g_documented_residual`. The reference-grounded **console-type** DMC-glitch axis (Mesen2 `isNesBehavior`) is a separate deferred knob (`T-PS-dmc-glitch-console-type`) | ## Oracles / regression nets