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

Marked instruction with std::maker::sync. #23

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@felberj

felberj commented Nov 20, 2017

This is done to make it usable by multiple threads. Capstone claims to be 'Thread-safe by design', so this should work.

Note that there are two flaky tests: They sometimes fail with and without my commit.

test test::test_instruction_group_ids ... FAILED
failures:

---- test::test_instruction_group_ids stdout ----
	thread 'test::test_instruction_group_ids' panicked at 'Expected groups {1} does NOT match computed insn groups {110} with ', src/lib.rs:287:8
note: Run with `RUST_BACKTRACE=1` for a backtrace.
test test::test_syntax ... FAILED
failures:

---- test::test_syntax stdout ----
	thread 'test::test_syntax' panicked at 'index 86 out of range for slice of length 8', src/libcore/slice/mod.rs:735:4
note: Run with `RUST_BACKTRACE=1` for a backtrace.
Marked instruction with std::maker::sync.
This is done to make it usable by multiple threads. Capstone claims to be 'Thread-safe by design', so this should work.
@felberj

This comment has been minimized.

Show comment
Hide comment
@felberj

felberj Nov 21, 2017

I just realised this needs more work:

error[E0277]: the trait bound `*mut capstone_sys::cs_detail: std::marker::Sync` is not satisfied in `capstone::Insn`
   --> src/lib.rs:190:9
    |
190 |         thread::spawn(move || {
    |         ^^^^^^^^^^^^^ `*mut capstone_sys::cs_detail` cannot be shared between threads safely
    |
    = help: within `capstone::Insn`, the trait `std::marker::Sync` is not implemented for `*mut capstone_sys::cs_detail`
    = note: required because it appears within the type `capstone_sys::cs_insn`
    = note: required because it appears within the type `capstone::Insn`

Do you think I can just edit this?

felberj commented Nov 21, 2017

I just realised this needs more work:

error[E0277]: the trait bound `*mut capstone_sys::cs_detail: std::marker::Sync` is not satisfied in `capstone::Insn`
   --> src/lib.rs:190:9
    |
190 |         thread::spawn(move || {
    |         ^^^^^^^^^^^^^ `*mut capstone_sys::cs_detail` cannot be shared between threads safely
    |
    = help: within `capstone::Insn`, the trait `std::marker::Sync` is not implemented for `*mut capstone_sys::cs_detail`
    = note: required because it appears within the type `capstone_sys::cs_insn`
    = note: required because it appears within the type `capstone::Insn`

Do you think I can just edit this?

@tmfink

This comment has been minimized.

Show comment
Hide comment
@tmfink

tmfink Nov 22, 2017

Member

The Capstone C library may be "thread safe", but that does not mean the capstone-rs Rust bindings are thread-safe. In order to be thread-safe, Rust has more stringent requirements.

As you noticed, it is unsafe to make Instructions Sync, which would imply that it is safe to access from multiple threads.

Member

tmfink commented Nov 22, 2017

The Capstone C library may be "thread safe", but that does not mean the capstone-rs Rust bindings are thread-safe. In order to be thread-safe, Rust has more stringent requirements.

As you noticed, it is unsafe to make Instructions Sync, which would imply that it is safe to access from multiple threads.

@tmfink tmfink self-requested a review Nov 22, 2017

@tmfink

This comment has been minimized.

Show comment
Hide comment
@tmfink

tmfink Nov 22, 2017

Member

@felberj could you give me more details on your the setup? I'm unable to reproduce this issue.

Information:

  1. Operating system version
  2. CPU architecture
  3. capstone-rs features (if any)

As is, I can't merge this change. It would be incorrect to mark these structs as Sync.

Out of curiousity, what are you trying to do that would require Instructions to be Sync?

Member

tmfink commented Nov 22, 2017

@felberj could you give me more details on your the setup? I'm unable to reproduce this issue.

Information:

  1. Operating system version
  2. CPU architecture
  3. capstone-rs features (if any)

As is, I can't merge this change. It would be incorrect to mark these structs as Sync.

Out of curiousity, what are you trying to do that would require Instructions to be Sync?

@felberj

This comment has been minimized.

Show comment
Hide comment
@felberj

felberj Nov 22, 2017

Thank you for looking into it:

Out of curiousity, what are you trying to do that would require Instructions to be Sync?

I want to analyse instructions from multiple threads, so this is what I wanted to do. What I will probably do now is copy all the information in my own struct.

@felberj could you give me more details on your the setup? I'm unable to reproduce this issue.

If you mean the thread::spawn(move || { error, this was just me trying to send the Instruction over to another thread with this merge request merged. But it did not work.

If you mean the test errors:

Operating system version

macOS

CPU architecture

x86

capstone-rs features (if any)

well, I just invoked cargo test on HEAD multiple times and sometimes it failed.

felberj commented Nov 22, 2017

Thank you for looking into it:

Out of curiousity, what are you trying to do that would require Instructions to be Sync?

I want to analyse instructions from multiple threads, so this is what I wanted to do. What I will probably do now is copy all the information in my own struct.

@felberj could you give me more details on your the setup? I'm unable to reproduce this issue.

If you mean the thread::spawn(move || { error, this was just me trying to send the Instruction over to another thread with this merge request merged. But it did not work.

If you mean the test errors:

Operating system version

macOS

CPU architecture

x86

capstone-rs features (if any)

well, I just invoked cargo test on HEAD multiple times and sometimes it failed.

@tmfink

This comment has been minimized.

Show comment
Hide comment
@tmfink

tmfink Nov 25, 2017

Member

@felberj what version of macOS are you running? Also, what's the output of commands:

uname -a
git rev-parse HEAD
Member

tmfink commented Nov 25, 2017

@felberj what version of macOS are you running? Also, what's the output of commands:

uname -a
git rev-parse HEAD

@tmfink tmfink self-assigned this Nov 25, 2017

@felberj

This comment has been minimized.

Show comment
Hide comment
@felberj

felberj Nov 25, 2017

23:25[capstone-rs]$ uname -a
Darwin MacBook-Pro-2.local 16.7.0 Darwin Kernel Version 16.7.0: Wed Oct  4 00:17:00 PDT 2017; root:xnu-3789.71.6~1/RELEASE_X86_64 x86_64
23:25[capstone-rs]$ git rev-parse HEAD
fe64e02067f4df0bb336452c87f9ccaa619f71c9

macOS Sierra

felberj commented Nov 25, 2017

23:25[capstone-rs]$ uname -a
Darwin MacBook-Pro-2.local 16.7.0 Darwin Kernel Version 16.7.0: Wed Oct  4 00:17:00 PDT 2017; root:xnu-3789.71.6~1/RELEASE_X86_64 x86_64
23:25[capstone-rs]$ git rev-parse HEAD
fe64e02067f4df0bb336452c87f9ccaa619f71c9

macOS Sierra

@tmfink

This comment has been minimized.

Show comment
Hide comment
@tmfink

tmfink Dec 25, 2017

Member

Closing because current pull request does not solve issue.

Further discussion should occur in issue #26

Member

tmfink commented Dec 25, 2017

Closing because current pull request does not solve issue.

Further discussion should occur in issue #26

@tmfink tmfink closed this Dec 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment