Skip to content

fix(riscv): keep memory access for loads to x0 - #224

Closed
carlosqwqqwq wants to merge 2 commits into
HexRaysSA:masterfrom
carlosqwqqwq:fix-riscv-lifter-x0-load
Closed

fix(riscv): keep memory access for loads to x0#224
carlosqwqqwq wants to merge 2 commits into
HexRaysSA:masterfrom
carlosqwqqwq:fix-riscv-lifter-x0-load

Conversation

@carlosqwqqwq

Copy link
Copy Markdown
Contributor

fix(riscv): keep memory access for loads to x0

Closes #223

Summary

The SMIR lifter dropped the Load operation entirely when the destination register was x0, which removed the architectural memory read: out-of-bounds accesses no longer fault and MMIO read side effects disappeared on the translated/JIT path. A temporary destination keeps the access while x0 stays unwritten.

Validation

  • Regression lifts lw x0, 0(x1) and asserts a Load operation is present.
  • cargo test --lib: 8214 passed, 0 failed.

Notes

Translated-path fault/side-effect preservation, independent of 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.
The SMIR lifter dropped the Load operation entirely when the destination was x0, which removed the architectural memory read: out-of-bounds accesses no longer fault and MMIO read side effects disappeared on the translated path. A temporary destination keeps the access while x0 stays unwritten.

19h commented Aug 11, 2026

Copy link
Copy Markdown
Member

I reviewed the focused commit 8566844d against issue #223. The diagnosis and implementation shape are correct: x0 suppresses only the architectural register write, so lifting the memory access into an otherwise unused temporary preserves faults and MMIO read side effects.

Three changes are needed before this is merge-ready:

  1. This PR still contains 55043ddc, the closed refactor(riscv): split vector semantics out of cpu.rs #202 vector-module split. Please rebase onto current master and drop that commit so the PR contains only the focused scalar-memory change.
  2. The regression again has duplicate adjacent #[test] attributes, which register the same test twice. Please remove one marker.
  3. The test only checks that an OpKind::Load exists. Because the destination temporary is intentionally dead, that does not prove the optimizer retains the effectful load or that the interpreter/native helper publishes its trap. Please add an end-to-end out-of-bounds lw x0, 0(x1) regression through unoptimized SMIR, optimized SMIR, and the applicable native JIT path, asserting the exact load-access-fault cause/tval and that x0 remains zero. A counted-memory control for one successful read would directly cover the MMIO-side-effect claim.

This is a non-RVV scalar SMIR memory fix and should remain independent from #203.

19h commented Aug 11, 2026

Copy link
Copy Markdown
Member

Thank you @carlosqwqqwq for identifying that SMIR dropped observable loads to x0 and proposing a fix. We independently reimplemented and validated this behavior in #231 across every scalar load width plus successful and faulting O0/O2 production-JIT paths. It is consolidated there with #218, #220, and #222; no commits from this branch were taken. 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.

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 drops the memory access for loads to x0

2 participants