capstone: add cs_regs_access #77
Conversation
Codecov Report
@@ Coverage Diff @@
## master #77 +/- ##
==========================================
+ Coverage 93.81% 94.17% +0.35%
==========================================
Files 20 20
Lines 2701 3983 +1282
==========================================
+ Hits 2534 3751 +1217
- Misses 167 232 +65
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Overall, this looks really great.
Once you implement these changes, also add some tests in src/tests.rs. These should basically be an example program that shows how to use the new API, but also calls assert_eq!(...) to verify that you get the expected output.
capstone-rs/src/instruction.rs
Outdated
| @@ -328,6 +345,64 @@ impl<'a> InsnDetail<'a> { | |||
| } | |||
| } | |||
|
|
|||
| impl<'a> InsnRegsAccess { | |||
| fn new(cs: csh, ins: &cs_insn) -> Self { | |||
There was a problem hiding this comment.
Any function that can cause undefined behavior should be marked unsafe.
However, once you move the error checking (checking return value), then this shouldn't need to be marked unsafe.
There was a problem hiding this comment.
Have this return a CsResult<Self> instead
| /// 1. Instruction was created with detail enabled | ||
| /// 2. Skipdata is disabled | ||
| /// 3. Capstone was not compiled in diet mode | ||
| pub fn insn_regs_access<'s, 'i: 's>(&'s self, insn: &'i Insn) -> CsResult<InsnRegsAccess> { |
There was a problem hiding this comment.
Instead of trying to do all of the error handling here, check the returned integer and convert that to a CsResult.
You can move that logic into
That way, we don't have to manually track all possible errors that capstone should be handling anyway.
For examples, see disasm(). Also, From<capstone_sys::cs_err::Type> is implemented for Error, so you can use Error::from(capstone_sys_rc)` to do the error parsing for you.
| let mut regs_write_count = 0u8; | ||
|
|
||
| unsafe { | ||
| cs_regs_access( |
There was a problem hiding this comment.
check this return value to create a CsResult
| @@ -145,6 +145,14 @@ pub struct Insn<'a> { | |||
| /// `ArchDetail` enum. | |||
| pub struct InsnDetail<'a>(pub(crate) &'a cs_detail, pub(crate) Arch); | |||
|
|
|||
| /// Contains information about registers accessed by an instruction, either explicitly or implicitly | |||
| pub struct InsnRegsAccess { | |||
There was a problem hiding this comment.
Add #[derive(Debug, Clone, PartialEq, Eq)]
There was a problem hiding this comment.
Debug, PartialEq, Eq can't be derived because of the array with size >32, I added a comment
There was a problem hiding this comment.
Could you manually impl PartialEq, Eq?
| @@ -328,6 +345,64 @@ impl<'a> InsnDetail<'a> { | |||
| } | |||
| } | |||
|
|
|||
| impl<'a> InsnRegsAccess { | |||
| fn new(cs: csh, ins: &cs_insn) -> Self { | |||
| let mut regs_read = [0u16; 64]; | |||
There was a problem hiding this comment.
Use cs_regs type directly instead of manually declaring an array.
There was a problem hiding this comment.
Can you give an example how to initialize a variable with a type?
There was a problem hiding this comment.
It looks like cs_regs is a type alias:
pub type cs_regs = [u16; 64usize];Just add an explicit type instead of using type inference:
let mut regs_read: cs_regs = [0u16; 64];
|
@SWW13 Thanks for the pull-request! This is great work. If you have any questions related to review comments or capstone, feel free to reach out. |
| @@ -145,6 +145,14 @@ pub struct Insn<'a> { | |||
| /// `ArchDetail` enum. | |||
| pub struct InsnDetail<'a>(pub(crate) &'a cs_detail, pub(crate) Arch); | |||
|
|
|||
| /// Contains information about registers accessed by an instruction, either explicitly or implicitly | |||
| pub struct InsnRegsAccess { | |||
There was a problem hiding this comment.
Could you manually impl PartialEq, Eq?
| @@ -328,6 +345,64 @@ impl<'a> InsnDetail<'a> { | |||
| } | |||
| } | |||
|
|
|||
| impl<'a> InsnRegsAccess { | |||
| fn new(cs: csh, ins: &cs_insn) -> Self { | |||
| let mut regs_read = [0u16; 64]; | |||
There was a problem hiding this comment.
It looks like cs_regs is a type alias:
pub type cs_regs = [u16; 64usize];Just add an explicit type instead of using type inference:
let mut regs_read: cs_regs = [0u16; 64];
Adds support for
cs_regs_access, fixes #63.I'm not sure how to implement the tests yet, the
instructions_*tests are a bit complicated on first sight.