capstone-rust / capstone-rs

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

Allow Instructions to deref to slice of Insn #82

Closed ExPixel closed 4 years ago

ExPixel commented 4 years ago

Makes Insn transparent so that cs_insn can be safely converted to it. This makes it possible to just make Instructions deref to a slice of Insn so that is can be passed around more easily without an extra layer of indirection on top of the slice.

This is a small quality of life improvement that allows things like this:

pub fn use_instrs<'i>(insn: &[Insn<'i>]) {
    // use as a slice
}

pub fn create_instrs() {
    let cs = Capstone::new()
        .x86()
        .mode(arch::x86::ArchMode::Mode64)
        .syntax(arch::x86::ArchSyntax::Intel)
        .detail(true)
        .build().unwrap();
    let insn = cs
        .disasm_all(symbol_code, test_symbol.addr).unwrap();

    // Deref will coerce to &[Insn<'i>] here
    use_instrs(&insn);
}
ExPixel commented 4 years ago

Makes sense. Added a test to make sure that cs_insn and Insn always have the same size and alignment. It also checks the conversion itself.

tmfink commented 4 years ago

@ExPixel I want you to know that I haven't forgotten about this PR--I'm just trying to convince myself that this is actually safe.

ExPixel commented 4 years ago

Admittedly, the slice::from_raw_parts might be unnecessary and I might be able to get away with a couple of (still unsafe) casts using as instead. I think that might actually be the recommended way of doing what amounts to the same thing and it's less error prone. I'll do that instead and update the PR.

tmfink commented 4 years ago

Admittedly, the slice::from_raw_parts might be unnecessary and I might be able to get away with a couple of (still unsafe) casts using as instead. I think that might actually be the recommended way of doing what amounts to the same thing and it's less error prone. I'll do that instead and update the PR.

Great--could you also rebase your changes onto master (you don' need to squash)? I fixed some of the Cirrus CI config that fixes the FreeBSD tests.

tmfink commented 4 years ago

@ExPixel the more I think about this, the more I think we should rely on the Deref impl to cover what we want for Instructions. There's no point in defining an .iter() method or InstructionIterator.

If you want, you can try tackling the cleanup. Otherwise, I'll do it myself in a follow-up PR.

repr(transparent) wasn't stable when this logic was first implemented, but we may as well take advantage. :smile:

ExPixel commented 4 years ago

I'll look into cleaning up the uses of InstructionsIterator as well. I'll probably make another pull request for that in the next couple of days :smile:

tmfink commented 4 years ago

Merged as 4e07963

tmfink commented 4 years ago

(I squashed/merged manually)