Ensure that Locals passed to Methods are preserved

src/aml/mod.rs

@@ -2381,37 +2381,22 @@ where
     ///    - Locals are overwritten, unless they contain a reference, 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
+    ///      object is usually overwritten. References from Arg to Local without an intermediate
+    ///      `RefOf` cause the Arg to be overwritten.
+    ///      (see [issue #313](https://github.com/rust-osdev/acpi/issues/313))
+    ///    - Args that ultimately refer to a string *always* overwrite the string and not the Arg,
+    ///      as per the Windows NT behaviour (see `tests/store.asl`)
     ///    - Index references behave the same as locals
     ///    - Named objects are stored into, with implicit casting
     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,63 @@ 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 fn unwrap_ref_for_store(self) -> Result<(WrappedObject, bool), AmlError> {
+        let Object::Reference { .. } = *self else {
+            return Err(AmlError::ObjectNotOfExpectedType { expected: ObjectType::Reference, got: self.typ() });
+        };
+
+        let mut target = self;
+        let mut implicit_cast_reqd = false;
+
+        // Unwrap references, but with the following caveats:
+        // - If an Arg -> Local reference is found, we return the Arg so that it can be stored in.
+        //   Except...
+        // - The Windows NT interpreter allows strings stored in locals that are then passed as args
+        //   to be modified even if they aren't passed by reference... so we must continue
+        //   unwrapping to see if the end of the reference chain is a String or not. If it is,
+        //   return that instead (a bit like a normal `unwrap_reference`)
+        //
+        // See issue 313 and the `store.asl` tests for more details.
+        let mut found_arg_to_local: Option<Result<(WrappedObject, bool), AmlError>> = None;
+
+        loop {
+            let Object::Reference { kind, ref inner } = *target else {
+                if target.typ() == ObjectType::String {
+                    return Ok((target.clone(), true));
+                }
+                return found_arg_to_local.unwrap_or_else(|| Ok((target.clone(), implicit_cast_reqd)));
+            };
+
+            implicit_cast_reqd = match kind {
+                ReferenceKind::Named => true,
+                ReferenceKind::Local | ReferenceKind::Index | ReferenceKind::RefOf => false,
+                ReferenceKind::Arg => {
+                    if found_arg_to_local.is_none()
+                        && matches!(**inner, Object::Reference { kind: ReferenceKind::Local, inner: _ })
+                    {
+                        found_arg_to_local = Some(Ok((inner.clone(), implicit_cast_reqd)));
+                    }
+                    false
+                }
+                ReferenceKind::Unresolved => {
+                    if found_arg_to_local.is_none() {
+                        found_arg_to_local = Some(Err(AmlError::StoreToInvalidReferenceType));
+                    }
+                    implicit_cast_reqd
+                }
+            };
+
+            target = inner.clone();
+        }
+    }
 }
 
 impl ops::Deref for WrappedObject {
@@ -653,4 +710,60 @@ 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);
+    }
 }

tests/store.asl

@@ -0,0 +1,122 @@
+// Check that store handles simple references correctly
+//
+// 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", 1, "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)
+    }
+
+    // Test 8 is adapted from uACPI's `references-3.asl`. To quote that test:
+    // "This test seems bogus but it's actually correct, it produces the same output on NT."
+    Method (INR8, 1, NotSerialized)
+    {
+        Local0 = RefOf(Arg0)
+
+        // WHY? in little-endian ASCII
+        Local0 = 0x3F594857
+    }
+
+    Method (T8)
+    {
+        Local0 = "MyST"
+        INR8(Local0)
+        CHEK(Local0, "WHY?")
+    }
+
+    // This is the same as `T8` 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 (T8A) {
+        Local0 = "MyST"
+        IN8A(Local0)
+        CHEK(Local0, "WHY?")
+    }
+
+    Method (IN8A, 1, NotSerialized) {
+        INR8(Arg0)
+    }
+
+    // Test 8 not withstanding, non-string "pass by value" argument types show the expected behavior.
+    Method (INR9, 1) {
+        Local0 = RefOf(Arg0)
+        Local0 = 9
+    }
+
+    Method (T9) {
+       Local0 = 1
+       INR9(Local0)
+       CHEK(Local0, 1)
+    }
+
+    Method (MAIN, 0, NotSerialized) {
+        T1()
+        T2()
+        T3()
+        T4()
+        T5()
+        T6()
+        T7(0)
+        T8()
+        T8A()
+        T9()
+
+        Return (FCNT)
+    }
+}