iximeow / yaxpeax-x86

x86 decoders for the yaxpeax project
BSD Zero Clause License
132 stars 23 forks source link

RegSpec is annoying to match on #11

Open i509VCB opened 3 years ago

i509VCB commented 3 years ago

For my use case I need both the protected and real mode modules. To reduce duplicate code I need a way to convert the real and protected mode RegSpec into some enum I can match on for further operations.

However since RegSpec cannot be matched I have to resort to messy code that that can lead to bugs because of missing variant statements:

impl TryFrom<real_mode::RegSpec> for RegisterIndex {
    type Error = InvalidRegister;

    fn try_from(value: real_mode::RegSpec) -> Result<Self, Self::Error> {
        use real_mode::RegSpec;

        Ok(if value == RegSpec::ax() {
            RegisterIndex::AX
        } else if value == RegSpec::bx() {
            RegisterIndex::BX
        } else if value == RegSpec::cx() {
            RegisterIndex::CX
        } else if value == RegSpec::dx() {
            RegisterIndex::DX
        } else if value == RegSpec::si() {
            RegisterIndex::SI
        } else if value == RegSpec::di() {
            RegisterIndex::DI
        } else if value == RegSpec::sp() {
            RegisterIndex::SP
        } else if value == RegSpec::bp() {
            RegisterIndex::BP
        } else if value == RegSpec::EIP {
            RegisterIndex::IP
        } else if value == RegSpec::eflags() {
            RegisterIndex::FLAGS
        } else { // Yes this may be incomplete, I am certain of that...
            return Err(InvalidRegister::Real(value));
        })
    }
}

It would be nice if yaxpeax added a sort of RegEnum that could be matched on the to make these types of statements less annoying to work with.

iximeow commented 3 years ago

what do you need RegisterIndex to be usable for? Hash and Eq at least, i assume. maybe Ord too?

my first thought is that impl Into<long_mode::RegSpec> for protected_mode::RegSpec then similar for real -> long and real -> protected seem to make sense, then you can "promote" the RegSpec into whatever is a consistent type for your usage. how's that sound for what you're doing?

i'm not very familiar with how Into/From interact with type inference, or what issues users might have calling .into() with such impls in scope.

also i'm very curious what you're using yaxpeax-x86 for, i really appreciate the usability issues you've been opening!

i509VCB commented 3 years ago

My use case for yaxpeax-x86 is CPU emulation inside a larger pc98 emulation project I am working on.

I could parse instructions myself since I use a small subset of the available instructions x86 although Rust is a crate centric language so I'd rather shift the burden of the instruction parsing to some other crate I can contribute to so I can focus on the emulation rather than parsing opcodes and having to test for many hours to ensure my parsing is fool proof.

Some of the instructions I have to implement such as INC require deriving the register to increment from the opcode. In the case of yaxpeax-x86 the target register is available as the operand. To avoid needing many match statements I've implemented Index(Mut)<RegisterIndex> on my cpu state type:

impl Index<RegisterIndex> for CpuState {
    type Output = Register;
    ...
}

impl IndexMut<RegisterIndex> for CpuState { ... }

I could move to using some enum provided by yaxpeax-x86 of course, would just mean two index impls instead of one. Needing two impls isn't a huge deal with Index(Mut) and I would prefer not needing to declare even more duplicate types that yaxpeax-x86 would seem to provide.

The index impls are kept simple as they use a match statement and return a borrow of the specified register member of CpuState. At the moment since register classes are effectively const and unmatchable this means I need to content with

The register type is just a transparent u32 with some helper functions.

Hash and Eq at least, i assume. maybe Ord too?

Eq across real and protected mode would be nice.

Although this doesn't really solve my issue here since HashMap lookup would take far too long even if I used rustc_hash::FxHashMap

i'm not very familiar with how Into/From interact with type inference, or what issues users might have calling .into() with such impls in scope.

Generally rustc will do a good job with type inference here. Worst case you may need to From::<DesiredType>::from(in) once in a while.

chc4 commented 2 years ago

Would just like to point out you can match on RegSpec currently, it just requires an unstable feature to do so.

match r {
    const { RegSpec::al() } | const { RegSpec::bl() } |
    const { RegSpec::cl() } => {
        Location::Reg(r)
    },
}

works with #[feature(inline_const)]. Likewise you could express the if-ladder in the OP like this. I think not all of the RegSpec constructors are marked as const fns, though.