Refactoring the RISCV architecture to Auto-Sync on LLVM #2756
Conversation
github-actions
bot
added
RISCV
arch/RISCV/RISCVInstPrinter.c
Outdated
| if (SysReg && SysReg->haveRequiredFeatures(STI.getFeatureBits())) | ||
| SStream_concat0(markup(O, Markup_Register), SysReg->Name); | ||
| else | ||
| SStream_concat0(markup(O, Markup_Register), formatImm(Imm)); |
There was a problem hiding this comment.
Yeah, better get rid of these formatImm calls. Otherwise we have two places to take care of formatting and everything gets less consistent.
|
cc @hainest You are probably aware of this PR, just wanted to ping you as well. Because you mentioned in the Zydis discussion you use Capstone for RISCV as well. |
suite/cstest/include/test_mapping.h
Outdated
| { .str = "riscv32", .val = CS_MODE_RISCV32 }, | ||
| { .str = "riscv64", .val = CS_MODE_RISCV64 }, |
There was a problem hiding this comment.
In response to above. Please stick with the Capstone flag naming convention.
Is RSICV32 is anything else than just 32bit?
There was a problem hiding this comment.
You have a point, but I don't know, RISCV[32|64] and RISCVC were legacy flags that were already in the old plugin, so I kept them.
Removing them would probably not affect anything but just be a hassle, it could also be the case that the generated code references them from lots of places and changing the generated code is always a hassle.
There was a problem hiding this comment.
generated code references them
Which one? The test files from the MCUpdater? Those ones can be replaced as described here #2756 (comment)
There was a problem hiding this comment.
@Rot127 sorry for the late reply, yes, only the yaml MC tests reference those values, plus a few references in handwritten code in the cstool.c and so on, so it's doable to replace them, will do.
Although this might break APIs, because I see some Python usage from my editor search, until now I have avoided deleting enum members from includes/riscv.h, that's why we still have CS_MODE_RISCV_C and CS_MODE_RISCVC.
Rot127
left a comment
Rot127
left a comment
There was a problem hiding this comment.
Please check out the helper functions in Mapping.h. They should always be used for these tasks. Because they do bounds checking and such. Accessing the struct members directly (e.g. like details->groups[details->groups_count]) should be the absolute exception, not the rule.
You can also see how the helpers are used in other architectures and just do it the same way. LoongArch is a good example, because it is relatively simple.
AArch64 has many very complex operands. You can check its code if you need examples for these cases.
|
The conflicts are coming from the clang-format formatting we did recently btw. So nothing to preserve there. |
moste00
force-pushed
the
refactor_riscv_autosync
branch
from
12882dc to
1d59145
Compare
moste00
force-pushed
the
refactor_riscv_autosync
branch
2 times, most recently
from
0e5a2a8 to
f99aa52
Compare
…ol and the test yaml schema accordingly
…ift amounts, as it's a bug wtih LLVM upstream
…le in DIET configuration
…not always calls, formatting
moste00
force-pushed
the
refactor_riscv_autosync
branch
from
96b38b1 to
6b3d2f0
Compare
Rot127
left a comment
Rot127
left a comment
There was a problem hiding this comment.
@moste00 One exceptionally important thing still missing is the documentation in the v6 release guide. It is the first thing we will send people to for questions.
…t broke due to fixing it, add command line switches to turn on and off ZB* and ZBK* extensions
…nd line arguments for cpu options
5 participants
Your checklist for this pull request
Detailed description
This PR is attempting to refactor the RISCV architecture to be updatable from the Auto-Sync tool, such that it automatically follows RISCV-in-LLVM additions and fixes. Following the refactoring guide
depends-on: capstone-engine/llvm-capstone#83
Test plan
...
Closing issues
...