From 8ec1ce6d0ec4b8a2dcc67a2afb3007aff1cef9d0 Mon Sep 17 00:00:00 2001 From: Martin Hughes Date: Fri, 3 Jul 2026 17:01:15 +0100 Subject: [PATCH] Ensure that Locals passed to Methods are preserved Since both Locals and Args are stored as references internally, a plain `unwrap_reference` was causing Locals that got passed to methods to be treated as though they had been passed by reference, instead of by value. This corrects that logic, and also takes into account the strange behaviour of the Windows NT interpreter - it treats strings passed via Locals into methods as passed by reference, when appropriate. --- src/aml/mod.rs | 46 ++++------ src/aml/object.rs | 127 +++++++++++++++++++++++++++ tests/store.asl | 186 ++++++++++++++++++++++++++++++++++++++++ tests/uacpi_examples.rs | 1 - 4 files changed, 332 insertions(+), 28 deletions(-) create mode 100644 tests/store.asl diff --git a/src/aml/mod.rs b/src/aml/mod.rs index cb3af956..9ead67eb 100644 --- a/src/aml/mod.rs +++ b/src/aml/mod.rs @@ -498,7 +498,7 @@ where } Opcode::Increment | Opcode::Decrement => { let [Argument::Object(operand)] = &op.arguments[..] else { panic!() }; - let operand = operand.clone().unwrap_transparent_reference(); + let operand = operand.clone().unwrap_reference(); let token = self.object_token.lock(); let Object::Integer(operand) = (unsafe { operand.gain_mut(&token) }) else { @@ -2393,40 +2393,32 @@ where /// Perform a store of `object` into `target`, matching the expected behaviour of `DefStore`, /// which depends on the target: - /// - Locals are overwritten, unless they contain a reference, in which case a store is - /// performed to the referenced object with implicit casting + /// - Locals and Index references are overwritten, unless the Local contains a reference + /// (e.g. obtained via `RefOf`), in which case a store is performed to the referenced + /// object with implicit casting. /// - Args are overwritten, unless they contain a reference, in which case the referenced - /// object is overwritten - /// - Index references behave the same as locals + /// object is *usually* overwritten. References from Arg to Local without an intermediate + /// `RefOf` cause the Arg to be overwritten instead of the Local (see + /// [issue #313](https://github.com/rust-osdev/acpi/issues/313)). + /// - There is an exception to this, needed to match the Windows NT interpreter: if an Arg + /// referencing a Local is found whilst unwrapping a real reference (e.g. a Local holding + /// `RefOf(Arg0)`, where `Arg0` was itself passed a `Local` directly) and the final result + /// of unwrapping is a string, the string is modified instead of the Arg. This does not + /// apply if the Arg is the direct target of the store and only applies to Strings. (This + /// sounds odd, but is the Windows way) /// - Named objects are stored into, with implicit casting + /// + /// This is complex so may be explained better by `tests/store.asl` and + /// [`Object::unwrap_ref_for_store`]! fn do_store(&self, target: WrappedObject, object: WrappedObject) -> Result { let object = object.unwrap_transparent_reference(); let token = self.object_token.lock(); match unsafe { target.gain_mut(&token) } { - Object::Reference { kind, inner } => { - let (target_object, overwrite) = match kind { - ReferenceKind::Named => (inner.clone().unwrap_reference(), false), - ReferenceKind::Local | ReferenceKind::Index => { - if let Object::Reference { kind: _, inner: ref inner_inner } = **inner { - (inner_inner.clone(), false) - } else { - (inner.clone().unwrap_transparent_reference(), true) - } - } - ReferenceKind::Arg => { - if let Object::Reference { kind: _, inner: ref inner_inner } = **inner { - (inner_inner.clone(), true) - } else { - (inner.clone().unwrap_transparent_reference(), true) - } - } - ReferenceKind::RefOf | ReferenceKind::Unresolved => { - return Err(AmlError::StoreToInvalidReferenceType); - } - }; + Object::Reference { .. } => { + let (target_object, implicit_cast_reqd) = target.unwrap_ref_for_store()?; - if overwrite { + if !implicit_cast_reqd { unsafe { *target_object.gain_mut(&token) = (*object).clone(); } diff --git a/src/aml/object.rs b/src/aml/object.rs index 8ac820d1..ebaca18b 100644 --- a/src/aml/object.rs +++ b/src/aml/object.rs @@ -154,6 +154,61 @@ impl WrappedObject { } } } + + /// Unwrap a reference that is about to be stored to - find the target object. + /// + /// Take into account the store rules as enumerated by `Interpreter::do_store` + /// + /// Returns a tuple containing: + /// - The object that should be modified + /// - A boolean indicating whether an implicit cast should occur before the store + pub(crate) fn unwrap_ref_for_store(self) -> Result<(WrappedObject, bool), AmlError> { + let Object::Reference { kind: outer_kind, .. } = *self else { + return Err(AmlError::ObjectNotOfExpectedType { expected: ObjectType::Reference, got: self.typ() }); + }; + + // If we're storing directly to an Arg then the strange Windows NT behaviour for strings + // doesn't apply. (see the comments in `Interpreter::do_store` for more) + let store_to_arg = outer_kind == ReferenceKind::Arg; + + // Set once we've unwrapped through a true reference. This helps decide whether the target + // should be overwritten in full, or if an implicit cast to the target type is needed. + let mut crossed_true_reference = false; + + // If we reach an Arg that directly references a Local, we should store in the Arg instead + // of the Local (see [issue #313](https://github.com/rust-osdev/acpi/issues/313)). Except + // for the strange Windows NT behaviour handled in the `match arg_to_local` statement below. + let mut arg_to_local: Option = None; + + let mut target = self; + + loop { + let Object::Reference { kind, ref inner } = *target else { + return Ok(match arg_to_local { + Some(_) if !store_to_arg && target.typ() == ObjectType::String => (target.clone(), true), + Some(arg) => (arg, false), + None => (target.clone(), !store_to_arg && crossed_true_reference), + }); + }; + + match kind { + ReferenceKind::Named | ReferenceKind::RefOf | ReferenceKind::Index => { + crossed_true_reference = true + } + ReferenceKind::Local => {} + ReferenceKind::Arg => { + if arg_to_local.is_none() + && matches!(**inner, Object::Reference { kind: ReferenceKind::Local, .. }) + { + arg_to_local = Some(inner.clone()); + } + } + ReferenceKind::Unresolved => return Err(AmlError::StoreToInvalidReferenceType), + } + + target = inner.clone(); + } + } } impl ops::Deref for WrappedObject { @@ -653,4 +708,76 @@ mod tests { assert_eq!(buffer_field.to_integer(IntegerSize::EightBytes).unwrap(), 0x0000000f_00000000); } + + #[test] + fn store_local_ref_to_local() { + // As may be encountered in the last line of: + // Local1 = RefOf(Local0) + // Local1 = 2 (the actual store is omitted) + let local0 = Object::Reference { kind: ReferenceKind::Local, inner: Object::Integer(1).wrap() }.wrap(); + let ref_of = Object::Reference { kind: ReferenceKind::RefOf, inner: local0 }.wrap(); + let local1 = Object::Reference { kind: ReferenceKind::Local, inner: ref_of }.wrap(); + + let target = local1.unwrap_ref_for_store(); + let target = target.unwrap(); + + let target_obj = &*target.0; + let Object::Integer(x) = target_obj else { + panic!("Incorrect type"); + }; + assert_eq!(*x, 1); + } + + #[test] + fn store_arg_ref_to_local() { + // As if a Local was passed as an argument to a method, and then Arg0 were stored to e.g.: + // Local0 = 1 + // MEFD(Local0) + // ... and inside MEFD: Arg0 = 2 (the actual store is omitted) + let local0 = Object::Reference { kind: ReferenceKind::Local, inner: Object::Integer(1).wrap() }.wrap(); + let arg0 = Object::Reference { kind: ReferenceKind::Arg, inner: local0.clone() }.wrap(); + + let target = arg0.unwrap_ref_for_store(); + let (target, implicit_cast_reqd) = target.unwrap(); + + assert!(Arc::ptr_eq(&target.0, &local0.0)); + assert!(!implicit_cast_reqd); + } + + #[test] + fn store_arg_ref_of_local() { + // As may be encountered in the last line of: + // Local0 = 1 + // Arg0 = RefOf(Local0) + // Arg0 = 2 (the actual store is omitted) + let local0 = Object::Reference { kind: ReferenceKind::Local, inner: Object::Integer(1).wrap() }.wrap(); + let ref_of = Object::Reference { kind: ReferenceKind::RefOf, inner: local0 }.wrap(); + let arg0 = Object::Reference { kind: ReferenceKind::Arg, inner: ref_of }.wrap(); + + let target = arg0.unwrap_ref_for_store(); + let (target, implicit_cast_reqd) = target.unwrap(); + + let target_obj = &*target; + let Object::Integer(x) = target_obj else { + panic!("Incorrect type"); + }; + assert_eq!(*x, 1); + assert!(!implicit_cast_reqd); + } + + #[test] + fn store_local_refof_arg_to_local_string() { + // This covers the weird Windows NT handling of references that ultimately end up as Strings + let string = Object::String("string".to_string()).wrap(); + let outer_local = Object::Reference { kind: ReferenceKind::Local, inner: string.clone() }.wrap(); + let arg0 = Object::Reference { kind: ReferenceKind::Arg, inner: outer_local }.wrap(); + let ref_of = Object::Reference { kind: ReferenceKind::RefOf, inner: arg0 }.wrap(); + let inner_local = Object::Reference { kind: ReferenceKind::Local, inner: ref_of }.wrap(); + + let target = inner_local.unwrap_ref_for_store(); + let (target, implicit_cast_reqd) = target.unwrap(); + + assert!(Arc::ptr_eq(&target.0, &string.0)); + assert!(implicit_cast_reqd); + } } diff --git a/tests/store.asl b/tests/store.asl new file mode 100644 index 00000000..bbff1545 --- /dev/null +++ b/tests/store.asl @@ -0,0 +1,186 @@ +// Check that store handles simple references correctly +// +// Tests with a name that begins with a T are written by us for this crate. Tests with names that begin with a U have +// been adapted from the uACPI test case named in the associated comment. +// +// Tests T1 - T4 are very basic, to ensure any trivial errors in `do_store` are caught. +// +// These tests don't check any conversions - it's assumed that references and conversions are orthogonal. +DefinitionBlock ("", "DSDT", 2, "RSACPI", "TESTTABL", 0xF0F0F0F0) +{ + Name(FCNT, 0) + + Method (CHEK, 2) { + If (Arg0 != Arg1) { + FCNT++ + } + } + + Method (T1) { + Name(V1, 1) + V1 = 2 + CHEK(V1, 2) + } + + Method (T2) { + Name(V1, 1) + Alias(V1, V2) + V2 = 2 + CHEK(V1, 2) + } + + Method (T3) { + Local1 = 1 + Local2 = Local1 + Local2 = 2 + CHEK(Local1, 1) + } + + Method (T4) { + Local1 = 1 + Local2 = RefOf(Local1) + Local2 = 2 + CHEK(Local1, 2) + } + + Method (INR5, 1) { + Arg0 = 5 + } + + Method (T5) { + Local1 = 1 + INR5(Local1) + CHEK (Local1, 1) + } + + Method (T6) { + Local1 = 1 + INR5(RefOf(Local1)) + CHEK (Local1, 5) + } + + Method (T7, 1) { + Local1 = 1 + Arg0 = RefOf(Local1) + Arg0 = 2 + CHEK (Local1, 2) + } + + // uACPI equivalent: references-0.asl + Method (U1) { + Local0 = "MyString" + INR5(Local0) // Arg0 = 5 + CHEK (Local0, "MyString") + } + + // uACPI equivalent: references-3.asl. To quote that test: + // "This test seems bogus but it's actually correct, it produces the same output on NT." + Method (U2) + { + Local0 = "MyST" + U2IN(Local0) // Local0 = RefOf(Arg0) + CHEK(Local0, "WHY?") + } + + Method (U2IN, 1, NotSerialized) + { + Local0 = RefOf(Arg0) + + // WHY? in little-endian ASCII + Local0 = 0x3F594857 + } + + // This is the same as `U2` but with an extra function call to see if the Windows behaviour is + // limited to one level of the stack - but it is not, multiple calls behave the same as a + // single call. + Method (U2A) { + Local0 = "MyST" + IN2A(Local0) + CHEK(Local0, "WHY?") + } + + Method (IN2A, 1, NotSerialized) { + U2IN(Arg0) + } + + // uACPI test equivalent: references-4 + // Test U2 not withstanding, non-string "pass by value" argument types show the expected behavior. + Method (U3) { + Local0 = 1 + U3IN(Local0) + CHEK(Local0, 1) + } + + Method (U3IN, 1) { + Local0 = RefOf(Arg0) + Local0 = 9 + } + + // uACPI equivalent: references-8 + METHOD(U4) { + Local0 = "MyString" + U4IN(RefOf(Local0)) + CHEK(Local0, 0xDEADBEEF) + } + + Method (U4IN, 1, NotSerialized) + { + Store(0xDEADC0DE, Arg0) + Store(0xDEADBEEF, Arg0) + } + + // uACPI test equivalent: references-9 + Method (U5, 0, NotSerialized) + { + Local0 = 0xDEADC0DEDEADBEEF + U5IN(RefOf(Local0)) + CHEK (Local0, 0x676E6F6C79726576) + } + + Method (U5IN, 1, NotSerialized) + { + Local0 = RefOf(Arg0) + Local0 = "verylongstringbiggerthanint" + } + + // uACPI test equivalent: references-10 + Method (U6, 0, NotSerialized) + { + Local0 = 0x1000 + U6IN(RefOf(Local0), 10) + CHEK (Local0, 0x100A) + } + + Method (U6IN, 2, NotSerialized) + { + Local0 = RefOf(Arg0) + Local0++ + + Debug = Arg1 + Debug = DerefOf(Local0) + If (Arg1--) { + U6IN(RefOf(Arg0), Arg1) + } + } + + Method (MAIN, 0, NotSerialized) { + T1() + T2() + T3() + T4() + T5() + T6() + T7(0) + + // uACPI equivalents given in the comments: + U1() // references-0 + U2() // references-3 + U2A() // An extra test with another layer of indirection + U3() // references-4 + U4() // references-8 + U5() // references-9 + U6() // references-10 + + Return (FCNT) + } +} diff --git a/tests/uacpi_examples.rs b/tests/uacpi_examples.rs index 128fe30f..15b40909 100644 --- a/tests/uacpi_examples.rs +++ b/tests/uacpi_examples.rs @@ -72,7 +72,6 @@ DefinitionBlock("", "DSDT", 1, "RSACPI", "UACPI", 1) { } #[test] -#[ignore] // ParseFail(ObjectNotOfExpectedType { expected: Integer, got: Integer } (a referencing failure) fn increment_decrement() { const ASL: &str = r#" DefinitionBlock("", "DSDT", 1, "RSACPI", "UACPI", 1) {