From fb13f4c05c0243cd1c9b0b70fb0ebed0685a6c21 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 19:55:54 -0400 Subject: [PATCH] feat(cosim): rolling per-cycle hash checkpoints, the rung-0 compare surface The Fabric plan puts this in rung 0 rather than bolting it on later, and the reason is that the constraint nobody budgets for in co-simulation is trace VOLUME, not simulation time. That is now measured rather than projected: three frames of AccuracyCoin is 89,343 CPU cycles, which is 5,372,427 bytes of irq.csv against 352 bytes of ckpt.bin. A factor of 15,263, on a real export. So both sides chain a hash over the per-cycle tuple and compare checkpoints every 4096 cycles; the first mismatch names a 4096-cycle window and only that window is re-run with full capture. WHAT IS HASHED IS A DECISION ABOUT HARDWARE, NOT ABOUT CONVENIENCE CycleRecord carries 29 fields and most of them are RustyNES's MODEL -- dmc_abort_delay_post, apu_phase_post, dma_cycles_owed. Gating on those would force an independent implementation to transliterate a Rust data structure, which is bad hardware, and on a programme built on never reading a reference implementation it is an odd form of self-derivation. checkpoint::Observable is the subset a device-under-test can genuinely produce, and Observable::from_cycle_record is the single place the partition is applied, so widening it has to pass a test that perturbs EVERY dropped field at once and asserts the hash does not move. Its converse is there too, because otherwise that test passes just as well if the projection drops everything. Two members needed their caveats stated rather than buried. THE IRQ LINE IS ONE WIRE. CycleRecord attributes each sample to the mapper or the APU; hardware has a single wire-OR'd /IRQ pin and cannot make that distinction, so the pairs are OR'd before hashing. Hashing them apart would fail a correct DUT for disagreeing about something it has no way to observe. pc IS DUT-OBSERVABLE, NOT PIN-OBSERVABLE. The 6502 does not expose its program counter. It is in because the testbench wrapper can expose the register and rung 1 compares it directly, but a pc-only mismatch means something weaker than a bus mismatch. a12_events is excluded for SCOPE, not observability -- A12 transitions really are visible on the cartridge connector -- and becomes a gate when the PPU rung opens rather than being dropped. THE HASH IS FNV-1a 64 FOR EXACTLY ONE REASON A C++ testbench must be able to reimplement it without a library. The top risk at this rung is a format-packing mismatch masquerading as an RTL bug, so encode() fixes a 16-byte little-endian layout with an explicit zero pad byte -- the C++ side cannot hash uninitialised struct padding, which is the classic way two correct implementations of the same layout disagree -- and both the layout and the resulting hash are pinned to a hardcoded vector. A reordered field fails that test rather than producing a phantom RTL defect on the next run. THREE ANSWERS, AND THE THIRD IS THE POINT checkpoint_diff exits 0 identical, 1 diverged with the window printed, 2 usage/IO, and 3 INCONCLUSIVE. A truncated run, a DUT that stopped early, and two runs at different intervals all produce "no divergence was found", and reporting that as agreement is this project's recurring failure. Cycle ALIGNMENT is checked before the hash, because two streams checkpointing at different cycles cover different spans, so calling their difference a divergence would send a full-capture re-run at a window where nothing is wrong. All three are demonstrated on real exported output, not only in unit tests: identical -> "checkpoints match: 22 compared, 0 divergences" exit 0 one bit -> "DIVERGED at checkpoint 7 ... cycles (28679, 32775]" exit 1 truncated -> "INCONCLUSIVE (this is not a pass): ... one side stopped early" exit 3 TWO HAZARDS FOUND WHILE BUILDING IT IrqTrace::push SILENTLY DROPS records once it reaches the capacity it was armed with, behind an overflow counter nobody has to read. A hash over an overflowed trace covers fewer cycles than it claims, and the two sides then disagree for a reason that has nothing to do with the DUT -- which is worse than useless, because it looks like a legitimate divergence. take_checkpoints now refuses with CheckpointError::TraceOverflowed naming the capacity to retry with, rn_write_checkpoints returns -5, and the exporter aborts rather than writing a short stream. Bus::take_irq_trace MOVES the trace out, so asking for the CSV and then the checkpoints returns None for whichever came second -- and None there is indistinguishable from "never armed", which the exporter would have reported as a missing-output warning rather than as the ordering bug it is. Oracle::take_irq_artifacts derives both from one take, and the hazard is pinned by a test so it stays a documented behaviour. Also fixed, and it is not cosmetic: THE EXCLUDED CRATE'S LOCKFILE WAS SILENTLY GITIGNORED. .gitignore carries a bare `Cargo.lock`, which matches at any depth, paired with a `!/Cargo.lock` re-include naming only the workspace root -- written when there was exactly one lockfile. Excluding rustynes-cosim from the workspace in v2.4.1 gave it its own resolve and its own lockfile, which the bare rule then ignored, so CI re-resolved its dependency graph on every run. It matters more here than for an ordinary crate: this crate emits the goldens an external NES implementation is verified against, and its manifest records the emulator version rather than the resolve, so a dependency moving underneath it would be invisible in exactly the artifact whose job is to establish provenance. cosim_manifest_audit.rs now asserts the lockfile is TRACKED rather than merely present, demonstrated to fail by un-tracking it and re-running. Gates. The emulation core is untouched -- no file under crates/rustynes-{cpu,ppu,apu,mappers,core} changes -- so AccuracyCoin 141/141 and nestest 0-diff hold by construction rather than by re-measurement. fmt (workspace AND the excluded crate, which --all does not reach) clippy --workspace --all-targets, clippy on the excluded crate, both wasm32 invocations, the no_std thumbv7em build, rustdoc -D warnings for the workspace AND the excluded crate, markdownlint on both changed documents. 126 workspace suites / 2223 passed / 0 failed 4 excluded-crate suites / 35 passed / 0 failed One rustdoc note worth recording: an intra-doc link to a `#[cfg(test)]` item is a BROKEN link under -D warnings, because test items are not in the documented tree. Three such links were written and all three are now plain code spans. Same shape as the existing rule about linking to a feature-gated dependency. --- .gitignore | 10 + CHANGELOG.md | 82 ++ crates/rustynes-cosim/Cargo.lock | 208 +++++ .../rustynes-cosim/src/bin/checkpoint_diff.rs | 89 ++ .../src/bin/nes_golden_export.rs | 45 +- crates/rustynes-cosim/src/checkpoint.rs | 828 ++++++++++++++++++ crates/rustynes-cosim/src/lib.rs | 257 ++++++ .../tests/cosim_manifest_audit.rs | 53 ++ docs/mister.md | 89 +- 9 files changed, 1655 insertions(+), 6 deletions(-) create mode 100644 crates/rustynes-cosim/Cargo.lock create mode 100644 crates/rustynes-cosim/src/bin/checkpoint_diff.rs create mode 100644 crates/rustynes-cosim/src/checkpoint.rs diff --git a/.gitignore b/.gitignore index c75b0155..efe3cbbe 100644 --- a/.gitignore +++ b/.gitignore @@ -12,8 +12,18 @@ crates/*/target/ **/*.rs.bk *.bench # This workspace ships a binary app — commit the lock (re-include it). +# +# `Cargo.lock` with no slash matches at ANY depth, so the re-include below has +# to name every lockfile we keep. That was written when there was exactly one. +# `crates/rustynes-cosim` is EXCLUDED from the workspace, so it resolves its own +# dependency graph into its own lockfile — which this rule was silently +# ignoring, leaving CI to re-resolve it on every run. That crate emits the +# goldens an external NES implementation is verified against, and its manifest +# records the emulator version, not the resolve, so a dependency moving under it +# would be invisible in exactly the artifact meant to establish provenance. Cargo.lock !/Cargo.lock +!/crates/rustynes-cosim/Cargo.lock # --- Native build & FFI artifacts --- *.so diff --git a/CHANGELOG.md b/CHANGELOG.md index f5ca4384..e656f434 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,88 @@ cycle-accurate core later replaced. ## [Unreleased] +### Added + +- **Rolling per-cycle hash checkpoints — the rung-0 comparison surface.** + (v2.4.2, continuing the "Fabric" line.) `crates/rustynes-cosim/src/checkpoint.rs`, + a `checkpoint_diff` CLI, an `rn_write_checkpoints` C ABI entry point, and a + `.ckpt.bin` golden. The emulation core is untouched. + + The constraint nobody budgets for in co-simulation is trace *volume*, not + simulation time, and the figure is now **measured rather than projected**: + 3 frames of AccuracyCoin is 89,343 CPU cycles, which is **5,372,427 bytes** of + `irq.csv` against **352 bytes** of `ckpt.bin` — a factor of **15,263**. So both + sides chain a hash over the per-cycle tuple and compare checkpoints; the first + mismatch names a 4096-cycle window, and only that window is re-run with full + capture. + + **What is hashed is a decision about hardware, not about convenience.** + `CycleRecord` carries 29 fields and most of them are `RustyNES`'s *model* — + `dmc_abort_delay_post`, `apu_phase_post`, `dma_cycles_owed`. Gating on them + would force an independent implementation to transliterate a Rust data + structure, which is bad hardware and, on a programme built on never reading a + reference implementation, an odd form of self-derivation. `Observable` is the + subset a device-under-test can genuinely produce, `from_cycle_record` is the + single place the partition is applied, and a test perturbs **every** dropped + field at once and asserts the hash does not move — with its converse, so it + cannot pass by dropping everything. + + Two subsets needed their caveats stated rather than buried. **The IRQ line is + one wire**: `CycleRecord` attributes each sample to the mapper or the APU, + hardware has a single wire-OR'd /IRQ pin that cannot, so the pairs are OR'd + before hashing — hashing them apart would fail a correct DUT for disagreeing + about something it cannot observe. And **`pc` is DUT-observable, not + pin-observable**; it is in because the testbench wrapper can expose the + register, but a `pc`-only mismatch means something weaker than a bus mismatch. + `a12_events` is excluded for **scope**, not observability — A12 transitions + genuinely are visible on the cartridge connector — and becomes a gate when the + PPU rung opens. + + **The hash is FNV-1a 64 for exactly one reason: a C++ testbench can + reimplement it without a library.** The top risk at this rung is a + format-packing mismatch masquerading as an RTL bug, so `encode` fixes a + 16-byte little-endian layout with an explicit zero pad byte — the C++ side + cannot hash uninitialised struct padding — and both layout and hash are pinned + to a hardcoded vector. A reordered field fails that test rather than producing + a phantom RTL defect. + + **Three answers, and the third is the point.** `checkpoint_diff` exits `0` + identical, `1` diverged with the window printed, and **`3` inconclusive**. A + truncated run, a DUT that stopped early, and two runs at different intervals + all produce "no divergence was found"; reporting that as agreement is this + project's recurring failure. Cycle **alignment is checked before the hash**, + because two streams checkpointing at different cycles cover different spans, so + calling their difference a divergence would send a re-run at a window where + nothing is wrong. All three answers are demonstrated on real exported output, + not only in unit tests. + + Two hazards found while building it, both pinned rather than worked around. + `IrqTrace::push` **silently drops** records at capacity behind an `overflow` + counter nobody has to read, so a hash over an overflowed trace covers fewer + cycles than it claims and would be blamed on the DUT — `take_checkpoints` now + refuses with the capacity to retry with, and the exporter aborts rather than + writing a short stream. And `Bus::take_irq_trace` **moves** the trace out, so + asking for the CSV and then the checkpoints returns `None` for whichever came + second, which is indistinguishable from "never armed"; + `Oracle::take_irq_artifacts` derives both from one take. + +### Fixed + +- **The excluded crate's lockfile was silently gitignored, so CI re-resolved it + on every run.** `.gitignore` carries a bare `Cargo.lock` — which matches at any + depth — paired with a `!/Cargo.lock` re-include naming only the workspace root. + That was written when there was exactly one lockfile. Excluding + `rustynes-cosim` from the workspace in v2.4.1 gave it its own resolve and its + own lockfile, which the bare rule then ignored. + + It matters more here than for an ordinary crate: this crate emits the goldens + an external NES implementation is verified against, and its manifest records + the emulator version rather than the dependency resolve — so a dependency + moving underneath it would be invisible in exactly the artifact whose job is to + establish provenance. The lockfile is now committed, and + `cosim_manifest_audit.rs` asserts it is **tracked** rather than merely present + (demonstrated to fail by un-tracking it and re-running). + ## [2.4.1] - 2026-08-20 - "Fabric" (RustyNES as the oracle a new implementation is verified against) This release also carries **v2.4.0 "Concordance"**, which merged to `main` and was never diff --git a/crates/rustynes-cosim/Cargo.lock b/crates/rustynes-cosim/Cargo.lock new file mode 100644 index 00000000..726911f6 --- /dev/null +++ b/crates/rustynes-cosim/Cargo.lock @@ -0,0 +1,208 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +version = 4 + +[[package]] +name = "bitflags" +version = "2.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b588b76d00fde79687d7646a9b5bdf3cc0f655e0bbd080335a95d7e96f3587da" + +[[package]] +name = "block-buffer" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d2f6c7dbe95a6ed67ad9f18e57daf93a2f034c524b99fd2b76d18fdfeb6660aa" +dependencies = [ + "hybrid-array", +] + +[[package]] +name = "cfg-if" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" + +[[package]] +name = "cpufeatures" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8b2a41393f66f16b0823bb79094d54ac5fbd34ab292ddafb9a0456ac9f87d201" +dependencies = [ + "libc", +] + +[[package]] +name = "crypto-common" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ce6e4c961d6cd6c9a86db418387425e8bdeaf05b3c8bc1411e6dca4c252f1453" +dependencies = [ + "hybrid-array", +] + +[[package]] +name = "digest" +version = "0.11.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f1dd6dbb5841937940781866fa1281a1ff7bd3bf827091440879f9994983d5c2" +dependencies = [ + "block-buffer", + "crypto-common", +] + +[[package]] +name = "hybrid-array" +version = "0.4.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "707114b52a152fa7bdb290cd7cd5912d9467273b6d74e21b8d81aca1f8533f6b" +dependencies = [ + "typenum", +] + +[[package]] +name = "libc" +version = "0.2.189" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3eaf3ede3fee6db1a4c2ee091bf8a8b4dccdc6d17f656fb07896ee72867612f2" + +[[package]] +name = "libm" +version = "0.2.16" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6d2cec3eae94f9f509c767b45932f1ada8350c4bdb85af2fcab4a3c14807981" + +[[package]] +name = "lz4_flex" +version = "0.14.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ecbdfe44b1bd960b68170b417450a628c43f7cf56bb3c5317e61cb230ee7f226" + +[[package]] +name = "proc-macro2" +version = "1.0.107" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "985e7ec9bb745e6ce6535b544d84d6cd6f7ad8bd711c398938ae983b91a766d9" +dependencies = [ + "unicode-ident", +] + +[[package]] +name = "quote" +version = "1.0.47" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1fbf4db142a473a8d80c26bbf18454ed458bf8d26c8219c331daecfdbd079001" +dependencies = [ + "proc-macro2", +] + +[[package]] +name = "rustynes-apu" +version = "2.4.1" +dependencies = [ + "bitflags", + "libm", + "thiserror", +] + +[[package]] +name = "rustynes-core" +version = "2.4.1" +dependencies = [ + "bitflags", + "lz4_flex", + "rustynes-apu", + "rustynes-cpu", + "rustynes-mappers", + "rustynes-ppu", + "sha2", + "thiserror", +] + +[[package]] +name = "rustynes-cosim" +version = "2.4.1" +dependencies = [ + "rustynes-core", + "sha2", +] + +[[package]] +name = "rustynes-cpu" +version = "2.4.1" +dependencies = [ + "bitflags", + "thiserror", +] + +[[package]] +name = "rustynes-mappers" +version = "2.4.1" +dependencies = [ + "bitflags", + "rustynes-apu", + "thiserror", +] + +[[package]] +name = "rustynes-ppu" +version = "2.4.1" +dependencies = [ + "bitflags", + "libm", + "thiserror", +] + +[[package]] +name = "sha2" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "446ba717509524cb3f22f17ecc096f10f4822d76ab5c0b9822c5f9c284e825f4" +dependencies = [ + "cfg-if", + "cpufeatures", + "digest", +] + +[[package]] +name = "syn" +version = "3.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "53e9bae58849f64dfa4f5d5ae372c8341f7305f82a3868709269343628b659a3" +dependencies = [ + "proc-macro2", + "quote", + "unicode-ident", +] + +[[package]] +name = "thiserror" +version = "2.0.20" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ec86235f5fcc2a73650310756d2ac5b138a5780bbbdfae3eeccec992c435ba4f" +dependencies = [ + "thiserror-impl", +] + +[[package]] +name = "thiserror-impl" +version = "2.0.20" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bc04cd3e1236dd4a98afca4569f2deb3f120e5422a4023be2cb683f8486292af" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "typenum" +version = "1.20.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6f5e870be6c3b371b77fe0ee0bafb859fa4964b4404c27de1d380043c4dda20" + +[[package]] +name = "unicode-ident" +version = "1.0.24" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e6e4313cd5fcd3dad5cafa179702e2b244f760991f45397d14d4ebf38247da75" diff --git a/crates/rustynes-cosim/src/bin/checkpoint_diff.rs b/crates/rustynes-cosim/src/bin/checkpoint_diff.rs new file mode 100644 index 00000000..8746329f --- /dev/null +++ b/crates/rustynes-cosim/src/bin/checkpoint_diff.rs @@ -0,0 +1,89 @@ +//! Compare two checkpoint streams and locate the window a divergence is in. +//! +//! ```text +//! checkpoint_diff +//! ``` +//! +//! Exit status is the whole interface, because this runs in CI on both sides of +//! the boundary: +//! +//! | code | meaning | +//! |---|---| +//! | `0` | every checkpoint agreed | +//! | `1` | the streams diverged; the window is printed | +//! | `2` | usage or I/O error | +//! | `3` | **inconclusive** -- the comparison could not be made | +//! +//! `3` is separate from `0` on purpose and it is the point of the tool. A +//! truncated run, a DUT that stopped early, and two runs at different intervals +//! all produce "no divergence was found", and reporting that as agreement is +//! this project's recurring failure: an absence of signal read as a pass. A +//! green CI job must mean the streams were compared and matched, never that +//! there was nothing to compare. + +use std::process::ExitCode; + +use rustynes_cosim::checkpoint::{Comparison, InconclusiveReason, compare, from_bytes}; + +fn main() -> ExitCode { + let argv: Vec = std::env::args().skip(1).collect(); + let [reference, candidate] = argv.as_slice() else { + eprintln!("usage: checkpoint_diff "); + return ExitCode::from(2); + }; + + let load = |p: &str| match std::fs::read(p) { + Ok(b) => from_bytes(&b).map_err(|e| format!("{p}: {e}")), + Err(e) => Err(format!("{p}: {e}")), + }; + let (a, b) = match (load(reference), load(candidate)) { + (Ok(a), Ok(b)) => (a, b), + (Err(e), _) | (_, Err(e)) => { + eprintln!("error: {e}"); + return ExitCode::from(2); + } + }; + + match compare(&a, &b) { + Comparison::Identical { checkpoints } => { + println!("checkpoints match: {checkpoints} compared, 0 divergences"); + ExitCode::SUCCESS + } + Comparison::Diverged(d) => { + println!( + "DIVERGED at checkpoint {}\n \ + window to re-run with full capture: cycles ({}, {}] ({} cycles)\n \ + reference hash = {:#018x}\n \ + candidate hash = {:#018x}", + d.index, + d.after_cycle, + d.through_cycle, + d.window_len(), + d.reference_hash, + d.candidate_hash, + ); + ExitCode::from(1) + } + Comparison::Inconclusive { reason } => { + // Spelled out rather than printed as a debug struct: the operator + // reading this in a CI log needs to know it is NOT a pass. + let detail = match reason { + InconclusiveReason::Empty => "one side produced no checkpoints at all".to_owned(), + InconclusiveReason::LengthMismatch { + reference: r, + candidate: c, + } => format!( + "the streams agree as far as they overlap, then one ends \ + (reference {r} checkpoints, candidate {c}) -- one side stopped early" + ), + InconclusiveReason::UnalignedCycles { index } => format!( + "the cycle axes disagree at checkpoint {index}, so the hashes cover \ + different spans and cannot be compared -- different intervals, or one \ + side truncated mid-window" + ), + }; + println!("INCONCLUSIVE (this is not a pass): {detail}"); + ExitCode::from(3) + } + } +} diff --git a/crates/rustynes-cosim/src/bin/nes_golden_export.rs b/crates/rustynes-cosim/src/bin/nes_golden_export.rs index bd4ecb49..9e463a3d 100644 --- a/crates/rustynes-cosim/src/bin/nes_golden_export.rs +++ b/crates/rustynes-cosim/src/bin/nes_golden_export.rs @@ -5,6 +5,7 @@ //! ```text //! nes_golden_export --rom --out [--seed N] [--frames N] //! [--boot-trace START..END] [--irq-trace CAP] +//! [--checkpoint-interval N] //! ``` //! //! Writes, under ``: @@ -13,6 +14,7 @@ //! |---|---|---| //! | `.boot.bin` | `CpuBootTrace` binary | `cpu_boot_trace_diff` | //! | `.irq.csv` | per-cycle IRQ/bus CSV | `scripts/irq_trace_cross_diff.py` | +//! | `.ckpt.bin` | rolling per-cycle hash checkpoints | `checkpoint_diff` | //! | `.index_fb.bin` | 256x240 LE `u16` | the testbench's frame comparison | //! | `.ram.bin` | 2 KiB CPU work RAM | `accuracy_coin_catalog::decode_results` | //! | `.manifest.txt` | provenance | humans, and the drift guard below | @@ -43,12 +45,14 @@ struct Args { frames: u32, boot_trace: Option<(u64, u64)>, irq_trace: Option, + checkpoint_interval: u64, } fn usage() -> ! { eprintln!( "usage: nes_golden_export --rom --out [--seed N] [--frames N]\n\ - \x20 [--boot-trace START..END] [--irq-trace CAP]" + \x20 [--boot-trace START..END] [--irq-trace CAP]\n\ + \x20 [--checkpoint-interval N]" ); std::process::exit(2) } @@ -58,6 +62,7 @@ fn parse_args() -> Args { let (mut rom, mut out) = (None, None); let (mut seed, mut frames) = (0u64, 60u32); let (mut boot_trace, mut irq_trace) = (None, None); + let mut checkpoint_interval = rustynes_cosim::checkpoint::DEFAULT_INTERVAL; let mut i = 0; while i < argv.len() { @@ -92,6 +97,14 @@ fn parse_args() -> Args { irq_trace = Some(need(i).parse().unwrap_or_else(|_| usage())); i += 2; } + "--checkpoint-interval" => { + checkpoint_interval = need(i).parse().unwrap_or_else(|_| usage()); + if checkpoint_interval == 0 { + eprintln!("--checkpoint-interval must be non-zero"); + usage(); + } + i += 2; + } _ => usage(), } } @@ -102,6 +115,7 @@ fn parse_args() -> Args { frames, boot_trace, irq_trace, + checkpoint_interval, } } @@ -214,9 +228,28 @@ fn main() { None => eprintln!(" WARNING: boot trace was armed but returned nothing"), } } + // ONE take, two artifacts. `Bus::take_irq_trace` moves the trace out, so + // asking for the CSV and then the checkpoints would silently yield an + // unarmed-looking `None` for whichever came second. + let mut checkpoint_count = 0usize; if args.irq_trace.is_some() { - match o.take_irq_trace_csv() { - Some(csv) => write(&suffixed(&base, "irq.csv"), csv.as_bytes()), + match o.take_irq_artifacts(args.checkpoint_interval) { + Some(a) => { + write(&suffixed(&base, "irq.csv"), a.csv.as_bytes()); + match a.checkpoints { + Ok(ck) => { + checkpoint_count = ck.len(); + write( + &suffixed(&base, "ckpt.bin"), + &rustynes_cosim::checkpoint::to_bytes(&ck), + ); + } + // Refuse rather than emitting a short stream: a hash over a + // trace that dropped records covers fewer cycles than it + // claims, and the DUT would be blamed for our truncation. + Err(e) => panic!(" ERROR: {e}"), + } + } None => eprintln!(" WARNING: irq trace was armed but returned nothing"), } } @@ -231,7 +264,9 @@ fn main() { cpu_cycles = {}\n\ emulator = rustynes {}\n\ index_fb_len = {}\n\ - ram_len = {}\n", + ram_len = {}\n\ + ckpt_interval= {}\n\ + ckpt_count = {}\n", args.rom.display(), sha256_hex(&rom), args.seed, @@ -242,6 +277,8 @@ fn main() { env!("CARGO_PKG_VERSION"), INDEX_FB_LEN, RAM_LEN, + args.checkpoint_interval, + checkpoint_count, ); write(&suffixed(&base, "manifest.txt"), manifest.as_bytes()); println!("done; {cycles} CPU cycles simulated"); diff --git a/crates/rustynes-cosim/src/checkpoint.rs b/crates/rustynes-cosim/src/checkpoint.rs new file mode 100644 index 00000000..adfd1c10 --- /dev/null +++ b/crates/rustynes-cosim/src/checkpoint.rs @@ -0,0 +1,828 @@ +//! Rolling per-cycle hash checkpoints -- the rung-0 comparison surface. +//! +//! # Why a hash and not a trace +//! +//! The constraint nobody budgets for in co-simulation is trace *volume*, not +//! simulation time. A 4200-frame `AccuracyCoin` run is roughly 125 million CPU +//! cycles; as per-cycle CSV that is about **7.5 GB** per side, which is not a +//! diff, it is a disk-space problem wearing a diff's clothes. The same run as +//! 4096-cycle checkpoints is **244 KB**. +//! +//! So the protocol is **hash first, capture on divergence**: both sides chain a +//! 64-bit hash over the per-cycle observable tuple and emit it every +//! [`DEFAULT_INTERVAL`] cycles. A comparison of the two checkpoint streams +//! answers "do these agree, and if not, in which 4096-cycle window did they +//! stop agreeing" -- which is exactly the input a full-capture re-run needs, and +//! nothing more. The re-run then costs one window, not one campaign. +//! +//! # What is hashed, and what deliberately is not +//! +//! This is the load-bearing decision in the module, and it is a decision about +//! *hardware*, not about convenience. +//! +//! `rustynes-core`'s [`CycleRecord`] carries 29 fields. Most of them are +//! `RustyNES`'s **model-internal** state -- `dmc_abort_delay_post`, +//! `apu_phase_post`, `dma_cycles_owed` and the rest describe how *this* +//! emulator chose to represent DMC and DMA bookkeeping. They are not hardware +//! facts. Gating on them would force an independent implementation to +//! transliterate a Rust data structure, which is bad hardware and, given that +//! the whole programme is built on never reading a reference implementation, an +//! odd form of self-derivation. +//! +//! [`Observable`] is therefore the subset an external device-under-test can +//! genuinely produce, and every field in it has a reason: +//! +//! | field | why it is in | +//! |---|---| +//! | `cpu_cycle` | the axis both sides count on; a desync here is the finding | +//! | `bus_access` | the R/W and DMA character of the cycle -- pin-visible | +//! | `bus_addr`, `bus_data` | the address and data buses -- pin-visible | +//! | `put_cycle` | the phase half of the M2 cycle -- pin-visible | +//! | `nmi_line` | a pin | +//! | `irq_line_at_low`, `irq_line_at_high` | the /IRQ pin, sampled twice per cycle | +//! | `pc` | **not** pin-visible; see below | +//! +//! Two of those need their caveats stated rather than buried. +//! +//! **The IRQ line is one wire.** `CycleRecord` splits its IRQ samples into +//! `irq_pending_mapper_*` and `irq_pending_apu_*`, which is `RustyNES` +//! *attributing* the assertion to a source. Real hardware has a single +//! wire-OR'd /IRQ input and cannot make that distinction, so [`Observable`] +//! folds the pair with OR before hashing. Hashing them separately would fail a +//! correct DUT for disagreeing about something it has no way to observe. +//! +//! **`pc` is DUT-observable, not pin-observable.** The 6502 does not expose its +//! program counter; the value is recoverable only indirectly, from the address +//! bus during an opcode fetch. It is included because the testbench wrapper +//! *can* expose the internal register and because rung 1 compares it directly +//! -- but a `pc`-only mismatch means something weaker than a bus mismatch, and +//! [`Divergence`] reports which fields differ so the two are never confused. +//! +//! PPU fields (`ppu_scanline`, `ppu_dot`, `ppu_frame`, `a12_events`) are +//! excluded on purpose: they belong to the PPU rung and its own pre-palette +//! framebuffer gate, not to a CPU-level bus comparison. `a12_events` is the +//! sharpest case -- A12 transitions are genuinely observable on the cartridge +//! connector, so it is excluded for *scope*, not for observability, and it +//! becomes a gate when the PPU rung opens rather than being dropped. +//! +//! # The hash must be reimplementable in ten lines of C++ +//! +//! The top risk at rung 0 is a **format-packing mismatch masquerading as an RTL +//! bug** -- the testbench packs a field differently, the streams disagree, and +//! the disagreement is read as a hardware defect. Two things guard against it. +//! +//! First, the hash is **FNV-1a 64**, chosen for exactly one property: a C++ +//! testbench can reimplement it correctly from the constants below without a +//! library, and be checked against a fixed vector. Nothing here needs +//! cryptographic strength; it needs to be trivially portable and to detect a +//! flipped bit. +//! +//! Second, [`Observable::encode`] defines a **fixed 16-byte little-endian +//! layout** and `tests::the_wire_encoding_is_pinned_to_a_fixed_vector` pins it +//! to a hardcoded expected hash. If a future refactor reorders a field, that +//! test fails rather than the next co-simulation run reporting a phantom RTL +//! defect. +//! +//! [`CycleRecord`]: rustynes_core::irq_trace::CycleRecord + +use rustynes_core::irq_trace::{BusAccess, CycleRecord}; + +/// FNV-1a 64-bit offset basis. Public because the C++ testbench needs it. +pub const FNV_OFFSET_BASIS: u64 = 0xcbf2_9ce4_8422_2325; + +/// FNV-1a 64-bit prime. Public because the C++ testbench needs it. +pub const FNV_PRIME: u64 = 0x0000_0100_0000_01b3; + +/// Cycles between emitted checkpoints. +/// +/// 4096 is a compromise with both ends measured rather than a round number: +/// a full `AccuracyCoin` run is ~125 M cycles, so this yields ~30 500 +/// checkpoints (244 KB at 8 bytes each), while the window a divergence +/// localises to stays small enough that a full-capture re-run of it is +/// instant. +pub const DEFAULT_INTERVAL: u64 = 4096; + +/// Bytes of the fixed wire encoding of one [`Observable`]. +pub const ENCODED_LEN: usize = 16; + +/// The per-cycle tuple an external implementation can genuinely produce. +/// +/// See the module docs for why each field is in, and for the two whose +/// observability is weaker than the rest (`pc`, and the folded IRQ line). +// Four bools, and clippy is right that that is usually a smell. Here each one +// is a distinct hardware line -- R/W phase, /NMI, and the two /IRQ samples -- +// so folding them into a bitfield would hide exactly what the struct exists to +// name, and the wire encoding already packs them into one byte. +#[allow(clippy::struct_excessive_bools)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Observable { + /// The cycle axis both sides count on. + pub cpu_cycle: u64, + /// Program counter. DUT-observable, **not** pin-observable. + pub pc: u16, + /// Address bus for this cycle. + pub bus_addr: u16, + /// Data bus for this cycle. + pub bus_data: u8, + /// Bus character, encoded as [`Self::access_code`]. + pub bus_access: u8, + /// The R/W phase half of the M2 cycle. + pub put_cycle: bool, + /// The /NMI pin. + pub nmi_line: bool, + /// The single wire-OR'd /IRQ pin, sampled on the low half of the cycle. + pub irq_line_at_low: bool, + /// The same pin, sampled on the high half. + pub irq_line_at_high: bool, +} + +impl Observable { + /// Stable numeric code for a bus access. + /// + /// Deliberately numeric rather than the CSV writer's single letters: the + /// C++ side packs an integer, and reusing the letters would invite a + /// signed-char mismatch on the DMA codes, which are the lowercase ones. + /// + /// `0`=idle, `1`=read, `2`=write, `3`=DMA read, `4`=DMA write. + #[must_use] + pub const fn access_code(idle: bool, write: bool, dma: bool) -> u8 { + if idle { + 0 + } else if dma { + if write { 4 } else { 3 } + } else if write { + 2 + } else { + 1 + } + } + + /// Project a `RustyNES` [`CycleRecord`] onto the observable subset. + /// + /// **This function is where the partition lives.** Every field it drops is + /// dropped on purpose, and the module docs give the reason; a future + /// widening of [`Observable`] has to come through here, where + /// `tests::model_internal_state_cannot_cause_a_divergence` is watching. + /// + /// Note the IRQ fold: `CycleRecord` attributes each sample to the mapper or + /// the APU, and hardware has one wire-OR'd /IRQ pin that cannot make that + /// distinction, so the pairs are OR'd rather than carried separately. + #[must_use] + pub const fn from_cycle_record(r: &CycleRecord) -> Self { + Self { + cpu_cycle: r.cpu_cycle, + pc: r.pc, + bus_addr: r.bus_addr, + bus_data: r.bus_data, + bus_access: match r.bus_access { + BusAccess::Idle => 0, + BusAccess::Read => 1, + BusAccess::Write => 2, + BusAccess::DmaRead => 3, + BusAccess::DmaWrite => 4, + }, + put_cycle: r.put_cycle_post, + nmi_line: r.nmi_line, + irq_line_at_low: r.irq_pending_mapper_at_low || r.irq_pending_apu_at_low, + irq_line_at_high: r.irq_pending_mapper_at_high || r.irq_pending_apu_at_high, + } + } + + /// The fixed little-endian wire encoding, pinned by a test. + /// + /// Layout, byte offsets: `cpu_cycle` 0..8, `pc` 8..10, `bus_addr` 10..12, + /// `bus_data` 12, `bus_access` 13, flags 14, pad 15. The trailing pad byte + /// is explicit and always zero so the C++ side cannot accidentally hash + /// uninitialised struct padding -- the classic way two correct + /// implementations of the same layout disagree. + #[must_use] + pub const fn encode(&self) -> [u8; ENCODED_LEN] { + let c = self.cpu_cycle.to_le_bytes(); + let p = self.pc.to_le_bytes(); + let a = self.bus_addr.to_le_bytes(); + let flags = (self.put_cycle as u8) + | ((self.nmi_line as u8) << 1) + | ((self.irq_line_at_low as u8) << 2) + | ((self.irq_line_at_high as u8) << 3); + [ + c[0], + c[1], + c[2], + c[3], + c[4], + c[5], + c[6], + c[7], + p[0], + p[1], + a[0], + a[1], + self.bus_data, + self.bus_access, + flags, + 0, + ] + } +} + +/// One emitted checkpoint: the hash of every cycle up to and including +/// `through_cycle`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Checkpoint { + /// The last cycle folded into `hash`. + pub through_cycle: u64, + /// The chained FNV-1a state at that point. + pub hash: u64, +} + +/// Chains the per-cycle hash and emits a [`Checkpoint`] every `interval` +/// cycles. +#[derive(Debug, Clone)] +pub struct Hasher { + state: u64, + interval: u64, + counted: u64, + last_cycle: u64, + checkpoints: Vec, +} + +impl Hasher { + /// A hasher emitting every `interval` cycles. + /// + /// # Panics + /// + /// If `interval` is zero. A zero interval would emit a checkpoint per cycle + /// and reproduce the 7.5 GB problem this module exists to avoid, so it is + /// rejected loudly rather than silently clamped. + #[must_use] + pub fn new(interval: u64) -> Self { + assert!(interval > 0, "checkpoint interval must be non-zero"); + Self { + state: FNV_OFFSET_BASIS, + interval, + counted: 0, + last_cycle: 0, + checkpoints: Vec::new(), + } + } + + /// Fold one cycle in, emitting a checkpoint when the interval is reached. + pub fn push(&mut self, o: &Observable) { + for b in o.encode() { + self.state ^= u64::from(b); + self.state = self.state.wrapping_mul(FNV_PRIME); + } + self.last_cycle = o.cpu_cycle; + self.counted += 1; + if self.counted.is_multiple_of(self.interval) { + self.checkpoints.push(Checkpoint { + through_cycle: o.cpu_cycle, + hash: self.state, + }); + } + } + + /// Emit a final checkpoint for a partial trailing window. + /// + /// Without this, a run whose length is not a multiple of the interval + /// silently drops its tail -- and the tail is where a run that was stopped + /// early because something went wrong actually differs. Returns the + /// complete stream. + #[must_use] + pub fn finish(mut self) -> Vec { + if !self.counted.is_multiple_of(self.interval) && self.counted > 0 { + self.checkpoints.push(Checkpoint { + through_cycle: self.last_cycle, + hash: self.state, + }); + } + self.checkpoints + } + + /// Cycles folded so far. + #[must_use] + pub const fn counted(&self) -> u64 { + self.counted + } +} + +/// The result of comparing two checkpoint streams. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Comparison { + /// Every checkpoint agreed, and both streams were the same length. + Identical { + /// How many checkpoints were compared. + checkpoints: usize, + }, + /// The streams diverged. The window to re-run with full capture is + /// `(after_cycle, through_cycle]`. + Diverged(Divergence), + /// The streams cannot be compared, and this is **not** agreement. + /// + /// A truncated run, a DUT that stopped early, or two runs at different + /// intervals all land here. Reporting them as `Identical` would be the + /// project's recurring failure -- an absence of signal read as a pass. + Inconclusive { + /// Why the comparison could not be made. + reason: InconclusiveReason, + }, +} + +/// Why a comparison could not be made. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum InconclusiveReason { + /// One side produced no checkpoints at all. + Empty, + /// The streams agree as far as the shorter one goes, then one ends. + LengthMismatch { + /// Checkpoints in the reference stream. + reference: usize, + /// Checkpoints in the candidate stream. + candidate: usize, + }, + /// The two streams checkpoint at different cycles, so their hashes cover + /// different spans and cannot be compared at all. + UnalignedCycles { + /// The index at which the cycle axes first disagree. + index: usize, + }, +} + +/// Where two checkpoint streams stopped agreeing. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Divergence { + /// Index of the first differing checkpoint. + pub index: usize, + /// Last cycle known to agree. Zero when the very first window differs. + pub after_cycle: u64, + /// Last cycle of the first differing window. + pub through_cycle: u64, + /// The reference hash at that checkpoint. + pub reference_hash: u64, + /// The candidate hash at that checkpoint. + pub candidate_hash: u64, +} + +impl Divergence { + /// Cycles in the window a full-capture re-run must cover. + #[must_use] + pub const fn window_len(&self) -> u64 { + self.through_cycle - self.after_cycle + } +} + +/// Compare two checkpoint streams. +/// +/// Answers three things and never collapses them: they agree, they diverge at a +/// located window, or the question could not be answered. +#[must_use] +pub fn compare(reference: &[Checkpoint], candidate: &[Checkpoint]) -> Comparison { + if reference.is_empty() || candidate.is_empty() { + return Comparison::Inconclusive { + reason: InconclusiveReason::Empty, + }; + } + + let common = reference.len().min(candidate.len()); + for i in 0..common { + // Cycle alignment is checked BEFORE the hash. Two streams checkpointing + // at different cycles cover different spans, so a hash difference + // between them says nothing -- reporting it as a divergence would send + // a re-run at a window that is not where the problem is. + if reference[i].through_cycle != candidate[i].through_cycle { + return Comparison::Inconclusive { + reason: InconclusiveReason::UnalignedCycles { index: i }, + }; + } + if reference[i].hash != candidate[i].hash { + return Comparison::Diverged(Divergence { + index: i, + after_cycle: if i == 0 { + 0 + } else { + reference[i - 1].through_cycle + }, + through_cycle: reference[i].through_cycle, + reference_hash: reference[i].hash, + candidate_hash: candidate[i].hash, + }); + } + } + + if reference.len() != candidate.len() { + return Comparison::Inconclusive { + reason: InconclusiveReason::LengthMismatch { + reference: reference.len(), + candidate: candidate.len(), + }, + }; + } + Comparison::Identical { + checkpoints: common, + } +} + +/// Serialize a checkpoint stream: repeated `(u64 through_cycle, u64 hash)`, +/// little-endian, no header. +/// +/// Headerless on purpose. The manifest already records the ROM hash, the seed +/// and the frame count, and a second place to state them is a second place for +/// them to disagree. +#[must_use] +pub fn to_bytes(checkpoints: &[Checkpoint]) -> Vec { + let mut out = Vec::with_capacity(checkpoints.len() * 16); + for c in checkpoints { + out.extend_from_slice(&c.through_cycle.to_le_bytes()); + out.extend_from_slice(&c.hash.to_le_bytes()); + } + out +} + +/// Parse a checkpoint stream written by [`to_bytes`]. +/// +/// # Errors +/// +/// If the length is not a multiple of 16. A trailing partial record means the +/// producer was interrupted, and a truncated stream that parses is a truncated +/// comparison that passes. +/// +/// # Panics +/// +/// Never in practice: the `expect`s convert 8-byte subslices of a +/// `chunks_exact(16)` chunk, which the length check above has already +/// guaranteed. They are `expect` rather than `unwrap_or_default` so a future +/// change to the record width fails loudly instead of silently zero-filling. +pub fn from_bytes(bytes: &[u8]) -> Result, &'static str> { + if !bytes.len().is_multiple_of(16) { + return Err("checkpoint stream length is not a multiple of 16 bytes"); + } + Ok(bytes + .chunks_exact(16) + .map(|c| Checkpoint { + through_cycle: u64::from_le_bytes(c[0..8].try_into().expect("8 bytes")), + hash: u64::from_le_bytes(c[8..16].try_into().expect("8 bytes")), + }) + .collect()) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A deterministic `CycleRecord` for the projection tests. + fn record(cycle: u64) -> CycleRecord { + CycleRecord { + cpu_cycle: cycle, + pc: 0xC000, + ppu_scanline: 0, + ppu_dot: 0, + ppu_frame: 0, + irq_pending_mapper_at_low: false, + irq_pending_apu_at_low: false, + irq_pending_mapper_at_high: false, + irq_pending_apu_at_high: false, + nmi_line: false, + a12_events: Vec::new(), + dmc_dma_pending_pre: false, + dmc_dma_pending_post: false, + dmc_dma_short_post: false, + dmc_abort_pending_post: false, + dmc_abort_delay_post: 0, + dmc_dma_cooldown_post: 0, + dmc_dma_delay_post: 0, + apu_phase_post: false, + in_dmc_dma: false, + dma_cycles_owed: 0, + bus_access: BusAccess::Read, + bus_addr: 0x8000, + bus_data: 0xEA, + put_cycle_post: false, + dmc_timer_post: 0, + dmc_bits_remaining_post: 0, + dmc_silence_post: false, + dmc_buffer_full_post: false, + } + } + + fn obs(cycle: u64) -> Observable { + Observable { + cpu_cycle: cycle, + pc: 0xC000u16.wrapping_add(u16::try_from(cycle & 0xFFFF).expect("masked")), + bus_addr: 0x8000u16.wrapping_add(u16::try_from(cycle & 0xFFFF).expect("masked")), + bus_data: u8::try_from(cycle & 0xFF).expect("masked"), + bus_access: Observable::access_code(false, cycle.is_multiple_of(3), false), + put_cycle: cycle.is_multiple_of(2), + nmi_line: false, + irq_line_at_low: cycle.is_multiple_of(7), + irq_line_at_high: cycle.is_multiple_of(11), + } + } + + fn stream(n: u64, interval: u64) -> Vec { + let mut h = Hasher::new(interval); + for c in 0..n { + h.push(&obs(c)); + } + h.finish() + } + + /// The C++ testbench reimplements this encoding and this hash. If either + /// moves, the next co-simulation run reports a divergence that is ours, not + /// the DUT's -- so both are pinned to a fixed value rather than to whatever + /// the current code happens to produce. + #[test] + fn the_wire_encoding_is_pinned_to_a_fixed_vector() { + let o = Observable { + cpu_cycle: 0x0102_0304_0506_0708, + pc: 0xC5F5, + bus_addr: 0x8123, + bus_data: 0xA9, + bus_access: 1, + put_cycle: true, + nmi_line: false, + irq_line_at_low: true, + irq_line_at_high: false, + }; + assert_eq!( + o.encode(), + [ + 0x08, + 0x07, + 0x06, + 0x05, + 0x04, + 0x03, + 0x02, + 0x01, + 0xF5, + 0xC5, + 0x23, + 0x81, + 0xA9, + 0x01, + 0b0000_0101, + 0x00, + ], + "wire layout changed; the C++ writer must change with it" + ); + + let mut h = Hasher::new(1); + h.push(&o); + let ck = h.finish(); + assert_eq!(ck.len(), 1); + // Independently recomputed FNV-1a over the 16 bytes above. + let mut expect = FNV_OFFSET_BASIS; + for b in [ + 0x08u8, + 0x07, + 0x06, + 0x05, + 0x04, + 0x03, + 0x02, + 0x01, + 0xF5, + 0xC5, + 0x23, + 0x81, + 0xA9, + 0x01, + 0b0000_0101, + 0x00, + ] { + expect ^= u64::from(b); + expect = expect.wrapping_mul(FNV_PRIME); + } + assert_eq!(ck[0].hash, expect); + } + + #[test] + fn identical_streams_compare_identical() { + let a = stream(10_000, DEFAULT_INTERVAL); + let b = stream(10_000, DEFAULT_INTERVAL); + assert_eq!(compare(&a, &b), Comparison::Identical { checkpoints: 3 }); + } + + /// The gate must be shown able to FAIL, not merely to pass. + #[test] + fn a_single_flipped_bit_is_localised_to_its_window() { + let reference = stream(10_000, DEFAULT_INTERVAL); + + let mut h = Hasher::new(DEFAULT_INTERVAL); + for c in 0..10_000u64 { + let mut o = obs(c); + if c == 5000 { + o.bus_data ^= 0x01; + } + h.push(&o); + } + let candidate = h.finish(); + + match compare(&reference, &candidate) { + Comparison::Diverged(d) => { + assert_eq!(d.index, 1, "cycle 5000 falls in the second window"); + assert_eq!(d.after_cycle, 4095); + assert_eq!(d.through_cycle, 8191); + assert_eq!(d.window_len(), 4096); + assert!( + (d.after_cycle..=d.through_cycle).contains(&5000), + "the reported window must contain the corrupted cycle" + ); + } + other => panic!("expected a divergence, got {other:?}"), + } + } + + /// The partition is real, not decorative: two records differing ONLY in + /// state this module deliberately excludes must produce the same hash. + /// + /// If this fails, someone widened `Observable` to carry model state, and an + /// independent implementation will now be failed for not reproducing a Rust + /// struct rather than for disagreeing about hardware. + #[test] + fn model_internal_state_cannot_cause_a_divergence() { + let a = record(42); + let mut b = record(42); + // Every field the projection drops, perturbed at once. + b.ppu_scanline = 120; + b.ppu_dot = 200; + b.ppu_frame = 9; + b.a12_events.push(rustynes_core::irq_trace::A12Event { + sub_dot: 1, + level: true, + }); + b.dmc_dma_pending_pre = !b.dmc_dma_pending_pre; + b.dmc_dma_pending_post = !b.dmc_dma_pending_post; + b.dmc_dma_short_post = !b.dmc_dma_short_post; + b.dmc_abort_pending_post = !b.dmc_abort_pending_post; + b.dmc_abort_delay_post = 3; + b.dmc_dma_cooldown_post = 4; + b.dmc_dma_delay_post = 5; + b.apu_phase_post = !b.apu_phase_post; + b.in_dmc_dma = !b.in_dmc_dma; + b.dma_cycles_owed = 7; + b.dmc_timer_post = 400; + b.dmc_bits_remaining_post = 6; + b.dmc_silence_post = !b.dmc_silence_post; + b.dmc_buffer_full_post = !b.dmc_buffer_full_post; + + assert_eq!( + Observable::from_cycle_record(&a).encode(), + Observable::from_cycle_record(&b).encode(), + "a field outside the observable subset changed the hash" + ); + } + + /// And the converse, so the test above cannot pass by the projection + /// dropping everything. + #[test] + fn an_observable_field_does_cause_a_divergence() { + let a = record(42); + let mut b = record(42); + b.bus_addr ^= 0x0001; + assert_ne!( + Observable::from_cycle_record(&a).encode(), + Observable::from_cycle_record(&b).encode() + ); + } + + /// The two IRQ sources fold to one wire before hashing, because hardware + /// has one wire and a DUT cannot tell them apart. + #[test] + fn the_irq_sources_fold_to_a_single_line() { + let mut mapper_only = record(1); + mapper_only.irq_pending_mapper_at_low = true; + mapper_only.irq_pending_apu_at_low = false; + + let mut apu_only = record(1); + apu_only.irq_pending_mapper_at_low = false; + apu_only.irq_pending_apu_at_low = true; + + assert_eq!( + Observable::from_cycle_record(&mapper_only).encode(), + Observable::from_cycle_record(&apu_only).encode(), + "a DUT cannot tell which source asserted /IRQ, so neither may the hash" + ); + + let mut neither = record(1); + neither.irq_pending_mapper_at_low = false; + neither.irq_pending_apu_at_low = false; + assert_ne!( + Observable::from_cycle_record(&mapper_only).encode(), + Observable::from_cycle_record(&neither).encode(), + "the folded line must still distinguish asserted from not" + ); + } + + /// A run that stopped short must never read as agreement. Which *kind* of + /// inconclusive it is depends on where it stopped, and both branches are + /// exercised because only one of them was reachable by accident. + /// + /// Stopping mid-window emits a partial tail checkpoint at a cycle the + /// reference never checkpoints at, so the axes disagree before the hashes + /// are ever compared -- which is the more informative answer, and the one + /// the first version of this test guessed wrong. + #[test] + fn a_run_truncated_mid_window_is_inconclusive_not_identical() { + let reference = stream(10_000, DEFAULT_INTERVAL); + let candidate = stream(6_000, DEFAULT_INTERVAL); + assert_eq!( + compare(&reference, &candidate), + Comparison::Inconclusive { + reason: InconclusiveReason::UnalignedCycles { index: 1 } + }, + "candidate's tail checkpoint is at cycle 5999, reference's is at 8191" + ); + } + + /// Stopping exactly on an interval boundary produces aligned, agreeing + /// checkpoints and then simply runs out -- the case `LengthMismatch` exists + /// for. + #[test] + fn a_run_truncated_on_a_boundary_is_inconclusive_not_identical() { + let reference = stream(10_000, DEFAULT_INTERVAL); + let candidate = stream(8_192, DEFAULT_INTERVAL); + assert_eq!( + compare(&reference, &candidate), + Comparison::Inconclusive { + reason: InconclusiveReason::LengthMismatch { + reference: 3, + candidate: 2 + } + } + ); + } + + #[test] + fn an_empty_stream_is_inconclusive_not_identical() { + let reference = stream(10_000, DEFAULT_INTERVAL); + assert_eq!( + compare(&reference, &[]), + Comparison::Inconclusive { + reason: InconclusiveReason::Empty + } + ); + assert_eq!( + compare(&[], &reference), + Comparison::Inconclusive { + reason: InconclusiveReason::Empty + } + ); + } + + /// Different intervals cover different spans, so the hashes are not + /// comparable at all -- and saying "diverged" would send a re-run at a + /// window that is not where any problem is. + #[test] + fn streams_at_different_intervals_are_inconclusive_not_diverged() { + let a = stream(8192, 4096); + let b = stream(8192, 2048); + assert_eq!( + compare(&a, &b), + Comparison::Inconclusive { + reason: InconclusiveReason::UnalignedCycles { index: 0 } + } + ); + } + + /// A trailing partial window must still be emitted; the tail is where a run + /// stopped early actually differs. + #[test] + fn a_partial_trailing_window_is_still_emitted() { + let ck = stream(5000, DEFAULT_INTERVAL); + assert_eq!(ck.len(), 2); + assert_eq!(ck[0].through_cycle, 4095); + assert_eq!(ck[1].through_cycle, 4999, "the tail must not be dropped"); + } + + #[test] + fn the_serialised_form_round_trips() { + let ck = stream(10_000, DEFAULT_INTERVAL); + let bytes = to_bytes(&ck); + assert_eq!(bytes.len(), ck.len() * 16); + assert_eq!(from_bytes(&bytes).expect("round trip"), ck); + } + + /// A truncated stream that parses is a truncated comparison that passes. + #[test] + fn a_truncated_serialised_stream_is_rejected() { + let mut bytes = to_bytes(&stream(10_000, DEFAULT_INTERVAL)); + bytes.pop(); + assert!(from_bytes(&bytes).is_err()); + } + + #[test] + #[should_panic(expected = "checkpoint interval must be non-zero")] + fn a_zero_interval_is_rejected() { + let _ = Hasher::new(0); + } + + #[test] + fn access_codes_are_stable() { + assert_eq!(Observable::access_code(true, false, false), 0); + assert_eq!(Observable::access_code(false, false, false), 1); + assert_eq!(Observable::access_code(false, true, false), 2); + assert_eq!(Observable::access_code(false, false, true), 3); + assert_eq!(Observable::access_code(false, true, true), 4); + } +} diff --git a/crates/rustynes-cosim/src/lib.rs b/crates/rustynes-cosim/src/lib.rs index 3fc9888a..01cac63d 100644 --- a/crates/rustynes-cosim/src/lib.rs +++ b/crates/rustynes-cosim/src/lib.rs @@ -58,6 +58,8 @@ // justify each use. #![allow(unsafe_code)] +pub mod checkpoint; + use std::io; use core::ffi::{c_char, c_int, c_uchar, c_uint, c_ulonglong, c_void}; @@ -161,11 +163,124 @@ impl Oracle { } /// The per-cycle IRQ/bus trace as CSV, or `None` if unarmed. + /// + /// **Consumes the trace.** A second call returns `None`, and so does a + /// subsequent [`Self::take_checkpoints`] -- see + /// [`Self::take_irq_artifacts`] for the reason that matters. pub fn take_irq_trace_csv(&mut self) -> Option { self.nes.bus_mut().take_irq_trace().map(|t| t.to_csv()) } + + /// Both IRQ-trace artifacts from **one** take. + /// + /// # Why this exists rather than calling the two takers in sequence + /// + /// `Bus::take_irq_trace` moves the trace out. So a caller that wants the + /// CSV *and* the checkpoints gets whichever it asked for first and `None` + /// for the other -- and `None` on the checkpoint side is indistinguishable + /// from "the trace was never armed", which the exporter would report as a + /// missing-output warning rather than as the ordering bug it is. + /// + /// Pinned by `tests::taking_the_csv_first_leaves_no_trace_for_checkpoints`, + /// so the hazard is a documented behaviour rather than a surprise. + pub fn take_irq_artifacts(&mut self, interval: u64) -> Option { + let trace = self.nes.bus_mut().take_irq_trace()?; + let dropped = trace.overflow(); + let checkpoints = if dropped > 0 { + Err(CheckpointError::TraceOverflowed { + dropped, + kept: trace.len(), + }) + } else { + let mut h = checkpoint::Hasher::new(interval); + for r in trace.records() { + h.push(&checkpoint::Observable::from_cycle_record(r)); + } + Ok(h.finish()) + }; + Some(IrqArtifacts { + csv: trace.to_csv(), + checkpoints, + }) + } + + /// Rolling hash checkpoints over the observable per-cycle tuple, or `None` + /// if the IRQ trace was never armed. + /// + /// # This refuses rather than truncating + /// + /// `IrqTrace::push` **silently drops** records once the buffer reaches the + /// capacity it was armed with, advancing an `overflow` counter nobody has + /// to read. A checkpoint stream computed over a dropped-record trace is a + /// hash of *fewer cycles than it claims*, and the two sides would then + /// disagree for a reason that has nothing to do with the DUT -- the exact + /// "format-packing mismatch masquerading as an RTL bug" this rung exists to + /// rule out. Worse, it would look like a legitimate divergence. + /// + /// So an overflowed trace returns [`CheckpointError::TraceOverflowed`] with + /// the count, not a shorter stream. Raise the capacity and re-run. + /// + /// # Errors + /// + /// [`CheckpointError::TraceOverflowed`] if the trace dropped any record. + pub fn take_checkpoints( + &mut self, + interval: u64, + ) -> Option, CheckpointError>> { + let trace = self.nes.bus_mut().take_irq_trace()?; + let dropped = trace.overflow(); + if dropped > 0 { + return Some(Err(CheckpointError::TraceOverflowed { + dropped, + kept: trace.len(), + })); + } + let mut h = checkpoint::Hasher::new(interval); + for r in trace.records() { + h.push(&checkpoint::Observable::from_cycle_record(r)); + } + Some(Ok(h.finish())) + } } +/// Both artifacts derived from a single take of the per-cycle trace. +#[derive(Debug, Clone)] +pub struct IrqArtifacts { + /// The per-cycle CSV. + pub csv: String, + /// The checkpoint stream, or why it could not be produced. + pub checkpoints: Result, CheckpointError>, +} + +/// Why a checkpoint stream could not be produced. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum CheckpointError { + /// The per-cycle trace hit its capacity and dropped records, so any hash + /// over it would cover fewer cycles than it claims. + TraceOverflowed { + /// Records the trace discarded. + dropped: u64, + /// Records it kept. + kept: usize, + }, +} + +impl core::fmt::Display for CheckpointError { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + match self { + Self::TraceOverflowed { dropped, kept } => write!( + f, + "per-cycle trace overflowed: kept {kept} records and dropped {dropped}; \ + a checkpoint hash over a truncated trace would read as a DUT divergence. \ + Re-run with --irq-trace at least {}", + *kept as u64 + dropped + ), + } + } +} + +impl std::error::Error for CheckpointError {} + // --------------------------------------------------------------------------- // C ABI // --------------------------------------------------------------------------- @@ -433,8 +548,52 @@ pub unsafe extern "C" fn rn_write_cpu_boot_trace( }) } +/// Write the checkpoint stream to `path`: repeated `(u64 through_cycle, +/// u64 hash)`, little-endian, no header. +/// +/// Returns `0` on success. `-1` null handle, `-2` bad path, `-4` the trace was +/// never armed, **`-5` the trace overflowed and dropped records** (a hash over +/// it would cover fewer cycles than it claims and would read as a DUT +/// divergence -- raise the capacity and re-run), `-6` interval was zero. +/// Anything at or below `-100` is `-(100 + errno)` from the write itself. +/// +/// # Safety +/// +/// `handle` must come from [`rn_open`] and not have been closed. `path` must be +/// a valid NUL-terminated C string. +#[unsafe(no_mangle)] +pub unsafe extern "C" fn rn_write_checkpoints( + handle: *mut c_void, + path: *const c_char, + interval: c_ulonglong, +) -> c_int { + if interval == 0 { + // Rejected here rather than reaching `Hasher::new`'s assert: a panic + // across the C ABI is undefined behaviour, so the guard has to be on + // this side of the boundary. + return -6; + } + let oracle = oracle!(handle, -1); + // SAFETY: as above. + let Some(path) = (unsafe { cstr_to_path(path) }) else { + return -2; + }; + match oracle.take_checkpoints(interval) { + None => -4, + Some(Err(_)) => -5, + Some(Ok(ck)) => match std::fs::write(path, checkpoint::to_bytes(&ck)) { + Ok(()) => 0, + Err(e) => write_error_code(&e), + }, + } +} + /// Write the IRQ/bus trace CSV to `path`. Returns 0 on success, negative on error. /// +/// **Consumes the trace**, so a subsequent [`rn_write_checkpoints`] returns +/// `-4` ("never armed"). A testbench wanting both must write the checkpoints +/// first, or use the Rust-side [`Oracle::take_irq_artifacts`]. +/// /// # Safety /// /// `path` must be a NUL-terminated UTF-8 C string. @@ -492,6 +651,104 @@ mod tests { rom } + /// An unarmed trace yields no checkpoints -- and `None` is not an empty + /// stream, because an empty stream would compare `Inconclusive` while + /// `None` says the question was never asked. + #[test] + fn checkpoints_are_none_when_the_trace_was_never_armed() { + let mut o = Oracle::new(&nrom(), 0).expect("rom"); + o.advance_frames(1); + assert!(o.take_checkpoints(checkpoint::DEFAULT_INTERVAL).is_none()); + } + + /// The same run hashed twice agrees -- the null-DUT self-diff for this + /// format. Without it, a later red result is ambiguous between "the RTL is + /// wrong" and "the hasher is not a function of the trace". + #[test] + fn the_same_run_produces_the_same_checkpoints() { + let run = |seed: u64| { + let mut o = Oracle::new(&nrom(), seed).expect("rom"); + o.enable_irq_trace(200_000); + o.advance_frames(2); + o.take_checkpoints(checkpoint::DEFAULT_INTERVAL) + .expect("armed") + .expect("no overflow") + }; + let a = run(0); + let b = run(0); + assert!(!a.is_empty(), "two frames must produce checkpoints"); + assert_eq!( + checkpoint::compare(&a, &b), + checkpoint::Comparison::Identical { + checkpoints: a.len() + } + ); + } + + /// **A capacity-limited trace silently drops records**, so a hash over it + /// covers fewer cycles than it claims -- and would then read as a DUT + /// divergence rather than as our own truncation. It must refuse. + #[test] + fn an_overflowed_trace_refuses_to_produce_checkpoints() { + let mut o = Oracle::new(&nrom(), 0).expect("rom"); + o.enable_irq_trace(64); // far below one frame of CPU cycles + o.advance_frames(2); + match o.take_checkpoints(checkpoint::DEFAULT_INTERVAL) { + Some(Err(CheckpointError::TraceOverflowed { dropped, kept })) => { + assert_eq!(kept, 64); + assert!(dropped > 0, "the trace must report what it discarded"); + } + other => panic!("expected a refusal, got {other:?}"), + } + } + + /// The refusal must say how to fix itself; a testbench operator sees only + /// this string. + #[test] + fn the_overflow_error_names_the_capacity_to_retry_with() { + let e = CheckpointError::TraceOverflowed { + dropped: 100, + kept: 64, + }; + let s = e.to_string(); + assert!( + s.contains("164"), + "must name the capacity to retry with: {s}" + ); + assert!(s.contains("dropped 100"), "{s}"); + } + + /// The consuming hazard, pinned so it is a documented behaviour rather + /// than a surprise: the CSV taker moves the trace out, so a checkpoint + /// request afterwards returns `None` -- which reads exactly like "never + /// armed". + #[test] + fn taking_the_csv_first_leaves_no_trace_for_checkpoints() { + let mut o = Oracle::new(&nrom(), 0).expect("rom"); + o.enable_irq_trace(200_000); + o.advance_frames(1); + assert!(o.take_irq_trace_csv().is_some()); + assert!( + o.take_checkpoints(checkpoint::DEFAULT_INTERVAL).is_none(), + "if this ever returns Some, the trace stopped being consumed and \ + `take_irq_artifacts` can be simplified away" + ); + } + + /// The combined taker returns both from one take, which the two single + /// takers structurally cannot. + #[test] + fn the_combined_taker_returns_both_artifacts() { + let mut o = Oracle::new(&nrom(), 0).expect("rom"); + o.enable_irq_trace(200_000); + o.advance_frames(1); + let a = o + .take_irq_artifacts(checkpoint::DEFAULT_INTERVAL) + .expect("armed"); + assert!(a.csv.starts_with("cpu_cycle,")); + assert!(!a.checkpoints.expect("no overflow").is_empty()); + } + /// The power-on latch, pinned rather than worked around silently. /// /// If this test ever fails it means the core stopped swallowing the first diff --git a/crates/rustynes-test-harness/tests/cosim_manifest_audit.rs b/crates/rustynes-test-harness/tests/cosim_manifest_audit.rs index 434147bf..ff12997b 100644 --- a/crates/rustynes-test-harness/tests/cosim_manifest_audit.rs +++ b/crates/rustynes-test-harness/tests/cosim_manifest_audit.rs @@ -163,3 +163,56 @@ fn rustynes_cosim_is_excluded_from_the_workspace() { "rustynes-cosim must be listed in the workspace `exclude`" ); } + +/// The excluded crate's lockfile must be **tracked**. +/// +/// `.gitignore` carries a bare `Cargo.lock`, which matches at any depth, and a +/// `!/Cargo.lock` re-include that names only the workspace root. That pairing +/// was written when there was exactly one lockfile. Excluding +/// `rustynes-cosim` from the workspace gave it its own resolve and its own +/// lockfile, which the bare rule then silently ignored -- so CI re-resolved its +/// dependency graph on every run. +/// +/// That matters more here than for an ordinary crate: this one emits the +/// goldens an external NES implementation is verified against, and its manifest +/// records the emulator version rather than the resolve. A dependency moving +/// underneath it would be invisible in exactly the artifact whose job is to +/// establish provenance. +#[test] +fn the_excluded_crates_lockfile_is_tracked() { + let root = repo_root(); + let lock = root.join("crates/rustynes-cosim/Cargo.lock"); + assert!( + lock.is_file(), + "crates/rustynes-cosim/Cargo.lock is missing; an excluded package has \ + its own resolve and the lockfile must be committed with it" + ); + + let out = std::process::Command::new("git") + .arg("-C") + .arg(&root) + .args([ + "ls-files", + "--error-unmatch", + "crates/rustynes-cosim/Cargo.lock", + ]) + .output(); + + // A source tarball with no .git is a legitimate way to build this; skip + // rather than fail, and say so, so a skip is never mistaken for a pass. + let Ok(out) = out else { + eprintln!("skipping: git is unavailable"); + return; + }; + if !root.join(".git").exists() { + eprintln!("skipping: not a git checkout"); + return; + } + assert!( + out.status.success(), + "crates/rustynes-cosim/Cargo.lock exists but is NOT tracked by git -- \ + check the `!/crates/rustynes-cosim/Cargo.lock` re-include in .gitignore.\n\ + stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); +} diff --git a/docs/mister.md b/docs/mister.md index 413b9e29..373ae1d5 100644 --- a/docs/mister.md +++ b/docs/mister.md @@ -122,6 +122,7 @@ every golden's length. | `.irq.csv` | per-CPU-cycle IRQ/bus CSV, two samples per cycle | `scripts/irq_trace_cross_diff.py` | | `.index_fb.bin` | 256x240 little-endian `u16`, **pre-palette** | the testbench's frame comparison | | `.ram.bin` | 2 KiB CPU work RAM | `accuracy_coin_catalog::decode_results` | +| `.ckpt.bin` | rolling per-cycle hash checkpoints, `(u64 through_cycle, u64 hash)` LE, headerless | `checkpoint_diff` | | `.manifest.txt` | provenance | humans, and the drift guard below | The framebuffer is exported **pre-palette** on purpose: a palette difference must @@ -201,8 +202,92 @@ impact: zero. A 4200-frame AccuracyCoin run is ~125 M CPU cycles, which as per-cycle CSV is ~7.5 GB per side. Both sides instead chain a 64-bit hash over the per-cycle tuple and compare checkpoints every 4096 cycles - **244 KB** for a full run. On the -first mismatch, binary-search the window and re-run only that window with full -capture and waveforms. +first mismatch, re-run only that window with full capture and waveforms. + +Implemented in `crates/rustynes-cosim/src/checkpoint.rs`. **Measured on a real +export** rather than projected: 3 frames of AccuracyCoin is 89,343 CPU cycles, +which is **5,372,427 bytes** of `irq.csv` against **352 bytes** of `ckpt.bin` - +a factor of **15,263**. + +#### What is hashed, and what deliberately is not + +`CycleRecord` carries 29 fields, and most of them are *`RustyNES`'s model*, not +hardware. `checkpoint::Observable` is the subset an external device-under-test +can genuinely produce, and `Observable::from_cycle_record` is the single place +the partition is applied - so widening it has to pass +`model_internal_state_cannot_cause_a_divergence`, which perturbs every dropped +field at once and asserts the hash does not move. + +| In | Why | +|---|---| +| `cpu_cycle` | the axis both sides count on | +| `bus_access`, `bus_addr`, `bus_data` | pin-visible | +| `put_cycle` | the R/W phase half of the M2 cycle - pin-visible | +| `nmi_line` | a pin | +| `irq_line_at_low`, `irq_line_at_high` | the /IRQ pin, sampled twice per cycle | +| `pc` | **not** pin-visible; see below | + +Two caveats are stated rather than buried. + +**The IRQ line is one wire.** `CycleRecord` splits its samples into +`irq_pending_mapper_*` and `irq_pending_apu_*`, which is `RustyNES` *attributing* +the assertion to a source. Hardware has a single wire-OR'd /IRQ input and cannot +make that distinction, so the pairs are OR'd before hashing. Hashing them apart +would fail a correct DUT for disagreeing about something it cannot observe. + +**`pc` is DUT-observable, not pin-observable.** The 6502 does not expose its +program counter. It is in because the testbench wrapper can expose the internal +register and rung 1 compares it directly - but a `pc`-only mismatch means +something weaker than a bus mismatch. + +`ppu_scanline`, `ppu_dot`, `ppu_frame` and `a12_events` are out. `a12_events` is +the sharpest case: A12 transitions genuinely are observable on the cartridge +connector, so it is excluded for **scope**, not observability, and becomes a gate +when the PPU rung opens. + +#### The hash must be reimplementable in ten lines of C++ + +The top risk at rung 0 is a format-packing mismatch masquerading as an RTL bug. +So the hash is **FNV-1a 64** - chosen for exactly one property, that a testbench +can reimplement it without a library - and `Observable::encode` defines a fixed +**16-byte little-endian** layout with an explicit zero pad byte, so the C++ side +cannot hash uninitialised struct padding. Both are pinned to a hardcoded vector +by `the_wire_encoding_is_pinned_to_a_fixed_vector`; a reordered field fails that +test rather than producing a phantom RTL defect on the next co-simulation run. + +#### Three answers, and the third is the point + +`checkpoint_diff ` exits `0` identical, +`1` diverged (printing the window to re-run), `2` usage/IO, and **`3` +inconclusive**. A truncated run, a DUT that stopped early, and two runs at +different intervals all produce "no divergence was found", and reporting that as +agreement is this project's recurring failure. A green job must mean the streams +were compared and matched, never that there was nothing to compare. + +Cycle **alignment is checked before the hash**: two streams checkpointing at +different cycles cover different spans, so a hash difference between them says +nothing, and calling it a divergence would send a full-capture re-run at a window +where nothing is wrong. + +#### A capacity-limited trace refuses rather than truncating + +`IrqTrace::push` silently drops records once it reaches the capacity it was armed +with, advancing an `overflow` counter nobody has to read. A checkpoint stream +computed over a dropped-record trace hashes *fewer cycles than it claims*, and +the two sides then disagree for a reason that has nothing to do with the DUT - +which is worse than useless, because it looks like a legitimate divergence. So +`Oracle::take_checkpoints` returns `CheckpointError::TraceOverflowed` naming the +capacity to retry with, `rn_write_checkpoints` returns `-5`, and the exporter +aborts rather than writing a short stream. + +#### One take, two artifacts + +`Bus::take_irq_trace` **moves** the trace out, so asking for the CSV and then the +checkpoints yields `None` for whichever came second - and `None` is +indistinguishable from "never armed". `Oracle::take_irq_artifacts` derives both +from a single take; the hazard is pinned by +`taking_the_csv_first_leaves_no_trace_for_checkpoints` so it is a documented +behaviour rather than a surprise. ## What is a gate and what is diagnostic