From 2b0b21dd88f56385641cf6e42d27500580a0e8b6 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 22:00:41 -0400 Subject: [PATCH] feat(cosim): add the full-capture observable golden, and close the rung-0 gate The rung-0 gate is "feed RustyNES's golden back in as if it were the DUT and get 0 divergences from every diff CLI". It was not implementable for the checkpoint format, and that was found by trying to build it rather than by reading the spec. THE CSV CANNOT RE-DERIVE THE CHECKPOINTS irq.csv carries 23 columns. Neither pc nor put_cycle_post is among them -- two of the nine fields Observable folds into the hash are simply absent from the file. Verified by parsing the header against CycleRecord rather than assumed: six of the struct's 29 fields never reach the CSV, and two of those six are observable. So an external testbench reading the CSV cannot reproduce the numbers in the .ckpt.bin sitting beside it. The self-diff had nothing to run on. .obs.bin closes that: repeated 16-byte records in the SAME wire encoding the hash folds, headerless for the same reason the checkpoint stream is -- 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. Additive on purpose. The CSV is untouched, because scripts/irq_trace_cross_diff.py and the committed golden/irq_trace/*.csv both depend on its shape, and a format bump there to serve a new consumer would be paid for by every existing one. The stream is also what a re-run of a located window consumes, so it would have been needed regardless. Checkpoints say WHICH 4096-cycle window differs; the re-run needs the records in it. DECODE REFUSES WHAT IT DOES NOT UNDERSTAND Observable::decode rejects a non-zero reserved pad byte, an undefined flag bit, an unknown bus-access code, a short record, and a stream length that is not a multiple of 16. That is not pedantry. Reading a record from a newer producer as though nothing had changed is how a FORMAT divergence gets reported as a DUT divergence -- and at this rung that is the single most expensive way to be wrong, because the DUT is the thing under suspicion and the testbench is not. The stream is emitted even when the checkpoints are REFUSED for overflow. A hash over a truncated trace claims a coverage it does not have; the records themselves are just records, and are worth keeping for a re-run. MEASURED ACROSS THE REPOSITORY BOUNDARY, NOT ONLY IN UNIT TESTS $ tb/selfdiff_check.sh (in the RustyNES_MiSTer checkout) 1/3 exporting goldens from the oracle 2/3 re-deriving checkpoints on this side re-derived 22 checkpoints from 89335 records 3/3 comparing checkpoints match: 22 compared, 0 divergences negative control: one flipped bit must be located DIVERGED at checkpoint 10 window to re-run with full capture: cycles (40967, 45063] (4096 cycles) rung 0 self-diff: agreement recognised, and disagreement located 89,335 records of AccuracyCoin, re-derived in C++ from .obs.bin alone, hashing to byte-identical checkpoints. The negative control runs in the SAME invocation, because a positive control alone is satisfiable by a comparison that always agrees -- and a harness that only ever says "match" passes everything. Also verified: a malformed record and a truncated stream are both refused with exit 3 rather than hashed. nes_golden_export's main() crossed clippy's too_many_lines while this landed. Extracted write_irq_artifacts rather than allowing the lint -- main was doing argument parsing, running, and five writes, and the extracted function is where the one-take-three-artifacts constraint is now documented. 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. fmt (workspace and the excluded crate), clippy on both, rustdoc -D warnings on both, markdownlint on both changed documents. 126 workspace suites / 2223 passed / 0 failed 4 excluded-crate suites / 42 passed / 0 failed (was 39) --- CHANGELOG.md | 32 +++ .../src/bin/nes_golden_export.rs | 77 +++++--- crates/rustynes-cosim/src/checkpoint.rs | 182 ++++++++++++++++++ crates/rustynes-cosim/src/lib.rs | 50 +++++ docs/mister.md | 56 ++++++ 5 files changed, 371 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3bbdc646..e69b64af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -120,6 +120,38 @@ cycle-accurate core later replaced. an assumed `through_cycle + 1`. `checkpoint_diff` prints that window open-ended rather than as `(0, N]`. +- **`.obs.bin`, the full-capture observable golden — because the CSV + cannot re-derive the checkpoints.** Found by trying to build the rung-0 + self-diff on the CSV: `irq.csv` carries **23 columns** and neither `pc` nor + `put_cycle_post` is among them, so two of the nine observable fields are + simply absent from it. An external testbench reading the CSV therefore cannot + reproduce the checkpoint hashes, and "feed `RustyNES`'s golden back in as if + it were the DUT and get zero divergences" — the rung-0 gate — was not + implementable as designed. + + The new golden is repeated 16-byte records in the **same wire encoding the + hash folds**, headerless. It is the only artifact the checkpoints can be + independently re-derived from, and it is also the input a re-run of a located + window consumes, so it would have been needed regardless. Additive: the CSV is + untouched, which matters because `scripts/irq_trace_cross_diff.py` and the + committed `golden/irq_trace/*.csv` both depend on its shape. + + `Observable::decode` is the inverse and **refuses what it does not + understand** — a non-zero reserved pad byte, an undefined flag bit, an unknown + bus-access code, a short record, a stream length that is not a multiple of 16. + Reading a record from a newer producer as though nothing had changed is how a + *format* divergence gets reported as a *DUT* divergence. The stream is emitted + even when the checkpoints are refused for overflow: a hash over a truncated + trace claims a coverage it does not have, while the records themselves are + just records. + + Measured across the repository boundary, not only in unit tests: **89,335 + records** of AccuracyCoin, re-derived in C++ from `.obs.bin` alone, hashing to + byte-identical checkpoints — and a one-bit corruption at the halfway record + located to the 4096-cycle window containing it, in the same invocation, + because a positive control alone is satisfiable by a comparison that always + agrees. + ### Fixed - **The excluded crate's lockfile was silently gitignored, so CI re-resolved it diff --git a/crates/rustynes-cosim/src/bin/nes_golden_export.rs b/crates/rustynes-cosim/src/bin/nes_golden_export.rs index 9e463a3d..441e1b0a 100644 --- a/crates/rustynes-cosim/src/bin/nes_golden_export.rs +++ b/crates/rustynes-cosim/src/bin/nes_golden_export.rs @@ -15,6 +15,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` | +//! | `.obs.bin` | full-capture observable stream, 16-byte records | the testbench's self-diff, and a window re-run | //! | `.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 | @@ -164,6 +165,48 @@ fn write(path: &Path, bytes: &[u8]) { println!(" wrote {} ({} bytes)", path.display(), bytes.len()); } +/// Write the three artifacts derived from the per-cycle trace, and return the +/// counts the manifest records. +/// +/// One take, three 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 -- and `None` there is +/// indistinguishable from "the trace was never armed". +fn write_irq_artifacts(o: &mut Oracle, base: &Path, interval: u64) -> (usize, usize) { + let Some(a) = o.take_irq_artifacts(interval) else { + eprintln!(" WARNING: irq trace was armed but returned nothing"); + return (0, 0); + }; + write(&suffixed(base, "irq.csv"), a.csv.as_bytes()); + + // Written BEFORE the checkpoints, and outside the `Err` arm below, on + // purpose. This is the full-capture stream: it is the only artifact the + // checkpoint hashes can be independently re-derived from -- the CSV cannot, + // because it carries neither `pc` nor `put_cycle_post` -- and it is what a + // re-run of a located window consumes. An overflowed trace still holds real + // records, and those are worth keeping even when hashing them would claim a + // coverage they do not have. + let observable_count = a.observables.len(); + write( + &suffixed(base, "obs.bin"), + &rustynes_cosim::checkpoint::observables_to_bytes(&a.observables), + ); + + match a.checkpoints { + Ok(ck) => { + write( + &suffixed(base, "ckpt.bin"), + &rustynes_cosim::checkpoint::to_bytes(&ck), + ); + (ck.len(), observable_count) + } + // 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}"), + } +} + fn main() { let args = parse_args(); let rom = @@ -228,31 +271,11 @@ 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_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"), - } - } + let (checkpoint_count, observable_count) = if args.irq_trace.is_some() { + write_irq_artifacts(&mut o, &base, args.checkpoint_interval) + } else { + (0, 0) + }; let manifest = format!( "rom = {}\n\ @@ -266,7 +289,8 @@ fn main() { index_fb_len = {}\n\ ram_len = {}\n\ ckpt_interval= {}\n\ - ckpt_count = {}\n", + ckpt_count = {}\n\ + obs_count = {}\n", args.rom.display(), sha256_hex(&rom), args.seed, @@ -279,6 +303,7 @@ fn main() { RAM_LEN, args.checkpoint_interval, checkpoint_count, + observable_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 index 7f62ded7..2245bead 100644 --- a/crates/rustynes-cosim/src/checkpoint.rs +++ b/crates/rustynes-cosim/src/checkpoint.rs @@ -225,6 +225,89 @@ impl Observable { } } +impl Observable { + /// Parse one record written by [`Self::encode`]. + /// + /// The inverse exists because the **golden `.irq.csv` cannot reconstruct an + /// `Observable`** — it has 23 columns and neither `pc` nor `put_cycle_post` + /// is among them. Without a decodable observable stream there is no way for + /// an external testbench to re-derive the checkpoint hashes from a golden, + /// which is exactly the rung-0 self-diff: feed `RustyNES`'s own output back + /// in as if it were the DUT and require zero divergences. + /// + /// # Errors + /// + /// If `bytes` is not exactly [`ENCODED_LEN`] long, or if the flag byte has + /// a bit set that this version does not define. The second check is not + /// pedantry: byte 15 is a reserved pad, and a producer that starts writing + /// something there is a producer this reader no longer understands. Failing + /// loudly beats silently ignoring a field that has come to mean something. + /// + /// # Panics + /// + /// Never in practice: the `expect` converts an 8-byte subslice of a slice + /// the `try_into` above has already fixed at 16 bytes. It is an `expect` + /// rather than a fallback so a future change to the record width fails + /// loudly instead of silently decoding garbage. + pub fn decode(bytes: &[u8]) -> Result { + let b: [u8; ENCODED_LEN] = bytes + .try_into() + .map_err(|_| "observable record is not 16 bytes")?; + if b[14] & 0xF0 != 0 { + return Err("observable record has undefined flag bits set"); + } + if b[15] != 0 { + return Err("observable record has a non-zero pad byte"); + } + if b[13] > 4 { + return Err("observable record has an unknown bus-access code"); + } + Ok(Self { + cpu_cycle: u64::from_le_bytes(b[0..8].try_into().expect("8 bytes")), + pc: u16::from_le_bytes([b[8], b[9]]), + bus_addr: u16::from_le_bytes([b[10], b[11]]), + bus_data: b[12], + bus_access: b[13], + put_cycle: b[14] & 1 != 0, + nmi_line: b[14] & 2 != 0, + irq_line_at_low: b[14] & 4 != 0, + irq_line_at_high: b[14] & 8 != 0, + }) + } +} + +/// Serialize an observable stream: repeated 16-byte [`Observable::encode`] +/// records, headerless. +/// +/// This is the **full-capture** golden — the input a re-run of a located window +/// needs, and the only artifact from which the checkpoint hashes can be +/// independently re-derived. +#[must_use] +pub fn observables_to_bytes(records: &[Observable]) -> Vec { + let mut out = Vec::with_capacity(records.len() * ENCODED_LEN); + for r in records { + out.extend_from_slice(&r.encode()); + } + out +} + +/// Parse an observable stream written by [`observables_to_bytes`]. +/// +/// # Errors +/// +/// If the length is not a multiple of [`ENCODED_LEN`], or if any record is +/// malformed. A trailing partial record means the producer was interrupted, and +/// a truncated stream that parses is a truncated comparison that passes. +pub fn observables_from_bytes(bytes: &[u8]) -> Result, &'static str> { + if !bytes.len().is_multiple_of(ENCODED_LEN) { + return Err("observable stream length is not a multiple of 16 bytes"); + } + bytes + .chunks_exact(ENCODED_LEN) + .map(Observable::decode) + .collect() +} + /// One emitted checkpoint: the hash of every cycle up to and including /// `through_cycle`. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -1138,6 +1221,105 @@ mod tests { assert_eq!(first_full_capture_difference(&long, &long), None); } + /// Every record must survive `encode` -> `decode` unchanged, or the + /// self-diff is comparing a lossy copy against the original and calling the + /// difference a DUT defect. + #[test] + fn every_observable_round_trips_through_the_wire_encoding() { + for cycle in 0..512u64 { + let o = obs(cycle); + let back = Observable::decode(&o.encode()).expect("round trip"); + assert_eq!(o, back, "cycle {cycle} did not survive the round trip"); + } + // Every bus-access code, including the DMA ones the CSV writer spells + // with lowercase letters. + for code in 0..=4u8 { + let mut o = obs(1); + o.bus_access = code; + assert_eq!( + Observable::decode(&o.encode()).expect("rt").bus_access, + code + ); + } + // Every flag combination, so a bit-order slip cannot hide. + for bits in 0..16u8 { + let mut o = obs(1); + o.put_cycle = bits & 1 != 0; + o.nmi_line = bits & 2 != 0; + o.irq_line_at_low = bits & 4 != 0; + o.irq_line_at_high = bits & 8 != 0; + assert_eq!( + Observable::decode(&o.encode()).expect("rt"), + o, + "flags {bits:#06b}" + ); + } + } + + /// The decoder refuses what it does not understand rather than silently + /// ignoring it. + /// + /// Byte 15 is a reserved pad and bits 4-7 of byte 14 are undefined. A + /// producer writing something there is a producer this reader no longer + /// understands, and reading its records as if nothing had changed is how a + /// format divergence becomes a DUT divergence. + #[test] + fn the_decoder_rejects_records_it_does_not_understand() { + let good = obs(7).encode(); + assert!(Observable::decode(&good).is_ok()); + + let mut pad = good; + pad[15] = 1; + assert!(Observable::decode(&pad).is_err(), "non-zero pad accepted"); + + let mut flags = good; + flags[14] |= 0x10; + assert!( + Observable::decode(&flags).is_err(), + "undefined flag bit accepted" + ); + + let mut access = good; + access[13] = 5; + assert!( + Observable::decode(&access).is_err(), + "unknown access code accepted" + ); + + assert!( + Observable::decode(&good[..15]).is_err(), + "short record accepted" + ); + assert!( + observables_from_bytes(&good[..15]).is_err(), + "truncated stream accepted" + ); + } + + /// **The rung-0 self-diff, in miniature.** Hashing the decoded stream must + /// reproduce the checkpoints hashed from the originals — otherwise an + /// external testbench reading `.obs.bin` gets different numbers from the + /// `.ckpt.bin` beside it, and the disagreement looks like a DUT defect. + #[test] + fn checkpoints_re_derived_from_the_observable_stream_match() { + let records: Vec = (0..10_000).map(obs).collect(); + let direct = hash_stream(&records, DEFAULT_INTERVAL); + + let bytes = observables_to_bytes(&records); + assert_eq!(bytes.len(), records.len() * ENCODED_LEN); + let parsed = observables_from_bytes(&bytes).expect("parse"); + assert_eq!(parsed, records, "the stream is not a faithful copy"); + + let re_derived = hash_stream(&parsed, DEFAULT_INTERVAL); + assert_eq!( + compare(&direct, &re_derived), + Comparison::Identical { + checkpoints: direct.len() + }, + "checkpoints re-derived from the observable stream do not match" + ); + } + #[test] fn access_codes_are_stable() { assert_eq!(Observable::access_code(true, false, false), 0); diff --git a/crates/rustynes-cosim/src/lib.rs b/crates/rustynes-cosim/src/lib.rs index 01cac63d..9860c684 100644 --- a/crates/rustynes-cosim/src/lib.rs +++ b/crates/rustynes-cosim/src/lib.rs @@ -198,9 +198,15 @@ impl Oracle { } Ok(h.finish()) }; + let observables: Vec = trace + .records() + .iter() + .map(checkpoint::Observable::from_cycle_record) + .collect(); Some(IrqArtifacts { csv: trace.to_csv(), checkpoints, + observables, }) } @@ -250,6 +256,19 @@ pub struct IrqArtifacts { pub csv: String, /// The checkpoint stream, or why it could not be produced. pub checkpoints: Result, CheckpointError>, + /// The full-capture observable stream. + /// + /// **Not derivable from `csv`.** The CSV carries 23 columns and neither + /// `pc` nor `put_cycle_post` is among them, so an external testbench cannot + /// re-derive the checkpoint hashes from it -- which is exactly the rung-0 + /// self-diff. This is the artifact that makes that possible, and it is also + /// the input a full-capture re-run of a located window needs. + /// + /// Emitted even when `checkpoints` is an `Err`: an overflowed trace still + /// contains real cycles, and the records it *did* keep are worth having for + /// a re-run, even though hashing them would claim a coverage they do not + /// have. + pub observables: Vec, } /// Why a checkpoint stream could not be produced. @@ -548,6 +567,37 @@ pub unsafe extern "C" fn rn_write_cpu_boot_trace( }) } +/// Write the full-capture observable stream to `path`: repeated 16-byte +/// records in the same wire encoding the checkpoint hash folds. +/// +/// Returns `0` on success, `-1` null handle, `-2` bad path, `-4` the trace was +/// never armed. Anything at or below `-100` is `-(100 + errno)`. +/// +/// Unlike [`rn_write_checkpoints`] this does **not** refuse an overflowed +/// trace. A hash over a truncated trace claims a coverage it does not have; the +/// records themselves are simply the records, and are worth having for a +/// full-capture re-run. +/// +/// # 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_observables(handle: *mut c_void, path: *const c_char) -> c_int { + let oracle = oracle!(handle, -1); + // SAFETY: as above. + let Some(p) = (unsafe { cstr_to_path(path) }) else { + return -2; + }; + match oracle.take_irq_artifacts(checkpoint::DEFAULT_INTERVAL) { + None => -4, + Some(a) => match std::fs::write(p, checkpoint::observables_to_bytes(&a.observables)) { + Ok(()) => 0, + Err(e) => write_error_code(&e), + }, + } +} + /// Write the checkpoint stream to `path`: repeated `(u64 through_cycle, /// u64 hash)`, little-endian, no header. /// diff --git a/docs/mister.md b/docs/mister.md index 09f12ef4..2a92d30d 100644 --- a/docs/mister.md +++ b/docs/mister.md @@ -148,6 +148,7 @@ every golden's length. | `.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` | +| `.obs.bin` | full-capture observable stream, repeated 16-byte records | the testbench's self-diff, and a located-window re-run | | `.manifest.txt` | provenance | humans, and the drift guard below | The framebuffer is exported **pre-palette** on purpose: a palette difference must @@ -324,6 +325,61 @@ meant both "no prior checkpoint" and "cycle zero", so the first window read as so call sites do not reimplement a boundary that is half-open at one end and open-ended at the other. +#### The CSV cannot re-derive the checkpoints, so `.obs.bin` exists + +Found by trying to build the rung-0 self-diff on the CSV. `irq.csv` carries **23 +columns**, and neither `pc` nor `put_cycle_post` is among them - two of the nine +observable fields are simply absent. So an external testbench reading the CSV +cannot reproduce the checkpoint hashes, and "feed `RustyNES`'s golden back in as +if it were the DUT and get zero divergences" was not implementable as designed. + +`.obs.bin` closes that: repeated 16-byte records in the **same wire +encoding the hash folds**, headerless. It is the only artifact the checkpoints +can be independently re-derived from, and it is also the input a re-run of a +located window consumes - so it would have been needed regardless. + +Additive. The CSV is untouched, which matters because `scripts/irq_trace_cross_diff.py` +and the committed `golden/irq_trace/*.csv` both depend on its shape. + +`Observable::decode` is the inverse, and it **refuses what it does not +understand**: a non-zero reserved pad byte, an undefined flag bit, an unknown +bus-access code, a short record, a stream length that is not a multiple of 16. +None of that is pedantry - reading a record from a newer producer as though +nothing had changed is how a *format* divergence gets reported as a *DUT* +divergence. + +The stream is emitted even when the checkpoints are refused for overflow. A hash +over a truncated trace claims a coverage it does not have; the records +themselves are just records, and are worth keeping for a re-run. + +#### The rung-0 self-diff, measured + +The oracle's own output fed back in as though it were the DUT, across the +repository boundary: + +```console +$ tb/selfdiff_check.sh +1/3 exporting goldens from the oracle +2/3 re-deriving checkpoints on this side +re-derived 22 checkpoints from 89335 records +3/3 comparing +checkpoints match: 22 compared, 0 divergences + +negative control: one flipped bit must be located +DIVERGED at checkpoint 10 + window to re-run with full capture: cycles (40967, 45063] (4096 cycles) + +rung 0 self-diff: agreement recognised, and disagreement located +``` + +89,335 records of AccuracyCoin, re-derived in C++ from `.obs.bin` alone, hashing +to byte-identical checkpoints. The negative control runs **in the same +invocation**, because a positive control alone is satisfiable by a comparison +that always agrees. + +It is not in CI: it needs both repositories and a test ROM. It exits **77** and +says why when it cannot run - a skip that reports itself, never a silent pass. + #### A capacity-limited trace refuses rather than truncating `IrqTrace::push` silently drops records once it reaches the capacity it was armed