cs: eagerly call regs_access() in insn_detail()

When getting instruction details, we now eagerly call regs_access() to
get "extended" read/write registers. This is simpler than requiring
users to manually call it to get the "actual" accessed registers.

Author: tmfink

Cargo.lock

@@ -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/Cargo.toml

@@ -1,3 +1,4 @@
+use alloc::boxed::Box;
 use alloc::string::String;
 use core::convert::From;
 use core::marker::PhantomData;
@@ -10,11 +11,24 @@ 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};
 
+/// 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
 ///
 /// Create with an instance with [`.new()`](Self::new) and disassemble bytes with [`.disasm_all()`](Self::disasm_all).
@@ -101,17 +115,19 @@ impl Iterator for EmptyExtraModeIter {
     }
 }
 
-#[derive(Clone, Debug, PartialEq, Eq, Hash)]
-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 RegAccess {
-    /// Sort read and write fields
-    pub fn sort(&mut self) {
-        self.read.sort_unstable();
-        self.write.sort_unstable();
+impl RegAccessRef<'_> {
+    pub fn read(&self) -> &[RegId] {
+        self.read
+    }
+
+    pub fn write(&self) -> &[RegId] {
+        self.write
     }
 }
 
@@ -380,39 +396,24 @@ impl Capstone {
         }
     }
 
-    // todo(tmfink): integrate into regs_read()/regs_write() methods to avoid the need to call
-    /// Get the registers which are read and written
-    pub fn regs_access(&self, insn: &Insn) -> CsResult<RegAccess> {
-        let mut read = Vec::new();
-        let mut write = Vec::new();
-
-        self.regs_access_buf(insn, &mut read, &mut write)?;
-
-        Ok(RegAccess { read, write })
-    }
-
-    /// Get the registers are which are read and written.
-    /// The registers are pushed to the back of the provided buffers
-    pub(crate) fn regs_access_buf(
+    /// 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: MaybeUninit<cs_regs> = MaybeUninit::uninit();
-            let mut regs_read: MaybeUninit<cs_regs> = MaybeUninit::uninit();
-
             let err = unsafe {
                 cs_regs_access(
                     self.csh(),
                     &insn.insn as *const cs_insn,
-                    regs_read.as_mut_ptr(),
+                    regs_read.as_mut_ptr() as *mut cs_regs,
                     &mut regs_read_count as *mut _,
-                    regs_write.as_mut_ptr(),
+                    regs_write.as_mut_ptr() as *mut cs_regs,
                     &mut regs_write_count as *mut _,
                 )
             };
@@ -423,23 +424,24 @@ impl Capstone {
 
             // SAFETY: count indicates how many elements are initialized;
             let regs_read_slice: &[RegId] = unsafe {
-                std::slice::from_raw_parts(
+                core::slice::from_raw_parts(
                     regs_read.as_mut_ptr() as *mut RegId,
                     regs_read_count as usize,
                 )
             };
-            read.extend_from_slice(regs_read_slice);
 
             // SAFETY: count indicates how many elements are initialized
             let regs_write_slice: &[RegId] = unsafe {
-                std::slice::from_raw_parts(
+                core::slice::from_raw_parts(
                     regs_write.as_mut_ptr() as *mut RegId,
                     regs_write_count as usize,
                 )
             };
-            write.extend_from_slice(regs_write_slice);
 
-            Ok(())
+            Ok(RegAccessRef {
+                read: regs_read_slice,
+                write: regs_write_slice,
+            })
         } else {
             Err(Error::DetailOff)
         }
@@ -472,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/capstone.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 }))
                         }
                     )*
                 }