Skip to content

decode: Pin the rv_insn_t/opcode_fuse_t shared prefix - #766

Open
alanhc wants to merge 1 commit into
sysprog21:masterfrom
alanhc:rvv-static-assert
Open

alanhc wants to merge 1 commit into
sysprog21:masterfrom
alanhc:rvv-static-assert

Conversation

@alanhc

@alanhc alanhc commented Sep 10, 2026

Copy link
Copy Markdown

Addresses the third item of #735.

try_fuse_sequence() populates the fuse array with
memcpy(ir->fuse + j, next_ir, sizeof(opcode_fuse_t)), reinterpreting the
leading bytes of rv_insn_t as an opcode_fuse_t. That is only valid while
both structures agree on the offset of every field the fused handlers read
back, but the existing _Static_asserts in decode.h pin the layout of
opcode_fuse_t alone. Nothing catches a change on the rv_insn_t side.

The issue mentions opcode, but since the memcpy copies the whole 12-byte
prefix, the contract covers imm, rd, rs1, rs2 and opcode, so this
pins all five plus a size relation.

Note that rv_insn_t gains members under EXT_C, EXT_V and EXT_F, so
the prefix layout is configuration-dependent; the asserts were checked with
EXT_V both enabled and disabled.

Verified by inserting a layout-shifting member ahead of opcode, which the
new assert rejects at compile time. Note that a uint8_t inserted there is
absorbed by existing alignment padding and does not shift opcode, so the
negative test needs a wider type.

make check passes; clang-format 20.1.7 clean.


Summary by cubic

Pins the shared prefix layout between rv_insn_t and opcode_fuse_t so the memcpy in try_fuse_sequence() can't silently corrupt fused sequences when the structs drift. Addresses the third item of #735.

  • Adds compile-time asserts that the offsets of imm, rd, rs1, rs2, and opcode match, plus a size check.

Written for commit 9b6058c. Summary will update on new commits.

Review in cubic

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No issues found across 1 file

Re-trigger cubic

try_fuse_sequence() reinterprets the leading bytes of rv_insn_t as an
opcode_fuse_t via memcpy, which is only valid while both structures agree
on the offset of every field the fused handlers read back. The existing
asserts cover opcode_fuse_t alone, so a change to rv_insn_t could break
the contract silently.

Add cross-structure offset asserts for the whole shared prefix.

@jserv jserv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Avoid decode tag in the subject of both pull request and git commits.

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.

2 participants