test(rsp): replace a tautological save-state assertion with a live-counting one - #251
Conversation
…unting one A reviewer caught that `assert_eq!(restored.vu_funct_histogram().len(), 64)` is tautological. It is: the field is `Box<[u64; 64]>`, a FIXED-size array, so `.len()` is a compile-time constant and the assertion could never fail. Worse than useless — I had justified it on the sibling thread, telling a reviewer that summing to zero is also what an empty box would do and that the length check distinguished "reset" from "destroyed". That reasoning belongs to `Box<[T]>`, an unsized slice, which is what #245's `rdram_dirty` was and why THAT one needed the check. For a fixed-size array the failure mode is not representable. Replaced with the property actually worth pinning, which is the one the `retired` test already uses: the restored histogram is still LIVE. Execute another COP2 instruction and assert its bucket moves. Writing it caught a second thing. The restored PC carries over from the first tick and is 4, so filling only IMEM word 0 fetches a zero and counts nothing — the test failed until the instruction was written where the PC actually points. The tautological version could not have found that, because it never executed anything after the restore. Mutation-checked: making `count_vu_funct` a no-op turns it red.
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR strengthens the work-counters RSP save-state test by replacing a tautological histogram length assertion with an execution-based assertion that the restored histogram remains “live” (i.e., a post-restore COP2 computational op increments the expected bucket).
Changes:
- Replace an always-true
len() == 64assertion with a post-restore instruction execution that must incrementfunct 0x06. - Make the restored instance mutable and resume execution (
set_halted(false)+tick()). - Populate IMEM beyond word 0 to ensure the restored PC fetches the intended instruction.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Three review findings, all cheap. `restored` is deserialized straight into a `mut` binding rather than an immutable one immediately shadowed — which was also inconsistent with the sibling `retired` test. `0x06` becomes `VMUDN_FUNCT`, used at both the encode site and the assertion so the two provably name the same opcode. And the comment loses the revision history — which PR caught what, what the first version did. That belongs in git. What stays is the part a future reader needs: that asserting `.len()` on a `Box<[u64; 64]>` proves nothing because the length is a compile-time constant, and that the empty-container failure mode belongs to `Box<[T]>` instead.
Adjudication — Antigravity reviewAll three adopted, in Suggestion: remove the PR meta-commentary — adoptedFair, and the line is where you drew it. Gone: which PR caught what, what the first version did, the Kept, because a future reader needs it at the point of the decision rather than in a commit message they will not go looking for:
That distinction is the reason this test looks the way it does, and it is exactly the mistake that was made here — so a comment saying "assert execution, not shape" without saying why would invite it back. Plus your point about the eight IMEM words and the carried-over PC, which stays. Suggestion: name the magic
|
Antigravity review (Gemini via Ultra)This PR replaces a trivial array length assertion on a deserialized RSP state with an execution step verifying that work counters remain functional after state restoration. Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
A follow-up to #250, from a review that landed as it merged
Two findings arrived on #250 after I had merged it. One repeated the
required-featuresbuild claim already refuted by running the command. The other was right and is fixed here.The tautological assertion
The field is
Box<[u64; 64]>— a fixed-size array..len()is a compile-time constant, so this asserts64 == 64and can never fail.And I had defended it, telling a reviewer on the sibling thread:
That reasoning is correct for
Box<[T]>— an unsized slice, which is exactly what #245'srdram_dirtywas and why that one needed the check. For a fixed-size array the failure mode is not representable. I applied a real lesson to a type it does not apply to.What replaces it
The property actually worth pinning, and the one the sibling
retiredtest already uses: the restored histogram is live. Execute another COP2 instruction and assert its bucket moves.Mutation-checked — making
count_vu_functa no-op turns it red.Writing it caught a second thing
The test failed on the first attempt, and correctly: the restored PC carries over from the earlier tick and is
4, so filling only IMEM word 0 fetches a zero and counts nothing. The instruction now goes in where the PC actually points, with a comment saying why.The tautological version could not have found that — it never executed anything after the restore. That is the concrete cost of an assertion that cannot fail: it does not just fail to guard, it displaces the test that would have.
Gates
cargo fmt --all --check·cargo clippy --workspace --all-targets -- -D warnings· the same forwork-counters·cargo test --workspace·cargo test -p rustyn64-rsp --features work-counters·RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps