Skip to content

fix(riscv): keep no-C control flow on the interpreter path - #218

Closed
carlosqwqqwq wants to merge 2 commits into
HexRaysSA:masterfrom
carlosqwqqwq:fix-riscv-lifter-noc-ialign
Closed

fix(riscv): keep no-C control flow on the interpreter path#218
carlosqwqqwq wants to merge 2 commits into
HexRaysSA:masterfrom
carlosqwqqwq:fix-riscv-lifter-noc-ialign

Conversation

@carlosqwqqwq

Copy link
Copy Markdown
Contributor

fix(riscv): keep no-C control flow on the interpreter path

Closes #217

Summary

Without C, IALIGN=32: JAL/branch targets must be 4-byte aligned and JALR targets are runtime-computed, so none of the three can be emitted as a translated branch without an alignment check. The lifter now returns Unsupported for these cases so the interpreter delivers the architectural instruction-address-misaligned trap.

Validation

  • Regression covers misaligned JAL/branch targets and JALR under a no-C profile (must not lift), and the same words under C (must lift).
  • cargo test --lib: 8214 passed, 0 failed.

Notes

Translated-path IALIGN gate, distinct from the no-C JALR handling in the direct interpreter.

cpu.rs exceeded the AGENTS.md hard split triggers (2000 lines / 150 kB). Move the vector (RVV) element access and data-path execution group, including set_vtype, into cpu/vector.rs with no behavior change.
Without C, IALIGN=32: JAL/branch targets must be 4-byte aligned and JALR targets are runtime-computed, so none of the three can be emitted as a translated branch without an alignment check. They now fall back to the interpreter, which delivers the architectural instruction-address-misaligned trap.

19h commented Aug 11, 2026

Copy link
Copy Markdown
Member

I reviewed the focused commit bec7348f against issue #217. The fail-closed behavior is sound: a statically misaligned no-C JAL/branch target falls back, and all no-C JALR instructions fall back because the target alignment is runtime-dependent; legal C-enabled controls remain liftable.

Two changes are needed before this is merge-ready:

  1. This PR still contains 55043ddc, the broad refactor(riscv): split vector semantics out of cpu.rs #202 vector-module split that was closed as superseded by the focused structure in fix(riscv): enforce RVV reserved encoding constraints #203. Please rebase onto current master and drop that commit so this PR contains only the two SMIR lifter files.
  2. The regression has two adjacent #[test] attributes. I ran the focused filter and confirmed Cargo registers and executes the same test twice (2/2 with the identical name). Please remove the duplicate marker.

Please also add aligned no-C JAL and conditional-branch controls so the regression proves the new fallback is limited to the misaligned static targets. This fix is non-RVV and should remain independent from #203.

19h commented Aug 11, 2026

Copy link
Copy Markdown
Member

Thank you @carlosqwqqwq for identifying the no-C IALIGN gap and proposing a fix. We independently reimplemented and validated this behavior in #231, where it is consolidated with #220, #222, and #224. No commits from this branch were taken; the rollup was derived from the issue reproducer and the RISC-V ISA requirements. To keep review and CI on one maintained branch, I’m closing this PR as superseded by #231. You are credited as co-author on the consolidation commit—please review #231, particularly its conservative dynamic-JALR fallback.

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.

SMIR lifts no-C JAL/JALR/branch without IALIGN=32 checks

2 participants