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
slate5
force-pushed
the
fix/uncompressed-alias
branch
from
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. |
slate5
force-pushed
the
fix/uncompressed-alias
branch
from
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) |
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
...