Allow BufferField -> Integer conversion

src/aml/mod.rs

@@ -105,7 +105,7 @@ where
     handler: H,
     pub namespace: Spinlock<Namespace>,
     pub object_token: Spinlock<ObjectToken>,
-    dsdt_revision: u8,
+    integer_size: IntegerSize,
     region_handlers: Spinlock<BTreeMap<RegionSpace, Box<dyn RegionHandler>>>,
 
     global_lock_mutex: Handle,
@@ -140,7 +140,7 @@ where
             handler,
             namespace: Spinlock::new(Namespace::new(global_lock_mutex)),
             object_token: Spinlock::new(unsafe { ObjectToken::create_interpreter_token() }),
-            dsdt_revision,
+            integer_size: IntegerSize::from_revision(dsdt_revision),
             region_handlers: Spinlock::new(BTreeMap::new()),
             global_lock_mutex,
             registers,
@@ -403,12 +403,6 @@ where
         }
     }
 
-    /// Returns the size of an integer (in bytes) for the set of tables parsed so far. This depends
-    /// on the revision of the initial DSDT.
-    pub fn integer_size(&self) -> usize {
-        if self.dsdt_revision >= 2 { 8 } else { 4 }
-    }
-
     fn do_execute_method(&self, mut context: MethodContext) -> Result<WrappedObject, AmlError> {
         /*
          * This is the main loop that executes operations. Every op is handled at the top-level of
@@ -838,17 +832,19 @@ where
                     }
                     Opcode::DerefOf => {
                         extract_args!(op => [Argument::Object(object)]);
-                        let result = if object.typ() == ObjectType::Reference {
-                            object.clone().unwrap_reference()
-                        } else if object.typ() == ObjectType::String {
-                            let path = AmlName::from_str(&object.as_string().unwrap())?;
-                            let (_, object) = self.namespace.lock().search(&path, &context.current_scope)?;
-                            object.clone()
-                        } else {
-                            return Err(AmlError::ObjectNotOfExpectedType {
-                                expected: ObjectType::Reference,
-                                got: object.typ(),
-                            });
+                        let result = match **object {
+                            Object::Reference { kind: _, inner: _ } => object.clone().unwrap_reference(),
+                            Object::String(_) => {
+                                let path = AmlName::from_str(&object.as_string().unwrap())?;
+                                let (_, object) = self.namespace.lock().search(&path, &context.current_scope)?;
+                                object.clone()
+                            }
+                            _ => {
+                                return Err(AmlError::ObjectNotOfExpectedType {
+                                    expected: ObjectType::Reference,
+                                    got: object.typ(),
+                                });
+                            }
                         };
                         context.contribute_arg(Argument::Object(result));
                         context.retire_op(op);
@@ -1619,7 +1615,7 @@ where
                                         let value = self.do_field_read(field)?;
                                         context.contribute_arg(Argument::Object(value));
                                     } else if let Object::BufferField { .. } = *object {
-                                        let value = object.read_buffer_field(self.integer_size())?;
+                                        let value = object.read_buffer_field(self.integer_size)?;
                                         context.contribute_arg(Argument::Object(value.wrap()));
                                     } else {
                                         context.contribute_arg(Argument::Object(object));
@@ -1866,8 +1862,8 @@ where
         extract_args!(op[0..3] => [Argument::Object(left), Argument::Object(right), Argument::Object(target)]);
         let target2 = if op.op == Opcode::Divide { Some(&op.arguments[3]) } else { None };
 
-        let left = left.clone().unwrap_transparent_reference().as_integer()?;
-        let right = right.clone().unwrap_transparent_reference().as_integer()?;
+        let left = left.clone().unwrap_transparent_reference().to_integer(self.integer_size)?;
+        let right = right.clone().unwrap_transparent_reference().to_integer(self.integer_size)?;
 
         let result = match op.op {
             Opcode::Add => left.wrapping_add(right),
@@ -1910,10 +1906,9 @@ where
                      * This is a particularly important place to respect the integer width as set
                      * by the DSDT revision.
                      */
-                    match self.integer_size() {
-                        4 => ((operand as u32).leading_zeros() + 1) as u64,
-                        8 => (operand.leading_zeros() + 1) as u64,
-                        _ => unreachable!(),
+                    match self.integer_size {
+                        IntegerSize::FourBytes => ((operand as u32).leading_zeros() + 1) as u64,
+                        IntegerSize::EightBytes => (operand.leading_zeros() + 1) as u64,
                     }
                 }
             }
@@ -2021,7 +2016,7 @@ where
         let result = match *operand {
             Object::Buffer(ref bytes) => Object::Buffer(bytes.clone()),
             Object::Integer(value) => {
-                if self.integer_size() == 8 {
+                if self.integer_size == IntegerSize::EightBytes {
                     Object::Buffer(value.to_le_bytes().to_vec())
                 } else {
                     Object::Buffer((value as u32).to_le_bytes().to_vec())
@@ -2051,42 +2046,7 @@ where
         extract_args!(op => [Argument::Object(operand), Argument::Object(target)]);
         let operand = operand.clone().unwrap_transparent_reference();
 
-        let result = match *operand {
-            Object::Integer(value) => Object::Integer(value),
-            Object::Buffer(ref bytes) => {
-                /*
-                 * The spec says this should respect the revision of the current definition block.
-                 * Apparently, the NT interpreter always uses the first 8 bytes of the buffer.
-                 */
-                let mut to_interpret = [0u8; 8];
-                (to_interpret[0..usize::min(bytes.len(), 8)]).copy_from_slice(bytes);
-                Object::Integer(u64::from_le_bytes(to_interpret))
-            }
-            Object::String(ref value) => {
-                /*
-                 * TODO:
-                 * This is about the same level of effort as ACPICA puts in. The uACPI test suite
-                 * has tests that this fails - namely because of support for octal, signs, strings
-                 * that won't fit in a `u64` etc. We probably need to write a more robust parser
-                 * 'real' parser to handle those cases.
-                 */
-                let value = value.trim();
-                let value = value.to_ascii_lowercase();
-                let (value, radix): (&str, u32) = match value.strip_prefix("0x") {
-                    Some(value) => (value.split(|c: char| !c.is_ascii_hexdigit()).next().unwrap_or(""), 16),
-                    None => (value.split(|c: char| !c.is_ascii_digit()).next().unwrap_or(""), 10),
-                };
-                match value.len() {
-                    0 => Object::Integer(0),
-                    _ => Object::Integer(u64::from_str_radix(value, radix).map_err(|_| {
-                        AmlError::InvalidOperationOnObject { op: Operation::ToInteger, typ: ObjectType::String }
-                    })?),
-                }
-            }
-            _ => Err(AmlError::InvalidOperationOnObject { op: Operation::ToBuffer, typ: operand.typ() })?,
-        }
-        .wrap();
-
+        let result = Object::Integer(operand.to_integer(self.integer_size)?).wrap();
         let result = self.do_store(target.clone(), result)?;
         context.contribute_arg(Argument::Object(result));
         context.retire_op(op);
@@ -2227,9 +2187,9 @@ where
         let result = match source1.typ() {
             ObjectType::Integer => {
                 let source1 = source1.as_integer()?;
-                let source2 = source2.to_integer(self.integer_size())?;
+                let source2 = source2.to_integer(self.integer_size)?;
                 let mut buffer = Vec::new();
-                if self.integer_size() == 8 {
+                if self.integer_size == IntegerSize::EightBytes {
                     buffer.extend_from_slice(&source1.to_le_bytes());
                     buffer.extend_from_slice(&source2.to_le_bytes());
                 } else {
@@ -2240,7 +2200,7 @@ where
             }
             ObjectType::Buffer => {
                 let mut buffer = source1.as_buffer()?.to_vec();
-                buffer.extend(source2.to_buffer(self.integer_size())?);
+                buffer.extend(source2.to_buffer(self.integer_size)?);
                 Object::Buffer(buffer).wrap()
             }
             _ => {
@@ -2478,7 +2438,7 @@ where
     /// return either an `Integer` or `Buffer` as appropriate, guided by the size of the field
     /// and expected integer size (as per the DSDT revision).
     fn do_field_read(&self, field: &FieldUnit) -> Result<WrappedObject, AmlError> {
-        let needs_buffer = field.bit_length > (self.integer_size() * 8);
+        let needs_buffer = field.bit_length > (self.integer_size as usize * 8);
         let access_width_bits = field.flags.access_type_bytes()? * 8;
 
         trace!("AML field read. Field = {:?}", field);
@@ -3533,3 +3493,15 @@ pub enum AmlError {
     /// state. More information may be given in the contained value.
     InternalError(String),
 }
+
+#[derive(Debug, Clone, Copy, PartialEq)]
+pub enum IntegerSize {
+    FourBytes = 4,
+    EightBytes = 8,
+}
+
+impl IntegerSize {
+    pub fn from_revision(revision: u8) -> IntegerSize {
+        if revision >= 2 { IntegerSize::EightBytes } else { IntegerSize::FourBytes }
+    }
+}

src/aml/object.rs

@@ -1,4 +1,4 @@
-use crate::aml::{AmlError, Handle, Operation, op_region::OpRegion};
+use crate::aml::{AmlError, Handle, IntegerSize, Operation, op_region::OpRegion};
 use alloc::{
     borrow::Cow,
     string::{String, ToString},
@@ -179,6 +179,9 @@ impl Object {
         WrappedObject::new(self)
     }
 
+    /// Unwraps an integer object. Errors if not already an integer.
+    ///
+    /// For casting to integer, use [`Object::to_integer`] instead.
     pub fn as_integer(&self) -> Result<u64, AmlError> {
         if let Object::Integer(value) = self {
             Ok(*value)
@@ -203,42 +206,67 @@ impl Object {
         }
     }
 
-    pub fn to_integer(&self, allowed_bytes: usize) -> Result<u64, AmlError> {
+    /// Converts the object to an integer. Used for both implicit and explicit conversions.
+    ///
+    /// To avoid the cast, use [`Object::as_integer`] instead.
+    pub fn to_integer(&self, integer_size: IntegerSize) -> Result<u64, AmlError> {
         match self {
             Object::Integer(value) => Ok(*value),
-            Object::Buffer(value) => {
-                let length = usize::min(value.len(), allowed_bytes);
-                let mut bytes = [0u8; 8];
-                bytes[0..length].copy_from_slice(&value[0..length]);
-                Ok(u64::from_le_bytes(bytes))
+            Object::Buffer(bytes) => {
+                /*
+                 * The spec says this should respect the revision of the current definition block.
+                 * Apparently, the NT interpreter always uses the first 8 bytes of the buffer.
+                 */
+                let length = usize::min(bytes.len(), integer_size as usize);
+                let mut to_interpret = [0u8; 8];
+                to_interpret[0..length].copy_from_slice(&bytes[0..length]);
+                Ok(u64::from_le_bytes(to_interpret))
+            }
+            Object::String(value) => {
+                /*
+                 * This is about the same level of effort as ACPICA puts in. The uACPI test suite
+                 * has tests that this fails - namely because of support for octal, signs, strings
+                 * that won't fit in a `u64` etc. We probably need to write a more robust parser
+                 * 'real' parser to handle those cases.
+                 */
+                let value = value.trim();
+                let value = value.to_ascii_lowercase();
+                let (value, radix): (&str, u32) = match value.strip_prefix("0x") {
+                    Some(value) => (value.split(|c: char| !c.is_ascii_hexdigit()).next().unwrap_or(""), 16),
+                    None => (value.split(|c: char| !c.is_ascii_digit()).next().unwrap_or(""), 10),
+                };
+                match value.len() {
+                    0 => Ok(0),
+                    _ => Ok(u64::from_str_radix(value, radix).map_err(|_| {
+                        AmlError::InvalidOperationOnObject { op: Operation::ToInteger, typ: ObjectType::String }
+                    })?),
+                }
             }
-            // TODO: how should we handle invalid inputs? What does NT do here?
-            Object::String(value) => Ok(value.parse::<u64>().unwrap_or(0)),
-            _ => Ok(0),
+            Object::BufferField { .. } => self.read_buffer_field(integer_size)?.to_integer(integer_size),
+            _ => Err(AmlError::InvalidOperationOnObject { op: Operation::ToInteger, typ: self.typ() })?,
         }
     }
 
-    pub fn to_buffer(&self, allowed_bytes: usize) -> Result<Vec<u8>, AmlError> {
+    pub fn to_buffer(&self, integer_size: IntegerSize) -> Result<Vec<u8>, AmlError> {
         match self {
             Object::Buffer(bytes) => Ok(bytes.clone()),
-            Object::Integer(value) => match allowed_bytes {
-                4 => Ok((*value as u32).to_le_bytes().to_vec()),
-                8 => Ok(value.to_le_bytes().to_vec()),
-                _ => panic!(),
+            Object::Integer(value) => match integer_size {
+                IntegerSize::FourBytes => Ok((*value as u32).to_le_bytes().to_vec()),
+                IntegerSize::EightBytes => Ok(value.to_le_bytes().to_vec()),
             },
             Object::String(value) => Ok(value.as_bytes().to_vec()),
             _ => Err(AmlError::InvalidOperationOnObject { op: Operation::ConvertToBuffer, typ: self.typ() }),
         }
     }
 
-    pub fn read_buffer_field(&self, integer_size: usize) -> Result<Object, AmlError> {
+    pub fn read_buffer_field(&self, integer_size: IntegerSize) -> Result<Object, AmlError> {
         if let Self::BufferField { buffer, offset, length } = self {
             let buffer = match **buffer {
                 Object::Buffer(ref buffer) => buffer.as_slice(),
                 Object::String(ref string) => string.as_bytes(),
                 _ => panic!(),
             };
-            if *length <= integer_size {
+            if *length <= integer_size as usize {
                 let mut dst = [0u8; 8];
                 copy_bits(buffer, *offset, &mut dst, 0, *length);
                 Ok(Object::Integer(u64::from_le_bytes(dst)))
@@ -555,4 +583,41 @@ mod tests {
         copy_bits(&src, 0, &mut dst, 2, 15);
         assert_eq!(dst, [0b1111_1101, 0b1101_1110, 0b0000_0001, 0b0000_0000, 0b0000_0000]);
     }
+
+    #[test]
+    fn buffer_to_integer() {
+        let buffer = Object::Buffer(Vec::from([0xab, 0xcd, 0xef, 0x01, 0xff]));
+        assert_eq!(buffer.to_integer(IntegerSize::FourBytes).unwrap(), 0x01efcdab);
+    }
+    #[test]
+    fn buffer_field_to_integer() {
+        const BUFFER: [u8; 5] = [0xffu8; 5];
+        let buffer = Object::Buffer(Vec::from(BUFFER)).wrap();
+        let buffer_field = Object::BufferField { buffer, offset: 5, length: 9 };
+
+        assert_eq!(buffer_field.to_integer(IntegerSize::FourBytes).unwrap(), 0x1ff);
+    }
+
+    #[test]
+    fn buffer_field_to_4_byte_integer() {
+        // The ones in this buffer are strategically chosen to not make it to the final integer.
+        const BUFFER: [u8; 5] = [0x0f, 0x00, 0x00, 0x00, 0xf0];
+        let buffer = Object::Buffer(Vec::from(BUFFER)).wrap();
+        let buffer_field = Object::BufferField {
+            buffer,
+            offset: 4,
+            length: 36, // This should be truncated to 32 bits in the conversion
+        };
+
+        assert_eq!(buffer_field.to_integer(IntegerSize::FourBytes).unwrap(), 0);
+    }
+
+    #[test]
+    fn buffer_field_to_8_byte_integer() {
+        const BUFFER: [u8; 6] = [0x0f, 0x00, 0x00, 0x00, 0xf0, 0xff];
+        let buffer = Object::Buffer(Vec::from(BUFFER)).wrap();
+        let buffer_field = Object::BufferField { buffer, offset: 4, length: 36 };
+
+        assert_eq!(buffer_field.to_integer(IntegerSize::EightBytes).unwrap(), 0x0000000f_00000000);
+    }
 }

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/de_ref_of.rs

@@ -0,0 +1,28 @@
+use aml_test_tools::handlers::null_handler::NullHandler;
+
+mod test_infra;
+
+#[test]
+fn test_deref_of_buffer_field() {
+    const AML: &str = r#"
+DefinitionBlock ("", "SSDT", 2, "RSACPI", "DerefOf", 0x00000002) {
+    Scope (\_SB) {
+        Name (ADAT, Buffer (0x0010) {
+            /* 0000 */  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+            /* 0008 */  0x00, 0xaa, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+        })
+    }
+
+    Method(MAIN, 0, NotSerialized) {
+        Local0 = (DerefOf(\_SB.ADAT[0x09]))
+        // This relies on subtraction rather than equality as logical ops on BufferFields don't work
+        // yet.
+        // TODO: Use logical ops for clarity, when available.
+        return (Local0 - 0xaa)
+    }
+}
+"#;
+
+    let handler = NullHandler {};
+    test_infra::run_aml_test(AML, handler);
+}