Feedback on Bindings for an x86 Decoder (Fadec)

code review
1

I have written Rust bindings for my x86(-64) decoder, Fadec. Besides appreciating general feedback, I have some specific questions:

  • Using Result<Instr, Error> generates copies that seem unavoidable, but are quite costly. I suppose there is no way to avoid this other than using unsafe? (Cf. my workaround here with an out-pointer parameter.)
  • The Reg enum is rather ugly, but I do want niche optimization so that Option<Reg> is one byte. I also expect users to cast the reg index to integers regularly. I suppose there is no alternative right now?
  • I noticed that constants and enumerators show their assigned value in the documentation. Is there a way to avoid this, as the exact numbers should be considered as unstable?
  • Duplicating all logic and documentation from C macros is annoying and mismatches are currently only mitigated through somewhat extensive testing. Are there viable alternatives?

Thanks in advance to whoever spends time looking at this.

2

Are there any benchmarks to verify that this unreachable_unchecked improves performance? if it does a comment should mention them, if it doesn't it should probably be replaced with normal unreachable!()

Using Result<Instr, Error> generates copies that seem unavoidable, but are quite costly. I suppose there is no way to avoid this other than using unsafe?

You could write a macro for calling the unsafe function correctly and assigning the result to a variable, the macro can be safe if the unsafe code it generates is always sound.

3

Let me know if you find a non-horrific way to write the encoder side, the best I've seen so far is directly implementing the encoder into a proc-macro!

I'd mildly suggest looking into bytemuck to verify you don't have padding bytes and make the decode methods a touch safer, but if you're concerned about move elision I don't know what else you could do other than move the decode to Rust - which kind of defeats the point.

Since it's Copy, it should be equivalent to take a &mut Instr other than the required init on the caller side: I assume the use-case here is a tight loop that could reuse a local, so that might be a desirable trade-off for getting rid of the caller-unsafe.

If you want to hide enum values I think the best you can do is use eg #[repr(transparent)] pub struct OpKind(OpKindInner); - it might be overkill though since it's pretty hard to accidentally depend on them.

I'm a bit confused by the Reg IP=R16/R32 stuff, AFAIK like all the x87, control and vector registers RIP is completely separate from the GPRs. Is Reg just a generic index, not specific to GPRs? I guess that's what you were meaning by saying you wanted to keep the niche if so, but then the variant naming is a bit misleading.

Otherwise at a glance this all looks pretty typical. Maybe using some common crates like bitflags for Effects or thiserror for Error, that sort of thing, but it's fairly minor.

The advice for sharing C definitions is to use bindgen, it might be more work to get set up how you like than just handwriting them though