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

Add op access for OpCodeInfo and Code #614

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

susitsm
Copy link
Contributor

@susitsm susitsm commented Sep 20, 2024

Iced provides OpAccess through the InstructionInfoFactory interface which does advanced analysis based on concrete operands of an instruction. This merge request aims to add a simpler OpAccess API which uses data derivable from the Code enum and does not need full instructions.

The code compiles but it is a WORK IN PROGRESS.

Unresolved issues:

  • Some code generator generated code was modified in 40f17c3. Those modifications should be moved to the code generator
  • Some new functions are missing documentation
  • Add documentation to InstructionInfo about how it simplifies operand accesses.
  • Move functions to where they belong: most new code was put into src/rust/iced-x86/src/encoder/code_flags.rs, some of it might be better placed somewhere else.
  • Fix the remaining todo!()-s in Code::op0_access. I didn't know how some of them should be converted. I would appreciate any feedback/help you can give me.

@wtfsck
Copy link
Member

wtfsck commented Sep 25, 2024

Makes sense to add those functions to Code/OpCodeInfo.

Not sure if OpCodeInfo needs the extra op_accesses field since it can just call the Code functions.

One thing missing is tests for all new APIs. Could be a lot more work so maybe I'll add that sometime. Would require (C# code, see generator code) generating a new txt file for at least the Code test that would include the Code value and the OpAccess values, write a parser for it (should be easy, split the line), add the test code somewhere.

@susitsm
Copy link
Contributor Author

susitsm commented Sep 29, 2024

Removed op access from OpCodeInfo, there is no need to do the same thing multiple ways. I also added a miniscule amount of docs about the optimizations done by InstructionInfoFactory.

There is one more thing I am not sure about, and that is OpInfo0::WriteMem_ReadWriteReg. Currently, I convert it to OpAccess::ReadWrite, but I am thinking that maybe we should create a CodeOpAccess enum, and just leave it as-is. Similarly to OpCodeOperandKind and OpKind.

Also, I could not find any info about OpInfo0::CondWrite32_ReadWrite64. We might also want to leave that as its own kind.

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

Successfully merging this pull request may close these issues.

2 participants