Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regs access #149

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Regs access #149

wants to merge 10 commits into from

Conversation

joleeee
Copy link

@joleeee joleeee commented Oct 13, 2023

I think the if cfg! is kinda weird and should probably rather have it just gate the entire function instead? Anyway, I'm not sure how to handle assert!(len <= ints.len()); better, but it should never happen anyway so it probably doesn't matter?

Also I just picked 64 because python uses that, though I have a hard time imagining reading/writing to more than 64 registers: https://github.com/capstone-engine/capstone/blob/5fb8a423d4455cade99b12912142fd3a0c10d957/bindings/python/capstone/__init__.py#L929

@joleeee
Copy link
Author

joleeee commented Oct 13, 2023

Also @tmfink, whats the current status on the project? Seems like it's missing some updates? I could possibly step in and try and get things up to date.

@tmfink
Copy link
Member

tmfink commented Oct 16, 2023

I think the if cfg! is kinda weird and should probably rather have it just gate the entire function instead? Anyway, I'm not sure how to handle assert!(len <= ints.len()); better, but it should never happen anyway so it probably doesn't matter?

See the review comments (to come).

Also I just picked 64 because python uses that, though I have a hard time imagining reading/writing to more than 64 registers: https://github.com/capstone-engine/capstone/blob/5fb8a423d4455cade99b12912142fd3a0c10d957/bindings/python/capstone/__init__.py#L929

64 is a fine value, but should probably be defined as a constant (magic numbers are not self-describing).

Looks like this would fix #63.

@tmfink tmfink self-assigned this Oct 16, 2023
capstone-rs/src/capstone.rs Outdated Show resolved Hide resolved
to_vec(regs_write, regs_write_count as usize),
)
}))
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use an early return in the case of an error to reduce the indentation of the "happy" case

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you referring to the entire outer if cfg!(...) {? I agree it's cleaner, however the entire file is written this way currently :)

capstone-rs/src/capstone.rs Outdated Show resolved Hide resolved
@@ -365,6 +365,44 @@ impl Capstone {
}
}

/// Get the registers are which are read to and written to, in that order.
pub fn regs_access(&self, insn: &Insn) -> Option<CsResult<(Vec<RegId>, Vec<RegId>)>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like forcing allocations here by returning Vec. You can keep an allocation version for convenience, but please also create a regs_access_buf() where the user can provide their own buffers (which may just be stack allocated).

For simplicity, the regs_access() can call regs_access_buf()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I just append the registers to the provided buffers, or should i clear them first?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user may provide stack allocated buffers, doesn't that mean they generally have to provide the length aswell? If they can pass a regs_read: &mut [u16; MAX_NUM_REGISTERS], how will we specify how many were actually read?

@@ -365,6 +365,44 @@ impl Capstone {
}
}

/// Get the registers are which are read to and written to, in that order.
pub fn regs_access(&self, insn: &Insn) -> Option<CsResult<(Vec<RegId>, Vec<RegId>)>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using a tuple, please make a new struct that makes it clear what is the read and write fields:

pub struct RegAccess {
    read: Vec<RegId>,
    write: Vec<RegId>,
}

The return type would be CsResult<RegAccess>.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used that struct but made the fields public, is it good now?

capstone-rs/src/capstone.rs Outdated Show resolved Hide resolved
@tmfink
Copy link
Member

tmfink commented Oct 16, 2023

Also @tmfink, whats the current status on the project? Seems like it's missing some updates? I could possibly step in and try and get things up to date.

I'll file a separate issue to address this question.

To update the internal capstone version, please:

  1. Update the update_capstone.rs script and run it to update the C library:
    CAPSTONE_REVISION="3b2984212fe524fd99217aa11d4486933a79f123"
  2. capstone-sys/pre_generated/update-bindings.md
  3. Fix any build/test issues that come up.

@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Merging #149 (105a803) into master (7c9a51f) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 105a803 differs from pull request most recent head 29e13a0. Consider uploading reports for the commit 29e13a0 to get more accurate results

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
- Coverage   93.99%   93.98%   -0.01%     
==========================================
  Files          22       22              
  Lines        2731     2728       -3     
  Branches     2685     2682       -3     
==========================================
- Hits         2567     2564       -3     
  Misses        164      164              
Files Coverage Δ
capstone-rs/src/capstone.rs 92.36% <ø> (ø)

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tmfink
Copy link
Member

tmfink commented Oct 16, 2023

Also @tmfink, whats the current status on the project? Seems like it's missing some updates? I could possibly step in and try and get things up to date.

I'll file a separate issue to address this question.

I just filed #151.
Thanks for asking about this. I've thought about this for a while but you finally gave me the push I needed. 😄

@joleeee joleeee requested a review from tmfink October 16, 2023 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants