Directly test a stream of opcodes

src/aml/mod.rs

@@ -3018,13 +3018,6 @@ impl MethodContext {
         }
     }
 
-    fn last_op(&mut self) -> Result<&mut OpInFlight, AmlError> {
-        match self.in_flight.last_mut() {
-            Some(op) => Ok(op),
-            None => Err(AmlError::NoCurrentOp),
-        }
-    }
-
     fn contribute_arg(&mut self, arg: Argument) {
         if let Some(in_flight) = self.in_flight.last_mut()
             && in_flight.arguments.len() < in_flight.expected_arguments

src/aml/resource.rs

@@ -551,7 +551,6 @@ fn extended_interrupt_descriptor(bytes: &[u8]) -> Result<Resource, AmlError> {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use alloc::sync::Arc;
 
     #[test]
     fn test_parses_keyboard_crs() {

tests/stray_opcodes_test.rs

@@ -0,0 +1,14 @@
+mod test_infra;
+
+use crate::test_infra::run_opcodes_test;
+use aml_test_tools::handlers::null_handler::NullHandler;
+
+#[test]
+fn test_stray_opcode() {
+    // Make sure that a stray `One` opcode is ignored (see issue #289)
+    // The first opcode is `One`, the remainder are `Return (0)`.
+    let opcodes: Vec<u8> = vec![0x01, 0xA4, 0x0A, 0x00];
+
+    let handler = NullHandler;
+    run_opcodes_test(&opcodes, handler);
+}

tests/test_infra/mod.rs

@@ -1,12 +1,14 @@
 use acpi::Handler;
-use aml_test_tools::{
-    RunTestResult,
-    TestResult,
-    handlers::logging_handler::LoggingHandler,
-    new_interpreter,
-    run_test_for_string,
-};
+use aml_test_tools::{RunTestResult, TestResult, handlers::logging_handler::LoggingHandler, new_interpreter, run_test_for_string, run_test_for_opcodes};
 
+// The following two functions are very similar in structure, but whilst there are only two of them
+// it's not worth adding complexity to make them DRY.
+
+/// Run a test against an ASL string.
+///
+/// The string `asl` represents a compile-able ASL string, so needs to include the `DefinitionBlock`
+/// statement.
+#[allow(dead_code)]
 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();
@@ -17,3 +19,19 @@ pub fn run_aml_test(asl: &'static str, handler: impl Handler) {
     let result = run_test_for_string(asl, interpreter, &None);
     assert!(matches!(result, RunTestResult::Pass(_)), "Test failed with: {:?}", TestResult::from(&result));
 }
+
+/// Run a test against a sequence of AML opcodes.
+///
+/// The provided opcodes are wrapped in a `MAIN` method and also have a table header prepended. The
+/// `MAIN` function is executed as part of the test, so `opcodes` only needs to include the actual
+/// opcodes to execute.
+#[allow(dead_code)]
+pub fn run_opcodes_test(opcodes: &[u8], handler: impl Handler) {
+    let _ = pretty_env_logger::try_init();
+
+    let logged_handler = LoggingHandler::new(handler);
+    let interpreter = new_interpreter(logged_handler);
+
+    let result = run_test_for_opcodes(opcodes, interpreter, &None);
+    assert!(matches!(result, RunTestResult::Pass(_)), "Test failed with: {:?}", TestResult::from(&result));
+}

tools/aml_test_tools/src/lib.rs

@@ -5,6 +5,7 @@
 //! As always, feel free to offer PRs for improvements.
 
 pub mod handlers;
+pub mod pkglength;
 pub mod result;
 pub mod tables;
 
@@ -17,7 +18,7 @@ use acpi::{
     PhysicalMapping,
     address::MappedGas,
     aml::{AmlError, Interpreter, namespace::AmlName},
-    sdt::{Signature, facs::Facs},
+    sdt::{SdtHeader, Signature, facs::Facs},
 };
 use log::{error, trace};
 use std::{
@@ -330,6 +331,61 @@ where
     run_test(tables, interpreter, expected_result)
 }
 
+/// Test a sequence of opcodes.
+///
+/// This may be useful for testing a sequence of opcodes that are seen in the wild, but which
+/// can't be generated by the AML compiler. (For example, see issue #289 where stray `One` opcodes
+/// were seen in AML - the `iasl` compiler will not generate this sequence.)
+///
+/// The provided opcodes are wrapped in a `MAIN` method and also have a table header prepended.
+///
+/// As with all other tests: if `MAIN` returns either 0 or undefined, then the test is considered
+/// successful.
+pub fn run_test_for_opcodes<T>(
+    opcodes: &[u8],
+    interpreter: Interpreter<T>,
+    expected_result: &Option<ExpectedResult>,
+) -> RunTestResult<T>
+where
+    T: Handler,
+{
+    // DefMethod := MethodOp PkgLength NameString MethodFlags TermList
+    let mut method_bytes: Vec<u8> = vec![0x14];
+    // PkgLength - add 5 to cover the length of the method name and flags.
+    method_bytes.extend(pkglength::encode(opcodes.len() as u32 + 5).iter());
+    method_bytes.extend(r"MAIN".as_bytes()); // NameString
+    method_bytes.extend([0]); // MethodFlags
+    // Nothing for termlist
+
+    let table_header = SdtHeader {
+        signature: Signature::DSDT,
+        length: (size_of::<SdtHeader>() + opcodes.len() + method_bytes.len()) as u32,
+        revision: 1,
+        checksum: 0, // The crate doesn't check checksums, so no need to set it.
+        oem_id: [0; 6],
+        oem_table_id: [0; 8],
+        oem_revision: 1,
+        creator_id: [0; 4],
+        creator_revision: 1,
+    };
+
+    // Safe as we retain ownership of table_header and then drop both it and the slice at the end
+    // of this function.
+    let table_hdr_slice =
+        unsafe { std::slice::from_raw_parts(&table_header as *const _ as *const u8, size_of::<SdtHeader>()) };
+
+    let mut table_data = vec![];
+    table_data.extend(table_hdr_slice);
+    table_data.extend(method_bytes);
+    table_data.extend(opcodes);
+
+    let Ok(tables) = bytes_to_tables(&table_data) else {
+        return RunTestResult::Failed(interpreter, TestFailureReason::TablesErr);
+    };
+
+    run_test(tables, interpreter, expected_result)
+}
+
 /// Internal function to create a temporary script file from an ASL string, plus to calculate the
 /// name of the post-compilation AML file.
 fn create_script_file(asl: &'static str) -> TempScriptFile {

tools/aml_test_tools/src/pkglength.rs

@@ -0,0 +1,54 @@
+//! pkglength encoding.
+
+use std::{vec, vec::Vec};
+
+/// Encode a pkglength field.
+///
+/// This is a variable length field used to define the length of other variable-length items in
+/// ACPI - see [the spec](https://uefi.org/specs/ACPI/6.6/20_AML_Specification.html#package-length-encoding)
+/// for details.
+///
+/// This is less straightforward than it could be, because pkglength needs to include the length of
+/// the pkglength field that gets output. This function adds that extra length.
+///
+/// Panics if length >= 2^28. Otherwise, returns the encoded pkglength in a vec, LSB-first.
+pub fn encode(data_length: u32) -> Vec<u8> {
+    let extra_length = match data_length {
+        0..63 => 1,
+        63..0xFFE => 2,
+        0xFFE..0xFFFFD => 3,
+        _ => 4,
+    };
+    let result = encode_raw(data_length + extra_length);
+
+    // Check the table above is correct.
+    assert_eq!(result.len(), extra_length as usize);
+
+    result
+}
+
+/// Encode a pkglength field, without taking into account the extra length of the pkglength field.
+///
+/// Most callers should use [`encode`] instead.
+///
+/// Panics if length >= 2^28. Otherwise, returns the encoded pkglength in a vec, LSB-first.
+pub fn encode_raw(mut length: u32) -> Vec<u8> {
+    assert_eq!(length & 0xF0000000, 0);
+
+    if length < 64 {
+        vec![length as u8]
+    } else {
+        let mut result = vec![(length & 0xF) as u8];
+        length >>= 4;
+
+        while length != 0 {
+            result.push((length & 0xff) as u8);
+            length >>= 8;
+        }
+
+        let num_bytes = result.len() as u8 - 1;
+        result[0] |= num_bytes << 6;
+
+        result
+    }
+}