-
-
Notifications
You must be signed in to change notification settings - Fork 489
Updating the project RISC-V asm and analysis capabilities #5482
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
base: dev
Are you sure you want to change the base?
Conversation
librz/arch/meson.build
Outdated
| ] | ||
|
|
||
| if capstone_dep.version() == 'next' or capstone_dep.version().split('.')[0].to_int() > 4 | ||
| if capstone_dep.version() == 'next' or capstone_dep.version() == 'capstone-riscv-updated' or capstone_dep.version().split('.')[0].to_int() > 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be reverted (leave this comment here till the branch is merged)
7f4c55c to
1e5730c
Compare
… tests passing but analysis still incomplete
… map to Rizin Analysis op type
…nd added vector registers to the register profile
b2c38f8 to
4e63a52
Compare
| case RISCV_INS_FADD_S: | ||
| case RISCV_INS_FADD_D: | ||
| case RISCV_INS_TH_ADDSL: | ||
| op->type = RZ_ANALYSIS_OP_TYPE_ADD; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For arithmetic functions you need to provide all necessary op-> fields and operations, so that analysis engine knows enough to work even without ESIL. Take a look how it's done in analysis_mips_cs.c. Setting op->src and op->dst where appropriate, as well as op->ptr and op->refptr, op->jump, op->stackop, op->stackptr, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, op->stack* are set sepearetly after the switch by set_stack_effect though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@moste00 don't forget about this please
notxvilka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't forget to remove the old GNU plugin. I recommend squashing it into two commits:
- one is to remove old RISC-V GNU plugins
- second is to add new plugins
Rot127
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just recognized that this whole effort means the RISCV features in Capstone are not well enough defined I think. Rizin is the first tool using the new CS plugin and implements a work around for the features? This is not a good sign.
@moste00 I didn't recognized this when we talked in Mattermost, but I think we should at least consider using alternatives.
c5ee3d6 to
fe6d6e4
Compare
Your checklist for this pull request
RZ_APIfunction and struct this PR changes.RZ_API).Detailed description
As per #5275, this PR builds on the refactored Capstone RISCV plugin in capstone-engine/capstone#2756 and propagates the change to Rizin, adding lots of Rizin-specific capbilities.
(1- AI Disclosure) I use Gemini CLI in the context of the codebase, mostly or almost exclusively for read-only type of usage: as an intelligent search engine to navigate and query the codebase, I almost always reject edit requests by it. I also use Claude Web for general questions, routine text editing, and other context-free tasks.
(2- Diff Density) I expect most of the changes to be concentrated heavily in the
analysis-riscv-cs.cfile, because this file wasn't updated in 5 years and is in heavy need for updates to take advantage of the new Capstone module, however the first state of this PR mostly focuses on minimal changes to get it to compile and pass tests, most of the diff is instead build file glue and tests modification.(3- Temp Changes) Some changes are temporary and won't survive the merging of this PR, for example a legacy GNU plugin is kept as-is to reduce diff noise but just renamed to
riscv.gnuso it's not the default (and the RISCV plugin is renamed toriscvfromriscv.cs), and some build system logic is added so that we can build with a local development version of Capstone instead of a published release.Test plan
With the exception of relevant tests under
db/esil, all old tests are relevant and will be kept and made to pass. New tests must be added from a variety of source to ensure the new capabilities. This section is evolving as more tests are added.NOTE, to any reviewer running locally: Change the local path in
subprojects/capstone-capstone-riscv-updated.wrapto a Capstone archive containing the RISCV updates.Closing issues
** Depedents **
Depends-on: rizinorg/rizin-testbins#223
Depends-on: #5666
Depends-on: capstone-engine/capstone#2756