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

Migrate from Capstone to Zydis (x86 architecture) #4832

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

tushar3q34
Copy link
Contributor

@tushar3q34 tushar3q34 commented Jan 10, 2025

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description

  • Used ZydisDecodedInstruction and ZydisDecodedOperand instead of cs_x86 and cs_x86_op
  • Changed ESIL, RzIL and asm for x86 architecture
  • Currently, this PR fails ~300 tests due to different formatting and some x86-16 specific instructions

TODO in further commits :

  • Change Tests/code to account for differences in outputs presented by Capstone and Zydis
  • Find a workaround for x86-16 specific instructions (lcall,ljump etc)
  • Document functions of analysis_x86 and asm_x86 files

Test plan

...

Closing issues

closes #4709

...

@tushar3q34
Copy link
Contributor Author

tushar3q34 commented Jan 10, 2025

Changes in the tests/code :

  • Extra spaces, extra keywords (qword,dword) in disassembly. For the keywords part, I think there are some formatting options in Zydis so I will try looking at that
  • Changes in ESIL outputs
  • Handle ljmp and lcall. Zydis only uses call and jmp with deciding whether to far jump/call or not depending on operands. So I will look into it and handle the code and necessary testcases in further commits.
  • Handle warnings

@wargio
Copy link
Member

wargio commented Jan 10, 2025

Very well done.

@wargio
Copy link
Member

wargio commented Jan 12, 2025

Please fix the compilation issues. see the CI jobs. also run clang format (via the python script)

@tushar3q34
Copy link
Contributor Author

Please fix the compilation issues. see the CI jobs. also run clang format (via the python script)

Yes, I did not get time yesterday. I will do it asap.

Copy link
Member

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice job! Looks good so far.

Regarding the renaming:
I think it is better if you add a file which maps ZYDIS_ enums to the X86_ enums. Simply for readability and less changes.

Building:
Capstone can be built without any x86 code. You just have to remove -DCAPSTONE_HAS_X86 in subprojects/packagefiles/capstone-*/meson.build. This also makes the binary way smaller.

cc @wargio

.gitignore Outdated Show resolved Hide resolved
[ZYDIS_REGISTER_RDI] = "rdi",
[ZYDIS_REGISTER_RDX] = "rdx",
[ZYDIS_REGISTER_RIP] = "rip",
//[ZYDIS_REGISTER_RIZ] = "riz",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the unused code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am currently keeping some of the unused code because they might be required while resolving tests. Once that is done, I will get rid of the unused code.

librz/arch/isa/x86/common.c Outdated Show resolved Hide resolved
librz/arch/isa/x86/il_ops.inc Outdated Show resolved Hide resolved
librz/arch/isa/x86/x86_dwarf_regnum_table.h Outdated Show resolved Hide resolved
librz/arch/p/asm/asm_x86_zydis.c Outdated Show resolved Hide resolved
librz/arch/p/asm/asm_x86_zydis.c Outdated Show resolved Hide resolved
librz/arch/p/asm/asm_x86_zydis.c Outdated Show resolved Hide resolved
subprojects/zydis.wrap Outdated Show resolved Hide resolved
subprojects/packagefiles/zydis/meson.build Outdated Show resolved Hide resolved
@tushar3q34
Copy link
Contributor Author

Hello, I am working on these and I will commit the changes soon.
Also, in one of the jobs
subprojects/zydis/meson.build:1:0: ERROR: Got unknown keyword arguments "license_files"
This is because it uses meson==0.61.5 while the requirement for Zydis is meson_version >= 1.3
One of the solutions could be rewriting the build file of Zydis using a patch to support >=0.59 versions. Is there any better way to handle this?

@Rot127
Copy link
Member

Rot127 commented Jan 12, 2025

One of the solutions could be rewriting the build file of Zydis using a patch to support >=0.59 versions. Is there any better way to handle this?

Ouh, nice. Didn't know it uses Meson as well. Why is subprojects/packagefiles/zydis/meson.build needed then?
Doesn't it overwrite the original file?

cc @wargio For what to do about updating meson. But bumping the version seems fine to me. I build with 1.4.0 normally.

@Rot127
Copy link
Member

Rot127 commented Jan 12, 2025

Seems like we need the old meson version for TinyCC and old debian builds. So it must be the patch then :(

@tushar3q34
Copy link
Contributor Author

Doesn't it overwrite the original file?

It customises the build process, doesn't overwrite it.

So it must be the patch then

Okay, i will look into this. There are a few things that old version of meson doesn't have like enable_auto_if() and c_static_args etc as an argument in library. So I will patch them using .diff files while cloning using wrap-git or wrap-file

@wargio
Copy link
Member

wargio commented Jan 13, 2025

So I will patch them using .diff files while cloning using wrap-git or wrap-file

dont diff patch. just create a folder with the patched files and define it in the wrap file. this will overwrite the files in the pulled folder.

@tushar3q34
Copy link
Contributor Author

The latest version of Zydis (4.1.0) does not support meson build system so we cannot use a wrap-file.
The older versions support CMake and it is possible to run CMake command using meson (which will also not cause the issue with meson version ) but some of the CI jobs do not have CMake.
Since we cannot use wrap-file, we cannot directly patch files using a folder as wrap-git doesn't support it. What should be done? @wargio

@wargio
Copy link
Member

wargio commented Jan 13, 2025

so we cannot use a wrap-file.

capstone does not support meson neither but we use meson anyway. just create a meson.build file in the patch dir and add the sources and defines.

Examples:

@tushar3q34 tushar3q34 force-pushed the dist-capstone-to-zydis-x86 branch 2 times, most recently from 8854385 to ed87201 Compare January 14, 2025 23:27
@tushar3q34
Copy link
Contributor Author

@wargio So the current issue with the builds is that there is no Zycore dependency installed in Zydis by default. To do that, we would have to build Zycore inside Zydis using another meson.build file.
A better way would be to use amalgamated form of Zydis. So we only need to compile one file, Zydis.c using only one header Zydis.h. A disadvantage to that will be that we will not have a well defined structure of the subproject.
Which one do you suggest?

@wargio
Copy link
Member

wargio commented Jan 16, 2025

Why not just add a folder with the wrap file of the dependency within the patch folder of zydis? Or if available just use a source which contains all the deps needed. This is quite common

@Rot127
Copy link
Member

Rot127 commented Jan 16, 2025

Agree with @wargio. Meson has (luckily) the ability to make subprojects depend on subprojects.

@tushar3q34 tushar3q34 force-pushed the dist-capstone-to-zydis-x86 branch from ed87201 to 75ecbfd Compare January 21, 2025 16:54
@tushar3q34
Copy link
Contributor Author

tushar3q34 commented Jan 21, 2025

Hello. So I tried building zycore inside zydis but I faced the following issues:

  • When I try to get zycore from zydis as a subproject, it gets redirected as wrap-redirect and zycore is created outside. Hence we need to git clean subproject every time before rebuilding it.
  • Since zycore also uses meson ver >= 1.3, we have to write patch for it too and writing a patch in the subprojects directory of the patch of zydis does not work due to the wrap-redirect (it cannot identify the location of build file, according to meson docs, subproject of a subproject is always built outside and then transferred inside).

I think there must definitely be a solution but I cannot seem to get it due to my inexperience in meson build system. So I would really like some help in this direction. As of now, I have pushed the changes with amalgamated form of zydis.

In the meanwhile, I resolved ~150 tests that were mostly formatting related. There are still ~70 tests that I need to review and debug code accordingly. I'm working on it. Thanks.

Edit : I think I merged dev into my branch as well. Should I remove it?
After merging from dev, more tests fail. I will change tests and debug code accordingly.
For now, opex, x86_16 and RzIL code has to be fixed.

@tushar3q34 tushar3q34 force-pushed the dist-capstone-to-zydis-x86 branch from 3e72d6c to 187ebe4 Compare January 22, 2025 06:44
@github-actions github-actions bot removed the RzSearch label Feb 2, 2025
@tushar3q34 tushar3q34 force-pushed the dist-capstone-to-zydis-x86 branch 3 times, most recently from edbe9fc to e04b29e Compare February 2, 2025 16:57
@tushar3q34
Copy link
Contributor Author

I have resolved ~700 tests and there are ~650 more to resolve. Most of them are simple op_size errors in RzIL, mostly because of the difference in the way Zydis handles the operand size as compared to Capstone. And the others are related to argument/variable handling which I haven't resolved yet.

I am also working on making a separate file with all X86_INS/REG_* to ZYDIS_MNEMONIC/REGISTER_* etc. conversion as suggested.

For the appveyor fails, I think it is because the default setting uses dynamic linking instead of a static one, so I have to remove a flag while compiling.

@tushar3q34 tushar3q34 force-pushed the dist-capstone-to-zydis-x86 branch 11 times, most recently from 8d5b4d4 to 1ab01fd Compare February 10, 2025 11:45
@tushar3q34 tushar3q34 force-pushed the dist-capstone-to-zydis-x86 branch from 1ab01fd to 5c45b2a Compare February 10, 2025 12:03
@tushar3q34
Copy link
Contributor Author

tushar3q34 commented Feb 10, 2025

The issue with appveyor builds was not related to dynamic linking, as by default, every dependency is built statically in rizin. The issue was with ZYDIS_STATIC_BUILD flag which was needed for linker to identify function definitions. I resolved it now. I also changed appveyor.yml to show verbosed output, and I will revert it to the original state in the following commits. Working on remaining tests.
Also created a new file which has all X86_* v/s ZYDIS_*

@wargio
Copy link
Member

wargio commented Feb 10, 2025

nice

@tushar3q34 tushar3q34 force-pushed the dist-capstone-to-zydis-x86 branch from 5c45b2a to f1fe51a Compare February 10, 2025 17:16
@tushar3q34
Copy link
Contributor Author

Is rz-bindgen failure related to this PR?

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

Successfully merging this pull request may close these issues.

Migrate from Capstone to Zydis for x86 and x86_64 architectures
3 participants