Migrate from Capstone to Zydis (x86 architecture) #4832

tushar3q34 · 2025-01-10T15:07:09Z

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 · 2025-01-10T15:17:11Z

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 · 2025-01-10T15:48:58Z

Very well done.

wargio · 2025-01-12T07:34:23Z

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

tushar3q34 · 2025-01-12T07:38:46Z

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.

Rot127

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

librz/arch/isa/x86/common.c

librz/arch/isa/x86/il_ops.inc

librz/arch/isa/x86/x86_dwarf_regnum_table.h

librz/arch/p/asm/asm_x86_zydis.c

Rot127 · 2025-01-12T16:40:10Z

subprojects/zydis.wrap

+directory = zydis
+patch-directory = zydis
+depth = 1
+revision = master


Is there any big change in the master branch? Otherwise sticking to a released package (and using wrap-file) is preferred.

Ping on this one :)
Did we agreed on something and I just forgot it?

subprojects/packagefiles/zydis/meson.build

tushar3q34 · 2025-01-12T17:23:55Z

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 · 2025-01-12T17:52:20Z

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 · 2025-01-12T17:56:43Z

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

tushar3q34 · 2025-01-12T18:02:28Z

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 · 2025-01-13T01:53:10Z

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 · 2025-01-13T11:33:12Z

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 · 2025-01-13T13:27:50Z

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 · 2025-01-16T04:39:15Z

@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 · 2025-01-16T05:19:28Z

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 · 2025-01-16T16:23:53Z

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

tushar3q34 · 2025-01-21T17:02:42Z

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 · 2025-02-20T15:26:08Z

@wargio, @notxvilka, @Rot127 I have resolved the tests. Please review the changes.

Should I also refactor analysis_x86_zydis.c file? It had a context struct X86CSContext (now X86ZYDISContext) which was implemented but not used. As of now, I'm passing operands, instructions and pc value separately as parameters to all the functions while passing only the context object can be sufficient.

I will also remove the unused code asap.

librz/arch/isa/x86/il_ops.inc

librz/arch/isa/x86/x86_il.c

librz/core/canalysis.c

wargio · 2025-02-20T15:34:58Z

test/db/abi/compilers/pelles_c

-   76 * pointers
+   66 * pointers


can you check what exactly is different? you can use the fl commands and diff this.

fl result remains unchanged

wargio · 2025-02-20T15:37:51Z

test/db/analysis/x86_64

-0x00400730      f3c3           repz  ret
+0x00400730      f3c3           ret


hmm, interesting change.

wargio

Wow, so many changes, great work. most of them looks good. i will take some days to review everything.

Could you do us a favour and try to use rz-ghidra and jsdec on various x86 binaries just to see if they crashes?

wargio · 2025-02-20T15:43:13Z

Yes, you can refactor that file.

tushar3q34 · 2025-02-20T15:46:02Z

Could you do us a favour and try to use rz-ghidra and jsdec on various x86 binaries just to see if they crashes?

Yes, I will check.

notxvilka · 2025-02-22T05:45:28Z

@tushar3q34 please rebase.

Rot127

I only reviewed the first two commits, but so far fantastic job!

I have mostly nitpicks (they should be really quick to resolve, so don't be scared of the number :) ).

I can't see who made the original edits, so apologies if I requested changes to code not from you. Please be so kind and fix them anyways, since it improves the overall stability.

Please also compile Rizin with ASAN enabled (see BUILDING.md) and run the asm tests with the LEAK sanitizer enabled.
Also the x86 esil tests (db/analysis/x86*).

You can ignore leaks which are not part of the x86 code (you can also suppress them: https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer#suppressions).

Some general comments:

Take care of marking all pointers as const if you know they should not change (be it during initialization or for function parameters).
Use size_t instead of int if you know the value should never be negative.
Initialize indices (i) of loops always within the loop. If they are not used outside of it.

Rot127 · 2025-02-23T11:08:19Z

subprojects/zydis.wrap

+directory = zydis
+patch-directory = zydis
+depth = 1
+revision = master


Ping on this one :)
Did we agreed on something and I just forgot it?

Rot127 · 2025-02-23T11:09:45Z