capstone-rust / capstone-rs

high-level Capstone system bindings for Rust
220 stars 78 forks source link

Add not_diet feature #130

Closed clubby789 closed 2 years ago

clubby789 commented 2 years ago

This PR adds a new default feature, not_diet to both crates. If disabled, Capstone will be compiled in Diet mode, which would result in speed/size improvements. Currently, many tests will fail if run with this feature disabled, and will need to be gated behind it.

It might be more legible to invert this feature #[cfg(feature = "diet")] but I believe Cargo's dependency resolution requires that features always add new items, rather than removing them.

Rather than directly returning None, we could instead defer to Capstone's API (returning NULL pointers or empty strings) - this way is more explicit but with more repeated code.

codecov[bot] commented 2 years ago

Codecov Report

Merging #130 (c173f4f) into master (c314099) will decrease coverage by 0.00%. The diff coverage is 100.00%.

:exclamation: Current head c173f4f differs from pull request most recent head 74eec43. Consider uploading reports for the commit 74eec43 to get more accurate results

@@            Coverage Diff             @@
##           master     #130      +/-   ##
==========================================
- Coverage   94.64%   94.64%   -0.01%     
==========================================
  Files          22       22              
  Lines        2690     2687       -3     
  Branches     2651     2645       -6     
==========================================
- Hits         2546     2543       -3     
  Misses        144      144              
Impacted Files Coverage Δ
capstone-rs/examples/demo.rs 92.30% <ø> (-1.93%) :arrow_down:
capstone-rs/src/arch/m68k.rs 92.57% <ø> (ø)
capstone-rs/src/capstone.rs 94.16% <ø> (+0.63%) :arrow_up:
capstone-rs/src/ffi.rs 100.00% <ø> (ø)
capstone-rs/src/instruction.rs 93.91% <100.00%> (ø)
capstone-rs/src/test.rs 97.61% <100.00%> (-0.01%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c314099...74eec43. Read the comment docs.

clubby789 commented 2 years ago

Tests mostly pass now, but 3 issues:

---- test::test_arch_m68k_detail stdout ----
thread 'test::test_arch_m68k_detail' panicked at 'assertion failed: `(left == right)`
  left: `[M68kOperand(Reg(RegId(1))), M68kOperand(RegPair(RegId(5), RegId(6)))]`,
 right: `[]`: operands do not match for "0x1000: " (bytes=[4c, 00, 54, 04])', capstone-rs/src/test.rs:269:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- test::test_arch_ppc_detail stdout ----
thread 'test::test_arch_ppc_detail' panicked at 'assertion failed: `(left == right)`
  left: `[PpcOperand(Reg(RegId(88))), PpcOperand(Mem(PpcOpMem(ppc_op_mem { base: 0, disp: 0 })))]`,
 right: `[PpcOperand(Reg(RegId(88))), PpcOperand(Mem(PpcOpMem(ppc_op_mem { base: 10, disp: 0 })))]`: operands do not match for "0x1000: " (bytes=[80, 20, 00, 00])', capstone-rs/src/test.rs:269:5

---- test::test_arch_x86_detail stdout ----
thread 'test::test_arch_x86_detail' panicked at 'assertion failed: `(left == right)`
  left: `[X86Operand(X86Operand { size: 2, access: Some(WriteOnly), avx_bcast: X86_AVX_BCAST_INVALID, avx_zero_opmask: false, op_type: Reg(RegId(12)) }), X86Operand(X86Operand { size: 2, access: Some(ReadOnly), avx_bcast: X86_AVX_BCAST_INVALID, avx_zero_opmask: false, op_type: Mem(X86OpMem(x86_op_mem { segment: 0, base: 45, index: 0, scale: 1, disp: 50 })) })]`,
 right: `[X86Operand(X86Operand { size: 2, access: None, avx_bcast: X86_AVX_BCAST_INVALID, avx_zero_opmask: false, op_type: Reg(RegId(12)) }), X86Operand(X86Operand { size: 2, access: None, avx_bcast: X86_AVX_BCAST_INVALID, avx_zero_opmask: false, op_type: Mem(X86OpMem(x86_op_mem { segment: 0, base: 45, index: 0, scale: 1, disp: 50 })) })]`: operands do not match for "0x1000: " (bytes=[8d, 4c, 32])', capstone-rs/src/test.rs:269:5

Operands seem to being incorrectly processed in some arches.

I also removed the diet mode check from insn_detail - diet mode only removes the regs read/written and insn group fields from detail.

clubby789 commented 2 years ago

This behaviour seems to have been fixed in upstream - merging this PR on top of #129 makes all tests pass

tmfink commented 2 years ago

Sorry for the late reply. Thanks for the changes--I've considered adding the diet support before.

clubby789 commented 2 years ago

Any changes needed? The test failures look like a bug with Valgrind/the test harness itself

tmfink commented 2 years ago

Thanks for the change and sorry for taking so long. I created #133 to track the Valgrind issue.