search

capstone-rust / capstone-rs

high-level Capstone system bindings for Rust
213 stars 75 forks source link

Add Support for BPF arch #144

Closed deepaksirone closed 3 months ago

deepaksirone commented 11 months ago

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 commented 10 months ago

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

tmfink commented 10 months ago

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[bot] commented 10 months ago

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.

:exclamation: 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 :warning:
capstone-rs/src/test.rs 89.18% 4 Missing :warning:
capstone-rs/src/constants.rs 66.66% 1 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Lancern commented 10 months ago

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 commented 10 months ago

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 commented 9 months ago

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

chengshuyi commented 4 months ago

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

deepaksirone commented 4 months ago

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

tmfink commented 3 months ago

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

tmfink commented 3 months ago

Did some minor clean-up w/ 38bbcce944e42a44638f99489b669f4516cd7001