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