From e9e3e248096ccb0c24fa2b3817b0d20b5480ae20 Mon Sep 17 00:00:00 2001 From: Aria Beingessner Date: Sun, 7 Jul 2024 09:45:09 -0400 Subject: [PATCH] feat: make enums semantic And improve quality of error output --- src/error.rs | 39 +++++++---- src/harness/check.rs | 125 +++++++++++++++++++++++------------ src/log.rs | 42 ++++++------ src/report.rs | 59 +++++++++++++++-- src/toolchains/c/write.rs | 32 ++++++++- src/toolchains/rust/write.rs | 32 ++++++++- 6 files changed, 237 insertions(+), 92 deletions(-) diff --git a/src/error.rs b/src/error.rs index 2c13f1b..a468730 100644 --- a/src/error.rs +++ b/src/error.rs @@ -72,12 +72,16 @@ pub enum BuildError { #[derive(Debug, thiserror::Error, Diagnostic)] pub enum CheckFailure { #[error( - " {func_name} {arg_kind} differed: - {arg_kind:<6}: {arg_name}: {arg_ty_name} - field : {val_path}: {val_ty_name} - expect: {expected:02X?} - caller: {caller:02X?} - callee: {callee:02X?}" + " func {func_name}'s values differed + values (native-endian hex bytes): + expect: {} + caller: {} + callee: {} + the value was {val_path}: {val_ty_name} + whose arg was {arg_name}: {arg_ty_name}", + fmt_bytes(expected), + fmt_bytes(caller), + fmt_bytes(callee) )] ValMismatch { func_idx: usize, @@ -85,7 +89,6 @@ pub enum CheckFailure { val_idx: usize, func_name: String, arg_name: String, - arg_kind: String, arg_ty_name: String, val_path: String, val_ty_name: String, @@ -94,12 +97,13 @@ pub enum CheckFailure { callee: Vec, }, #[error( - " {func_name} {arg_kind} had unexpected tagged variant: - {arg_kind:<6}: {arg_name}: {arg_ty_name} - field : {val_path}: {val_ty_name} - expect: {expected} - caller: {caller} - callee: {callee}" + " func {func_name}'s value had unexpected variant + values: + expect: {expected} + caller: {caller} + callee: {callee} + the value was {val_path}: {val_ty_name} + whose arg was {arg_name}: {arg_ty_name}" )] TagMismatch { func_idx: usize, @@ -107,7 +111,6 @@ pub enum CheckFailure { val_idx: usize, func_name: String, arg_name: String, - arg_kind: String, arg_ty_name: String, val_path: String, val_ty_name: String, @@ -136,3 +139,11 @@ pub enum RunError { #[error("test impl called write_val on func {func} val {val} twice")] DoubleWrite { func: usize, val: usize }, } + +fn fmt_bytes(bytes: &[u8]) -> String { + bytes + .iter() + .map(|b| format!("{b:02X}")) + .collect::>() + .join(" ") +} diff --git a/src/harness/check.rs b/src/harness/check.rs index ce23b77..87e771e 100644 --- a/src/harness/check.rs +++ b/src/harness/check.rs @@ -87,8 +87,12 @@ impl TestHarness { info!("Test {subtest_name:width$} passed", width = max_name_len); } Err(e) => { - info!("Test {subtest_name:width$} failed!", width = max_name_len); - info!("{}", e); + let red = console::Style::new().red(); + let message = format!( + "Test {subtest_name:width$} failed!\n{e}", + width = max_name_len + ); + info!("{}", red.apply_to(message)); } } } @@ -116,54 +120,40 @@ impl TestHarness { callee_val: &ValBuffer, ) -> Result<(), CheckFailure> { let types = &test.types; - let func = expected_val.func(); - let arg = expected_val.arg(); + // Enums and Taggeds are "fake" fields representing the semantic value (tag). + // In this case showing the bytes doesn't make sense, so show the Variant name + // (although we get bytes here they're the array index into the variant, + // a purely magical value that only makes sense to the harness itself!). + // + // Also we use u32::MAX to represent a poison "i dunno what it is, but it's + // definitely not the One variant we statically expected!", so most of the + // time we're here to print and shrug. if let Ty::Tagged(tagged_ty) = types.realize_ty(expected_val.ty) { - // This value is "fake" and is actually the semantic tag of tagged union. - // In this case showing the bytes doesn't make sense, so show the Variant name - // (although we get bytes here they're the array index into the variant, - // a purely magical value that only makes sense to the harness itself!). - // - // Also we use u32::MAX to represent a poison "i dunno what it is, but it's - // definitely not the One variant we statically expected!", so most of the - // time we're here to print and shrug. let expected_tag = expected_val.generate_idx(tagged_ty.variants.len()); - let caller_tag = - u32::from_ne_bytes(<[u8; 4]>::try_from(&caller_val.bytes[..4]).unwrap()) as usize; - let callee_tag = - u32::from_ne_bytes(<[u8; 4]>::try_from(&callee_val.bytes[..4]).unwrap()) as usize; + let caller_tag = load_tag(caller_val); + let callee_tag = load_tag(callee_val); + + if caller_tag != expected_tag || callee_tag != expected_tag { + let expected = tagged_variant_name(tagged_ty, expected_tag); + let caller = tagged_variant_name(tagged_ty, caller_tag); + let callee = tagged_variant_name(tagged_ty, callee_tag); + return Err(tag_error(types, &expected_val, expected, caller, callee)); + } + } else if let Ty::Enum(enum_ty) = types.realize_ty(expected_val.ty) { + let expected_tag = expected_val.generate_idx(enum_ty.variants.len()); + let caller_tag = load_tag(caller_val); + let callee_tag = load_tag(callee_val); if caller_tag != expected_tag || callee_tag != expected_tag { - let expected = tagged_ty.variants[expected_tag].name.to_string(); - let caller = tagged_ty - .variants - .get(caller_tag) - .map(|v| v.name.as_str()) - .unwrap_or("") - .to_owned(); - let callee = tagged_ty - .variants - .get(callee_tag) - .map(|v| v.name.as_str()) - .unwrap_or("") - .to_owned(); - return Err(CheckFailure::TagMismatch { - func_idx: expected_val.func_idx, - arg_idx: expected_val.arg_idx, - val_idx: expected_val.val_idx, - arg_kind: "argument".to_owned(), - func_name: func.func_name.to_string(), - arg_name: arg.arg_name.to_string(), - arg_ty_name: types.format_ty(arg.ty), - val_path: expected_val.path.to_string(), - val_ty_name: types.format_ty(expected_val.ty), - expected, - caller, - callee, - }); + let expected = enum_variant_name(enum_ty, expected_tag); + let caller = enum_variant_name(enum_ty, caller_tag); + let callee = enum_variant_name(enum_ty, callee_tag); + return Err(tag_error(types, &expected_val, expected, caller, callee)); } } else if caller_val.bytes != callee_val.bytes { // General case, just get a pile of bytes to span both values + let func = expected_val.func(); + let arg = expected_val.arg(); let mut expected = vec![0; caller_val.bytes.len().max(callee_val.bytes.len())]; expected_val.fill_bytes(&mut expected); // FIXME: this doesn't do the right thing for enums @@ -172,7 +162,6 @@ impl TestHarness { func_idx: expected_val.func_idx, arg_idx: expected_val.arg_idx, val_idx: expected_val.val_idx, - arg_kind: "argument".to_owned(), func_name: func.func_name.to_string(), arg_name: arg.arg_name.to_string(), arg_ty_name: types.format_ty(arg.ty), @@ -187,3 +176,51 @@ impl TestHarness { Ok(()) } } + +fn load_tag(val: &ValBuffer) -> usize { + u32::from_ne_bytes(<[u8; 4]>::try_from(&val.bytes[..4]).unwrap()) as usize +} + +fn tagged_variant_name(tagged_ty: &kdl_script::types::TaggedTy, tag: usize) -> String { + let tagged_name = &tagged_ty.name; + let variant_name = tagged_ty + .variants + .get(tag) + .map(|v| v.name.as_str()) + .unwrap_or(""); + format!("{tagged_name}::{variant_name}") +} + +fn enum_variant_name(enum_ty: &kdl_script::types::EnumTy, tag: usize) -> String { + let enum_name = &enum_ty.name; + let variant_name = enum_ty + .variants + .get(tag) + .map(|v| v.name.as_str()) + .unwrap_or(""); + format!("{enum_name}::{variant_name}") +} + +fn tag_error( + types: &kdl_script::TypedProgram, + expected_val: &ValueRef, + expected: String, + caller: String, + callee: String, +) -> CheckFailure { + let func = expected_val.func(); + let arg = expected_val.arg(); + CheckFailure::TagMismatch { + func_idx: expected_val.func_idx, + arg_idx: expected_val.arg_idx, + val_idx: expected_val.val_idx, + func_name: func.func_name.to_string(), + arg_name: arg.arg_name.to_string(), + arg_ty_name: types.format_ty(arg.ty), + val_path: expected_val.path.to_string(), + val_ty_name: types.format_ty(expected_val.ty), + expected, + caller, + callee, + } +} diff --git a/src/log.rs b/src/log.rs index 5de98f6..7069606 100644 --- a/src/log.rs +++ b/src/log.rs @@ -11,8 +11,11 @@ use tracing_subscriber::Layer; use std::sync::{Arc, Mutex}; +use crate::fivemat::Fivemat; + const TRACE_TEST_SPAN: &str = "test"; const IGNORE_LIST: &[&str] = &[]; +const INDENT: &str = " "; /// An in-memory logger that lets us view particular /// spans of the logs, and understands minidump-stackwalk's @@ -155,24 +158,22 @@ impl MapLogger { write!(output, "{:indent$}", "", indent = depth * 4).unwrap(); } fn print_span_recursive( - output: &mut String, + f: &mut Fivemat, sub_spans: &LinkedHashMap, - depth: usize, span: &SpanEntry, range: Option>, ) { if !span.name.is_empty() { - print_indent(output, depth); let style = Style::new().blue(); - write!(output, "{}", style.apply_to(&span.name)).unwrap(); + write!(f, "{}", style.apply_to(&span.name)).unwrap(); for (key, val) in &span.fields { if key == "id" { - write!(output, " {}", style.apply_to(val)).unwrap(); + write!(f, " {}", style.apply_to(val)).unwrap(); } else { - write!(output, "{key}: {val}").unwrap(); + write!(f, "{key}: {val}").unwrap(); } } - writeln!(output).unwrap(); + writeln!(f).unwrap(); } let event_range = if let Some(range) = range { @@ -180,25 +181,20 @@ impl MapLogger { } else { &span.events[..] }; + f.add_indent(1); for event in event_range { match event { EventEntry::Message(event) => { if event.fields.contains_key("message") { - print_indent(output, depth + 1); - print_event(output, event); + print_event(f, event); } } EventEntry::Span(sub_span) => { - print_span_recursive( - output, - sub_spans, - depth + 1, - &sub_spans[sub_span], - None, - ); + print_span_recursive(f, sub_spans, &sub_spans[sub_span], None); } } } + f.sub_indent(1); } let mut log = self.state.lock().unwrap(); @@ -209,16 +205,17 @@ impl MapLogger { } log.last_query = Some(query); - let mut output = String::new(); + let mut output_buf = String::new(); + let mut f = Fivemat::new(&mut output_buf, INDENT); let (span_to_print, range) = match query { Query::All => (&log.root_span, None), Query::Span(span_id) => (&log.sub_spans[&span_id], None), }; - print_span_recursive(&mut output, &log.sub_spans, 0, span_to_print, range); + print_span_recursive(&mut f, &log.sub_spans, span_to_print, range); - let result = Arc::new(output); + let result = Arc::new(output_buf); log.cur_string = Some(result.clone()); result } @@ -226,11 +223,12 @@ impl MapLogger { fn immediate_event(event: &MessageEntry) { let mut output = String::new(); - print_event(&mut output, event); + let mut f = Fivemat::new(&mut output, INDENT); + print_event(&mut f, event); eprintln!("{}", output); } -fn print_event(output: &mut String, event: &MessageEntry) { +fn print_event(f: &mut Fivemat, event: &MessageEntry) { use std::fmt::Write; if let Some(message) = event.fields.get("message") { let style = match event.level { @@ -241,7 +239,7 @@ fn print_event(output: &mut String, event: &MessageEntry) { Level::TRACE => Style::new().green(), }; // writeln!(output, "[{:5}] {}", event.level, message).unwrap(); - writeln!(output, "{}", style.apply_to(message)).unwrap(); + writeln!(f, "{}", style.apply_to(message)).unwrap(); } } diff --git a/src/report.rs b/src/report.rs index 15e4c1e..f82d067 100644 --- a/src/report.rs +++ b/src/report.rs @@ -306,12 +306,12 @@ pub struct CheckOutput { pub subtest_checks: Vec>, } -#[derive(Debug, Clone, Serialize)] +#[derive(Debug, Copy, Clone, Serialize, PartialEq, Eq, PartialOrd, Ord)] pub enum TestConclusion { Skipped, Passed, - Failed, Busted, + Failed, } impl FullReport { @@ -327,7 +327,9 @@ impl FullReport { let red = Style::new().red(); let green = Style::new().green(); let blue = Style::new().blue(); - for test in &self.tests { + let mut sorted_tests = self.tests.iter().collect::>(); + sorted_tests.sort_by_key(|t| t.conclusion); + for test in sorted_tests { if let Skipped = test.conclusion { continue; } @@ -343,7 +345,51 @@ impl FullReport { (Passed, Random) => write!(f, "passed (random, result ignored)")?, (Passed, Fail(_)) => write!(f, "passed (failed as expected)")?, - (Failed, Pass(_)) => write!(f, "{}", red.apply_to("failed"))?, + (Failed, Pass(_)) => { + write!(f, "{}", red.apply_to("failed"))?; + if test.results.ran_to < TestRunMode::Check { + let (msg, err) = match &test.results.ran_to { + TestRunMode::Generate => ( + "generate source code", + format!( + "{}", + test.results + .source + .as_ref() + .unwrap() + .as_ref() + .err() + .unwrap() + ), + ), + TestRunMode::Build => ( + "compile source code", + format!( + "{}", + test.results.build.as_ref().unwrap().as_ref().err().unwrap() + ), + ), + TestRunMode::Link => ( + "link both sides together", + format!( + "{}", + test.results.link.as_ref().unwrap().as_ref().err().unwrap() + ), + ), + TestRunMode::Run => ( + "run the program", + format!( + "{}", + test.results.run.as_ref().unwrap().as_ref().err().unwrap() + ), + ), + TestRunMode::Skip | TestRunMode::Check => ("", String::new()), + }; + write!(f, "{}", red.apply_to(" to "))?; + writeln!(f, "{}", red.apply_to(msg))?; + writeln!(f, " {}", red.apply_to(err))?; + } + } (Failed, Random) => { write!(f, "{}", red.apply_to("failed!? (failed but random!?)"))? } @@ -383,8 +429,9 @@ impl FullReport { for (subtest_name, result) in check_result.subtest_names.iter().zip(sub_results.iter()) { write!(f, " {:width$} ", subtest_name, width = max_name_len)?; - if let Err(_e) = result { - writeln!(f, "failed!")?; + if let Err(e) = result { + writeln!(f, "{}", red.apply_to("failed!"))?; + writeln!(f, "{}", red.apply_to(e))?; } else { writeln!(f)?; } diff --git a/src/toolchains/c/write.rs b/src/toolchains/c/write.rs index 3eb9d99..1bf4610 100644 --- a/src/toolchains/c/write.rs +++ b/src/toolchains/c/write.rs @@ -65,13 +65,34 @@ impl CcToolchain { vals: &mut ArgValuesIter, ) -> Result<(), GenerateError> { match state.types.realize_ty(var_ty) { - Ty::Primitive(_) | Ty::Enum(_) => { + Ty::Primitive(_) => { // Hey an actual leaf, report it (and burn a value) let val = vals.next_val(); if val.should_write_val(&state.options) { self.write_leaf_field(f, state, to, from, &val)?; } } + Ty::Enum(enum_ty) => { + // enums are leaves but we care about their semantic value (variant case) + // so don't just pass the raw bytes, treat this like a tag we match on + let tag_generator = vals.next_val(); + let tag_idx = tag_generator.generate_idx(enum_ty.variants.len()); + if let Some(variant) = enum_ty.variants.get(tag_idx) { + let enum_name = &enum_ty.name; + let variant_name = &variant.name; + if tag_generator.should_write_val(&state.options) { + writeln!(f, "if ({enum_name}_{variant_name} == {from}) {{")?; + f.add_indent(1); + self.write_tag_field(f, state, to, from, tag_idx, &tag_generator)?; + f.sub_indent(1); + writeln!(f, "}} else {{")?; + f.add_indent(1); + self.write_error_tag_field(f, state, to, &tag_generator)?; + f.sub_indent(1); + writeln!(f, "}}")?; + } + } + } Ty::Empty => { // nothing worth producing } @@ -184,7 +205,7 @@ impl CcToolchain { let tag_generator = vals.next_val(); let tag_idx = tag_generator.generate_idx(union_ty.fields.len()); if tag_generator.should_write_val(&state.options) { - self.write_tag_field(f, state, to, tag_idx, &tag_generator)?; + self.write_tag_field(f, state, to, from, tag_idx, &tag_generator)?; } if let Some(field) = union_ty.fields.get(tag_idx) { let field_name = &field.ident; @@ -236,11 +257,16 @@ impl CcToolchain { f: &mut Fivemat, state: &TestState, to: &str, + path: &str, variant_idx: usize, val: &ValueRef, ) -> Result<(), GenerateError> { match state.options.val_writer { WriteImpl::HarnessCallback => { + // Convenience for triggering test failures + if path.contains("abicafepoison") && to.contains(CALLEE_VALS) { + return self.write_error_tag_field(f, state, to, val); + } let val_idx = val.absolute_val_idx; writeln!(f, "{{")?; f.add_indent(1); @@ -276,7 +302,7 @@ impl CcToolchain { writeln!(f, "{{")?; f.add_indent(1); writeln!(f, "uint32_t _temp = {};", u32::MAX)?; - writeln!(f, "write_va;({to}, {val_idx}, _temp);")?; + writeln!(f, "write_val({to}, {val_idx}, _temp);")?; f.sub_indent(1); writeln!(f, "}}")?; } diff --git a/src/toolchains/rust/write.rs b/src/toolchains/rust/write.rs index 7e5b5d8..4ce837c 100644 --- a/src/toolchains/rust/write.rs +++ b/src/toolchains/rust/write.rs @@ -85,13 +85,34 @@ impl RustcToolchain { vals: &mut ArgValuesIter, ) -> Result<(), GenerateError> { match state.types.realize_ty(var_ty) { - Ty::Primitive(_) | Ty::Enum(_) => { + Ty::Primitive(_) => { // Hey an actual leaf, report it (and burn a value) let val = vals.next_val(); if val.should_write_val(&state.options) { self.write_leaf_field(f, state, to, from, &val)?; } } + Ty::Enum(enum_ty) => { + // enums are leaves but we care about their semantic value (variant case) + // so don't just pass the raw bytes, treat this like a tag we match on + let tag_generator = vals.next_val(); + let tag_idx = tag_generator.generate_idx(enum_ty.variants.len()); + if let Some(variant) = enum_ty.variants.get(tag_idx) { + let enum_name = &enum_ty.name; + let variant_name = &variant.name; + if tag_generator.should_write_val(&state.options) { + writeln!(f, "if let {enum_name}::{variant_name} = &{from} {{")?; + f.add_indent(1); + self.write_tag_field(f, state, to, from, tag_idx, &tag_generator)?; + f.sub_indent(1); + writeln!(f, "}} else {{")?; + f.add_indent(1); + self.write_error_tag_field(f, state, to, &tag_generator)?; + f.sub_indent(1); + writeln!(f, "}}")?; + } + } + } Ty::Empty => { // nothing worth producing } @@ -151,7 +172,7 @@ impl RustcToolchain { let f = &mut Fivemat::new(&mut temp_out, INDENT); f.add_indent(1); if tag_generator.should_write_val(&state.options) { - self.write_tag_field(f, state, to, tag_idx, &tag_generator)?; + self.write_tag_field(f, state, to, from, tag_idx, &tag_generator)?; } if let Some(fields) = &variant.fields { for field in fields { @@ -199,7 +220,7 @@ impl RustcToolchain { let tag_generator = vals.next_val(); let tag_idx = tag_generator.generate_idx(union_ty.fields.len()); if tag_generator.should_write_val(&state.options) { - self.write_tag_field(f, state, to, tag_idx, &tag_generator)?; + self.write_tag_field(f, state, to, from, tag_idx, &tag_generator)?; } if let Some(field) = union_ty.fields.get(tag_idx) { let field_name = &field.ident; @@ -251,11 +272,16 @@ impl RustcToolchain { f: &mut Fivemat, state: &TestState, to: &str, + path: &str, variant_idx: usize, val: &ValueRef, ) -> Result<(), GenerateError> { match state.options.val_writer { WriteImpl::HarnessCallback => { + // Convenience for triggering test failures + if path.contains("abicafepoison") && to.contains(CALLEE_VALS) { + return self.write_error_tag_field(f, state, to, val); + } let val_idx = val.absolute_val_idx; writeln!(f, "write_val({to}, {val_idx}, &{}u32);", variant_idx)?; }