fix(cpu): sign-extend LWR only for the full word (R-20 closed, 18/18) - #135
Merged
Conversation
The final 4 `tlb64` failures were the `do_all_loads` battery in 64-bit mode. A focused reproduction harness (calling `Pipeline::access_unaligned` directly with the four base addresses under 64-bit kernel mode and comparing each load against the ROM's `EXPECTED`) pinned the bug precisely and proved it MODE-INDEPENDENT: `mem::lwr` sign-extended unconditionally, but the VR4300 sign-extends `LWR` only for the full-word case (`byte == 3`, the one that writes bit 31). A partial `LWR` (bytes 0-2) leaves bits 63:32 of `rt` UNCHANGED. The `tlb64` battery exposes it because its sentinel's upper half (`0xBEEF_0000`) is non-zero; `LWL`/`LDL`/`LDR` all passed because they always write bit 31 or the whole register. This never surfaced before because the R-19 hang sat in front of the `tlb64` group, and no 32-bit test used a non-zero upper sentinel. - `mem::lwr`: sign-extend iff `byte == 3`, else preserve `rt & 0xFFFF_FFFF_0000_0000`. - Add mutation-checked `a_partial_lwr_preserves_rt_upper_half_and_only_the_full_word_sign_extends`. - `tests/systemtest.rs`: add the `emux_exited` COMPLETION WITNESS promised in R-19 -- the gate now requires the guest's own `xioctl(EXIT)`, so a mid-suite hang that leaves a partial Phase-1 `Failed: 0` can never pass again. Result: n64-systemtest Phase-1 categories `Failed: 0` **with the suite running to completion**; suite-wide 94 -> 90 (remainder are RSP/RCP/RDP, later phases). This closes R-20 (18/18) and satisfies the user's "keep the gate at literal Failed: 0" decision genuinely, not by baseline. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change corrects partial ChangesLWR semantics and suite completion
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
Comment |
Antigravity review (Gemini via Ultra)This PR modifies Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/accuracy-ledger.md`:
- Line 282: Correct the final R-20 rationale describing the passing load
instructions: update the LDL/LDR explanation to acknowledge that they can
perform partial 64-bit merges, and state that they avoid this failure because
they do not apply 32-bit sign extension. Keep the existing mem::lwr diagnosis
and fix description unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fa33faf8-016a-480d-b52e-0a75ac1651f7
📒 Files selected for processing (3)
crates/rustyn64-cpu/src/mem.rscrates/rustyn64-test-harness/tests/systemtest.rsdocs/accuracy-ledger.md
| | R-18 | A **commercial ROM boots and executes real code but does not reach video** (Phase 5 capstone). Through the retail HLE boot (`rom::hle_boot`) the game's own IPL3 runs, the CPU fetches the cartridge's instruction stream, and the PC advances through hundreds of millions of retired instructions across varied routines — but no frame is scanned out: over ~10 s of emulated time `VI_CTRL` stays 0, `VI_ORIGIN` is never set, and **no interrupt of any kind fires** (SM64 witnessed at `retired ≈ 9.4×10⁸`, all MI interrupt lines clear) | The retail OS-boot runtime the game waits on is not yet modelled. A commercial title's boot is interrupt-driven: after its OS initialises, its main loop blocks on the **VI vblank interrupt**, which the emulator only raises once the game programs `VI_CTRL`/`VI_V_INTR` — and the game does not reach that programming, indicating an earlier dependency (the **RI/RDRAM interface** registers used for RDRAM sizing, and/or the OS thread/interrupt setup). This is a cross-subsystem gap spanning the VI vblank loop, the RI registers, and the F3DEX graphics microcode — all **outside the Phase 5 cart/boot/saves boundary** (ADR 0003; the cart phase delivers PI/SI/PIF/CIC + saves, not the OS runtime) | absolute — a coverage boundary across subsystems, not a fitted constant or a timing interval | **Open — characterised, not a regression.** The committable Phase 5 gate (n64-systemtest cart/PIF/SI, save round-trips, homebrew boot) is met; the commercial capstone is asserted at its honest achievable level — `a_commercial_rom_boots_and_executes` (local, `#[ignore]`d) proves the ROM boots and retires ≥ 10⁶ real instructions without panicking, and *reports* the lit-pixel count (0) rather than asserting it. Reaching a title frame is deferred to the VI/RI/F3DEX work of a later phase and validated then. This gap was surfaced by the capstone exactly as the plan's escalation gate intended: **ship v0.6.0 on the committable gates + an honest "boots and executes" capstone, not a faked pass or an unbounded chase.** n64-systemtest impact: none — the boot/video path has no systemtest driver; the suite-wide count is **90** (see C-32) | | ||
| | R-19 | **The emulator hangs on the n64-systemtest test `TLB: Execute mapped branch with a non-mapped delay slot`** — a mapped branch whose delay slot lies in a page not currently in the TLB. Both the committed **base** ROM and the `--features timing` ROM stop dead there: `started = 917`, `emux_exited = false`, no test after it ever starts, at an 8×10⁹-tick budget (~2× a normal base completion). It is a genuine loop, not slowness. | **The committed `systemtest` gate masks it**: `tests/systemtest.rs` asserts Phase-1 *category* `Failed: 0` (those results are captured before test 917) and witnesses `started > 0`, but never requires the ROM to run to `xioctl(EXIT)` — so a mid-suite hang is invisible (the failure mode engineering-lessons §2.2 warns about, one level up). **Fully traced 2026-07-24 — every architectural field is CORRECT, so the defect is NOT in the delivered exception state.** The loop oscillates between `pc = 0x1234_5000` (the non-mapped delay-slot fetch) and `pc = 0x8000_0180` (the general vector), sustaining `EXL = 1`, with: `BadVAddr = 0x1234_5000` (✓ the delay slot), `EPC = 0x1234_4FFC` (✓ the branch, `pc − 4`), `Cause.BD = 1` (✓), `Cause.ExcCode = 2` = `TLBL` (✓), `Context`/`XContext` `BadVPN2 = 0x0_91A2` (✓ `= BadVAddr >> 13`), `EntryHi` VPN2 `= 0x1234_4000` + ASID (✓), 32-bit mode (`Status.KX/SX/UX = 0`). The general vector is correct *given* `EXL = 1` (a refill with `EXL` set uses `0x180`, S-3). `ERET` clears `EXL` correctly (tested). **So `EPC`, `BD`, `Cause`, `BadVAddr`, `Context`, `XContext`, `EntryHi`, the vector, and `ERET` are all right** — the earlier "vector/EPC/EXL is off" guess is disproven. The remaining suspects are in the finer *sequencing* the trace hasn't yet caught: (a) the **`EXL = 0` first fault** (does it reach the refill vector `0x8000_0000` and n64-systemtest's *test* handler, or does the refill handler's own page-table load fault nested straight to the general/"unexpected-exception spin" handler?); and (b) whether n64-systemtest's handler **maps the page and ERETs** expecting the fetch to now hit — in which case a stale **micro-ITLB** (not refilled from the JTLB after the map) would keep the fetch missing. | absolute — a hang is a coverage boundary, not a fitted constant | **Open — fully characterised, not yet root-caused. Blocks the `timing` suite from completing (so it blocks the clean `M` measurement, C-1).** **The test + handler are now understood** (`tlb/exceptions.rs:388` + `exception_handler.rs:247`): the JALR is the last instruction of the mapped page, its delay slot is the first of the next (unmapped) page; the test runs it under `expect_exception(TLBL, -4, …)`, which sets `EXCEPTION_SKIP = -4`, so the handler resumes at `return_to = exceptpc + skip*4 = EPC − 16` — back inside the mapped block, expecting the block's own code there to escape back to the `0x80…` test. n64-systemtest asserts `exceptpc == fault_address − 4` (line 436), and **our `EPC = 0x1234_4FFC` matches that exactly** — a third confirmation the exception state is right. So the loop is not a wrong `EPC`/vector; it is that after the skip-return our CPU re-reaches the JALR and re-faults instead of escaping. **RESOLVED 2026-07-24 (root-caused by a full pipeline-latch trace, not by reasoning).** The defect was NOT in the exception state (all correct, as characterised) but in the **branch-redirect vs. exception-vector race** in `ex_stage`. Sequence: the delay-slot fetch (`0x1234_5000`) faults and the exception is dispatched at the end of the cycle, setting `next_pc = 0x8000_0180` — but the JALR is still sitting **unexecuted** in `rf_ex`. On the next active cycle the JALR reaches EX and unconditionally applied its redirect (`*next_pc = r.target`), and this JALR's target is its **own address** (`v1 = 0x1234_4FFC`), so it clobbered the vector, re-fetched itself, re-faulted its delay slot, and looped forever — exactly the two-state oscillation the latch trace showed (JALR + aborted delay slot circulating, never retiring). Fix (`pipeline.rs::resolve_branch_control`): a branch whose delay slot has aborted (its `ic_rf` latch carries `in_delay_slot` + an abort) **still writes its link** — from the architectural `pc + 8`, since `next_pc` now holds the vector — but its **redirect is suppressed**, so the exception PC wins. This is hardware-accurate: the older branch retires and links (n64-systemtest asserts `RA == fault_address + 4`) while the precise exception on the younger delay slot takes over control flow. With the fix the delay-slot test passes and **the full suite runs to `xioctl(EXIT)` for the first time (950 tests, ~30 s), so `emux_exited` is now `true`.** No regression: golden-log 0-diff, determinism, residue-invariant, and all workspace tests stay green. Completing the run unmasked a distinct pre-existing cluster the hang had hidden — see **R-20**. Discovered + traced + fixed 2026-07-24 during the Stage-C/D timing work | | ||
| | R-20 | **64-bit addressing mode is not implemented** — the n64-systemtest `tlb64` group reports **18 failures** (14 `LW TLB Miss or Address Exception (64 bit addressing mode)` cases where `EntryHi`/`Context`/`XContext` read back `0` instead of the 64-bit VPN2, plus 4 `Loads from 32/64 bit address while using 64 bit addressing mode` returning wrong data). These tests run only in 64-bit addressing mode (`Status.KX/SX/UX = 1`) and exercise the `XKPHYS`/`XKSEG` segments and the `R` (region) field of the 64-bit `EntryHi`/`Context`/`XContext` decomposition | The emulator's segment map and TLB-miss register write-back model the **32-bit** address decomposition; the 64-bit `R:VPN2` layout (bits 63:62 region + the wider VPN2) and the wide-address segment ranges are not decoded, so a 64-bit TLB miss leaves `EntryHi`/`Context` at their reset `0`. **This cluster was masked by R-19**: the `tlb64` tests run *after* the delay-slot test that hung, so the suite never reached them — Phase 1's `Failed: 0` was only ever true *up to the hang point*, which is precisely the vacuous-pass failure mode the R-19 gate now witnesses against (`emux_exited`) | absolute — an address-decode / register-decode fact, not a timing interval | **Open — newly exposed, Stage D (CPU accuracy).** A genuine 64-bit-addressing feature gap (region-field decode + wide segment map + 64-bit miss write-back), not a regression from the R-19 fix (the fix touches only branch-delay-slot control flow). Pin against the `tlb64` group and implement the `R:VPN2` decomposition + `XKPHYS`/`XKSEG` ranges; read the expected `EntryHi`/`Context` values as a table from the suite's own assertions (do not compute against them — engineering-lessons §3.x). Surfaced 2026-07-24 the moment the suite could complete. **Progress 2026-07-24: 14 of 18 closed.** Root cause of the 14 `LW TLB Miss…(false, …)` cases was that **`EntryHi`'s VPN2/R was not written on a data address error** — the UM (§6.4.7) calls it "undefined", but the oracle pins `(VPN2 << 13) \| (R << 62)` from the faulting address, exactly as `Context`/`XContext` are already filled (which is why only `EntryHi` mismatched). Fixed by gating the `EntryHi` write on `writes_bad_vaddr` (address errors included), deleting the superseded `writes_tlb_context`, and replacing the wrong `an_address_error_leaves_entry_hi_alone` unit test with `an_address_error_writes_entry_hi_vpn2_and_region` (mutation-checked). Suite-wide 108→94. **Remaining 4:** the `do_all_loads` battery (`Loads from 0x80/0xA0/0x90/0x98 … in 64-bit mode`) — the **scalar** loads (LB…LD, LWU, LDC1, LL/LLD) all pass, so base translation (CKSEG0/CKSEG1/XKPHYS) is correct; the first mismatch is an **array** field (LWL/LWR/LDL/LDR). Segment map + `access_unaligned` are both mode-agnostic and the XKPHYS→physical translation checks out, so the cause is not yet pinned — needs the actual per-load `a` vs `b` diff (the systemtest harness truncates the array message), captured via a focused reproduction, before any change. Do NOT guess | | ||
| | R-20 | **64-bit addressing mode is not implemented** — the n64-systemtest `tlb64` group reports **18 failures** (14 `LW TLB Miss or Address Exception (64 bit addressing mode)` cases where `EntryHi`/`Context`/`XContext` read back `0` instead of the 64-bit VPN2, plus 4 `Loads from 32/64 bit address while using 64 bit addressing mode` returning wrong data). These tests run only in 64-bit addressing mode (`Status.KX/SX/UX = 1`) and exercise the `XKPHYS`/`XKSEG` segments and the `R` (region) field of the 64-bit `EntryHi`/`Context`/`XContext` decomposition | The emulator's segment map and TLB-miss register write-back model the **32-bit** address decomposition; the 64-bit `R:VPN2` layout (bits 63:62 region + the wider VPN2) and the wide-address segment ranges are not decoded, so a 64-bit TLB miss leaves `EntryHi`/`Context` at their reset `0`. **This cluster was masked by R-19**: the `tlb64` tests run *after* the delay-slot test that hung, so the suite never reached them — Phase 1's `Failed: 0` was only ever true *up to the hang point*, which is precisely the vacuous-pass failure mode the R-19 gate now witnesses against (`emux_exited`) | absolute — an address-decode / register-decode fact, not a timing interval | **Open — newly exposed, Stage D (CPU accuracy).** A genuine 64-bit-addressing feature gap (region-field decode + wide segment map + 64-bit miss write-back), not a regression from the R-19 fix (the fix touches only branch-delay-slot control flow). Pin against the `tlb64` group and implement the `R:VPN2` decomposition + `XKPHYS`/`XKSEG` ranges; read the expected `EntryHi`/`Context` values as a table from the suite's own assertions (do not compute against them — engineering-lessons §3.x). Surfaced 2026-07-24 the moment the suite could complete. **Progress 2026-07-24: 14 of 18 closed.** Root cause of the 14 `LW TLB Miss…(false, …)` cases was that **`EntryHi`'s VPN2/R was not written on a data address error** — the UM (§6.4.7) calls it "undefined", but the oracle pins `(VPN2 << 13) \| (R << 62)` from the faulting address, exactly as `Context`/`XContext` are already filled (which is why only `EntryHi` mismatched). Fixed by gating the `EntryHi` write on `writes_bad_vaddr` (address errors included), deleting the superseded `writes_tlb_context`, and replacing the wrong `an_address_error_leaves_entry_hi_alone` unit test with `an_address_error_writes_entry_hi_vpn2_and_region` (mutation-checked). Suite-wide 108→94. **Closed 2026-07-24 (18/18).** The last 4 were the `do_all_loads` battery (`Loads from 0x80/0xA0/0x90/0x98 … in 64-bit mode`). A focused reproduction harness (call `Pipeline::access_unaligned` directly with the four base addresses in 64-bit kernel mode, compare per-load against the ROM's `EXPECTED`) pinned the bug precisely and proved it **mode-independent**: **`mem::lwr` was unconditionally sign-extending**, but the VR4300 sign-extends `LWR` only for the **full-word** case (`byte == 3`, which writes bit 31); a **partial** `LWR` (bytes 0–2) leaves bits 63:32 of `rt` UNCHANGED. The `tlb64` battery exposes it because its sentinel's upper half (`0xBEEF_0000`) is non-zero — `LWL`, `LDL`, `LDR` all passed (they always write bit 31 or the whole register). Fixed in `mem::lwr` (sign-extend iff `byte == 3`, else preserve `rt & 0xFFFF_FFFF_0000_0000`), pinned by the mutation-checked `a_partial_lwr_preserves_rt_upper_half_and_only_the_full_word_sign_extends`. Result: **Phase 1 categories `Failed: 0` with the suite running to `xioctl(EXIT)`; suite-wide 94 → 90** (the rest are RSP/RCP/RDP, later phases). With R-20 closed, `tests/systemtest.rs` gained the `emux_exited` **completion witness** promised in R-19, so this class of mid-suite hang can never hide behind a partial Phase-1 zero again | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the LDL/LDR rationale.
LDL and LDR can perform partial 64-bit merges; they do not always write the whole register. State that they avoid this failure because they do not apply 32-bit sign extension.
As per path instructions, a comment that disagrees with the code is an independent high-priority claim.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/accuracy-ledger.md` at line 282, Correct the final R-20 rationale
describing the passing load instructions: update the LDL/LDR explanation to
acknowledge that they can perform partial 64-bit merges, and state that they
avoid this failure because they do not apply 32-bit sign extension. Keep the
existing mem::lwr diagnosis and fix description unchanged.
Source: Path instructions
Owner
Author
|
Thanks — no blocking issues; adjudicating the two minor points. Suggestion: bounds-check / mask
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Closes the final 4 of ledger R-20 (18/18 done) and adds the completion
witness promised when R-19 was fixed. With this, n64-systemtest's Phase-1
categories report
Failed: 0while the suite runs toxioctl(EXIT).Root cause (found by a reproduction harness, not by guessing)
The 4 remaining failures were the
do_all_loadsbattery in 64-bit mode. Sincethe harness truncates the array diff at
a=[, I built a focused reproduction:call
Pipeline::access_unaligneddirectly with the four base addresses(CKSEG0
0x80, CKSEG10xA0, XKPHYS0x90/0x98) under 64-bit kernel mode andcompare each load against the ROM's
EXPECTED. It pinned the bug exactly andshowed it is mode-independent:
mem::lwrsign-extended unconditionally. The VR4300 sign-extendsLWRonlyfor the full-word case (
byte == 3, the one that writes bit 31); a partialLWR(bytes 0–2) leaves bits 63:32 ofrtunchanged. Thetlb64batteryexposes it because its sentinel's upper half (
0xBEEF_0000) is non-zero, and no32-bit LWR test used a non-zero upper sentinel — so it stayed hidden behind the
R-19 hang.
Changes
mem::lwr: sign-extend iffbyte == 3, else preservert & 0xFFFF_FFFF_0000_0000.LWL/LDL/LDRare unaffected (they alwayswrite bit 31 or the whole register).
a_partial_lwr_preserves_rt_upper_half_and_only_the_full_word_sign_extends(verified red when
lwrreverts to unconditionalsext32).tests/systemtest.rscompletion witness: the gate now asserts the guestreached
xioctl(EXIT). R-19 was a mid-suite hang that left a full Phase-1Failed: 0(captured before the hang) while a whole later category never ran —requiring EXIT closes that blind spot permanently.
Verification
Failed: 0, suite runs toxioctl(EXIT);suite-wide 94 → 90 (remainder are RSP/RCP/RDP — later phases).
and a partial LWR now preserves them — they coincide by construction).
clippy --workspace --all-targets -D warnings,cargo test --workspace, rustdoc, no_std, markdownlint.This satisfies the maintainer's "keep the gate at literal
Failed: 0" decisiongenuinely (all 18
tlb64tests pass), not via an xfail baseline.🤖 Generated with Claude Code