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

RegSpec is annoying to match on #11

Open
i509VCB opened this issue Oct 13, 2021 · 3 comments
Open

RegSpec is annoying to match on #11

i509VCB opened this issue Oct 13, 2021 · 3 comments

Comments

@i509VCB
Copy link
Contributor

i509VCB commented Oct 13, 2021

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
Copy link
Owner

iximeow commented Oct 13, 2021

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
Copy link
Contributor Author

i509VCB commented Oct 13, 2021

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
Copy link
Contributor

chc4 commented Apr 23, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants