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 cs_buffer. Breaking API change. #2367

Closed
wants to merge 24 commits into from
Closed

Conversation

numas13
Copy link

@numas13 numas13 commented May 25, 2024

Remove cs_malloc() and cs_free().

cs_disasm_iter() is a tiny wrapper around cs_disasm(count=1).

Your checklist for this pull request

  • I've documented or updated the documentation of every API function and struct this PR changes.
  • I've added tests that prove my fix is effective or that my feature works (if possible)

Detailed description

API change unifies disassembly process. Before, there were two separate functions to disassemble one instruction at a time in a loop and many instructions into a dynamic buffer. Commit will introduce user allocatable buffer that can be used in both situations with one function.

Updating the use of cs_disasm_iter() is a little more complicated than cs_disasm():

// old api
cs_insn *insn = cs_malloc(handle);
while (cs_disasm_iter(handle, &code, &code_size, &ip, insn)) {
    disassembled_instructions += 1;
}
cs_free(insn);

Must be changed to:

// new api
cs_buffer *buffer = cs_buffer_new(1); // create buffer with 1 element
while (cs_disasm_iter(handle, &code, &code_size, &ip, buffer)) {
    cs_insn *insn = &buffer->insn[0]; // get first insn in a buffer
    disassembled_instructions += 1;
}
cs_buffer_free(buffer); // free buffer

Updating the use of cs_disasm() is straightforward, just use cs_buffer_new(0) to create a buffer and pass it to cs_disasm().

Benchmarks

❯ hyperfine -N ./orig/test_iter_benchmark ./test_iter_benchmark
Benchmark 1: ./orig/test_iter_benchmark
  Time (mean ± σ):     10.632 s ±  0.227 s    [User: 10.622 s, System: 0.004 s]
  Range (min … max):   10.327 s … 11.007 s    10 runs
 
Benchmark 2: ./test_iter_benchmark
  Time (mean ± σ):     10.815 s ±  0.082 s    [User: 10.812 s, System: 0.001 s]
  Range (min … max):   10.695 s … 10.986 s    10 runs
 
Summary
  './orig/test_iter_benchmark' ran
    1.02 ± 0.02 times faster than './test_iter_benchmark'
❯ ARGS="10 0x028800 0x1517dd libc.so"; hyperfine -N "./orig/test_file_benchmark $ARGS" "./test_file_benchmark $ARGS"
Benchmark 1: ./orig/test_file_benchmark 10 0x028800 0x1517dd libc.so
  Time (mean ± σ):      2.352 s ±  0.035 s    [User: 2.349 s, System: 0.002 s]
  Range (min … max):    2.299 s …  2.388 s    10 runs
 
Benchmark 2: ./test_file_benchmark 10 0x028800 0x1517dd libc.so
  Time (mean ± σ):      2.392 s ±  0.053 s    [User: 2.387 s, System: 0.003 s]
  Range (min … max):    2.329 s …  2.497 s    10 runs
 
Summary
  './orig/test_file_benchmark 10 0x028800 0x1517dd libc.so' ran
    1.02 ± 0.03 times faster than './test_file_benchmark 10 0x028800 0x1517dd libc.so'

Test plan

Update bindings and user software.

Closing issues

...

Copy link
Collaborator

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

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

Looks awesome! It really cleans up the API a lot. Especially internally.
Please rebase this on onto the ASAN PR from me or wait until it is merged: #2313
Just so we get extra test coverage.

@aquynh @kabeor Please take a look. I am very much in favor of this.

cs.c Show resolved Hide resolved
cs.c Show resolved Hide resolved
cs.c Outdated Show resolved Hide resolved
cs.c Show resolved Hide resolved
include/capstone/capstone.h Show resolved Hide resolved
cs.c Outdated Show resolved Hide resolved
@numas13
Copy link
Author

numas13 commented May 26, 2024

Please rebase this on onto the ASAN PR from me or wait until it is merged: #2313 Just so we get extra test coverage.

There is one problem in test_arm_regression. Is it expected?

@numas13
Copy link
Author

numas13 commented May 26, 2024

I've updated python bindings and review is appreciated.

@numas13 numas13 force-pushed the next branch 2 times, most recently from 9b792b2 to 01397fe Compare May 31, 2024 15:27
@aquynh
Copy link
Collaborator

aquynh commented May 31, 2024

please avoid breaking API

Remove cs_malloc() and cs_free().

API change unifies disassembly process. Before, there were two separate
functions to disassemble one instruction at a time in a loop and many
instructions into a dynamic buffer. Commit will introduce user allocatable
buffer that can be used in both situations with one function.

cs_disasm_iter() is a tiny wrapper around cs_disasm().

Updating the use of cs_disasm_iter():

    // old api
    cs_insn *insn = cs_malloc(handle);
    while (cs_disasm_iter(handle, &code, &code_size, &ip, insn)) {
        disassembled_instructions += 1;
    }
    cs_free(insn);

Must be changed to:

    // new api
    cs_buffer *buffer = cs_buffer_new(1); // create buffer with 1 element
    while (cs_disasm_iter(handle, &code, &code_size, &ip, buffer)) {
        cs_insn *insn = &buffer->insn[0]; // get first insn in a buffer
        disassembled_instructions += 1;
    }
    cs_buffer_free(buffer); // free buffer

Updating the use of cs_disasm() is straightforward, just use
cs_buffer_new(0) to create a buffer and pass it to cs_disasm().
@numas13
Copy link
Author

numas13 commented Jun 1, 2024

I'm closing PRs. We'll think about what can be done.

@numas13 numas13 closed this Jun 1, 2024
@Rot127 Rot127 mentioned this pull request Oct 7, 2024
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.

3 participants