Skip to content

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

Merged
kabeor merged 3 commits intocapstone-engine:nextfrom
peace-maker:riscv_insn_groups
Jun 10, 2023
Merged

RISCV: Add call, int and branch_relative instruction groups#2007
kabeor merged 3 commits intocapstone-engine:nextfrom
peace-maker:riscv_insn_groups

Conversation

@peace-maker
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

XVilka commented Jun 9, 2023

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

@kabeor
Copy link
Copy Markdown
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