Revamp flags to allow fine grained control over aliasing - #3008
Conversation
Rot127
left a comment
There was a problem hiding this comment.
I might have overlooked it because Github lags. But I don't see any tests with other combinations than real text.
Please add all the cases we discussed in the threads before here.
Especially the edge cases. And document them appropriately please.
|
@slate5 |
…d details flag, other renaming
Looks buggy, the PR has 3500 lines added and 3400 or so deleted, so less than 10K changed overall. |
Co-authored-by: Rot127 <45763064+Rot127@users.noreply.github.com>
1656592 to
2f401d1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 93 out of 101 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
arch/RISCV/RISCVInstPrinter.c:97
- isUncompressedRealDetail() currently treats any detail mode other than REAL or ALIAS as "uncompressed real". This makes the default "CS_OPT_DETAIL" (only CS_OPT_ON set) look like uncompressed-real details, which is likely unintended and also breaks text/detail mode matching.
arch/RISCV/RISCVInstPrinter.c:102 - isAliasDetail() currently requires CS_OPT_DETAIL_ALIAS to be set, but the historical/default behavior for "CS_OPT_DETAIL" is alias-derived details unless CS_OPT_DETAIL_REAL (or now UNCOMPRESSED_REAL) is requested. Treating the default as non-alias makes textAndDetailModesMatch false in the common default case and forces a second decode pass.
|
@moste00 Also check the suppressed comments please. |
Done, it's really only complaining about a typo and a check for the default-alias-syntax logic not being general enough (it catches only the explicit flag, not the absence of any flag). There is a complaint about the uncompressed real details being the default for details that I don't think is relevant, we agreed before that it should be the default when no details flag is mentioned. I prettified the tables for both syntax and details and linked them as both images and as tables. I think this long PR is finally ready to land? |
Rot127
left a comment
There was a problem hiding this comment.
Those nitpicks. Otherwise looks good to me
| [](html/details_flags_table.html) | ||
|
|
||
| * Notice that despite the apparent complexity of the rules above, there are really only 4 distinct outcomes: | ||
| - An instruction is treated normally (as itself) |
There was a problem hiding this comment.
| - An instruction is treated normally (as itself) | |
| - The default case: An instruction is printed as by LLVM. If it has an alias, then the alias is printed. If it doesn't, then it just prints the instruction (no uncompression happens). |
There was a problem hiding this comment.
This point is reverting to describing a sequence again, but this section is "outcomes" only, it really describes all possible things that could happen to an instruction regardless of decision logic needed to get there, so this point should still be "treated exactly as decoded", that's the atomic outcome here, the sequencing of checks is done by the mode.
240722a to
3e23362
Compare
3e23362 to
82d38ca
Compare
|
Done @Rot127 , last batch of changes: 1- Implemented your suggested wording of the docs, except your description of the first outcome in the "4 distinct outcomes" section, it was describing a whole sequence (printing the alias, then printing the original), whereas the section is meant to describing only the actual distinct outcomes that can happen without any sequencing or decision (the modes do the sequencing). 2- Made the defaut details when provided with So This is the same behaviour as for other archs, meaning I actually deleted code to make this happen (it was already happening by accident) Let's merge when green. |
Your checklist for this pull request
Detailed description
Implementing the extensive discussions here #2923 and here #2959.
RISC-V: Revamp flags to allow fine grained control over aliasing (#3008)
This PR adds more flags to control the asm text and detail output.
RISC-V has instructions which have alias or compressed representations.
The new flags now allow to choose which asm text and which details Capstone should decode.
Please see the documentation for details.
https://github.com/capstone-engine/capstone/blob/next/docs/cs_v6_release_guide.md
Test plan
Added
Closing issues
...