From a4d3de1acfb1188b48c9a9a695d0c493b627c961 Mon Sep 17 00:00:00 2001 From: Martin Hughes Date: Sun, 4 Oct 2026 22:04:52 +0100 Subject: [PATCH 1/9] Start prototyping match expression --- src/aml/mod.rs | 99 +++++++++++++++++++++++++++++++++++++++---------- tests/match.asl | 6 +++ 2 files changed, 85 insertions(+), 20 deletions(-) create mode 100644 tests/match.asl diff --git a/src/aml/mod.rs b/src/aml/mod.rs index e317f65d..cbd02168 100644 --- a/src/aml/mod.rs +++ b/src/aml/mod.rs @@ -1076,6 +1076,11 @@ where context.retire_op(op); } } + Opcode::Match => { + extract_args!(op => [Argument::Object(search_pkg), Argument::ByteData(opcode_a), Argument::Object(operand_a), Argument::ByteData(opcode_b), Argument::Object(operand_b), Argument::Object(start_index)]); + + panic!("Match!"); + } _ => panic!("Unexpected operation has created in-flight op!"), } } @@ -1200,17 +1205,35 @@ where } Err(other_err) => return Err(other_err), }; + let resolve_behaviour = context.in_flight.last().map(|op| op.resolve_behaviour()); match opcode { Opcode::Zero => { - /* - * This represents a `Zero` operand that should create an `Integer` operand in - * most places, but could also encode a `NullName` if we are expecting a - * `Target`. We handle the latter in logic for stores to targets. - */ - context.contribute_arg(Argument::Object(Object::Integer(0).wrap())); + if Some(ResolveBehaviour::MatchOpcode) == resolve_behaviour { + // This represents a 0 argument to a match statement (`MTR`) + context.contribute_arg(Argument::ByteData(0)); + } else { + /* + * This represents a `Zero` operand that should create an `Integer` operand in + * most places, but could also encode a `NullName` if we are expecting a + * `Target`. We handle the latter in logic for stores to targets. + */ + context.contribute_arg(Argument::Object(Object::Integer(0).wrap())); + } } Opcode::One => { - context.contribute_arg(Argument::Object(Object::Integer(1).wrap())); + if Some(ResolveBehaviour::MatchOpcode) == resolve_behaviour { + // As above, this represents a 1 argument to a match statement (`MEQ`) + context.contribute_arg(Argument::ByteData(1)); + } else { + context.contribute_arg(Argument::Object(Object::Integer(1).wrap())); + } + } + Opcode::MatchOpcode(opcode) => { + if Some(ResolveBehaviour::MatchOpcode) == resolve_behaviour { + context.contribute_arg(Argument::ByteData(opcode)); + } else { + Err(AmlError::InternalError("TODO: Better error".into()))? + } } Opcode::Ones => { context.contribute_arg(Argument::Object(Object::Integer(u64::MAX).wrap())); @@ -1590,11 +1613,7 @@ where context.current_block.pc -= 1; let name = context.namestring()?; - let behaviour = context - .in_flight - .last() - .map(|op| op.resolve_behaviour()) - .unwrap_or(ResolveBehaviour::TermArg); + let behaviour = resolve_behaviour.unwrap_or(ResolveBehaviour::TermArg); match behaviour { // XXX: `NullName` is handled separately given its ambiguity with `Zero` ResolveBehaviour::SimpleName | ResolveBehaviour::SuperName | ResolveBehaviour::Target => { @@ -1676,6 +1695,9 @@ where ResolveBehaviour::Placeholder => { panic!("Invalid resolve behaviour for name to be resolved!") } + ResolveBehaviour::MatchOpcode => { + Err(AmlError::InternalError("TODO: Better error".into()))? + } } } @@ -1726,14 +1748,17 @@ where opcode, &[ResolveBehaviour::TermArg, ResolveBehaviour::TermArg, ResolveBehaviour::Target], )), - /* - * TODO - * Match is a difficult opcode to parse, as it interleaves dynamic arguments and - * random bytes that need to be extracted as you go. I think we'll need to use 1+ - * internal in-flight ops to parse the static bytedatas as we go, and then retire - * the real op at the end. - */ - Opcode::Match => todo!(), + Opcode::Match => context.start(OpInFlight::new( + opcode, + &[ + ResolveBehaviour::TermArg, + ResolveBehaviour::MatchOpcode, + ResolveBehaviour::TermArg, + ResolveBehaviour::MatchOpcode, + ResolveBehaviour::TermArg, + ResolveBehaviour::TermArg, + ], + )), Opcode::CreateBitField | Opcode::CreateByteField @@ -2955,6 +2980,8 @@ enum ResolveBehaviour { /// Used with [`OpInFlight::new_with`] to represent arguments that have already been resolved /// when an operation enters flight. Placeholder, + /// The match statement takes two single byte arguments - the "match opcode". + MatchOpcode, } #[derive(Debug)] @@ -3117,6 +3144,7 @@ impl MethodContext { Ok(match opcode { 0x00 => Opcode::Zero, 0x01 => Opcode::One, + 0x02..0x06 => Opcode::MatchOpcode(opcode as u8), 0x06 => Opcode::Alias, 0x08 => Opcode::Name, 0x0a => Opcode::BytePrefix, @@ -3368,6 +3396,7 @@ impl MethodContext { enum Opcode { Zero, One, + MatchOpcode(u8), Alias, Name, BytePrefix, @@ -3603,3 +3632,33 @@ impl IntegerSize { if revision >= 2 { IntegerSize::EightBytes } else { IntegerSize::FourBytes } } } + +#[derive(Debug, Clone)] +pub enum MatchOp { + MTR, + MEQ, + MLE, + MLT, + MGE, + MGT, +} + +impl TryFrom<&Argument> for MatchOp { + type Error = AmlError; + + fn try_from(value: &Argument) -> Result { + let Argument::ByteData(opcode) = value else { + return Err(AmlError::InternalError("TODO: Better error".into())); + }; + + Ok(match opcode { + 0 => Self::MTR, + 1 => Self::MEQ, + 2 => Self::MLE, + 3 => Self::MLT, + 4 => Self::MGE, + 5 => Self::MGT, + _ => Err(AmlError::InternalError("TODO: Better error".into()))?, + }) + } +} diff --git a/tests/match.asl b/tests/match.asl new file mode 100644 index 00000000..3f47e884 --- /dev/null +++ b/tests/match.asl @@ -0,0 +1,6 @@ +DefinitionBlock("", "DSDT", 1, "RSACPI", "METHOD", 1) { + Name (AAAA, Package (1) { 0xaa }) + Method (MAIN) { + Local0 = Match (AAAA, MEQ, 2, MTR, 0, 0) + } +} \ No newline at end of file From 36bdcae9267a0e669bbf7662b02342a32963916c Mon Sep 17 00:00:00 2001 From: Martin Hughes Date: Mon, 5 Oct 2026 13:31:56 +0100 Subject: [PATCH 2/9] Running prototype --- src/aml/match_expr.rs | 80 +++++++++++++++++++++++++++++++++++++++++++ src/aml/mod.rs | 53 ++++++++++++---------------- src/aml/object.rs | 4 ++- tests/match.asl | 6 ++-- 4 files changed, 109 insertions(+), 34 deletions(-) create mode 100644 src/aml/match_expr.rs diff --git a/src/aml/match_expr.rs b/src/aml/match_expr.rs new file mode 100644 index 00000000..d521b078 --- /dev/null +++ b/src/aml/match_expr.rs @@ -0,0 +1,80 @@ +use crate::aml::{AmlError, object::Package}; + +const ONES: u64 = !0; + +pub fn do_match( + search_package: &Package, + op_a: MatchOp, + operand_a: u64, + op_b: MatchOp, + operand_b: u64, + start_index: u64, +) -> u64 { + for (idx, row) in search_package[start_index as usize..].iter().enumerate() { + let Ok(v) = row.as_integer() else { + continue; + }; + + if match_comparison(v, op_a, operand_a) && match_comparison(v, op_b, operand_b) { + return idx as u64; + } + } + + ONES +} + +fn match_comparison(value: u64, op: MatchOp, operand: u64) -> bool { + match op { + MatchOp::MTR => true, + MatchOp::MEQ => value == operand, + MatchOp::MLE => value <= operand, + MatchOp::MLT => value < operand, + MatchOp::MGE => value >= operand, + MatchOp::MGT => value > operand, + } +} + +#[allow(clippy::upper_case_acronyms)] // Allow the same capitalisation as the spec. +#[derive(Debug, Clone, Copy)] +pub enum MatchOp { + MTR, + MEQ, + MLE, + MLT, + MGE, + MGT, +} + +impl TryFrom<&u8> for MatchOp { + type Error = AmlError; + + fn try_from(value: &u8) -> Result { + Ok(match value { + 0 => Self::MTR, + 1 => Self::MEQ, + 2 => Self::MLE, + 3 => Self::MLT, + 4 => Self::MGE, + 5 => Self::MGT, + _ => Err(AmlError::InternalError("TODO: Better error".into()))?, + }) + } +} + +#[cfg(test)] +mod tests { + use crate::aml::match_expr::MatchOp::MEQ; + use crate::aml::object::Object; + use super::*; + + #[test] + fn empty_package_doesnt_match() { + assert_eq!(ONES, do_match(&vec![], MatchOp::MEQ, 0, MatchOp::MTR, 0, 0)); + } + + #[test] + fn basic_match() { + let pkg = &([0, 1, 2, 3].iter().map(|i| Object::Integer(*i as u64).wrap()).collect()); + assert_eq!(2, do_match(pkg, MEQ, 2, MatchOp::MTR, 0, 0)); + } +} diff --git a/src/aml/mod.rs b/src/aml/mod.rs index cbd02168..43efc618 100644 --- a/src/aml/mod.rs +++ b/src/aml/mod.rs @@ -17,6 +17,8 @@ */ mod interrupt_model_used; +mod match_expr; + pub mod namespace; pub mod object; pub mod op_region; @@ -51,6 +53,7 @@ use core::{ }; pub use interrupt_model_used::InterruptModelUsed; use log::{error, info, trace, warn}; +use match_expr::do_match; use namespace::{AmlName, Namespace, NamespaceLevelKind}; use object::{ DeviceStatus, @@ -1079,7 +1082,21 @@ where Opcode::Match => { extract_args!(op => [Argument::Object(search_pkg), Argument::ByteData(opcode_a), Argument::Object(operand_a), Argument::ByteData(opcode_b), Argument::Object(operand_b), Argument::Object(start_index)]); - panic!("Match!"); + let Object::Package(ref pkg) = **search_pkg else { + return Err(AmlError::InternalError("TODO: Better error".into())); + }; + + let result = do_match( + pkg, + opcode_a.try_into()?, + operand_a.as_integer()?, + opcode_b.try_into()?, + operand_b.as_integer()?, + start_index.as_integer()?, + ); + + context.contribute_arg(Argument::Object(Object::Integer(result).wrap())); + context.retire_op(op); } _ => panic!("Unexpected operation has created in-flight op!"), } @@ -1207,6 +1224,10 @@ where }; let resolve_behaviour = context.in_flight.last().map(|op| op.resolve_behaviour()); match opcode { + // Opcodes Zero and One overlap with the Match Opcodes that are only used in the + // match expression. It's reasonable to assume that if Zero or One appear at the + // time we expect a Match Opcode, then they *are* Match Opcodes instead of regular + // arguments. Opcode::Zero => { if Some(ResolveBehaviour::MatchOpcode) == resolve_behaviour { // This represents a 0 argument to a match statement (`MTR`) @@ -3632,33 +3653,3 @@ impl IntegerSize { if revision >= 2 { IntegerSize::EightBytes } else { IntegerSize::FourBytes } } } - -#[derive(Debug, Clone)] -pub enum MatchOp { - MTR, - MEQ, - MLE, - MLT, - MGE, - MGT, -} - -impl TryFrom<&Argument> for MatchOp { - type Error = AmlError; - - fn try_from(value: &Argument) -> Result { - let Argument::ByteData(opcode) = value else { - return Err(AmlError::InternalError("TODO: Better error".into())); - }; - - Ok(match opcode { - 0 => Self::MTR, - 1 => Self::MEQ, - 2 => Self::MLE, - 3 => Self::MLT, - 4 => Self::MGE, - 5 => Self::MGT, - _ => Err(AmlError::InternalError("TODO: Better error".into()))?, - }) - } -} diff --git a/src/aml/object.rs b/src/aml/object.rs index 7c788467..740043c2 100644 --- a/src/aml/object.rs +++ b/src/aml/object.rs @@ -11,6 +11,8 @@ use static_assertions::assert_impl_all; type NativeMethod = dyn Fn(&[WrappedObject]) -> Result + Send + Sync; +pub type Package = Vec; + #[derive(Clone)] pub enum Object { Uninitialized, @@ -26,7 +28,7 @@ pub enum Object { Reference { kind: ReferenceKind, inner: WrappedObject }, NamePath { name: AmlName, scope: AmlName }, OpRegion(OpRegion), - Package(Vec), + Package(Package), PowerResource { system_level: u8, resource_order: u16 }, Processor { proc_id: u8, pblk_address: u32, pblk_length: u8 }, RawDataBuffer, diff --git a/tests/match.asl b/tests/match.asl index 3f47e884..6b7880b6 100644 --- a/tests/match.asl +++ b/tests/match.asl @@ -1,6 +1,8 @@ -DefinitionBlock("", "DSDT", 1, "RSACPI", "METHOD", 1) { - Name (AAAA, Package (1) { 0xaa }) +DefinitionBlock("", "DSDT", 1, "RSACPI", "MATCH", 1) { + Name (AAAA, Package (4) { 0x00, 0x01, 0x02, 0x03 }) Method (MAIN) { Local0 = Match (AAAA, MEQ, 2, MTR, 0, 0) + + Return (!(Local0 == 2)) } } \ No newline at end of file From 1b49d6bf861ad7d36a1b46a9eb53ded38d4ebffe Mon Sep 17 00:00:00 2001 From: Martin Hughes Date: Thu, 8 Oct 2026 15:50:40 +0100 Subject: [PATCH 3/9] Make match opcode decoding part of `opcode` --- src/aml/match_expr.rs | 27 +++++++------- src/aml/mod.rs | 82 +++++++++++++++++++++++++------------------ 2 files changed, 60 insertions(+), 49 deletions(-) diff --git a/src/aml/match_expr.rs b/src/aml/match_expr.rs index d521b078..3e303eb2 100644 --- a/src/aml/match_expr.rs +++ b/src/aml/match_expr.rs @@ -4,9 +4,9 @@ const ONES: u64 = !0; pub fn do_match( search_package: &Package, - op_a: MatchOp, + op_a: &MatchOp, operand_a: u64, - op_b: MatchOp, + op_b: &MatchOp, operand_b: u64, start_index: u64, ) -> u64 { @@ -23,7 +23,7 @@ pub fn do_match( ONES } -fn match_comparison(value: u64, op: MatchOp, operand: u64) -> bool { +fn match_comparison(value: u64, op: &MatchOp, operand: u64) -> bool { match op { MatchOp::MTR => true, MatchOp::MEQ => value == operand, @@ -35,7 +35,7 @@ fn match_comparison(value: u64, op: MatchOp, operand: u64) -> bool { } #[allow(clippy::upper_case_acronyms)] // Allow the same capitalisation as the spec. -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum MatchOp { MTR, MEQ, @@ -45,36 +45,33 @@ pub enum MatchOp { MGT, } -impl TryFrom<&u8> for MatchOp { - type Error = AmlError; - - fn try_from(value: &u8) -> Result { - Ok(match value { +impl From for MatchOp { + fn from(value: u16) -> Self { + match value { 0 => Self::MTR, 1 => Self::MEQ, 2 => Self::MLE, 3 => Self::MLT, 4 => Self::MGE, 5 => Self::MGT, - _ => Err(AmlError::InternalError("TODO: Better error".into()))?, - }) + _ => panic!("Invalid Match Opcode"), + } } } #[cfg(test)] mod tests { - use crate::aml::match_expr::MatchOp::MEQ; - use crate::aml::object::Object; use super::*; + use crate::aml::object::Object; #[test] fn empty_package_doesnt_match() { - assert_eq!(ONES, do_match(&vec![], MatchOp::MEQ, 0, MatchOp::MTR, 0, 0)); + assert_eq!(ONES, do_match(&vec![], &MatchOp::MEQ, 0, &MatchOp::MTR, 0, 0)); } #[test] fn basic_match() { let pkg = &([0, 1, 2, 3].iter().map(|i| Object::Integer(*i as u64).wrap()).collect()); - assert_eq!(2, do_match(pkg, MEQ, 2, MatchOp::MTR, 0, 0)); + assert_eq!(2, do_match(pkg, &MatchOp::MEQ, 2, &MatchOp::MTR, 0, 0)); } } diff --git a/src/aml/mod.rs b/src/aml/mod.rs index 43efc618..623977ed 100644 --- a/src/aml/mod.rs +++ b/src/aml/mod.rs @@ -9,7 +9,6 @@ * - Load and LoadTable * - DefDataRegion * - Notify - * - DefMatch * * - Method recursion depth? * - Loop timeouts @@ -53,7 +52,7 @@ use core::{ }; pub use interrupt_model_used::InterruptModelUsed; use log::{error, info, trace, warn}; -use match_expr::do_match; +use match_expr::{MatchOp, do_match}; use namespace::{AmlName, Namespace, NamespaceLevelKind}; use object::{ DeviceStatus, @@ -1080,7 +1079,14 @@ where } } Opcode::Match => { - extract_args!(op => [Argument::Object(search_pkg), Argument::ByteData(opcode_a), Argument::Object(operand_a), Argument::ByteData(opcode_b), Argument::Object(operand_b), Argument::Object(start_index)]); + extract_args!(op => [ + Argument::Object(search_pkg), + Argument::MatchOp(opcode_a), + Argument::Object(operand_a), + Argument::MatchOp(opcode_b), + Argument::Object(operand_b), + Argument::Object(start_index)] + ); let Object::Package(ref pkg) = **search_pkg else { return Err(AmlError::InternalError("TODO: Better error".into())); @@ -1088,9 +1094,9 @@ where let result = do_match( pkg, - opcode_a.try_into()?, + opcode_a, operand_a.as_integer()?, - opcode_b.try_into()?, + opcode_b, operand_b.as_integer()?, start_index.as_integer()?, ); @@ -1222,36 +1228,22 @@ where } Err(other_err) => return Err(other_err), }; - let resolve_behaviour = context.in_flight.last().map(|op| op.resolve_behaviour()); + match opcode { - // Opcodes Zero and One overlap with the Match Opcodes that are only used in the - // match expression. It's reasonable to assume that if Zero or One appear at the - // time we expect a Match Opcode, then they *are* Match Opcodes instead of regular - // arguments. Opcode::Zero => { - if Some(ResolveBehaviour::MatchOpcode) == resolve_behaviour { - // This represents a 0 argument to a match statement (`MTR`) - context.contribute_arg(Argument::ByteData(0)); - } else { - /* - * This represents a `Zero` operand that should create an `Integer` operand in - * most places, but could also encode a `NullName` if we are expecting a - * `Target`. We handle the latter in logic for stores to targets. - */ - context.contribute_arg(Argument::Object(Object::Integer(0).wrap())); - } + /* + * This represents a `Zero` operand that should create an `Integer` operand in + * most places, but could also encode a `NullName` if we are expecting a + * `Target`. We handle the latter in logic for stores to targets. + */ + context.contribute_arg(Argument::Object(Object::Integer(0).wrap())); } Opcode::One => { - if Some(ResolveBehaviour::MatchOpcode) == resolve_behaviour { - // As above, this represents a 1 argument to a match statement (`MEQ`) - context.contribute_arg(Argument::ByteData(1)); - } else { - context.contribute_arg(Argument::Object(Object::Integer(1).wrap())); - } + context.contribute_arg(Argument::Object(Object::Integer(1).wrap())); } - Opcode::MatchOpcode(opcode) => { - if Some(ResolveBehaviour::MatchOpcode) == resolve_behaviour { - context.contribute_arg(Argument::ByteData(opcode)); + Opcode::MatchOp(opcode) => { + if context.expecting_match_opcode() { + context.contribute_arg(Argument::MatchOp(opcode)); } else { Err(AmlError::InternalError("TODO: Better error".into()))? } @@ -1634,6 +1626,7 @@ where context.current_block.pc -= 1; let name = context.namestring()?; + let resolve_behaviour = context.in_flight.last().map(|op| op.resolve_behaviour()); let behaviour = resolve_behaviour.unwrap_or(ResolveBehaviour::TermArg); match behaviour { // XXX: `NullName` is handled separately given its ambiguity with `Zero` @@ -3021,6 +3014,7 @@ enum Argument { DWordData(u32), TrackedPc(usize), PkgLength(usize), + MatchOp(MatchOp), } impl OpInFlight { @@ -3162,10 +3156,25 @@ impl MethodContext { other => other as u16, }; + // Opcodes Zero and One overlap with the Match Opcodes that are only used in the match + // expression. It's reasonable to assume that if Zero or One appear at the time we expect a + // Match Opcode, then they *are* Match Opcodes instead of regular arguments. Ok(match opcode { - 0x00 => Opcode::Zero, - 0x01 => Opcode::One, - 0x02..0x06 => Opcode::MatchOpcode(opcode as u8), + 0x00 => { + if self.expecting_match_opcode() { + Opcode::MatchOp(opcode.into()) + } else { + Opcode::Zero + } + } + 0x01 => { + if self.expecting_match_opcode() { + Opcode::MatchOp(opcode.into()) + } else { + Opcode::One + } + } + 0x02..0x06 => Opcode::MatchOp(opcode.into()), 0x06 => Opcode::Alias, 0x08 => Opcode::Name, 0x0a => Opcode::BytePrefix, @@ -3411,13 +3420,18 @@ impl MethodContext { Ok(self.current_block.stream()[self.current_block.pc]) } + + fn expecting_match_opcode(&self) -> bool { + let resolve_behaviour = self.in_flight.last().map(|op| op.resolve_behaviour()); + resolve_behaviour.is_some_and(|rb| matches!(rb, ResolveBehaviour::MatchOpcode)) + } } #[derive(Clone, Copy, PartialEq, Debug)] enum Opcode { Zero, One, - MatchOpcode(u8), + MatchOp(MatchOp), Alias, Name, BytePrefix, From 9eeeb5cd91772a3363596cc428ae5259c37dff2c Mon Sep 17 00:00:00 2001 From: Martin Hughes Date: Fri, 9 Oct 2026 08:38:48 +0100 Subject: [PATCH 4/9] Remove `MatchOp::from` --- src/aml/match_expr.rs | 16 +--------------- src/aml/mod.rs | 17 +++++++++++------ 2 files changed, 12 insertions(+), 21 deletions(-) diff --git a/src/aml/match_expr.rs b/src/aml/match_expr.rs index 3e303eb2..0c6922da 100644 --- a/src/aml/match_expr.rs +++ b/src/aml/match_expr.rs @@ -1,4 +1,4 @@ -use crate::aml::{AmlError, object::Package}; +use crate::aml::object::Package; const ONES: u64 = !0; @@ -45,20 +45,6 @@ pub enum MatchOp { MGT, } -impl From for MatchOp { - fn from(value: u16) -> Self { - match value { - 0 => Self::MTR, - 1 => Self::MEQ, - 2 => Self::MLE, - 3 => Self::MLT, - 4 => Self::MGE, - 5 => Self::MGT, - _ => panic!("Invalid Match Opcode"), - } - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/src/aml/mod.rs b/src/aml/mod.rs index 623977ed..cc1dc834 100644 --- a/src/aml/mod.rs +++ b/src/aml/mod.rs @@ -1228,7 +1228,6 @@ where } Err(other_err) => return Err(other_err), }; - match opcode { Opcode::Zero => { /* @@ -1626,8 +1625,11 @@ where context.current_block.pc -= 1; let name = context.namestring()?; - let resolve_behaviour = context.in_flight.last().map(|op| op.resolve_behaviour()); - let behaviour = resolve_behaviour.unwrap_or(ResolveBehaviour::TermArg); + let behaviour = context + .in_flight + .last() + .map(|op| op.resolve_behaviour()) + .unwrap_or(ResolveBehaviour::TermArg); match behaviour { // XXX: `NullName` is handled separately given its ambiguity with `Zero` ResolveBehaviour::SimpleName | ResolveBehaviour::SuperName | ResolveBehaviour::Target => { @@ -3162,19 +3164,22 @@ impl MethodContext { Ok(match opcode { 0x00 => { if self.expecting_match_opcode() { - Opcode::MatchOp(opcode.into()) + Opcode::MatchOp(MatchOp::MTR) } else { Opcode::Zero } } 0x01 => { if self.expecting_match_opcode() { - Opcode::MatchOp(opcode.into()) + Opcode::MatchOp(MatchOp::MEQ) } else { Opcode::One } } - 0x02..0x06 => Opcode::MatchOp(opcode.into()), + 0x02 => Opcode::MatchOp(MatchOp::MLE), + 0x03 => Opcode::MatchOp(MatchOp::MLT), + 0x04 => Opcode::MatchOp(MatchOp::MGE), + 0x05 => Opcode::MatchOp(MatchOp::MGT), 0x06 => Opcode::Alias, 0x08 => Opcode::Name, 0x0a => Opcode::BytePrefix, From b174260be80ca911254ac62c962f565497b7053b Mon Sep 17 00:00:00 2001 From: Martin Hughes Date: Fri, 9 Oct 2026 08:50:02 +0100 Subject: [PATCH 5/9] Improve error codes --- src/aml/match_expr.rs | 13 +++++++------ src/aml/mod.rs | 17 ++++++++++++----- tests/match.asl | 2 +- 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/aml/match_expr.rs b/src/aml/match_expr.rs index 0c6922da..3a54b873 100644 --- a/src/aml/match_expr.rs +++ b/src/aml/match_expr.rs @@ -36,13 +36,14 @@ fn match_comparison(value: u64, op: &MatchOp, operand: u64) -> bool { #[allow(clippy::upper_case_acronyms)] // Allow the same capitalisation as the spec. #[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[repr(u16)] pub enum MatchOp { - MTR, - MEQ, - MLE, - MLT, - MGE, - MGT, + MTR = 0, + MEQ = 1, + MLE = 2, + MLT = 3, + MGE = 4, + MGT = 5, } #[cfg(test)] diff --git a/src/aml/mod.rs b/src/aml/mod.rs index cc1dc834..e9c23206 100644 --- a/src/aml/mod.rs +++ b/src/aml/mod.rs @@ -1089,7 +1089,10 @@ where ); let Object::Package(ref pkg) = **search_pkg else { - return Err(AmlError::InternalError("TODO: Better error".into())); + return Err(AmlError::ObjectNotOfExpectedType { + expected: ObjectType::Package, + got: search_pkg.typ(), + }); }; let result = do_match( @@ -1244,7 +1247,7 @@ where if context.expecting_match_opcode() { context.contribute_arg(Argument::MatchOp(opcode)); } else { - Err(AmlError::InternalError("TODO: Better error".into()))? + Err(AmlError::IllegalOpcode(opcode as u16))? } } Opcode::Ones => { @@ -1711,9 +1714,7 @@ where ResolveBehaviour::Placeholder => { panic!("Invalid resolve behaviour for name to be resolved!") } - ResolveBehaviour::MatchOpcode => { - Err(AmlError::InternalError("TODO: Better error".into()))? - } + ResolveBehaviour::MatchOpcode => Err(AmlError::InvalidArgType)?, } } @@ -3659,6 +3660,12 @@ pub enum AmlError { /// The maximum length of time a loop ([`LOOP_TIMEOUT_NS`]) can execute for has been exceeded. LoopTimeout, + + /// The provided argument to the current operation is invalid. + /// + /// For example, trying to pass an object as the Operation parameter in a Match expression is + /// invalid. + InvalidArgType, } #[derive(Debug, Clone, Copy, PartialEq)] diff --git a/tests/match.asl b/tests/match.asl index 6b7880b6..91d41327 100644 --- a/tests/match.asl +++ b/tests/match.asl @@ -5,4 +5,4 @@ DefinitionBlock("", "DSDT", 1, "RSACPI", "MATCH", 1) { Return (!(Local0 == 2)) } -} \ No newline at end of file +} From c5de42a999749959dd7f262ed73bcccd78977e99 Mon Sep 17 00:00:00 2001 From: Martin Hughes Date: Fri, 9 Oct 2026 12:42:43 +0100 Subject: [PATCH 6/9] Start adding tests Still needs to be able to check failures from an opcodes test --- src/aml/match_expr.rs | 46 ++++++++++++++++++++++++++++++++++++++++++- tests/match.asl | 32 +++++++++++++++++++++++++++--- tests/match_expr.rs | 45 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 4 deletions(-) create mode 100644 tests/match_expr.rs diff --git a/src/aml/match_expr.rs b/src/aml/match_expr.rs index 3a54b873..e46954e0 100644 --- a/src/aml/match_expr.rs +++ b/src/aml/match_expr.rs @@ -16,7 +16,7 @@ pub fn do_match( }; if match_comparison(v, op_a, operand_a) && match_comparison(v, op_b, operand_b) { - return idx as u64; + return start_index + idx as u64; } } @@ -50,6 +50,7 @@ pub enum MatchOp { mod tests { use super::*; use crate::aml::object::Object; + use std::string::ToString; #[test] fn empty_package_doesnt_match() { @@ -61,4 +62,47 @@ mod tests { let pkg = &([0, 1, 2, 3].iter().map(|i| Object::Integer(*i as u64).wrap()).collect()); assert_eq!(2, do_match(pkg, &MatchOp::MEQ, 2, &MatchOp::MTR, 0, 0)); } + + #[test] + fn skips_strings() { + let pkg = [ + Object::Integer(0), + Object::String("1".to_string()), + Object::String("2".to_string()), + Object::Integer(3), + ] + .into_iter() + .map(|o| o.wrap()) + .collect(); + + assert_eq!(3, do_match(&pkg, &MatchOp::MGT, 2, &MatchOp::MTR, 0, 0)); + } + + #[test] + fn skips_packages() { + let pkg = [Object::Integer(0), Object::Package(vec![Object::Integer(1).wrap()]), Object::Integer(2)] + .into_iter() + .map(|o| o.wrap()) + .collect(); + + assert_eq!(2, do_match(&pkg, &MatchOp::MGE, 2, &MatchOp::MTR, 0, 0)); + } + + #[test] + fn second_op_also_works() { + let pkg = &([0, 1, 2, 3].iter().map(|i| Object::Integer(*i as u64).wrap()).collect()); + assert_eq!(2, do_match(pkg, &MatchOp::MTR, 0, &MatchOp::MEQ, 2, 0)); + } + + #[test] + fn impossible_match_returns_ones() { + let pkg = &([0, 1, 2, 3].iter().map(|i| Object::Integer(*i as u64).wrap()).collect()); + assert_eq!(ONES, do_match(pkg, &MatchOp::MEQ, 2, &MatchOp::MLT, 1, 0)); + } + + #[test] + fn offset_works_correctly() { + let pkg = &([1, 1, 1, 1].iter().map(|i| Object::Integer(*i as u64).wrap()).collect()); + assert_eq!(2, do_match(pkg, &MatchOp::MEQ, 1, &MatchOp::MTR, 0, 2)); + } } diff --git a/tests/match.asl b/tests/match.asl index 91d41327..fb39bd91 100644 --- a/tests/match.asl +++ b/tests/match.asl @@ -1,8 +1,34 @@ DefinitionBlock("", "DSDT", 1, "RSACPI", "MATCH", 1) { - Name (AAAA, Package (4) { 0x00, 0x01, 0x02, 0x03 }) + Name (AAAA, Package (4) { 0x01, 0x01, 0x02, 0x03 }) + Name (BBBB, Package (4) { 0x01, 0x01, 0x01, 0x01 }) + + Name(FCNT, 0) + + Method (CHEK, 2) { + If (Arg0 != Arg1) { + FCNT++ + } + } + Method (MAIN) { - Local0 = Match (AAAA, MEQ, 2, MTR, 0, 0) + // Test all operations in first position. + CHEK(2, Match (AAAA, MEQ, 2, MTR, 0, 0)) + CHEK(Ones, Match (AAAA, MLT, 1, MTR, 0, 0)) + CHEK(0, Match (AAAA, MLE, 1, MTR, 0, 0)) + CHEK(3, Match (AAAA, MGT, 2, MTR, 0, 0)) + CHEK(2, Match (AAAA, MGE, 2, MTR, 0, 0)) + + // Test all operations in second position. + CHEK(2, Match (AAAA, MTR, 0, MEQ, 2, 0)) + CHEK(Ones, Match (AAAA, MTR, 0, MLT, 1, 0)) + CHEK(0, Match (AAAA, MTR, 0, MLE, 1, 0)) + CHEK(3, Match (AAAA, MTR, 0, MGT, 2, 0)) + CHEK(2, Match (AAAA, MTR, 0, MGE, 2, 0)) + + // Check offset works + CHEK(2, Match (BBBB, MEQ, 1, MTR, 0, 2)) + CHEK(Ones, Match (BBBB, MEQ, 2, MTR, 0, 0)) - Return (!(Local0 == 2)) + Return (FCNT) } } diff --git a/tests/match_expr.rs b/tests/match_expr.rs new file mode 100644 index 00000000..6a7d875b --- /dev/null +++ b/tests/match_expr.rs @@ -0,0 +1,45 @@ +//! Check that incorrect AML sequences in Match expressions appear to error correctly without panics + +mod test_infra; + +use aml_test_tools::handlers::null_handler::NullHandler; +use crate::test_infra::run_opcodes_test; + +#[test] +fn invalid_arg_to_match() { + // This AML is plausible looking but invalid - it attempts to pass Local0 instead of a match + // operand. + // + // Generated by compiling the following script and changing the first Match Opcode: + // ``` + // DefinitionBlock("", "DSDT", 1, "RSACPI", "MATCH", 1) { + // Name (AAAA, Package (4) { 0xAA, 0xAA}) + // + // Method (MAIN) { + // Local1 = Match (AAAA, MTR, 0, MGE, 2, 0) + // Return (0) + // } + // } + let opcodes = vec![ + 0x08, // Name op + 0x41, 0x41, 0x41, 0x41, // "AAAA" + 0x12, // Package + 0x06, // 6 bytes long + 0x02, // 4 elements? + 0x0A, 0xAA, // Byte 0xAA + 0x0A, 0xAA, // Byte 0xAA + 0x70, // Store + 0x89, // Match + 0x41, 0x41, 0x41, 0x41, // "AAAA" + 0x68, // ** Invalid match opcode Arg0 ** + 0x0A, 0x00, // Byte 0 + 0x04, // Match Opcode + 0x0A, 0x02, // Byte 2 (operand) + 0x0A, 0x00, // Byte 0 (start offset) + 0x61, // Local1 - store target + 0xA4, 0x0A, 0x00, // Return byte 0 + ]; + + let handler = NullHandler; + run_opcodes_test(&opcodes, handler); +} From a60633b6166779a40ad47438774a1188f2640d44 Mon Sep 17 00:00:00 2001 From: Martin Hughes Date: Sat, 10 Oct 2026 15:58:17 +0100 Subject: [PATCH 7/9] Test some dodgy opcode combos --- tests/match_expr.rs | 23 +++++++++++++++++++---- tests/test_infra/mod.rs | 14 ++++++++++++-- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/tests/match_expr.rs b/tests/match_expr.rs index 6a7d875b..a36bb0a5 100644 --- a/tests/match_expr.rs +++ b/tests/match_expr.rs @@ -2,8 +2,9 @@ mod test_infra; -use aml_test_tools::handlers::null_handler::NullHandler; -use crate::test_infra::run_opcodes_test; +use crate::test_infra::run_opcodes_test_with_result; +use aml_test_tools::{RunTestResult, TestFailureReason, handlers::null_handler::NullHandler}; +use std::assert_matches; #[test] fn invalid_arg_to_match() { @@ -40,6 +41,20 @@ fn invalid_arg_to_match() { 0xA4, 0x0A, 0x00, // Return byte 0 ]; - let handler = NullHandler; - run_opcodes_test(&opcodes, handler); + let r = run_opcodes_test_with_result(&opcodes, NullHandler); + assert_matches!(r, RunTestResult::Failed(_, TestFailureReason::ParseFail(_))) } + +#[test] +fn invalid_use_of_match_opcode() { + let opcodes = vec![ + // Attempt something like `Local1 = MGT` + 0x70, 0x05, 0x61, + // `Return (0)` (If the line above works, then 0 is returned and the test result is `Pass`, + // rather than the `Failed` that we expect) + 0xA4, 0x0A, 0x00 + ]; + + let r = run_opcodes_test_with_result(&opcodes, NullHandler); + assert_matches!(r, RunTestResult::Failed(_, TestFailureReason::ParseFail(_))) +} \ No newline at end of file diff --git a/tests/test_infra/mod.rs b/tests/test_infra/mod.rs index aa94b987..6b69e0f1 100644 --- a/tests/test_infra/mod.rs +++ b/tests/test_infra/mod.rs @@ -56,11 +56,21 @@ pub fn evaluate(interpreter: &Interpreter, path: &str) -> Object { /// opcodes to execute. #[allow(dead_code)] pub fn run_opcodes_test(opcodes: &[u8], handler: impl Handler) { + let result = run_opcodes_test_with_result(opcodes, handler); + assert!(matches!(result, RunTestResult::Pass(_)), "Test failed with: {:?}", TestResult::from(&result)); +} + +/// Run a test against a sequence of AML opcodes and return the raw result. +/// +/// 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_with_result(opcodes: &[u8], handler: H) -> RunTestResult> { 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)); + run_test_for_opcodes(opcodes, interpreter, &None) } From d10e91af90fb6b450dd84efc45b067ca3e771ea9 Mon Sep 17 00:00:00 2001 From: Martin Hughes Date: Sat, 10 Oct 2026 16:16:03 +0100 Subject: [PATCH 8/9] Shouldn't have added more after doing `cargo fmt` --- tests/match_expr.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/match_expr.rs b/tests/match_expr.rs index a36bb0a5..214b1928 100644 --- a/tests/match_expr.rs +++ b/tests/match_expr.rs @@ -21,6 +21,7 @@ fn invalid_arg_to_match() { // Return (0) // } // } + // ``` let opcodes = vec![ 0x08, // Name op 0x41, 0x41, 0x41, 0x41, // "AAAA" @@ -52,9 +53,9 @@ fn invalid_use_of_match_opcode() { 0x70, 0x05, 0x61, // `Return (0)` (If the line above works, then 0 is returned and the test result is `Pass`, // rather than the `Failed` that we expect) - 0xA4, 0x0A, 0x00 + 0xA4, 0x0A, 0x00, ]; let r = run_opcodes_test_with_result(&opcodes, NullHandler); assert_matches!(r, RunTestResult::Failed(_, TestFailureReason::ParseFail(_))) -} \ No newline at end of file +} From b7e1a87f466d8f78aa581cce2fe9240e81189ac8 Mon Sep 17 00:00:00 2001 From: Martin Hughes Date: Sat, 10 Oct 2026 16:17:58 +0100 Subject: [PATCH 9/9] Tweak for accuracy --- tests/match_expr.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/match_expr.rs b/tests/match_expr.rs index 214b1928..cdd87fc5 100644 --- a/tests/match_expr.rs +++ b/tests/match_expr.rs @@ -14,9 +14,8 @@ fn invalid_arg_to_match() { // Generated by compiling the following script and changing the first Match Opcode: // ``` // DefinitionBlock("", "DSDT", 1, "RSACPI", "MATCH", 1) { - // Name (AAAA, Package (4) { 0xAA, 0xAA}) - // // Method (MAIN) { + // Name (AAAA, Package (4) { 0xAA, 0xAA}) // Local1 = Match (AAAA, MTR, 0, MGE, 2, 0) // Return (0) // }