Ensure that Locals passed to Methods are preserved

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<WrappedObject, AmlError> {
         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();
                     }

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<H>::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<WrappedObject> = 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);
+    }
 }

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)
+    }
+}