Skip to content

Fix sysreg registers in RISC-V and modify the header patcher to not crash if a header doesn't include all tags of a generated .inc file#2894

Merged
Rot127 merged 2 commits intocapstone-engine:nextfrom
moste00:feature/riscv_sysregs_in_public_api
Apr 15, 2026
Merged

Fix sysreg registers in RISC-V and modify the header patcher to not crash if a header doesn't include all tags of a generated .inc file#2894
Rot127 merged 2 commits intocapstone-engine:nextfrom
moste00:feature/riscv_sysregs_in_public_api

Conversation

@moste00
Copy link
Copy Markdown
Contributor

@moste00 moste00 commented Apr 13, 2026

Your checklist for this pull request

  • I've documented or updated the documentation of every API function and struct this PR changes.
  • I've added tests that prove my fix is effective or that my feature works (if possible)

Detailed description

As per #2893, the public header riscv.h doesn't define an enum for system registers. Therefore, it was impossible to programmatically switch on the value of a system register without violating API safety (by e.g. simply duplicating the raw numbers from capstone sources).

This PR fixes this by adding enum constants in riscv.h to represent system registers.

In order to do this, the archiecture header patch tool had to be modified to not crash if a ARCH*.inc file defined several #ifdef blocks but the including header ARCH.h in capstone didn't include some of them. Previously, the ARCH.h file had to include every block of #ifdef content somewhere in it, and riscv.h doesn't need this.

Test plan

...

Closing issues

closes #2893.

@moste00 moste00 force-pushed the feature/riscv_sysregs_in_public_api branch from 800c8de to fa40ace Compare April 13, 2026 20:32
…rash if a header doesn't include all tags of a generated .inc file
@moste00 moste00 force-pushed the feature/riscv_sysregs_in_public_api branch from fa40ace to 3f519e7 Compare April 13, 2026 20:44
Copy link
Copy Markdown
Collaborator

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch with the script.

Copy link
Copy Markdown
Collaborator

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, before I forget, please add a test.

Never mind. Already there. But see below

@Rot127
Copy link
Copy Markdown
Collaborator

Rot127 commented Apr 14, 2026

Also update the Python bindings.

@github-actions github-actions Bot added the Python Bindings label Apr 14, 2026
@moste00
Copy link
Copy Markdown
Contributor Author

moste00 commented Apr 14, 2026

@Rot127 I added tests anyway because the all the existing tests on sysregs were just asserting on their string representation in yaml, which obviously didn't catch the API problem. The unit tests I added switch on CSR registers and fail if the switch didn't match, so it will fail to compile the C test and fail at runtime for the Python test if the API changed or removed the sysreg names.

There was no need to update the Python bindings because the sysregs were already defined as integer constants (the tables mapping back and fourth from the string representation are needed in Python too).

Copy link
Copy Markdown
Collaborator

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the extra tests. It warms my pedantic heart.

@Rot127 Rot127 merged commit 1084d36 into capstone-engine:next Apr 15, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Auto-Sync-files Auto-Sync Python Bindings RISCV Arch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

riscv64 system registers missing from riscv_reg

2 participants