From 70d9ccf9f6fac3c6659c86dafb7a05f13a0b0e21 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Mon, 17 Aug 2026 20:54:11 -0400 Subject: [PATCH 1/9] fix(vrc7): carry the live OPLL in the save state so rewind resumes the music MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the most actionable open row in `docs/accuracy-ledger.md`, open since v2.2.3 and latent since the ADR 0006 VRC7 audio landing. THE DEFECT `Vrc7::save_state` wrote the *shadow* OPLL register bytes — `addr_latch`, `data_latch`, `silenced`, `regs[0..64]` — and never the live synthesizer: not `self.opll`, not `opll_clock_counter`, not `last_opll_sample`. `load_state` restored those shadow bytes and never replayed them into the chip either. So after a rewind, a netplay rollback, or a TAS/save-state restore, the FM voice resumed from whatever envelope and phase state it happened to be holding — an arbitrary point in an unrelated note. Banking, IRQ, mirroring and PRG-RAM had always round-tripped correctly; this was audio-only, and only on mapper 85 with `mapper-audio` on. It was not serializable at the time it was found: `rustynes_apu::Opll` exposed no serialization surface at all, and its envelope generators, phase accumulators and LFO are private. That is what made this "a change of its own" rather than a release-cut drive-by, and it is what this commit does. THE SNAPSHOT SURFACE `Opll::snapshot` / `Opll::restore`, versioned by `OPLL_SNAPSHOT_VERSION` with a fixed `OPLL_SNAPSHOT_LEN`. Carried: the 64-byte register shadow, the current register address, the test flag, the key-status mask, the EG counter, both LFO phases and the AM output, the per-channel patch numbers, the user patch pair (writeable through `$00-$07`), all 18 operator slots in full — phase accumulators, envelope state machines, feedback history, the derived TLL/RKS and rate fields, and the pending-update mask — plus the per-channel outputs and the mix. NOT carried, because they are constants of construction and serializing them would be writing a copy of the binary into the save file: `waves` (the 1024-entry sine / half-sine tables), `tll_rks` (~128 KiB of TLL + RKS tables), and `patch_set[2..]` (the chip's patch ROM, selected by `chip_type`). `chip_type` itself is written as a tag ONLY, never assigned from — its job is to reject a cross-chip restore, since a YM2413 blob loaded into a VRC7 is structurally valid in every field and would silently reinterpret all 18 slot patches against the wrong instrument set. The reader is bounds-checked on every field and decodes the entire blob into locals BEFORE touching `self`, so a truncated or hand-edited save leaves the synthesizer on its previous state rather than half-overwritten — this parses untrusted input, and the caller reports the error and keeps running. Enum tags (`EgState`, `ChipType`) are explicit `to_tag`/`from_tag` mappings rather than `as u8`, so reordering a variant cannot silently reinterpret existing states. Its error type is deliberately NOT `ApuSnapshotError`: the blob rides in the *mapper* section of whichever board carries the chip, which is versioned independently of `APU_SNAPSHOT_VERSION`. Sharing the type would assert a coupling between two schemas that must be free to move apart. THE MAPPER SECTION VRC7 goes to section **v2**, appending `opll_clock_counter` (u16), `last_opll_sample` (i16) and the OPLL blob after the VRAM. Additive: `load_state` still accepts v1, and a v1 blob leaves the synthesizer untouched — which is precisely the pre-fix behaviour, 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 writes v1 and validates-then-ignores a v2 tail. That keeps the property this crate's feature documentation and ADR 0004 promise in both directions — an audio build's save loads in a no-audio build, and a no-audio build's save loads everywhere — which is why `VRC7_SECTION_VERSION` is build-dependent rather than a flat 2. THE ALTERNATIVE, AND WHY IT LOST Replaying `audio.regs` through `Opll::write_reg` on load needs no new format and is the repair a reader thinks of first. It restarts every keyed-on channel's envelope at attack, so every rewind frame produces an audible transient. The ledger recorded that there was no oracle to adjudicate which was worse; carrying the state verbatim removes the question, because it reproduces the sound that was actually playing. TESTS `vrc7_save_state_carries_the_live_opll_so_audio_resumes_identically` keys a note on channel 0 with a real melodic patch, advances 20,000 CPU cycles so the envelope is well past attack and the phase accumulators hold values no reset could coincidentally match, saves, then compares 4,000 mixed samples from the source against 4,000 from a FRESH mapper restored from the blob — equal sample for sample. Mutation-checked: making the tail carry a *reset* synthesizer reproduces the pre-fix failure on the first divergent sample. Plus, at the `Opll` level: a 2,000-sample stream round-trip into a fresh chip; snapshot -> restore -> snapshot byte idempotence (which catches a field written but not read back, a case the stream test can miss); cross-chip rejection; truncation rejection asserting the target is left UNMUTATED; unknown-version rejection; and a corrupt envelope-state tag. At the mapper level: a v1 back-compat load and a truncated-v2-tail rejection. `Opll` is now registered in `snapshot_schema_audit.rs`. That audit — the standing field-vs-schema check that found the v2.2.3 PPU and APU gaps mechanically — knew only about the three chips inside the console, so it could not see this surface at all. A save-state surface no audit can see is exactly how a gap this size survives four releases, so the surface is registered in the same change that creates it. Registering it immediately caught two of my own exclusion entries as false admissions (`chip_type` and `patch_set` ARE written), which is the audit working. Out of scope, checked and left alone: `nsf_expansion.rs` holds an `Opll` too and also carries no phase, but that is a written decision with a stated rationale (an NSF driver re-establishes channel state on the next play call), not an undocumented gap. VERIFICATION Nothing on the synthesis path moved, so emulation output is unchanged — but this touches `rustynes-apu` and `rustynes-mappers`, so the contract was verified rather than assumed: AccuracyCoin **141/141 (100.00%)** via the authoritative RAM decoder, nestest 0-diff. Workspace clippy, `mapper-audio`-off clippy, rustdoc with warnings denied, the `no_std` cross-build, and 124 workspace test binaries all green. --- CHANGELOG.md | 45 ++ crates/rustynes-apu/src/lib.rs | 5 +- crates/rustynes-apu/src/opll.rs | 527 ++++++++++++++++++ crates/rustynes-mappers/src/m085_vrc7.rs | 188 ++++++- .../tests/snapshot_schema_audit.rs | 36 ++ docs/accuracy-ledger.md | 2 +- 6 files changed, 793 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f55d9fea..aa86c882 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,51 @@ 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. + - **Pixel Provenance now works.** The v2.3.2 "Lucid" marquee returned an empty report for effectively every user, from release until now, because of two independent defects. 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..44c3a938 100644 --- a/crates/rustynes-apu/src/opll.rs +++ b/crates/rustynes-apu/src/opll.rs @@ -1586,6 +1586,400 @@ 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; + +/// 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; + Ok(Patch { + tl, + fb, + eg, + ml, + ar, + dr, + sl, + rr, + kr, + kl, + am, + pm, + ws, + }) + } +} + +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()?; + 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 { + *n = r.i32()?; + } + let user_patch = [r.patch()?, r.patch()?]; + let mut slots = [Slot::default(); SNAPSHOT_SLOTS]; + for s in &mut slots { + s.number = r.u8()?; + s.type_flags = r.u8()?; + s.patch = r.patch()?; + s.output = [r.i32()?, r.i32()?]; + s.wave_table_idx = r.u8()?; + s.pg_phase = r.u32()?; + s.pg_out = r.u32()?; + s.pg_keep = r.u8()?; + s.blk_fnum = r.u16()?; + s.fnum = r.u16()?; + s.blk = r.u8()?; + s.eg_state = EgState::from_tag(r.u8()?)?; + s.volume = r.i32()?; + s.key_flag = r.u8()?; + s.sus_flag = r.u8()?; + s.tll = r.u16()?; + s.rks = r.u8()?; + s.eg_rate_h = r.u8()?; + s.eg_rate_l = r.u8()?; + s.eg_shift = r.u32()?; + 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 +2616,137 @@ 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" + ); + } + + #[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..f745e6c1 100644 --- a/crates/rustynes-mappers/src/m085_vrc7.rs +++ b/crates/rustynes-mappers/src/m085_vrc7.rs @@ -44,6 +44,27 @@ 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 skips a v2 tail on load. 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 — which +/// a build-dependent *version byte* alone would not have given. +#[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 +566,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 +581,8 @@ 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; - let mut out = Vec::with_capacity(scalar_len + self.vram.len()); - out.push(1u8); // version + let mut out = Vec::with_capacity(scalar_len + self.vram.len() + VRC7_V2_TAIL_LEN); + out.push(VRC7_SECTION_VERSION); // version out.push(self.prg_0); out.push(self.prg_1); out.push(self.prg_2); @@ -576,6 +601,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,7 +627,7 @@ impl Mapper for Vrc7 { }); } let version = data[0]; - if version != 1 { + if version != 1 && version != VRC7_SECTION_VERSION { return Err(MapperError::UnsupportedVersion(version)); } self.prg_0 = data[1]; @@ -628,6 +660,32 @@ 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. + 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(), + }); + } + // A no-audio build validates the tail's LENGTH (above) and then + // ignores its contents — there is no synthesizer to restore into. + #[cfg(feature = "mapper-audio")] + { + self.opll_clock_counter = u16::from_le_bytes([tail[0], tail[1]]); + self.last_opll_sample = i16::from_le_bytes([tail[2], tail[3]]); + self.opll + .restore(&tail[4..]) + .map_err(|e| MapperError::Invalid(format!("VRC7 OPLL state: {e}")))?; + } + } Ok(()) } } @@ -940,7 +998,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 +1058,115 @@ 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); + } + + /// 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(); + + let mut target = vrc7_default(); + 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:?}" + ); + } } 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 From b080ececebbafb922a4a9fddac50781131103e07 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Mon, 17 Aug 2026 21:08:51 -0400 Subject: [PATCH 2/9] fix(vrc7): accept v2 on every build; run the no-audio config in CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found a real defect in the previous commit, and the reason no test could have caught it is the more useful half. THE DEFECT `load_state` gated its accept set on `VRC7_SECTION_VERSION`: if version != 1 && version != VRC7_SECTION_VERSION { reject } That constant is the version this build WRITES, and it is deliberately **1** on a `mapper-audio`-off build (no synthesizer to describe). So on that build the condition collapsed to `version != 1` and a v2 blob was rejected outright with `UnsupportedVersion(2)` — the precise opposite of the validate-then-ignore portability ADR 0004 asks for, and the opposite of what the constant's own doc comment asserted two lines above it. 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 the literals 1 and 2, with the reason recorded at both the check and the constant. WHY NOTHING CAUGHT IT The default build takes the other branch and was correct throughout, so every gate stayed green. CI **linted** the `--no-default-features` shape (`cargo clippy -p rustynes-mappers --no-default-features`) and never **ran** it. A configuration that is compiled but never executed is not covered, and this is what that costs. `cargo test -p rustynes-mappers --no-default-features` is now a CI step, Linux only and scoped to the one crate whose behaviour is feature-gated this way. One extra compile of a mid-sized crate, so it runs on every PR rather than waiting for a full run. TESTS `vrc7_load_state_accepts_a_v2_blob_on_every_build` asserts the property directly. On a `mapper-audio` build `save_state` already emits v2; on a no-audio build it emits v1, so the test synthesizes the v2 shape — correct, because that path validates the tail's LENGTH on every build and reads its CONTENTS only where there is a synthesizer. Mutation-checked in BOTH configurations, which is the part that documents the hazard: restoring the old condition turns the test red under `--no-default-features` with `UnsupportedVersion(2)`, and leaves it green on the default build. A single-configuration mutation check would have reported the bug as absent. Also from review: `save_state`'s `Vec::with_capacity` unconditionally reserved `VRC7_V2_TAIL_LEN`, over-allocating ~1.3 KiB per save on a build that never writes that tail. Now `cfg`-gated. Declined: aligning `OpllR::u32`'s `copy_from_slice` with `OpllR::patch`'s `try_into` destructuring. The difference is deliberate — `patch` destructures positionally so its field order is visibly the same list `OpllW::patch` writes, which is the property that keeps the two halves from drifting; a scalar read has no such list to mirror. --- .github/workflows/ci.yml | 16 +++++ CHANGELOG.md | 17 ++++++ crates/rustynes-mappers/src/m085_vrc7.rs | 75 ++++++++++++++++++++++-- 3 files changed, 103 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4965388f..fca39ee8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -258,6 +258,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 aa86c882..b9a5d820 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,23 @@ cycle-accurate core later replaced. 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. + - **Pixel Provenance now works.** The v2.3.2 "Lucid" marquee returned an empty report for effectively every user, from release until now, because of two independent defects. diff --git a/crates/rustynes-mappers/src/m085_vrc7.rs b/crates/rustynes-mappers/src/m085_vrc7.rs index f745e6c1..f9f1b99f 100644 --- a/crates/rustynes-mappers/src/m085_vrc7.rs +++ b/crates/rustynes-mappers/src/m085_vrc7.rs @@ -52,10 +52,19 @@ const NAMETABLE_SIZE_U16: u16 = 0x0400; /// `docs/accuracy-ledger.md`. `load_state` accepts both. /// /// A build without `mapper-audio` has no synthesizer to describe, so it writes -/// **v1** and skips a v2 tail on load. 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 — which -/// a build-dependent *version byte* alone would not have given. +/// **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"))] @@ -581,7 +590,13 @@ 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(VRC7_SECTION_VERSION); // version out.push(self.prg_0); out.push(self.prg_1); @@ -627,7 +642,16 @@ impl Mapper for Vrc7 { }); } let version = data[0]; - if version != 1 && version != VRC7_SECTION_VERSION { + // 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)); } self.prg_0 = data[1]; @@ -1150,6 +1174,47 @@ mod tests { 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")] From fd62fcb1260b5fd4c3cf4c9f24db1e5571c736c0 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Mon, 17 Aug 2026 22:00:01 -0400 Subject: [PATCH 3/9] docs(changelog): re-home this entry under [Unreleased] after the v2.3.6 cut Merging `main` after the release SILENTLY DROPPED this entry, and reported no conflict while doing it. Worth writing down, because "mergeable: MERGEABLE" is what GitHub said right up to the moment the text disappeared. The release moved everything out of `[Unreleased]` into `[2.3.6]`, deleting the `### Fixed` heading this entry was anchored under. Git saw one side delete a region and the other side add a line inside it, resolved in favour of the deletion, and produced a clean tree. No marker, no warning -- the entry was simply gone from the merged CHANGELOG. Caught by inspecting `git merge-tree --write-tree` output before merging rather than trusting the mergeability flag; all three post-release fix branches were affected the same way. The fix is mechanical (re-anchor under a fresh `### Fixed` in the now-empty `[Unreleased]`), but the failure mode is not: a CHANGELOG entry is exactly the kind of content whose absence nothing downstream detects. --- CHANGELOG.md | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3bd2634a..bd0b3c4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,70 @@ cycle-accurate core later replaced. ## [Unreleased] +### 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. + ## [2.3.6] - 2026-08-17 - "Sounding" (measuring, and what a measurement may claim) A *sounding* is a depth measured with its uncertainty attached, and that is what From 9eb0215e55d28ba7c3907e2883b655e2d0dc365b Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Mon, 17 Aug 2026 23:19:07 -0400 Subject: [PATCH 4/9] docs(changelog): remove the copy of this entry that landed inside [2.3.6] My own error, and the diagnosis that produced it is worth recording. Merging `main` after the v2.3.6 cut did not DROP this entry, as I concluded -- it MOVED it. The release relocated the whole `[Unreleased]` block into `[2.3.6]`, and git carried this branch's addition along with the block it was written inside. I checked for the entry only under `[Unreleased]`, saw nothing, called it dropped, and re-added it there. Two copies: one correctly under `[Unreleased]`, one wrongly inside a released section describing work that release does not contain. The check was too narrow, not wrong in kind: `git merge-tree --write-tree` was the right instrument and it did show the real merged content. I searched four lines of it instead of the whole file. Both review bots caught the duplicate. Removing the `[2.3.6]` copy; the `[Unreleased]` one stays, which is where a fix that ships in the NEXT release belongs. --- CHANGELOG.md | 62 ---------------------------------------------------- 1 file changed, 62 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd0b3c4d..6ba64143 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -149,68 +149,6 @@ optimization campaign is closed on the strength of three measured rejections. ### 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. - - **Pixel Provenance now works.** The v2.3.2 "Lucid" marquee returned an empty report for effectively every user, from release until now, because of two independent defects. From 74870e9f406da0fb91f6b5177c4f90f8f8cf86ea Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Mon, 17 Aug 2026 23:34:20 -0400 Subject: [PATCH 5/9] fix(vrc7): make the save-state parse total, and load_state atomic Two defects from a second review pass, both in code this PR introduced, 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 `Opll::restore` handed it raw deserialized bytes. A `tl` of 255 computes an index of 524,539 into a 32,768-entry table: index out of bounds: the len is 32768 but the index is 524539 `blk_fnum` reaches the same tables through `>> 5` (128 rows) and `>> 8` (16), so an unmasked u16 indexes far past both. A save state is a file on disk -- untrusted input, and module 60 is explicit that every field read from untrusted bytes is bounds-checked before use. Every register field is now masked to its hardware width AT THE PARSE BOUNDARY: the 13 patch parameters to their documented widths, `blk_fnum` to 0x0FFF (a legal value is `(blk3 << 9) | fnum9`), `fnum` to 9 bits, `blk` to 3, the single-bit flags to 1, and `number` into slot range. This is parse-don't- validate rather than a repair: these are register fields of fixed bit width, so a wider value does not describe a chip state that exists. The test is careful about one thing worth stating, because the obvious version of it is useless. An all-`0xFF` blob is REJECTED by `EgState::from_tag` before a single numeric field is read -- so the naive hostile input passes by accident and reports the emulator safe. The test therefore repairs the envelope-state tags and leaves everything else hostile: the interesting input is the one that satisfies every explicit check and is still nonsense. Mutation-checked -- removing the `tl` mask alone reproduces the panic. `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: v1 validated its whole length and version up front, so once it began writing it could not fail. A truncated or corrupt v2 tail returned `Err` with `prg_0`, `chr`, the IRQ state and 2 KiB of VRAM already overwritten -- a mapper in neither its old state nor its new one, while the caller reports the load as failed and keeps running. `Opll::restore` was already atomic internally, and that is exactly what made this easy to miss: the guarantee existed one level down and was silently discarded one level up. `load_state` now validates the tail length and parses the synthesizer into a staged clone BEFORE the first write, then commits infallibly. The no-audio build performs the same length validation, so the same blob is accepted or refused identically on every build. The truncation test asserted only on the return value, which is why review found this and the test did not. It now gives the target different state from the source and asserts it is byte-identical after the rejected load. Mutation-checked -- moving the length guard back after the writes turns it red. ALSO TAKEN `tail[0..2].try_into()` over manual `[tail[0], tail[1]]` indexing, guaranteed by the preceding length check. DECLINED Bounds-checking `patch_number`, `wave_table_idx` and `s.number` as suggested: traced, and none of them index anything. `wave_table_idx` selects through a `match idx { 0 => .., _ => .. }`; `patch_number` is only ever compared to zero; `s.number` is stored and compared, never used as a subscript. The reachable path was elsewhere, through the patch parameters and `blk_fnum` -- which is why it was worth tracing each one instead of masking on the strength of the report. VERIFIED AccuracyCoin 141/141 on the authoritative RAM decoder, nestest 0-diff. Workspace clippy, `mapper-audio`-off clippy AND tests, rustdoc warnings-denied, the no_std cross-build, 124 workspace test binaries. --- CHANGELOG.md | 23 ++++++ crates/rustynes-apu/src/opll.rs | 95 +++++++++++++++++++----- crates/rustynes-mappers/src/m085_vrc7.rs | 91 ++++++++++++++++++----- 3 files changed, 172 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ba64143..0c19ea8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -78,6 +78,29 @@ cycle-accurate core later replaced. 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. + + 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. + ## [2.3.6] - 2026-08-17 - "Sounding" (measuring, and what a measurement may claim) A *sounding* is a depth measured with its uncertainty attached, and that is what diff --git a/crates/rustynes-apu/src/opll.rs b/crates/rustynes-apu/src/opll.rs index 44c3a938..6faf9e19 100644 --- a/crates/rustynes-apu/src/opll.rs +++ b/crates/rustynes-apu/src/opll.rs @@ -1778,20 +1778,30 @@ impl OpllR<'_> { .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. + // + // 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, - fb, - eg, - ml, - ar, - dr, - sl, - rr, - kr, - kl, - am, - pm, - ws, + 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, }) } } @@ -1933,17 +1943,21 @@ impl Opll { let user_patch = [r.patch()?, r.patch()?]; let mut slots = [Slot::default(); SNAPSHOT_SLOTS]; for s in &mut slots { - s.number = r.u8()?; + // 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()?; s.patch = r.patch()?; s.output = [r.i32()?, r.i32()?]; - s.wave_table_idx = r.u8()?; + s.wave_table_idx = r.u8()? & 0x01; s.pg_phase = r.u32()?; s.pg_out = r.u32()?; - s.pg_keep = r.u8()?; - s.blk_fnum = r.u16()?; - s.fnum = r.u16()?; - s.blk = r.u8()?; + 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()?; @@ -2718,6 +2732,49 @@ mod tests { ); } + /// 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(); + } + + // 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(); diff --git a/crates/rustynes-mappers/src/m085_vrc7.rs b/crates/rustynes-mappers/src/m085_vrc7.rs index f9f1b99f..c20e5868 100644 --- a/crates/rustynes-mappers/src/m085_vrc7.rs +++ b/crates/rustynes-mappers/src/m085_vrc7.rs @@ -654,6 +654,54 @@ impl Mapper for Vrc7 { 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"))] + 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]; @@ -691,24 +739,13 @@ impl Mapper for Vrc7 { // 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. - 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(), - }); - } - // A no-audio build validates the tail's LENGTH (above) and then - // ignores its contents — there is no synthesizer to restore into. - #[cfg(feature = "mapper-audio")] - { - self.opll_clock_counter = u16::from_le_bytes([tail[0], tail[1]]); - self.last_opll_sample = i16::from_le_bytes([tail[2], tail[3]]); - self.opll - .restore(&tail[4..]) - .map_err(|e| MapperError::Invalid(format!("VRC7 OPLL state: {e}")))?; - } + // 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(()) } @@ -1225,7 +1262,13 @@ mod tests { 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"); @@ -1233,5 +1276,17 @@ mod tests { 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" + ); } } From 364a1e120e87b3be380ffbc59e382566ca29c27a Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Tue, 18 Aug 2026 00:48:39 -0400 Subject: [PATCH 6/9] fix(vrc7): drop a leaked conflict marker; make the tail-length check underflow-proof Two review findings. A LEAKED diff3 CONFLICT MARKER `CHANGELOG.md` carried a stray `||||||| 3f99a3dd`. My resolver checked for `<<<<<<<`, `=======` and `>>>>>>>` and reported zero markers left -- but this repo sets `merge.conflictStyle = diff3`, which adds a fourth marker naming the merge BASE. I verified the wrong alphabet and believed the answer. Content was unaffected (the base section was empty, so only the marker line survived), and all three post-release branches had it. THE NO-AUDIO TAIL-LENGTH CHECK COULD UNDERFLOW data.len() - core_expected < VRC7_V2_TAIL_LEN cannot underflow today: the guard at the top of `load_state` already proved `data.len() >= core_expected`. But the proof lives in a different statement thirty lines away, and the failure mode if it ever moves is not a panic -- the subtraction wraps to a huge value, the comparison goes false, and a TRUNCATED BLOB IS SILENTLY ACCEPTED. That is the worst available outcome for a save-state length check, and it is one refactor away. Rewritten as `data.len() < core_expected + VRC7_V2_TAIL_LEN`, which is correct independent of anything above it. Not worth spreading a correctness argument across two distant statements to save an addition. Verified in both feature configurations: `mapper-audio` on (20 VRC7 tests) and off (16), plus no-audio clippy. --- CHANGELOG.md | 2 -- crates/rustynes-mappers/src/m085_vrc7.rs | 9 ++++++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c21d3e9..002fcb2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -101,8 +101,6 @@ cycle-accurate core later replaced. on the return value, which is why review found this and the test did not; it now asserts the target is byte-identical afterwards. -||||||| 3f99a3dd - - **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 — diff --git a/crates/rustynes-mappers/src/m085_vrc7.rs b/crates/rustynes-mappers/src/m085_vrc7.rs index c20e5868..ea5c97c4 100644 --- a/crates/rustynes-mappers/src/m085_vrc7.rs +++ b/crates/rustynes-mappers/src/m085_vrc7.rs @@ -695,7 +695,14 @@ impl Mapper for Vrc7 { // 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"))] - if version >= 2 && data.len() - core_expected < VRC7_V2_TAIL_LEN { + // 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(), From 0cf7dab9c7298b8b06a1e20f89ec2b051a683349 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Tue, 18 Aug 2026 01:45:51 -0400 Subject: [PATCH 7/9] fix(vrc7): fuzz the OPLL parse boundary, and fix the three panics it found Review pushed back that my "every register field is masked at the parse boundary" claim did not match the code. It was right, and the pushback was worth far more than the specific fields it named. THE FIXED PAYLOAD WAS HIDING BUGS The previous test used one all-`0xFF` payload. Replacing it with a deterministic pseudo-random sweep (64 rounds, xorshift, no dependency) found THREE more panics immediately -- and one of them the fixed payload actively CONCEALED: * `eg_shift` is used as a shift amount, `1u32 << eg_shift`, which panics at 32 and above. Legal values are `13 - eg_rate_h`, so 0..=13. The all-`0xFF` blob never reached it, because 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. A blob that is maximally hostile in one dimension can be harmless in another. * `output[0] + output[1]`, the operator feedback pair, summed as two arbitrary `i32`s. The field is `i32` for headroom in that sum; the values are only ever `i32::from(out)` with `out: i16`. Clamped to i16 on restore. * `eg_rate_l` INDEXES `EG_STEP_TABLES`, outer dimension 4. Legal values are `rks & 3`. That last one is the one to remember. Review named `eg_rate_l` explicitly. I replied that I had traced it and it reached no subscript. My "trace" was a grep pipeline that emitted nothing because the pipeline itself was broken, and I read the empty output as evidence of absence. This is the second time tonight I verified with a broken instrument and believed the answer -- the first being a conflict-marker check that did not know about diff3. WHAT CHANGED `eg_shift` clamped to `EG_SHIFT_MAX`, `output` clamped to i16, and the flag fields masked to their real widths: `eg_rate_l & 3`, `eg_rate_h & 0x0F`, `rks & 0x0F`, `type_flags & 3`, `key_flag`/`sus_flag`/`test_flag & 1`. The comment that overclaimed now states the scope precisely -- what is masked, and for each field that is NOT, the specific mechanism that makes it safe (matched-on, clamped at use, recomputed before use, or self-masking index). It also points at the sweep, because that is what keeps the claim honest rather than a second round of hand-tracing. Soaked at 4,000 rounds in both debug (overflow checks on) and release before settling at 64 in the committed test. VERIFIED AccuracyCoin 141/141 on the authoritative RAM decoder, nestest 0-diff. Workspace clippy, no-audio clippy and tests, the no_std cross-build, VRC7 20/20. --- CHANGELOG.md | 12 +++- crates/rustynes-apu/src/opll.rs | 103 +++++++++++++++++++++++++++++--- 2 files changed, 105 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 002fcb2b..b8a0695e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -88,7 +88,17 @@ cycle-accurate core later replaced. 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. + 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 diff --git a/crates/rustynes-apu/src/opll.rs b/crates/rustynes-apu/src/opll.rs index 6faf9e19..d7c59f5c 100644 --- a/crates/rustynes-apu/src/opll.rs +++ b/crates/rustynes-apu/src/opll.rs @@ -1633,6 +1633,25 @@ const SNAPSHOT_SLOTS: usize = 18; /// `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; + +/// 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; @@ -1782,6 +1801,20 @@ impl OpllR<'_> { // 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 @@ -1930,7 +1963,7 @@ impl Opll { let mut reg = [0u8; 0x40]; reg.copy_from_slice(&r.src[r.pos..r.pos + 0x40]); r.pos += 0x40; - let test_flag = r.u8()?; + let test_flag = r.u8()? & 0x01; let slot_key_status = r.u32()?; let eg_counter = r.u32()?; let pm_phase = r.u32()?; @@ -1948,9 +1981,14 @@ impl Opll { // 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()?; + s.type_flags = r.u8()? & 0x03; s.patch = r.patch()?; - s.output = [r.i32()?, r.i32()?]; + // 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()?; @@ -1960,13 +1998,28 @@ impl Opll { s.blk = r.u8()? & 0x07; s.eg_state = EgState::from_tag(r.u8()?)?; s.volume = r.i32()?; - s.key_flag = r.u8()?; - s.sus_flag = r.u8()?; + s.key_flag = r.u8()? & 0x01; + s.sus_flag = r.u8()? & 0x01; s.tll = r.u16()?; - s.rks = r.u8()?; - s.eg_rate_h = r.u8()?; - s.eg_rate_l = r.u8()?; - s.eg_shift = r.u32()?; + 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()?; } @@ -2765,6 +2818,38 @@ mod tests { 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(); + } + } + // 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() { From 3f96eac2565789dd7fc49fac6fc4a9aed4351e68 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Tue, 18 Aug 2026 08:24:21 -0400 Subject: [PATCH 8/9] test(opll): drive the register port in the hostile-blob sweep too Review asked whether `adr` and `patch_number` are bounded, noting `adr` could index `self.reg` (length 64) on a subsequent data-port write. Answering that by reading the code is what I did twice tonight, and both times the reading was wrong -- once because a grep pipeline was broken and I took its empty output as proof. So the sweep answers it instead. The randomized sweep only called `calc()`. Synthesis alone never reaches `write_reg`, so any restored field consumed on a subsequent PORT write would sail straight past it -- which is precisely the hole review pointed at. Each round now also drives 64 randomized `write_reg` / `read_reg` pairs against the restored chip, interleaved with `calc()`. Clean, including a 3,000-round soak in debug with overflow checks on. That is a stronger statement than the trace review asked for: not "I looked and `adr` is never an index", but "randomized values in `adr` survive being used as one, on a restored chip, through the port". Covering both paths is also cheaper than adjudicating which fields reach a subscript -- and it keeps working when someone later adds an index that does. --- crates/rustynes-apu/src/opll.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/crates/rustynes-apu/src/opll.rs b/crates/rustynes-apu/src/opll.rs index d7c59f5c..14ec1d5e 100644 --- a/crates/rustynes-apu/src/opll.rs +++ b/crates/rustynes-apu/src/opll.rs @@ -2848,6 +2848,19 @@ mod tests { 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 From 3eea1b614c1f57dd5cf4c540390a7112bd2251ec Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Tue, 18 Aug 2026 08:49:32 -0400 Subject: [PATCH 9/9] fix(opll): clamp patch_number too, for consistency rather than for a bug Third review round flagging `patch_number`. It is still not a subscript -- it appears only as `patch_number[ch] == 0` comparisons and `patch_number[ch] = num as i32` assignments, and `set_patch_internal` bounds-checks its own `num` against `patch_set.len()` before indexing. The randomized sweep exercises it with arbitrary values, through both synthesis and the register port, and finds nothing. Clamped anyway, and the reason is not appeasement. Every other field in this parse is constrained by its own WIDTH: the value that comes out cannot describe a chip state that does not exist. `patch_number` was the one exception, safe only because nothing currently indexes it -- an invariant that lives in other functions and holds until someone adds an index. The legal domain is 0..=15 (the `$3x` high nibble is four bits), so clamping costs nothing and makes the field's safety self-evident from the parse rather than from a survey of its consumers. Three reviewers reading the same code and reaching the same worry is decent evidence the invariant was too subtle to be load-bearing, even if each individual report was wrong about the mechanism. No fidelity cost, and the existing tests prove it: `snapshot -> restore -> snapshot` byte idempotence and the 2,000-sample stream comparison both still pass, which they could not if a legitimate value were being clamped. --- crates/rustynes-apu/src/opll.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/crates/rustynes-apu/src/opll.rs b/crates/rustynes-apu/src/opll.rs index 14ec1d5e..47f1ff3e 100644 --- a/crates/rustynes-apu/src/opll.rs +++ b/crates/rustynes-apu/src/opll.rs @@ -1640,6 +1640,10 @@ const SLOT_BYTES: usize = 62; /// 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 { @@ -1971,7 +1975,18 @@ impl Opll { let lfo_am = r.u8()?; let mut patch_number = [0i32; 9]; for n in &mut patch_number { - *n = r.i32()?; + // 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];