Skip to content
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

RISCV: Add call, int and branch_relative instruction groups #2007

Merged
merged 3 commits into from
Jun 10, 2023

Conversation

peace-maker
Copy link
Contributor

@peace-maker peace-maker commented May 1, 2023

Add call, int and branch_relative instruction groups to riscv mappings.

I'm not sure if there are tests to be added somewhere as I'm not familiar with the codebase.

Add call, ret, int and branch_relative instruction groups to riscv
mappings.
@peace-maker peace-maker changed the title RISCV: Add more instruction groups RISCV: Add call, int and branch_relative instruction groups May 1, 2023
@Rot127
Copy link
Collaborator

Rot127 commented May 2, 2023

Are those groups all present in LLVM as well? call and branch relative are, but I am not sure about the others.
I am asking because once #1949 is merged and RISCV will be updated, the groups which are not part of LLVM are gone again.

Of cause we could implement something to add additional groups, but his needs a different approach.

@peace-maker
Copy link
Contributor Author

I have no clue. The only other one is int, which you say isn't present for arm, so I guess it isn't for riscv as well. So the way forward would be to add those groups to llvm? It's an unfortunate regression if they're dropped :(

@Rot127
Copy link
Collaborator

Rot127 commented May 2, 2023

It's an unfortunate regression if they're dropped

I rushed my comment. Ignore it. RISCV will likely not be update in the next two months so it another implementation is not necessary until then.

So the way forward would be to add those groups to llvm?

This would be one way. But this will take probably very long until it is merged (if it is accepted). The way to implemented it in Capstone is to add a function to the Mapper.c which is called after the detail was added. And add the additional groups to the instruction.
But as said, you can ignore this since it only becomes a necessity when RISCV is refactored to use auto-sync.

@XVilka
Copy link
Contributor

XVilka commented May 4, 2023

@peace-maker if you add tests for these cases, you will guarantee that whoever updates RISC-V with auto-sync later will not break them, since will need to address these somehow.

@Rot127
Copy link
Collaborator

Rot127 commented May 4, 2023

@peace-maker You can add test cases for these like this: https://github.com/capstone-engine/capstone/blob/79e78cffba6b400fa0545bf78b42b08d9f29b695/tests/cs_details/issue.cs
And test them with suite/cstest/build/cstest -f ../../tests/cs_detail/issue.cs.

Note that the file needs to have issue in the name and a test case must start with a !# issue currently.

Check for 32 and 64 bit mode as well as compressed instructions.
@peace-maker
Copy link
Contributor Author

@Rot127 I couldn't find the file you've linked to in the next branch of this repository, so I've added regression tests to the suite/cstest/issues.cs file instead.

Thank you both for your help!

@XVilka
Copy link
Contributor

XVilka commented Jun 9, 2023

@aquynh @kabeor these changes seem to be good to have in 5.0 as well

@kabeor
Copy link
Member

kabeor commented Jun 10, 2023

Thanks again. Merged.

@kabeor kabeor merged commit e9fd6f4 into capstone-engine:next Jun 10, 2023
@peace-maker peace-maker deleted the riscv_insn_groups branch June 11, 2023 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants