Simplify regs_access

CHANGELOG.md

@@ -5,6 +5,10 @@ Notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [UNRELEASED] - YYYY-MM-DD
+### Changed
+- `InsnDetail::regs_read()`/`InsnDetail::regs_write()` return more of the accessed registers
+
 ## [0.13.0] - 2025-02-04
 ### Added
 - BPF arch support
@@ -177,7 +181,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Removed
 - Dependency
 
-[0.13.0]: https://github.com/capstone-rust/capstone-rs/compare/capstone-v0.12.0...master
+[UNRELEASED]: https://github.com/capstone-rust/capstone-rs/compare/capstone-v0.13.0...master
+[0.13.0]: https://github.com/capstone-rust/capstone-rs/compare/capstone-v0.12.0...capstone-v0.13.0
 [0.12.0]: https://github.com/capstone-rust/capstone-rs/compare/capstone-v0.11.0...capstone-v0.12.0
 [0.11.0]: https://github.com/capstone-rust/capstone-rs/compare/capstone-v0.10.0...capstone-v0.11.0
 [0.10.0]: https://github.com/capstone-rust/capstone-rs/compare/capstone-v0.9.0...capstone-v0.10.0

capstone-rs/Cargo.toml

@@ -18,6 +18,7 @@ travis-ci = { repository = "capstone-rust/capstone-rs" }
 [dependencies]
 capstone-sys = { path = "../capstone-sys", version = "0.17.0",  default-features = false }
 libc = { version = "0.2", default-features = false }
+static_assertions = "1.1.0"
 
 [dev-dependencies]
 criterion = "0.5"

capstone-rs/src/capstone.rs

@@ -1,6 +1,8 @@
+use alloc::boxed::Box;
 use alloc::string::String;
 use core::convert::From;
 use core::marker::PhantomData;
+use core::mem::MaybeUninit;
 
 use libc::{c_int, c_void};
 
@@ -9,13 +11,23 @@ use capstone_sys::*;
 
 use crate::arch::CapstoneBuilder;
 use crate::constants::{Arch, Endian, ExtraMode, Mode, OptValue, Syntax};
-use crate::error::*;
 use crate::instruction::{Insn, InsnDetail, InsnGroupId, InsnId, Instructions, RegId};
+use crate::{error::*, PartialInitRegsAccess};
 
 use {crate::ffi::str_from_cstr_ptr, alloc::string::ToString, libc::c_uint};
 
-/// This is taken from the [python bindings](https://github.com/capstone-engine/capstone/blob/5fb8a423d4455cade99b12912142fd3a0c10d957/bindings/python/capstone/__init__.py#L929)
-const MAX_NUM_REGISTERS: usize = 64;
+/// Length of `cs_regs`
+pub(crate) const REGS_ACCESS_BUF_LEN: usize = 64;
+
+// todo(tmfink) When MSRV is 1.75 or later, can use:
+//pub(crate) const REGS_ACCESS_BUF_LEN: usize = unsafe { core::mem::zeroed::<cs_regs>() }.len();
+
+/// Equivalent to `MaybeUninit<cs_regs>`
+pub(crate) type RegsAccessBuf = [MaybeUninit<RegId>; REGS_ACCESS_BUF_LEN];
+
+static_assertions::assert_eq_size!(RegId, u16);
+static_assertions::assert_eq_size!(RegsAccessBuf, cs_regs);
+static_assertions::assert_type_eq_all!([u16; REGS_ACCESS_BUF_LEN], cs_regs);
 
 /// An instance of the capstone disassembler
 ///
@@ -103,9 +115,20 @@ impl Iterator for EmptyExtraModeIter {
     }
 }
 
-pub struct RegAccess {
-    pub read: Vec<RegId>,
-    pub write: Vec<RegId>,
+#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
+pub struct RegAccessRef<'a> {
+    pub(crate) read: &'a [RegId],
+    pub(crate) write: &'a [RegId],
+}
+
+impl RegAccessRef<'_> {
+    pub fn read(&self) -> &[RegId] {
+        self.read
+    }
+
+    pub fn write(&self) -> &[RegId] {
+        self.write
+    }
 }
 
 impl Capstone {
@@ -373,38 +396,24 @@ impl Capstone {
         }
     }
 
-    /// Get the registers are which are read to and written to
-    pub fn regs_access_buf(&self, insn: &Insn) -> CsResult<RegAccess> {
-        let mut read = Vec::new();
-        let mut write = Vec::new();
-
-        self.regs_access(insn, &mut read, &mut write)?;
-
-        Ok(RegAccess { read, write })
-    }
-
-    /// Get the registers are which are read to and written to\
-    /// the registers are pushed to the back of the provided buffers
-    pub fn regs_access(
+    /// Get the registers are which are read and written
+    pub(crate) fn regs_access<'buf>(
         &self,
         insn: &Insn,
-        read: &mut Vec<RegId>,
-        write: &mut Vec<RegId>,
-    ) -> CsResult<()> {
+        regs_read: &'buf mut RegsAccessBuf,
+        regs_write: &'buf mut RegsAccessBuf,
+    ) -> CsResult<RegAccessRef<'buf>> {
         if cfg!(feature = "full") {
             let mut regs_read_count: u8 = 0;
             let mut regs_write_count: u8 = 0;
 
-            let mut regs_write = [0u16; MAX_NUM_REGISTERS];
-            let mut regs_read = [0u16; MAX_NUM_REGISTERS];
-
             let err = unsafe {
                 cs_regs_access(
                     self.csh(),
                     &insn.insn as *const cs_insn,
-                    &mut regs_read as *mut _,
+                    regs_read.as_mut_ptr() as *mut cs_regs,
                     &mut regs_read_count as *mut _,
-                    &mut regs_write as *mut _,
+                    regs_write.as_mut_ptr() as *mut cs_regs,
                     &mut regs_write_count as *mut _,
                 )
             };
@@ -413,20 +422,26 @@ impl Capstone {
                 return Err(err.into());
             }
 
-            read.extend(
-                regs_read
-                    .iter()
-                    .take(regs_read_count as usize)
-                    .map(|x| RegId(*x)),
-            );
-            write.extend(
-                regs_write
-                    .iter()
-                    .take(regs_write_count as usize)
-                    .map(|x| RegId(*x)),
-            );
+            // SAFETY: count indicates how many elements are initialized;
+            let regs_read_slice: &[RegId] = unsafe {
+                core::slice::from_raw_parts(
+                    regs_read.as_mut_ptr() as *mut RegId,
+                    regs_read_count as usize,
+                )
+            };
 
-            Ok(())
+            // SAFETY: count indicates how many elements are initialized
+            let regs_write_slice: &[RegId] = unsafe {
+                core::slice::from_raw_parts(
+                    regs_write.as_mut_ptr() as *mut RegId,
+                    regs_write_count as usize,
+                )
+            };
+
+            Ok(RegAccessRef {
+                read: regs_read_slice,
+                write: regs_write_slice,
+            })
         } else {
             Err(Error::DetailOff)
         }
@@ -459,7 +474,30 @@ impl Capstone {
         } else if insn.id().0 == 0 {
             Err(Error::IrrelevantDataInSkipData)
         } else {
-            Ok(unsafe { insn.detail(self.arch) })
+            // Call regs_access to get "extra" read/write registers for the instruction.
+            // Capstone only supports this for some architectures, so ignore errors if there are
+            // any.
+            //
+            // This *could* results in wasted effort if the read/write regs are not checked. As
+            // an optimization, we could call regs_access() lazily (i.e. only if InsnDetail
+            // regs_read()/regs_write() are called).
+            let partial_init_regs_access = {
+                let mut regs_buf = Box::new(crate::RWRegsAccessBuf::new());
+                match self.regs_access(insn, &mut regs_buf.read_buf, &mut regs_buf.write_buf) {
+                    Ok(regs_access) => {
+                        let read_len = regs_access.read.len() as u16;
+                        let write_len = regs_access.write.len() as u16;
+                        Some(PartialInitRegsAccess {
+                            regs_buf,
+                            read_len,
+                            write_len,
+                        })
+                    }
+                    Err(_) => None,
+                }
+            };
+
+            Ok(unsafe { insn.detail(self.arch, partial_init_regs_access) })
         }
     }
 

capstone-rs/src/instruction.rs

@@ -2,6 +2,7 @@ use alloc::{self, boxed::Box};
 use core::convert::TryFrom;
 use core::fmt::{self, Debug, Display, Error, Formatter};
 use core::marker::PhantomData;
+use core::mem::MaybeUninit;
 use core::ops::Deref;
 use core::slice;
 use core::str;
@@ -12,6 +13,7 @@ use crate::arch::ArchDetail;
 use crate::constants::Arch;
 
 use crate::ffi::str_from_cstr_ptr;
+use crate::{RegsAccessBuf, REGS_ACCESS_BUF_LEN};
 
 /// Represents a slice of [`Insn`] returned by [`Capstone`](crate::Capstone) `disasm*()` methods.
 ///
@@ -167,6 +169,50 @@ pub struct Insn<'a> {
     pub(crate) _marker: PhantomData<&'a InsnDetail<'a>>,
 }
 
+pub(crate) struct RWRegsAccessBuf {
+    pub(crate) read_buf: RegsAccessBuf,
+    pub(crate) write_buf: RegsAccessBuf,
+}
+
+impl RWRegsAccessBuf {
+    pub(crate) fn new() -> Self {
+        Self {
+            read_buf: [MaybeUninit::uninit(); REGS_ACCESS_BUF_LEN],
+            write_buf: [MaybeUninit::uninit(); REGS_ACCESS_BUF_LEN],
+        }
+    }
+}
+
+/// Contains partially initialized buffer of registers
+#[cfg_attr(not(feature = "full"), allow(dead_code))]
+pub(crate) struct PartialInitRegsAccess {
+    pub(crate) regs_buf: Box<RWRegsAccessBuf>,
+    pub(crate) read_len: u16,
+    pub(crate) write_len: u16,
+}
+
+// make sure len fields can be stored as u16
+static_assertions::const_assert!(crate::REGS_ACCESS_BUF_LEN <= u16::MAX as usize);
+
+#[cfg_attr(not(feature = "full"), allow(dead_code))]
+impl PartialInitRegsAccess {
+    unsafe fn maybeuninit_slice_to_slice(buf: &[MaybeUninit<RegId>]) -> &[RegId] {
+        &*(buf as *const [MaybeUninit<RegId>] as *const [RegId])
+    }
+
+    pub(crate) fn read(&self) -> &[RegId] {
+        unsafe {
+            Self::maybeuninit_slice_to_slice(&self.regs_buf.read_buf[..self.read_len as usize])
+        }
+    }
+
+    pub(crate) fn write(&self) -> &[RegId] {
+        unsafe {
+            Self::maybeuninit_slice_to_slice(&self.regs_buf.write_buf[..self.write_len as usize])
+        }
+    }
+}
+
 /// Contains architecture-independent details about an [`Insn`].
 ///
 /// To get more detail about the instruction, enable extra details for the
@@ -196,7 +242,13 @@ pub struct Insn<'a> {
 /// To get additional architecture-specific information, use the
 /// [`.arch_detail()`](Self::arch_detail) method to get an `ArchDetail` enum.
 ///
-pub struct InsnDetail<'a>(pub(crate) &'a cs_detail, pub(crate) Arch);
+pub struct InsnDetail<'a> {
+    pub(crate) detail: &'a cs_detail,
+    pub(crate) arch: Arch,
+
+    #[cfg_attr(not(feature = "full"), allow(dead_code))]
+    partial_init_regs_access: Option<PartialInitRegsAccess>,
+}
 
 #[allow(clippy::len_without_is_empty)]
 impl Insn<'_> {
@@ -275,8 +327,16 @@ impl Insn<'_> {
     /// # Safety
     /// The [`cs_insn::detail`] pointer must be valid and non-null.
     #[inline]
-    pub(crate) unsafe fn detail(&self, arch: Arch) -> InsnDetail {
-        InsnDetail(&*self.insn.detail, arch)
+    pub(crate) unsafe fn detail(
+        &self,
+        arch: Arch,
+        partial_init_regs_access: Option<PartialInitRegsAccess>,
+    ) -> InsnDetail<'_> {
+        InsnDetail {
+            detail: &*self.insn.detail,
+            arch,
+            partial_init_regs_access,
+        }
     }
 }
 
@@ -371,28 +431,36 @@ impl Display for OwnedInsn<'_> {
 
 impl InsnDetail<'_> {
     #[cfg(feature = "full")]
-    /// Returns the implicit read registers
+    /// Returns the read registers
     pub fn regs_read(&self) -> &[RegId] {
-        unsafe {
-            &*(&self.0.regs_read[..self.0.regs_read_count as usize] as *const [RegIdInt]
-                as *const [RegId])
+        if let Some(partial) = self.partial_init_regs_access.as_ref() {
+            partial.read()
+        } else {
+            unsafe {
+                &*(&self.detail.regs_read[..self.detail.regs_read_count as usize]
+                    as *const [RegIdInt] as *const [RegId])
+            }
         }
     }
 
     #[cfg(feature = "full")]
-    /// Returns the implicit write registers
+    /// Returns the written to registers
     pub fn regs_write(&self) -> &[RegId] {
-        unsafe {
-            &*(&self.0.regs_write[..self.0.regs_write_count as usize] as *const [RegIdInt]
-                as *const [RegId])
+        if let Some(partial) = self.partial_init_regs_access.as_ref() {
+            partial.write()
+        } else {
+            unsafe {
+                &*(&self.detail.regs_write[..self.detail.regs_write_count as usize]
+                    as *const [RegIdInt] as *const [RegId])
+            }
         }
     }
 
     #[cfg(feature = "full")]
     /// Returns the groups to which this instruction belongs
     pub fn groups(&self) -> &[InsnGroupId] {
         unsafe {
-            &*(&self.0.groups[..self.0.groups_count as usize] as *const [InsnGroupIdInt]
+            &*(&self.detail.groups[..self.detail.groups_count as usize] as *const [InsnGroupIdInt]
                 as *const [InsnGroupId])
         }
     }
@@ -407,10 +475,10 @@ impl InsnDetail<'_> {
                 use crate::Arch::*;
                 $( use crate::arch::$arch::$insn_detail; )*
 
-                return match self.1 {
+                return match self.arch {
                     $(
                         $ARCH => {
-                            $detail($insn_detail(unsafe { &self.0.__bindgen_anon_1.$arch }))
+                            $detail($insn_detail(unsafe { &self.detail.__bindgen_anon_1.$arch }))
                         }
                     )*
                 }