Allow aml_tester to continue after a panic

Fix accidentally broken tests

Exit with a failure code if needed

This makes it easier for any user to see if a test has failed. Which may
be useful for example in CI environments.

Remove AML filename replacement

`resolve_and_compile` relies on the current `iasl` way of naming its
output, there's no need to duplicate that in `create_script_file`

Author: martin-hughes

tests/normal_fields.rs

@@ -16,7 +16,7 @@ mod test_infra;
 
 #[test]
 fn test_basic_store_and_load() {
-    const AML: &str = r#"DefinitionBlock("%FN%", "DSDT", 1, "RSACPI", "BUFFLD", 1) {
+    const AML: &str = r#"DefinitionBlock("", "DSDT", 1, "RSACPI", "BUFFLD", 1) {
     OperationRegion(MEM, SystemMemory, 0x40000, 0x1000)
     Field(MEM, WordAcc, NoLock, Preserve) {
         A, 16,
@@ -46,7 +46,7 @@ fn test_basic_store_and_load() {
 
 #[test]
 fn test_narrow_access_store_and_load() {
-    const AML: &str = r#"DefinitionBlock("%FN%", "DSDT", 1, "RSACPI", "BUFFLD", 1) {
+    const AML: &str = r#"DefinitionBlock("", "DSDT", 1, "RSACPI", "BUFFLD", 1) {
     OperationRegion(MEM, SystemIO, 0x40, 0x10)
     Field(MEM, ByteAcc, NoLock, Preserve) {
         A, 16,
@@ -79,7 +79,7 @@ fn test_narrow_access_store_and_load() {
 
 #[test]
 fn test_unaligned_field_store() {
-    const AML: &str = r#"DefinitionBlock("%FN%", "DSDT", 1, "RSACPI", "BUFFLD", 1) {
+    const AML: &str = r#"DefinitionBlock("", "DSDT", 1, "RSACPI", "BUFFLD", 1) {
     OperationRegion(MEM, SystemIO, 0x40, 0x10)
     Field(MEM, WordAcc, NoLock, Preserve) {
         A, 7,

tests/test_infra/mod.rs

@@ -1,13 +1,17 @@
 use acpi::Handler;
-use aml_test_tools::{new_interpreter, run_test_for_string, TestResult};
-use aml_test_tools::handlers::logging_handler::LoggingHandler;
+use aml_test_tools::{
+    RunTestResult,
+    handlers::logging_handler::LoggingHandler,
+    new_interpreter,
+    run_test_for_string,
+};
 
 pub fn run_aml_test(asl: &'static str, handler: impl Handler) {
     // Tests calling `run_aml_test` don't do much else, and we usually want logging, so initialize it here.
     let _ = pretty_env_logger::try_init();
-    
+
     let logged_handler = LoggingHandler::new(handler);
-    let mut interpreter = new_interpreter(logged_handler);
+    let interpreter = new_interpreter(logged_handler);
 
-    assert_eq!(run_test_for_string(asl, &mut interpreter), TestResult::Pass);
+    assert!(matches!(run_test_for_string(asl, interpreter), RunTestResult::Pass(_)));
 }

tools/aml_test_tools/src/lib.rs

@@ -14,8 +14,10 @@ use acpi::{
 use log::{error, trace};
 use std::{
     ffi::OsStr,
+    fmt::Debug,
     fs::File,
     io::{Read, Write},
+    panic::{AssertUnwindSafe, catch_unwind},
     path::PathBuf,
     process::Command,
     ptr::NonNull,
@@ -24,10 +26,59 @@ use std::{
 };
 use tempfile::{NamedTempFile, TempDir, tempdir};
 
+/// The result of a call to [`run_test`]. This is a bit of a combination - it contains the actual
+/// result of the test, but also the interpreter - if it is still valid.
+pub enum RunTestResult<T>
+where
+    T: Handler,
+{
+    /// The test passed, and the interpreter is still valid.
+    Pass(Interpreter<T>),
+
+    /// The test failed, but the interpreter is still valid. A failure reason is also provided.
+    Failed(Interpreter<T>, TestFailureReason),
+
+    /// The test failed, and the interpreter is no longer valid.
+    Panicked,
+}
+
+/// The result of a test
+#[derive(Debug, PartialEq)]
+pub enum TestResult {
+    /// The test passed.
+    Pass,
+
+    /// The test failed.
+    Failed(TestFailureReason),
+
+    /// The test caused a panic, probably in the interpreter. This is separated out from the failed
+    /// case for two reasons:
+    ///
+    /// 1. We want to highlight panics as they are in some ways worse than a failure - some AML
+    ///    might cause the problems for the interpreter, but the system as a whole shouldn't crash
+    /// 2. The interpreter that was being used for testing is no longer valid - a new one is needed.
+    Panicked,
+}
+
+impl<T> From<&RunTestResult<T>> for TestResult
+where
+    T: Handler,
+{
+    fn from(result: &RunTestResult<T>) -> Self {
+        match result {
+            RunTestResult::Pass(_) => TestResult::Pass,
+            RunTestResult::Failed(_, reason) => TestResult::Failed(reason.clone()),
+            RunTestResult::Panicked => TestResult::Panicked,
+        }
+    }
+}
+
 /// Possible results of [`resolve_and_compile`].
 pub enum CompilationOutcome {
     /// Not a valid ASL or AML file.
     Ignored,
+    /// A filesystem error occurred.
+    FilesystemErr(PathBuf),
     /// This file is already an AML file, does not need recompiling.
     IsAml(PathBuf),
     /// Both .asl and .aml files exist, and the .aml file is newer - it does not need recompiling.
@@ -41,17 +92,15 @@ pub enum CompilationOutcome {
     Succeeded(PathBuf),
 }
 
-/// Possible results of [`run_test_for_file`].
-#[derive(Clone, Debug, Eq, Hash, PartialEq)]
-pub enum TestResult {
-    /// The test passed.
-    Pass,
+/// Possible causes of a test failure.
+#[derive(Clone, Debug, /*Eq, Hash,*/ PartialEq)]
+pub enum TestFailureReason {
     /// The test ASL failed compilation by `iasl`.
     CompileFail,
-    /// Our interpreter failed to parse the resulting AML.
-    ParseFail,
-    // TODO: should we do this??
-    NotCompiled,
+    /// Some error occurred attempting to read or write the test file.
+    FilesystemErr,
+    /// Our interpreter failed to parse or execute the resulting AML.
+    ParseFail(AmlError),
 }
 
 /// An internal-only struct that helps keep track of temporary files generated by
@@ -62,7 +111,6 @@ struct TempScriptFile {
     #[allow(unused)]
     dir: TempDir,
     asl_file: NamedTempFile,
-    aml_file: PathBuf,
 }
 
 /// Determine what to do with this file - ignore, compile and parse, or just parse.
@@ -73,47 +121,62 @@ struct TempScriptFile {
 /// - `path`: The filename of an ASL file to (possibly) compile.
 /// - `can_compile`: Whether compilation is allowed for this file. If not, any existing AML file
 ///   with the same name (but different extension) will be used.
-pub fn resolve_and_compile(path: &PathBuf, can_compile: bool) -> std::io::Result<CompilationOutcome> {
+pub fn resolve_and_compile(path: &PathBuf, can_compile: bool) -> CompilationOutcome {
+    let Ok(meta) = path.metadata() else {
+        return CompilationOutcome::FilesystemErr(path.clone());
+    };
+
     // If this file is aml and it exists, it's ready for parsing
     // metadata() will error if the file does not exist
-    if path.extension() == Some(OsStr::new("aml")) && path.metadata()?.is_file() {
-        return Ok(CompilationOutcome::IsAml(path.clone()));
+    if path.extension() == Some(OsStr::new("aml")) && meta.is_file() {
+        return CompilationOutcome::IsAml(path.clone());
     }
 
     // If this file is not asl, it's not interesting. Error if the file does not exist.
-    if path.extension() != Some(OsStr::new("asl")) || !path.metadata()?.is_file() {
-        return Ok(CompilationOutcome::Ignored);
+    if path.extension() != Some(OsStr::new("asl")) || !meta.is_file() {
+        return CompilationOutcome::Ignored;
     }
 
     let aml_path = path.with_extension("aml");
-
     if aml_path.is_file() {
-        let asl_last_modified = path.metadata()?.modified()?;
-        let aml_last_modified = aml_path.metadata()?.modified()?;
+        let Ok(asl_last_modified) = meta.modified() else {
+            return CompilationOutcome::FilesystemErr(path.clone());
+        };
+        let Ok(aml_last_modified) = aml_path.metadata().and_then(|m| m.modified()) else {
+            return CompilationOutcome::FilesystemErr(path.clone());
+        };
         // If the aml is more recent than the asl, use the existing aml
         // Otherwise continue to compilation
         if asl_last_modified <= aml_last_modified {
-            return Ok(CompilationOutcome::Newer(aml_path));
+            return CompilationOutcome::Newer(aml_path);
         }
     }
 
     if !can_compile {
-        return Ok(CompilationOutcome::NotCompiled(path.clone()));
+        return CompilationOutcome::NotCompiled(path.clone());
     }
 
     // Compile the ASL file using `iasl`
     println!("Compiling file: {}", path.display());
-    let output = Command::new("iasl").arg(path).output()?;
+    let output = Command::new("iasl").arg(path).output();
 
-    if !output.status.success() {
-        println!(
-            "Failed to compile ASL file: {}. Output from iasl:\n {}",
-            path.display(),
-            String::from_utf8_lossy(&output.stderr)
-        );
-        Ok(CompilationOutcome::Failed(path.clone()))
-    } else {
-        Ok(CompilationOutcome::Succeeded(aml_path))
+    match output {
+        Ok(output) => {
+            if !output.status.success() {
+                println!(
+                    "Failed to compile ASL file: {}. Output from iasl:\n {}",
+                    path.display(),
+                    String::from_utf8_lossy(&output.stderr)
+                );
+                CompilationOutcome::Failed(path.clone())
+            } else {
+                CompilationOutcome::Succeeded(aml_path)
+            }
+        }
+        Err(e) => {
+            println!("Failed to compile ASL file: {}. Error: {}", path.display(), e);
+            CompilationOutcome::Failed(path.clone())
+        }
     }
 }
 
@@ -185,11 +248,17 @@ where
 /// * `asl`: A string slice containing an ASL script. This will be compiled to AML using `iasl` and
 ///   then tested using [`run_test`]
 /// * `interpreter`: The interpreter to use for testing.
-pub fn run_test_for_string(asl: &'static str, interpreter: &mut Interpreter<impl Handler + Clone>) -> TestResult {
+pub fn run_test_for_string<T>(asl: &'static str, interpreter: Interpreter<T>) -> RunTestResult<T>
+where
+    T: Handler,
+{
     let script = create_script_file(asl);
-    resolve_and_compile(&script.asl_file.path().to_path_buf(), true).unwrap();
-
-    run_test_for_file(&script.aml_file, interpreter)
+    match resolve_and_compile(&script.asl_file.path().to_path_buf(), true) {
+        CompilationOutcome::Succeeded(aml_path) | CompilationOutcome::IsAml(aml_path) => {
+            run_test_for_file(&aml_path, interpreter)
+        }
+        _ => RunTestResult::Failed(interpreter, TestFailureReason::CompileFail),
+    }
 }
 
 /// Test an AML file using [`run_test`]
@@ -199,23 +268,20 @@ pub fn run_test_for_string(asl: &'static str, interpreter: &mut Interpreter<impl
 /// * `file`: The path to the AML file to test. This must be an AML file otherwise the test will
 ///   fail very quickly.
 /// * `interpreter`: The interpreter to use for testing.
-pub fn run_test_for_file(file: &PathBuf, interpreter: &mut Interpreter<impl Handler + Clone>) -> TestResult {
-    let Ok(mut file) = File::open(&file) else {
-        return TestResult::CompileFail;
+pub fn run_test_for_file<T>(file: &PathBuf, interpreter: Interpreter<T>) -> RunTestResult<T>
+where
+    T: Handler,
+{
+    let Ok(mut file) = File::open(file) else {
+        return RunTestResult::Failed(interpreter, TestFailureReason::FilesystemErr);
     };
     let mut contents = Vec::new();
     file.read_to_end(&mut contents).unwrap();
 
     const AML_TABLE_HEADER_LENGTH: usize = 36;
     let stream = &contents[AML_TABLE_HEADER_LENGTH..];
 
-    match run_test(stream, interpreter) {
-        Ok(()) => TestResult::Pass,
-        Err(e) => {
-            error!("Error running test: {:?}", e);
-            TestResult::ParseFail
-        }
-    }
+    run_test(stream, interpreter)
 }
 
 /// Internal function to create a temporary script file from an ASL string, plus to calculate the
@@ -226,16 +292,10 @@ fn create_script_file(asl: &'static str) -> TempScriptFile {
     let mut script_file = NamedTempFile::with_suffix_in(".asl", &dir).unwrap();
     trace!("Created temp file: {:?}", script_file.path());
 
-    let output_stem = script_file.path().file_stem().unwrap().to_str().unwrap();
-    let output_name = format!("{}.aml", output_stem);
-    let output_full_name = dir.path().join(output_name.clone());
-
-    let new_asl = asl.replace("%FN%", output_name.as_str());
-
-    script_file.write_all(new_asl.as_bytes()).unwrap();
+    script_file.write_all(asl.as_bytes()).unwrap();
     script_file.flush().unwrap();
 
-    TempScriptFile { dir, asl_file: script_file, aml_file: output_full_name }
+    TempScriptFile { dir, asl_file: script_file }
 }
 
 /// Run a test on an AML stream.
@@ -248,25 +308,43 @@ fn create_script_file(asl: &'static str) -> TempScriptFile {
 /// Arguments:
 ///
 /// * `stream`: A slice containing the AML bytecode to test.
-/// * `interpreter`: The interpreter to test with.
-pub fn run_test(stream: &[u8], interpreter: &mut Interpreter<impl Handler + Clone>) -> Result<(), AmlError> {
-    interpreter.load_table(stream)?;
+/// * `interpreter`: The interpreter to test with. The interpreter is consumed to maintain unwind
+///   safety - if the interpreter panics, the caller should not be able to see the interpreter in
+///   an inconsistent state.
+pub fn run_test<T>(stream: &[u8], interpreter: Interpreter<T>) -> RunTestResult<T>
+where
+    T: Handler,
+{
+    // Without `AssertUnwindSafe`, the following code will not build as the Interpreter is not
+    // unwind safe. To avoid the caller being able to see an inconsistent Interpreter, if a panic
+    // occurs we drop the Interpreter, forcing the caller to create a new one.
+    let result = catch_unwind(AssertUnwindSafe(|| -> Result<(), AmlError> {
+        interpreter.load_table(stream)?;
 
-    if let Some(result) = interpreter.evaluate_if_present(AmlName::from_str("\\MAIN").unwrap(), vec![])? {
-        match *result {
-            Object::Integer(0) => Ok(()),
-            Object::Integer(other) => {
-                error!("Test _MAIN returned non-zero exit code: {}", other);
-                // TODO: wrong error - this should probs return a more complex err type
-                Err(AmlError::NoCurrentOp)
-            }
-            _ => {
-                error!("Test _MAIN returned unexpected object type: {}", *result);
-                // TODO: wrong error
-                Err(AmlError::NoCurrentOp)
+        if let Some(result) = interpreter.evaluate_if_present(AmlName::from_str("\\MAIN").unwrap(), vec![])? {
+            match *result {
+                Object::Integer(0) => Ok(()),
+                Object::Integer(other) => {
+                    error!("Test _MAIN returned non-zero exit code: {}", other);
+                    // TODO: wrong error - this should probs return a more complex err type
+                    Err(AmlError::NoCurrentOp)
+                }
+                _ => {
+                    error!("Test _MAIN returned unexpected object type: {}", *result);
+                    // TODO: wrong error
+                    Err(AmlError::NoCurrentOp)
+                }
             }
+        } else {
+            Ok(())
         }
-    } else {
-        Ok(())
+    }));
+
+    match result {
+        Ok(inner) => match inner {
+            Ok(()) => RunTestResult::Pass(interpreter),
+            Err(e) => RunTestResult::Failed(interpreter, TestFailureReason::ParseFail(e)),
+        },
+        Err(_e) => RunTestResult::Panicked,
     }
 }