test(gc): cover the root lowering that actually ships — native-roots assertions for #7502 - #7653
Conversation
📝 WalkthroughWalkthroughAdded test-only RS4GC inspection and stack-map decoding helpers. Added native-root coverage for pointer liveness, dead values, numeric locals, entry ordering, loops, duplicate locals, and scalar replacement on macOS ARM64 and Linux x86_64. ChangesNative Root Coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NativeRootTest
participant InprocessRS4GC
participant LLVM
participant GCMapDecoder
NativeRootTest->>InprocessRS4GC: build fixture and rewrite statepoints
InprocessRS4GC->>LLVM: emit target assembly
LLVM->>GCMapDecoder: provide stack-map records
GCMapDecoder-->>NativeRootTest: decode safepoint root sets
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/perry-codegen/src/native_root_coverage/mechanics.rs (1)
185-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the control safepoint count before indexing.
The subject half asserts
dead_allocs.len() == 2at line 165 before it indexes element 1. The control half indexeslive_allocs[1]with no such assertion. If the control program produces onejs_map_allocsafepoint, the test fails with an index panic instead of the intended message. The same gap exists at line 293 forheap_alloc[1].♻️ Proposed change
let live_allocs = live_allocs.at("js_map_alloc"); + assert_eq!( + live_allocs.len(), + 2, + "[{target}] CONTROL: one safepoint per allocation: {live_allocs:?}" + ); assert_eq!( live_allocs[1].live.len(),🤖 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 `@crates/perry-codegen/src/native_root_coverage/mechanics.rs` around lines 185 - 194, Add explicit safepoint-count assertions before indexing control results in the relevant tests. In the control path around live_allocs, assert that live_allocs contains two entries before accessing live_allocs[1], and apply the same guard to the heap_alloc control path near the later assertion. Preserve the existing diagnostic assertions after the bounds checks.crates/perry-codegen/src/native_root_coverage/mod.rs (1)
306-320: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGuard the function-body end detection.
function_slicesearches for"\n}\n"fromstart. If the target function is the last one in the module and the IR ends with}and a single trailing newline, the search fails and the slice extends to the end of the module text. Later helpers then count allocas and statepoints from unrelated trailing content, such as attribute groups or metadata. The fallback is silent.Consider making the fallback explicit so a mis-slice fails loudly.
♻️ Proposed change
let end = ir[start..] .find("\n}\n") .map(|o| start + o + 3) - .unwrap_or(ir.len()); + .or_else(|| ir[start..].find("\n}").map(|o| start + o + 2)) + .unwrap_or_else(|| panic!("function `{name}` has no closing brace in IR:\n{ir}"));🤖 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 `@crates/perry-codegen/src/native_root_coverage/mod.rs` around lines 306 - 320, Update function_slice’s function-body end detection to require and validate the target function’s closing brace, including when the IR ends with a closing brace followed by only one newline. Replace the silent ir.len() fallback with an explicit failure that identifies the function and indicates the closing delimiter was not found, preventing unrelated trailing IR from entering the slice.
🤖 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.
Nitpick comments:
In `@crates/perry-codegen/src/native_root_coverage/mechanics.rs`:
- Around line 185-194: Add explicit safepoint-count assertions before indexing
control results in the relevant tests. In the control path around live_allocs,
assert that live_allocs contains two entries before accessing live_allocs[1],
and apply the same guard to the heap_alloc control path near the later
assertion. Preserve the existing diagnostic assertions after the bounds checks.
In `@crates/perry-codegen/src/native_root_coverage/mod.rs`:
- Around line 306-320: Update function_slice’s function-body end detection to
require and validate the target function’s closing brace, including when the IR
ends with a closing brace followed by only one newline. Replace the silent
ir.len() fallback with an explicit failure that identifies the function and
indicates the closing delimiter was not found, preventing unrelated trailing IR
from entering the slice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e3cb0c62-c311-4529-beee-ef4ec2cd4a75
📒 Files selected for processing (7)
changelog.d/7653-native-root-coverage.mdcrates/perry-codegen/src/gc_map.rscrates/perry-codegen/src/inprocess.rscrates/perry-codegen/src/lib.rscrates/perry-codegen/src/native_root_coverage/harness_self_tests.rscrates/perry-codegen/src/native_root_coverage/mechanics.rscrates/perry-codegen/src/native_root_coverage/mod.rs
It was written before the PR number was known and collided with #7653's native-root-coverage fragment. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Native roots (RS4GC statepoints) have been the default on every target the runtime can walk since #7370, and had no assertions anywhere. The two suites that read as this area's coverage are pinned to the shadow stack (#7493) and stay that way — both lowerings are supported — but that left nine root-lowering mechanics untested against the lowering Perry emits, and three tests passing vacuously because they counted `js_shadow_slot_bind` calls the native lowering never emits. Adds `perry-codegen/src/native_root_coverage`, eight mechanic tests plus five harness self-tests, asserting at three vantages: the `ptr addrspace(1)` allocas codegen asks for, the `"gc-live"` bundle of each `gc.statepoint` after the production pass string, and the per-safepoint root lists decoded out of the compact `__perry_gcmap` blob the collector reads at run time. In-crate `#[cfg(test)]` so it runs in the per-PR `cargo-test` gate rather than the nightly-only tier. Every test is sabotage-verified — ten sabotages, each one confirmed to compile and to reach the test binary before its verdict was believed. Details per test in the doc comments. Two findings worth naming: * #7502's table calls row 9 (#7184's out-of-range slot index) `n/a` under native roots. It is not. `lower_precise_roots_to_native_stack` collects roots with `roots.get_mut(idx)` over a `slot_count`-sized vector, so an out-of-range index still drops a root silently — the same failure one layer up from the runtime bounds check. Sizing that vector one short removes a root from the emitted map with no diagnostic, and now fails a test. * `mem2reg` promoting every root alloca is a load-bearing precondition with no shadow-stack counterpart: RS4GC relocates `addrspace(1)` SSA values and does not scan allocas, so a root slot that escapes promotion is never rewritten. Asserted directly, and sabotage-verified by making the alloca's address escape. Production changes are confined to two test seams and one named constant: `gc_map::decode_stack_map_roots` and `inprocess::statepoint_rewritten_ir` are `#[cfg(test)]`, and `STATEPOINT_REWRITE_PASSES` replaces an inline string literal with the identical value so the suite cannot drift onto a pipeline production stopped using. Emitted IR is unchanged.
Two of its three vantages run the statepoint rewrite and emit assembly through that pipeline, so `cargo test -p perry-codegen --no-default-features` (the text path, kept for bisection) had nothing for them to assert against. Verified: that build now compiles clean with no new dead-code warnings.
… lowering that does not ship scripts/gc_root_dominance_corpus.sh has said so inline since #7370 flipped the default (it compiles under PERRY_RS4GC=0 because the checker anchors on @js_shadow_slot_bind calls the native lowering never emits). The invariant page did not, and that page is what a reader is told to read end to end — so a green gc-root-dominance read as evidence about the shipped lowering. Names the gap and points at the unit tests that now cover the native side.
Audit — merging as v0.5.1372Finding 3 is the most important thing produced today
I verified it: the script says so inline, and honestly ("KNOWN GAP, deliberately not papered over"), but Putting it on the page as a fourth, starred bullet is the right fix, and naming Sabotage — verified independently, two of themI re-ran two of your ten against my own tree rather than taking the table:
(My first attempt at the second sabotage didn't apply — my regex missed the actual Things you were right to push back onThe "nine mechanics, say which three you left" framing was mine and it was wrong — #7502 marks seven rows Pinning both shipped triples rather than deriving from the host is the right call for the stated reason — a suite that changes subject with the host is how a target-specific bug ships. Verified here22/22 lint commands, #7656 now tracks the sibling gap this shares a shape with: |
3defa9c to
5e468a3
Compare
1 participant
Closes #7502.
What was missing
Since #7370 native roots (RS4GC statepoints) are the lowering that ships on
every target whose frames the runtime can walk. #7493 correctly repaired
shadow_slot_hygieneandscalar_replaced_slot_rootsby pinning them to theshadow stack — but that repair made explicit that nine root-lowering mechanics
had no assertion at all against the lowering Perry emits, and that three tests
which read as coverage were measuring nothing: they asserted
js_shadow_slot_bindwas absent, which is true of every program under thenative default (CLAUDE.md hazard 4).
The shadow-pinned suites stay. Both lowerings are supported and both need
coverage; this is the missing half, not a replacement.
What this adds
crates/perry-codegen/src/native_root_coverage/— 8 mechanic tests + 5harness self-tests, in-crate
#[cfg(all(test, feature = "llvm-inprocess"))]so they run in the per-PR
cargo-testgate rather than the nightly-onlytests/*.rstier (#5960).Assertions are made at three vantages, each catching what the previous one
cannot:
optIR — theptr addrspace(1)allocas codegen asks for. The onlyvantage where "codegen never requested a root" and "LLVM removed one" are
still distinguishable, which is why the negative claims live here.
gc.statepoint's"gc-live"operand bundle, keyedby callee name, produced by running the production pass string
(
inprocess::STATEPOINT_REWRITE_PASSES). Turns "is this value a root acrossthat call" into a direct question. The shadow suites never had an equivalent.
the compact
__perry_gcmapblob the collector actually reads(
gc_map::decode_stack_map_roots, which round-trips throughencode_stream+verify_roundtrip, so a test asserts on what the binary ships).Both shipped native-roots targets (
arm64-apple-macosx,x86_64-unknown-linux-gnu)are compiled and emitted for on every host, pinned rather than host-derived:
cargo-testruns on x86_64 Linux and this repo is developed on arm64 macOS, anda suite that silently changes subject with the host is how a target-specific
lowering bug reaches a release.
(1, 2)against a heap twinjs_gc_initand__perry_init_strings_*Non-vacuity is the whole point, so it is structural
Statepoints::atpanics when thecallee it is asked about produced no safepoint;
map_records_forpanics whenthe function is missing from the map. "Zero roots" is only ever asserted about
a record that exists.
lowering that roots nothing fails the control half.
development. Both bugs would have made every "nothing is live here" assertion
pass for the wrong reason: the callee parser read
@llvm.experimental.gc.statepoint.p0for every safepoint (the intrinsic's ownsignature contains
) @), and the live-set parser truncated at the)insideptr addrspace(1)and reported an empty live set for every statepoint inevery program.
Sabotage results
Ten sabotages, each confirmed to compile (
error[count 0) and to reachthe test binary (
Running unittestspresent) before its verdict was believed.alloca doubleinstead ofptr addrspace(1)ret)root_scalar_replaced_slot'sroot_entry_allocaremoved (#6968 reintroduced)heap 3 vs numeric 3)expr_is_known_non_pointer_shadow_valueearly-out removedjs_gc_initdeleted frommain__perry_init_strings_*moved to a pre-return call#32 vs #4)rootsvector sizedslot_count - 1mem2regcannot promote itOne negative result is recorded in the code too, because it says something about
the mechanic: the weaker CFG-union sabotage leaves mechanic 6 green, and LLVM
is right about that — the previous iteration's value is genuinely dead at the
next iteration's allocation, since the phi carrying it to the return is redefined
in the body. Only a lowering that keeps a root live across the back edge itself
can break that row.
Three findings
#7502's table is wrong about row 9. It marks #7184's shape "unrepresentable"
because there is no frame bound under native roots. The frame bound is gone,
but the defect was never about a frame — it is about an index falling silently
outside the structure that collects roots, and
lower_precise_roots_to_native_stackstill has one: it collects withroots.get_mut(idx)over aslot_count-sized vector, so an out-of-range indexdrops the alloca from
root_ptrsand it is never retyped and never rooted, withno diagnostic. Sizing that vector one element short removes a root from the
emitted map while the function still compiles, verifies and emits. Now tested.
mem2regpromoting every root alloca is a native-only precondition with noshadow counterpart. RS4GC relocates
addrspace(1)SSA values and does not scanallocas, so a root slot that escapes promotion is one the collector never
rewrites — the value reads as rooted and is not, which is the #7184/#7192
presentation exactly. Asserted directly in
no_root_alloca_survives_the_statepoint_rewrite.docs/src/internals/gc-rooting-invariant.mdwas missing its most importantcaveat.
gc_root_dominance_corpus.shcompiles the corpus underPERRY_RS4GC=0— the shadow lowering — and says so inline, for a goodreason (the checker anchors on
@js_shadow_slot_bind, of which the nativelowering emits zero; under the default the corpus has 1251 statepoints, no binds,
and
--min-bindsfails the job). But the invariant page, which is what a readeris told to read end to end, listed three blind spots and not that one — so a
green
gc-root-dominanceread as evidence about the shipped lowering. Added asa fourth, starred, bullet, pointing at these tests as the native-side coverage
until the checker learns to read relocation bundles.
Production surface
Deliberately tiny, and it cannot change emitted IR:
gc_map::decode_stack_map_roots—#[cfg(all(test, feature = "llvm-inprocess"))].inprocess::statepoint_rewritten_ir—#[cfg(test)]inside a feature-gated module.inprocess::STATEPOINT_REWRITE_PASSES— a named constant replacing an inlinestring literal of the identical value, so the suite cannot go green against a
pipeline production no longer uses.
Verification
cargo test -p perry-codegen --lib --no-fail-fast— 724 passed, 0 failed.cargo test -p perry-runtime --lib --no-fail-fast— 1915 passed, 0 failed, 4 ignored.cargo test -p perry-codegen --lib --no-default-features --no-run— builds clean(the feature gate exists so it does).
cargo check --all-targets --workspace(cross-host UI crates excluded) — clean;re-run under
RUSTFLAGS=-D warningsand confirmed none of the residualmacOS-host warnings names a file this PR adds or touches.
cargo fmt --all -- --check— clean..github/workflows/test.yml— 22 run, 0 failed.gc_root_dominance_check.py --moving-only --seeded-violations 40— see thecomment below.