From ce8519b2dbc46bbac653e66bab90f30a9f311ecb Mon Sep 17 00:00:00 2001 From: Jacob Lifshay Date: Sun, 24 May 2026 18:45:37 -0700 Subject: [PATCH] util: add and use checked_vcd_output --- crates/cpu/src/util.rs | 160 +++++++++++++++++++++- crates/cpu/tests/rename_execute_retire.rs | 131 +++--------------- 2 files changed, 177 insertions(+), 114 deletions(-) diff --git a/crates/cpu/src/util.rs b/crates/cpu/src/util.rs index 5f9ec3c..ab402ed 100644 --- a/crates/cpu/src/util.rs +++ b/crates/cpu/src/util.rs @@ -1,8 +1,15 @@ // SPDX-License-Identifier: LGPL-3.0-or-later // See Notices.txt for copyright information -use fayalite::{expr::ops::ArrayLiteral, module::wire_with_loc, prelude::*}; -use std::num::NonZero; +use fayalite::{ + bundle::BundleType, expr::ops::ArrayLiteral, module::wire_with_loc, prelude::*, + sim::vcd::VcdWriterDecls, util::RcWriter, +}; +use std::{ + num::NonZero, + panic::Location, + path::{Path, PathBuf}, +}; pub mod array_vec; pub mod tree_reduce; @@ -310,3 +317,152 @@ impl LFSR31 { state } } + +pub struct CheckedVcdOutput { + writer: Option, + expected_path: PathBuf, + expected_contents: Result, std::io::Error)>, + location: &'static Location<'static>, +} + +impl CheckedVcdOutput { + #[must_use] + #[track_caller] + pub fn new(sim: &mut Simulation, expected_path: PathBuf) -> Self { + let writer = RcWriter::default(); + sim.add_trace_writer(VcdWriterDecls::new(writer.clone())); + Self { + writer: Some(writer), + expected_contents: std::fs::read_to_string(&expected_path).map_err(|e| { + eprintln!( + "error: failed to read expected VCD from: {}", + expected_path.display(), + ); + (std::env::current_dir().ok(), e) + }), + expected_path, + location: Location::caller(), + } + } + #[must_use] + #[track_caller] + #[doc(hidden)] + pub fn __checked_vcd_output_macro_helper( + sim: &mut Simulation, + cargo_manifest_dir: &'static str, + path: &'static str, + ) -> Self { + Self::new(sim, Path::new(cargo_manifest_dir).join(path)) + } + pub fn with_vcd_output(&self, f: impl FnOnce(&str) -> R) -> R { + let Some(writer) = &self.writer else { + unreachable!(); + }; + writer.clone().borrow(|output| { + let Ok(output) = str::from_utf8(output) else { + unreachable!("VcdWriter writes valid UTF-8"); + }; + f(output) + }) + } + #[track_caller] + pub fn finish(mut self) { + let Ok(()) = self.finish_impl(|msg| panic!("{msg}")); + } + fn finish_impl( + &mut self, + error: impl FnOnce(std::fmt::Arguments<'_>) -> E, + ) -> Result<(), E> { + let Self { + writer: Some(writer), + expected_path, + expected_contents, + location, + } = self + else { + // already finished + return Ok(()); + }; + let Ok(vcd) = String::from_utf8(writer.take()) else { + unreachable!("VcdWriter writes valid UTF-8"); + }; + let expected_path_d = expected_path.display(); + if expected_contents + .as_ref() + .is_ok_and(|expected_contents| *expected_contents == vcd) + { + // avoid written output from being split from threads interleaving writes to stdout + let _stdout = std::io::stderr().lock(); + // use println to get output captured by tests + println!("\n{location}: generated VCD matches the expected VCD in {expected_path_d}"); + return Ok(()); + } + // avoid written output from being split from threads interleaving writes to stderr + let _stderr = std::io::stderr().lock(); + let error = |msg: std::fmt::Arguments<'_>| { + // print msg at both beginning and end so it's easier to find when the vcd is huge + Err(error(format_args!( + "\n{msg}####### VCD:\n{vcd}\n#######\n{msg}" + ))) + }; + let error = |msg: std::fmt::Arguments<'_>| match &*expected_contents { + Ok(_) => error(format_args!( + "{location}: generated VCD doesn't match the expected VCD in {expected_path_d}\n\ + {msg}", + )), + Err((Some(current_dir), e)) => error(format_args!( + "{location}: generated VCD doesn't match the expected VCD in {expected_path_d}\n\ + error: failed to read: {e}\n\ + current dir: {current_dir}\n\ + {msg}", + current_dir = current_dir.display(), + )), + Err((None, e)) => error(format_args!( + "{location}: generated VCD doesn't match the expected VCD in {expected_path_d}\n\ + error: failed to read: {e}\n\ + {msg}", + )), + }; + const OVERWRITE_VAR_NAME: &str = "OVERWRITE_EXPECTED_VCD"; + const OVERWRITE_VAR_VALUE: &str = "overwrite"; + match std::env::var_os(OVERWRITE_VAR_NAME) { + Some(v) if v == OVERWRITE_VAR_VALUE => match std::fs::write(&expected_path, &vcd) { + Ok(()) => error(format_args!( + "warning: since `{OVERWRITE_VAR_NAME}={OVERWRITE_VAR_VALUE}` is set -- writing the generated VCD to {expected_path_d}\n" + )), + Err(e) => error(format_args!( + "error: since `{OVERWRITE_VAR_NAME}={OVERWRITE_VAR_VALUE}` is set -- tried to write the generated VCD to {expected_path_d}\n\ + error: failed to write: {e}" + )), + }, + _ => error(format_args!( + "note: rerun the test with the environment variable `{OVERWRITE_VAR_NAME}={OVERWRITE_VAR_VALUE}`\n\ + to update the expected output to match the generated output.\n" + )), + } + } +} + +impl Drop for CheckedVcdOutput { + #[track_caller] + fn drop(&mut self) { + let _ = self.finish_impl(|msg| { + if std::thread::panicking() { + eprintln!("{msg}"); // use eprintln to get output captured by tests + } else { + panic!("{msg}"); + } + }); + } +} + +#[macro_export] +macro_rules! checked_vcd_output { + ($sim:expr, $path_relative_to_manifest_dir:expr $(,)?) => { + $crate::util::CheckedVcdOutput::__checked_vcd_output_macro_helper( + $sim, + ::std::env!("CARGO_MANIFEST_DIR"), + ::std::concat!($path_relative_to_manifest_dir), + ) + }; +} diff --git a/crates/cpu/tests/rename_execute_retire.rs b/crates/cpu/tests/rename_execute_retire.rs index ddf6b7c..750795e 100644 --- a/crates/cpu/tests/rename_execute_retire.rs +++ b/crates/cpu/tests/rename_execute_retire.rs @@ -2,6 +2,7 @@ // See Notices.txt for copyright information use cpu::{ + checked_vcd_output, config::{ CpuConfig, CpuConfigFetchWidth, CpuConfigMaxUnitMaxInFlight, PhantomConstCpuConfig, UnitConfig, @@ -30,9 +31,7 @@ use fayalite::{ bundle::BundleType, module::instance_with_loc, prelude::*, - sim::vcd::VcdWriterDecls, ty::{OpaqueSimValue, StaticType}, - util::RcWriter, }; use serde::{Deserialize, Serialize}; use std::{ @@ -4127,22 +4126,10 @@ fn test_rename_execute_retire_fibonacci_non_combinatorial() { false, ); let mut sim = Simulation::new(m); - let writer = RcWriter::default(); - sim.add_trace_writer(VcdWriterDecls::new(writer.clone())); - struct DumpVcdOnDrop { - writer: Option, - } - impl Drop for DumpVcdOnDrop { - fn drop(&mut self) { - if let Some(mut writer) = self.writer.take() { - let vcd = String::from_utf8(writer.take()).unwrap(); - println!("####### VCD:\n{vcd}\n#######"); - } - } - } - let mut writer = DumpVcdOnDrop { - writer: Some(writer), - }; + let _checked_vcd_output = checked_vcd_output!( + &mut sim, + "tests/expected/rename_execute_retire_fibonacci_non_combinatorial.vcd", + ); sim.write_clock(sim.io().cd.clk, false); sim.write_reset(sim.io().cd.rst, true); for cycle in 0..200 { @@ -4154,12 +4141,6 @@ fn test_rename_execute_retire_fibonacci_non_combinatorial() { sim.write_reset(sim.io().cd.rst, false); } assert!(sim.read_bool(sim.io().all_outputs_written)); - // FIXME: vcd is just whatever rename_execute_retire does now, which isn't known to be correct - let vcd = String::from_utf8(writer.writer.take().unwrap().take()).unwrap(); - println!("####### VCD:\n{vcd}\n#######"); - if vcd != include_str!("expected/rename_execute_retire_fibonacci_non_combinatorial.vcd") { - panic!(); - } } #[hdl] @@ -4180,22 +4161,10 @@ fn test_rename_execute_retire_fibonacci_combinatorial() { let m = rename_execute_retire_test_harness::(PhantomConst::new_sized(config), true); let mut sim = Simulation::new(m); - let writer = RcWriter::default(); - sim.add_trace_writer(VcdWriterDecls::new(writer.clone())); - struct DumpVcdOnDrop { - writer: Option, - } - impl Drop for DumpVcdOnDrop { - fn drop(&mut self) { - if let Some(mut writer) = self.writer.take() { - let vcd = String::from_utf8(writer.take()).unwrap(); - println!("####### VCD:\n{vcd}\n#######"); - } - } - } - let mut writer = DumpVcdOnDrop { - writer: Some(writer), - }; + let _checked_vcd_output = checked_vcd_output!( + &mut sim, + "tests/expected/rename_execute_retire_fibonacci_combinatorial.vcd", + ); sim.write_clock(sim.io().cd.clk, false); sim.write_reset(sim.io().cd.rst, true); for cycle in 0..200 { @@ -4207,12 +4176,6 @@ fn test_rename_execute_retire_fibonacci_combinatorial() { sim.write_reset(sim.io().cd.rst, false); } assert!(sim.read_bool(sim.io().all_outputs_written)); - // FIXME: vcd is just whatever rename_execute_retire does now, which isn't known to be correct - let vcd = String::from_utf8(writer.writer.take().unwrap().take()).unwrap(); - println!("####### VCD:\n{vcd}\n#######"); - if vcd != include_str!("expected/rename_execute_retire_fibonacci_combinatorial.vcd") { - panic!(); - } } struct SlowLoopInsns; @@ -4313,22 +4276,10 @@ fn test_rename_execute_retire_slow_loop() { let m = rename_execute_retire_test_harness::(PhantomConst::new_sized(config), true); let mut sim = Simulation::new(m); - let writer = RcWriter::default(); - sim.add_trace_writer(VcdWriterDecls::new(writer.clone())); - struct DumpVcdOnDrop { - writer: Option, - } - impl Drop for DumpVcdOnDrop { - fn drop(&mut self) { - if let Some(mut writer) = self.writer.take() { - let vcd = String::from_utf8(writer.take()).unwrap(); - println!("####### VCD:\n{vcd}\n#######"); - } - } - } - let mut writer = DumpVcdOnDrop { - writer: Some(writer), - }; + let _checked_vcd_output = checked_vcd_output!( + &mut sim, + "tests/expected/rename_execute_retire_slow_loop.vcd", + ); sim.write_clock(sim.io().cd.clk, false); sim.write_reset(sim.io().cd.rst, true); for cycle in 0..350 { @@ -4342,12 +4293,6 @@ fn test_rename_execute_retire_slow_loop() { assert!(sim.read_bool(sim.io().all_outputs_written)); // make sure we're actually testing L2 reg file ops assert!(sim.read_bool(sim.io().started_any_l2_reg_file_ops)); - // FIXME: vcd is just whatever rename_execute_retire does now, which isn't known to be correct - let vcd = String::from_utf8(writer.writer.take().unwrap().take()).unwrap(); - println!("####### VCD:\n{vcd}\n#######"); - if vcd != include_str!("expected/rename_execute_retire_slow_loop.vcd") { - panic!(); - } } /// equivalent of Unix's `head -n1` @@ -4452,22 +4397,8 @@ fn test_rename_execute_retire_head_n1() { let m = rename_execute_retire_test_harness::(PhantomConst::new_sized(config), true); let mut sim = Simulation::new(m); - let writer = RcWriter::default(); - sim.add_trace_writer(VcdWriterDecls::new(writer.clone())); - struct DumpVcdOnDrop { - writer: Option, - } - impl Drop for DumpVcdOnDrop { - fn drop(&mut self) { - if let Some(mut writer) = self.writer.take() { - let vcd = String::from_utf8(writer.take()).unwrap(); - println!("####### VCD:\n{vcd}\n#######"); - } - } - } - let mut writer = DumpVcdOnDrop { - writer: Some(writer), - }; + let _checked_vcd_output = + checked_vcd_output!(&mut sim, "tests/expected/rename_execute_retire_head_n1.vcd"); sim.write_clock(sim.io().cd.clk, false); sim.write_reset(sim.io().cd.rst, true); for cycle in 0..300 { @@ -4479,12 +4410,6 @@ fn test_rename_execute_retire_head_n1() { sim.write_reset(sim.io().cd.rst, false); } assert!(sim.read_bool(sim.io().all_outputs_written)); - // FIXME: vcd is just whatever rename_execute_retire does now, which isn't known to be correct - let vcd = String::from_utf8(writer.writer.take().unwrap().take()).unwrap(); - println!("####### VCD:\n{vcd}\n#######"); - if vcd != include_str!("expected/rename_execute_retire_head_n1.vcd") { - panic!(); - } } struct SaveRestoreGprsInsns; @@ -4569,22 +4494,10 @@ fn test_rename_execute_retire_save_restore_gprs() { true, ); let mut sim = Simulation::new(m); - let writer = RcWriter::default(); - sim.add_trace_writer(VcdWriterDecls::new(writer.clone())); - struct DumpVcdOnDrop { - writer: Option, - } - impl Drop for DumpVcdOnDrop { - fn drop(&mut self) { - if let Some(mut writer) = self.writer.take() { - let vcd = String::from_utf8(writer.take()).unwrap(); - println!("####### VCD:\n{vcd}\n#######"); - } - } - } - let mut writer = DumpVcdOnDrop { - writer: Some(writer), - }; + let _checked_vcd_output = checked_vcd_output!( + &mut sim, + "tests/expected/rename_execute_retire_save_restore_gprs.vcd", + ); sim.write_clock(sim.io().cd.clk, false); sim.write_reset(sim.io().cd.rst, true); for cycle in 0..700 { @@ -4596,10 +4509,4 @@ fn test_rename_execute_retire_save_restore_gprs() { sim.write_reset(sim.io().cd.rst, false); } assert!(sim.read_bool(sim.io().all_outputs_written)); - // FIXME: vcd is just whatever rename_execute_retire does now, which isn't known to be correct - let vcd = String::from_utf8(writer.writer.take().unwrap().take()).unwrap(); - println!("####### VCD:\n{vcd}\n#######"); - if vcd != include_str!("expected/rename_execute_retire_save_restore_gprs.vcd") { - panic!(); - } }