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

Add Support for BPF arch #144

Merged
merged 4 commits into from
Apr 13, 2024
Merged

Conversation

deepaksirone
Copy link
Contributor

Hello,

Thanks for maintaining this project. This PR adds support for BPF in capstone-rs. I regenerated the bindings in capstone-sys using a newer version of bindgen in Cargo.toml since it did not work with clang-16. I am yet to write some tests for the implementation. Please let me know if I am on the right track with this or if I am missing something! :-)

@tmfink
Copy link
Member

tmfink commented Sep 17, 2023

Thank you for the contribution! I'll take a look at your changes.

@tmfink tmfink self-assigned this Sep 17, 2023
Copy link
Member

@tmfink tmfink left a comment

Choose a reason for hiding this comment

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

Overall the changes look good, but please also add tests in capstone-rs/src/test.rs.
You can find tests by building/running capstone-sys/capstone/tests/test_bpf.c.

Copy link
Member

Choose a reason for hiding this comment

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

Please read through the directions in capstone-sys/pre_generated/update-bindings.md; it looks like this file is unformatted

@tmfink
Copy link
Member

tmfink commented Sep 17, 2023

Aside from my comments, I may want to wait to merge this until #143 is merged since that's a big change and there would be conflicts.

@Lancern do you have any opinions?

@codecov
Copy link

codecov bot commented Sep 17, 2023

Codecov Report

Attention: Patch coverage is 67.64706% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 93.35%. Comparing base (74ccb09) to head (b1f6bf9).
Report is 7 commits behind head on master.

❗ Current head b1f6bf9 differs from pull request most recent head 75b4abf. Consider uploading reports for the commit 75b4abf to get more accurate results

Files Patch % Lines
capstone-rs/src/arch/bpf.rs 37.03% 17 Missing ⚠️
capstone-rs/src/test.rs 89.18% 4 Missing ⚠️
capstone-rs/src/constants.rs 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
- Coverage   93.99%   93.35%   -0.65%     
==========================================
  Files          22       23       +1     
  Lines        2733     2799      +66     
  Branches     2687     2753      +66     
==========================================
+ Hits         2569     2613      +44     
- Misses        164      186      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Lancern
Copy link

Lancern commented Sep 17, 2023

Aside from my comments, I may want to wait to merge this until #143 is merged since that's a big change and there would be conflicts.

@Lancern do you have any opinions?

The BPF arch added in this PR looks good and it should be easy to merge it with my PR. If you decide that this PR lands after my PR, I'll try finish my PR quickly.

@tmfink
Copy link
Member

tmfink commented Sep 18, 2023

The BPF arch added in this PR looks good and it should be easy to merge it with my PR. If you decide that this PR lands after my PR, I'll try finish my PR quickly.

@Lancern If you're fine handling any conflicts, then I'll just "merge" whichever PR is ready first.

Also, please don't literally git merge. Instead, please maintain linear history with something like git rebase -i.

@deepaksirone
Copy link
Contributor Author

Hi @tmfink, I have added some tests and fixed the formatting of the generated files.

Copy link
Member

@tmfink tmfink left a comment

Choose a reason for hiding this comment

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

After making the changes, please run cargo fmt to format the entire repo.


}

fn test_ebpf() {
Copy link
Member

Choose a reason for hiding this comment

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

Also add a test function test_arch_bpf_detail() that tests the details for each instruction. These test_arch_*_detail() tests catch a lot of bugs and changes in behavior when we upgrade the C library. For an example, see test_arch_x86_detail():

fn test_arch_x86_detail() {

@chengshuyi
Copy link

Hi, are there any plans to merge this PR? I look forward to using this feature. Thanks.

@deepaksirone
Copy link
Contributor Author

Hi @tmfink and @chengshuyi I have added the tests requested. Please let me know if there are any more changes needed

Copy link
Member

@tmfink tmfink left a comment

Choose a reason for hiding this comment

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

minor issues; I'll clean them up

capstone-rs/src/arch/mod.rs Show resolved Hide resolved
capstone-rs/src/test.rs Show resolved Hide resolved
capstone-rs/src/test.rs Show resolved Hide resolved
@tmfink tmfink changed the title [WIP] Add Support for BPF in capstone-rs Add Support for BPF Apr 13, 2024
@tmfink tmfink changed the title Add Support for BPF Add Support for BPF arch Apr 13, 2024
@tmfink tmfink merged commit 0fbf3c1 into capstone-rust:master Apr 13, 2024
4 of 9 checks passed
@tmfink
Copy link
Member

tmfink commented Apr 13, 2024

Thanks for the PR!
I'll do some minor clean-up myself

@tmfink
Copy link
Member

tmfink commented Apr 13, 2024

Did some minor clean-up w/ 38bbcce

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.

None yet

4 participants