RISCV: compressed insn treats the uncompressed version as an alias - #2959
RISCV: compressed insn treats the uncompressed version as an alias#2959slate5 wants to merge 2 commits into
Conversation
646c2ac to
bc9042a
Compare
Rot127
left a comment
Rot127
left a comment
There was a problem hiding this comment.
Please add some tests of course.
Otherwise I think we can start with that one.
|
Will do, but let's see what @moste00 says |
|
Hello @Rot127 @slate5, thanks for notifying me of this. So a couple of problems that I see: 1- We're still keeping the "(un)compressing is the same thing as aliasing" paradigm, and the flags of 2- The logic assigns a normal CS instruction ID (the ID of the uncompressed instruction) to the alias ID, but are the 2 ID spaces mutually exclusive? What if they intersect in some IDs ? 3- While I didn't run this yet as I'm not on my laptop now, the details logic will probably not solve my problem because when it prints the details for All in all: I think the main problem here is that is that we're just keeping most of the logic as-is and patching 1 problem ad-hoc instead of refactoring enough, but if both of you think we're not ready for the somewhat complicated approach in the other PR, and if (3) is addressed without breaking any existing tests, I'm okay with this PR. Personally, I would like the other approach I described in the other PR, it just exhaustively lists all possibilities and lays out the exact thing to do for each. I might be biased though and if you think it's unclear or too verbose a description then so will capstone users. Maybe I just mis-explained it ? |
|
Hi @moste00,
Yes, that is what I also addressed. Maybe it would be better to have a separate cs_insn element for this specifically (eg, uncompressed_id). On the other hand, if we start complicating with noalias, nocompressed, keep this or that, we have to expect that the user is an expert in riscv. I will be the first to forget this "mess" with compressed insn, and its "aliases" in a week. From the perspective of an asm writer, it makes sense like this because in your source file you put I'm making this long... I lean towards fewer features (flags) that tune options without the explicit need of end users. It's clutter, and i'm not sure if there is demand for it. To me, this differentiation between +noaliascompressed and +noalias (as it is now) is irrelevant. Either i want aliases or not, i don't care if insn is compressed. As you said in the other PR, "Or you can do uncompression alone but stop at just the alias mapping, using +noaliascompressed" makes more sense to redefine the purpose of it (it would stop the logic of this PR, no uncompressed aliases, e.g., c.addi doesn't have an alias ID).
They are in the same enum riscv_insn, separated by RISCV_INS_ENDING and RISCV_INS_ALIAS_BEGIN, if that is your question
But if you want real details, specify I don't think this PR solution is great, but i'm trying to stick to occam's razor :) And yes, I think if you explain to me a bit simpler in terms of what the end goal of your approach is, and what problem it solves (or what benefit it gives), it would help me a lot |
Hey, sorry for saying that I wil test this at the time then delaying it so much. I was busy. So what I mean is: I want to be able to give the In some sense, this is the opposite POV to what you're saying. You're thinking of However, my POV is that the "original" 4-byte instructions are the "real" ones, and the C style instructions are just aliases for shorthand operations. It's kinda how in Bash you define alias commands to achieve the same thing as a longer command but with shorter text, both truly fictitious aliases (like All in all, your PR doesn't break the test suite either way, even if you change |
| MI->flat_insn->detail->riscv.op_count = 0; | ||
| // re-disassemble again with no printing in order to obtain the full details | ||
| // including the whole operands array | ||
| printInstruction(MI, MI->address, O); |
There was a problem hiding this comment.
usse McInstr here instead of MI, so that if the instruction is uncompressed the details would be those of the uncompressed equivalent.
| MI->flat_insn->detail->riscv.op_count = 0; | ||
| // re-disassemble again with no printing in order to obtain the full details | ||
| // including the whole operands array | ||
| printInstruction(MI, MI->address, O); |
|
I left some comments on the code, if you apply them and no tests are broken, then LGTM. (I already applied them on my local branch and they didn't break any tests, but check again just in case.) |
|
Now I am sorry for the delay, hehe
Ok, so you want the opposite behaviour for uncompressed ones with Your explanations about aliases and Bash are okay with me. I cannot see it fully like that, but ofc c. can be seen as a shorthand alias for the real insn. Thinking about those writebacks reaffirms this perspective ( This is set then, but i would like to hear your opinion on @Rot127's last msg in #2923. I would like to replace noaliascompressed with keepcompressed, and the role would be to just ignore this uncompressed complexity. Is that ok? Also, how much complexity would be added to Rizin if we don't use alias_id for this uncompressed stuff, but instead add a new one, uncompressed_id? I'm just gonna swap those two (MI and McInstr) as u proposed, but to get this opposite functionality completely (even with +noalias), we have to do more changes. |
bc9042a to
66b162f
Compare
|
@slate5 Thank you! Yes, I read Rot's comment on the other PR and I like it, I was planning to implement it. If you want to implement it then of course be my welcome guest, although don't feel pressured to, it was originally my idea to do the refactoring so I don't want to just throw work on you! That said, if you want to implement it, of course I have no objections. If you don't feel like it then it's totally OK and I can resume the unfinished other PR. While you're at it, you can also seperate the details from the text, i.e. make it so the text is completely independent from details, such that I can request So in short: yes, definitely feel free to implement Rot's modification, but don't feel pressured because it might be a hassle and I don't want to assign boring work to you. (in particular, tests might break and require passing extra flags for it to work.)
No complexity either way, Rizin doesn't use any of those IDs to drive any interesting analysis as far as I'm aware, but adding |
|
@moste00, you're welcome :) I'm going through this implementation, but one last thing that we misunderstood each other. For user POV simplicity, I would treat c. text and details always as an alias by default (
Yes, separating +noalias and What do you think? |
|
@slate5 Could you change the |
|
Oh yea, i like that idea @Rot127 |
If I understand you correctly, did you mean to say that without the user giving any flags, It's also how I unerstand Rot's diagram from the other discussion.
Yup, no surprises there.
I think this output is wrong, isn't it ? The
I think this is correct, the absence of
Exactly. And the uncompressed flag here has two possiblities (1) Null, empty, whatever, the instruction was never uncompressed, so it's invalid (2) Optionally, you can make it mean "uncompresses to" in this case, so in fact it has two possible meanings, on compressed instructions it means "this instruction can uncompress to ...., but didn't", on uncompressed instruction it means "this instruction was uncompressed from ....", so it's always pointing in the other direction. But (2) can be confusing to users, so I prefer (1) , it always means "this uncompressed instruction originally come from this compressed ID", and it's undefined on compressed instructions. I think that's it. Don't forget to seperate the details flags from the text flags please, it's very important. |
|
@slate5 Hey, any progress ? The lifting PR depends on this to have a green CI. |
|
@moste00, I'm doing this in my free time, and I'm also traveling these days (so no extra monitor, just laptop. It's like programming on a Tamagotchi) I made c.addi (or any instruction that is not an alias but has an uncompressed counterpart) to act exactly as a full instruction would (i.e., being unfazed by |
|
@slate5 Sounds good, thanks! please push the code. Also please make a note of which tests break and need fixing, it will appear in the diff but you could make things more obvious by just noticing their names and files, if possible and they're not too many. I always review all the broken tests to make sure we didn't break too much.
By "unfazed" do you mean that the instruction doesn't change whether those flags are present or not ? If so then 1- In the case +noalias that makes sense, since there is no alias to begin with, so whether or not +noalias is present 2- But with The general rule is that |
|
@moste00, to avoid these kinds of theoretical discrepancies between which case should do what, I gave the full cstool output of all possible cases. The output above is unambiguous. |
|
@slate5 In the command where you do The c_addi command is printed as which is correct, the full operand set as expected. But in the command that does the c_addi which is the compressed way of spelling the details (READ | WRITE instead of two operands). Both have the |
|
Can u just tell me exactly what the output of P.S.
The misunderstanding between us comes from none of these being "correct", but a matter of preference... |
|
@moste00 I reread your answer, and it is clear that we did perceive keepcompressed differently. And this is how I see it: If that is the case, we could just keep the old flag CS_OPT_SYNTAX_NO_ALIAS_TEXT_COMPRESSED (or something like that) that explicitly states that it affects text only. In my head, keepcompressed was supposed to honor ISA logic of what is an alias and what is not (meaning, it cancels out this opposite logic that we created). |
|
@slate5 Yes, sorry if I didn't do a good job of making this clearer much earlier, I might have buried it during the very long discussion. But yes, that's essentially all what this PR is about: text and details is and should be different. One compelling reason for this is Rizin's way of consuming Capstone: instruction text is a user-facing thing printed as a result of disassembly commands, there might even be ways for Rizin's user to tweak it (Not very sure about doing that right now, but it's plausible and could be added in the future). Details, however, are internal structs that the lifting and the analysis code depends on. If we couple text to details, this means a slight tweak to the text (e.g. changing how the
I'm opposed to the naming of this flag just because it conflates being compressed and being an alias, I would prefer if you found a better naming, possibly To avoid more misunderstanding, here's the entire algorithm I'm thinking of: I think this does all what I want it to do:
Notice that I'm writing If you think there is something unintuitive here, what's an example case I missed ? Again, the only thing I care about here is the seperation between text and details, the exact logic governing the text printing is the one that @Rot127 described in his comment on the other issue, but I have no problem if you don't think it's intuitive and want to change it, the only thing blocking me in Rizin is that the details MUST be seperate from the text printing, right now they aren't. |
Please keep in mind that for Capstone development we do not focus on what is best for Rizin, but what makes most sense for all Capstone developers. Rizin is just one use case, although a pretty comprehensive one.
Would you mind drawing it as a flow chart, showing which flag does what? |
Text and details are already separated as u can see in my previous cstool output (the complete one with all combinations of real/alias text/details). The only misunderstanding is +keepcompressed, in your comment, it seems that you are contradicting yourself:
When I mentioned CS_OPT_SYNTAX_KEEP_COMPRESSED_TEXT (which we used before and was replaced by keepcompressed), it wasn't meant to start a discussion about naming, but about "does +keepcompressed affect only text or both". Nevertheless, I would interpret this paragraph as if you want it to affect only the text.
And now, this I would interpret as if you want +keepcompressed to affect both text and details. Do you know what you want with the +keepcompressed flag, or should I implement it as I said before (make c. insns act according to ISA when keepcompressed is present)? |
|
To make it very simple, c.mv is an example and in ISA that is a real instruction, while mv is a pseudo-instruction. This is what we do for real/real: Now the question is what happens when we provide +keepcompressed: Option 1 Option 2 (same as ISA) |
Yup, that's my intention. Naming is a side issue.
Why :'D ? The entire algorithm is only for text, there is a separate pass that will print details after it's done. It's really two runs of So tldr; No, I only meant text, this entire paragraph you're quoting is only ever talking about text, never details. When I say "printed as", it's text, not the details or the operands array.
By "ISA" do you mean some GNU util or
Your point is taken, my previous algorithm would choose option 1, since it always chooses the details of the full non-alias non-compressed 4-byte instruction as the "canonical" details when But I agree that The only thing I would say is: let's bite the bullet. Option 1.
It's okay to do a non-perfect thing now and revisit later when someone gets annoyed. |
Ohhhh ok, I missed that. I wasn't thorough enough in reading the pseudo-implementation because I just wanted a simple answer (it should affect only text or not) XD
If we are talking about the same ISA document (https://github.com/riscv/riscv-isa-manual), then there is a definition of details, aka operands. But, as much as I can see, the confusion between us comes from POV. I guess you take text (mnemonics + operand string) as part of the ISA, but not the details because that is C struct metadata. I prefer them to be linked together to minimize confusion due to capstone-specific logic/convention.
Personally, I find it very confusing to have, even after using +keepcompressed, a mnemonic I have both implementations ready, it's just a matter of preference now. Give me a final decision, and then let's wrap this up soon :) |
Sorry, that is not possible. It is unlikely someone will go down that path in this detail again. Especially before the Beta release. After the Beta is released, I don't want big API changes like these for RISCV anymore. Could you folks make a table what outcome the different flags have (like the one already shown in https://github.com/capstone-engine/capstone/blob/next/docs/cs_v6_release_guide.md under the RISV section)? And one more alternative: @slate5 Do you think it is possible to let the user choose freely what asm text they want and what details? So people can do things like: --asm_text:real --details:compressed
--asm_text:alias --details:uncompressed
--asm_text:compressed --details:alias
# etc...I had something like that planned before as well. But never found time to implement it yet. |
From a programming perspective, nothing stops us from implementing this. I would gladly switch to that approach, it's more explicit from the users' POV. If @moste00 likes it too, then we should decide upon flag names (both API's and cstool's). And please just confirm to me that the default (without explicit flags) should still show uncompressed text and details for compressed insns. As soon as I have confirmation, I'll change the current implementation, and that table in docs will come last when we fully agree how this all should look like... |
|
@Rot127 fair point, maybe that's a thorny enough issue that we need to completely decide on behaviour and freeze it here. @slate5 Here's the text table I'm imagining, the default mode when nothing is given is
It's a bit verbose because I was thinking about Capstone's user, but here's the tldr in non-table (2) compressed mode doesn't make real instruction compresss, real instruction stay is even if there is a compressed equivalent, but compressed instructions print as themselves in that mode, again never as aliases (3) In alias mode, everything tries to be an alias, 4-byte instruction are printed as aliases if possible (else as themselves), and 2-byte instruction are first attempted to be printed as aliases, if no aliases exist (the vast majority of them) they're then uncompressed to 4-byte form and printed as aliases, if still no aliases exist, then they're left as 4-byte uncompressed forms (or themselves if no such uncompression exist, which Claude tells me is extremly rare). A similar table can be done for details but let's approve this one first or discuss it. Again, this table doesn't imply ANYTHING about details, details are different configuration knob entirely. They have a table similar to this in my mind. What do you all think ? |
|
@slate5 @Rot127 Here's some diagrams courtesy of Claude to dilute the wall of text 1- Real mode, everything tries to be real and no alises are ever printed 
2- Compressed mode, everything stays as itself, no aliases are ever printed 
3- Alias mode, the default, everything tries to be an alias, and the compressed instruction be both its own aliases and the its expanded form aliases |
As I understand @moste00 summary above (@moste00 or did I miss something), an implementation which lets the user choose freely which asm text and details are emitted is covering that and any other case. Most importantly though: Thanks a lot for spending all your free time on it. I really appreciate that! RISCV is a very important architecture, and will be in the future as it looks like.
Pick some you find intuitive and which are not too verbose.
The default behavior should be the one of |
|
@moste00, I assume these flags like asm_real_text should be replicated (more-or-less) for details as well (ie, asm_real_detail)? Btw, thanks for trying to simplify your idea, it helps
You are welcome, @Rot127. It's in all our interest to make this good, Capstone is an important project
llvm-mc does the opposite of what we aim to do, alias and real: Last thing, would it make more sense to make asm_real_* show the real instruction, including c. (showing c.addi), asm_uncompressed_* show what we do with real now (everything is a full instruction if it can be), and asm_alias_* stay the same? |
I wasn't aware it allows to print the non-alias instruction 😮 |
Pretty much yeah, although I struggle to imagine why would someone want
Hmmm, what you posted seems pretty-aligned with what we do in the default case, which is
This looks pretty in-line with the 3rd diagram I posted above, which should be the one governing the behaviour in case user didn't give any flags. Just for curiosity, I told GPT 5.5 to actually look at the source of 
This is almost the same sequence of decisions we do in
That's valid, my bias is to always regard the compressed instruction as a form of aliases, but you're right, they're actually real instructions, So you can go ahead with this, no problem. |
Traced the snippet in a real editor just to verify GPT's explanations. |
I don't see why this would be used either, but if we are already implementing max flexibility, let's be flexible... :)
Yes, the default case (yes-aliases) is the same. But the difference is when you request no-aliases (aka, NoAliases, or real, as we call it), llvm shows c.addi (literally), and we show uncompressed. Just look at the code, it literally does the opposite of Capstone (this PR) when users request "real", aka no-alias text.
I'm glad we are exactly aligned now. I'll try to reimplement this in the following days. |
|
@slate5 Hey, any updates ? If you have any initial implementation you can push and I can refine it further by the way. |
|
@slate5 Hey!, The lifting PR is blocked on this PR, so we genuinely need it done. I'm planning to implement it all tomrrow, if you have something you want to push please let me know. |
|
Hi @moste00, I didn't have time to do this. I was planning to do it this weekend, but if you cannot wait, feel free to finish it today |
No worries, will do it today. |
3 participants
Your checklist for this pull request
Detailed description
This draft is connected to #2923.
It's a very simple change. All compressed instructions that have an uncompressed counterpart use that instruction as an alias. A better solution would be to include a new
uncompressed_idinstead of an alias, but that would require more modifications (cs_insn)...Test plan
...
Closing issues
...