test(gc): run the unit-test build in production's conservative-scan mode - #7147
Merged
Conversation
added 4 commits
July 31, 2026 22:27
…asserts `js_shadow_frame_pop`'s corrupted-handle guard used `debug_assert!(false, …)` inside an `extern "C"` fn. In a debug build the assert fires, the panic cannot unwind across the `extern "C"` boundary, and the process aborts — so `cargo test -p perry-runtime --lib` in the dev profile SIGABRTed the whole test binary at `gc::tests::shadow_stack_ops::out_of_range_frame_pop_is_ignored`, which drives that path on purpose. CI runs the suite `--release`, where `debug_assert!` compiles out, so the gate was structurally blind to it: an entire profile of the runtime test suite was un-runnable and no gate could go red. Both sites now use the `report_growth_stub_skipped_below_heap_min` pattern — a `#[cold]` fn emitting one line on stderr, once per process, so it cannot perturb a stdout parity comparison. Skip behaviour is unchanged in both profiles. `js_gc_temp_root_push`'s overflow guard had the identical defect (unreachable in practice — it needs u32::MAX live temp roots — hence never caught) and is fixed the same way rather than left as a latent abort. Closes #7145 Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9
…tale local `test_minor_preserves_old_to_young_edge_across_minors` asserted the parent's slot still held `ptr_bits(child)` -- the address captured *before* the first minor. That only held because the unit-test build defaults the conservative native-stack scan to `Full`, which makes the copying minor ineligible (`CopiedMinorFallbackReason::ConservativeStack`) and therefore non-moving. With precise roots only (`PERRY_CONSERVATIVE_STACK_SCAN=0`, which is what production resolves to) the copying minor runs and RELOCATES the child on every cycle, correctly rewriting the parent's slot -- and the assertion compared the new address against the stale local: left: 9222531965582835720 right: 9222531965580476424 Reading a raw local across a relocating collection is precisely the defect class this suite exists to catch, so the test now does what generated code does: it re-reads the child out of the parent's slot after each minor. Measured (probe, scan off): cycles 0-2 run `copied=1 promoted=0` with the RS edge intact; cycle 3 reports `promoted=1` and the child leaves the nursery, at which point the edge is old->old and the remembered set correctly retires it. The loop now handles that explicitly -- a tenured child must be live old-gen memory, not swept -- instead of asserting an RS entry that should not exist. A `rs_covered_cycles >= 2` floor keeps the loop from going vacuous if the child is ever tenured early, and the slot is checked to still round-trip through `ptr_bits` so a corrupted tag cannot pass as a relocation. Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9
The test build defaulted `conservative_stack_scan_mode()` to `Full` while production resolves `Auto -> SkipDisabled`. That made the suite a DIFFERENT GC configuration from the thing it certifies, in the direction that hides bugs: * a missing precise root -- the #7055 shape -- was rescued by the native scan under test and was a live wrong answer in production, so the suite filtered out exactly the failure class it exists to catch; * the copying minor was ineligible under test (`CopiedMinorFallbackReason::ConservativeStack`), so relocation went largely unexercised; * a conservative scan keeps arbitrary native-stack garbage alive, which is exactly the wrong direction for "this object should have been collected". The justification in the comment described how some tests were written, not a requirement of testing. The correct mechanism already existed and the tests already used it -- `RuntimeHandleScope` / `gc_temp_root_*`, plus the isolation guards, which pin `Auto` themselves. 351 of the 531 gc tests were already running in production's mode; flipping the default for the rest cost exactly one test conversion (the preceding commit). Measured on the mini (release, --test-threads=1): 1574 passed / 0 failed with the default flipped, and unchanged under an explicit `PERRY_CONSERVATIVE_STACK_SCAN=0`. Also drops `ConservativeScanAutoGuard`. Its whole job was opting a thread back to `Auto` from the `Full` default; with the default already `Auto` it set the value it was going to get anyway and could no longer fail, so per CLAUDE.md's kill-policy it is deleted rather than left as an untested no-op. Its single user moves to `ConservativeScanDisabledGuard`, which asserts the stronger property that test actually needs (objects held only as native locals must be COLLECTED). Unrelated, and folded in only because it blocks `lint` on every open PR: `crates/perry-codegen/src/linker.rs:96` landed with a rustfmt violation in #7135, so `cargo fmt --all -- --check` is red on `main` as of a3b31c0. Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9
#7135 landed a formatting violation at linker.rs:96, so `cargo fmt --all -- --check` is red on `main` (a3b31c0) and blocks `lint` on every open PR. rustfmt-only, no behaviour change. Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9
📝 WalkthroughWalkthroughThe change aligns test and production conservative GC scanning, updates GC tests for relocated references, replaces C-ABI debug assertions with non-panicking diagnostics, removes an obsolete test guard, and reformats one linker expression. ChangesGC root handling
Linker formatting
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 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.
Actionable comments posted: 3
🤖 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 `@crates/perry-runtime/src/gc/roots.rs`:
- Around line 253-277: Update the comment for
ScopedRootScannerRegistryGuard::new to remove the claim that its Auto override
opts out of a test-only Full default. Keep the override if it preserves test
isolation, and describe that current isolation purpose instead.
In `@crates/perry-runtime/src/gc/roots/shadow_stack.rs`:
- Around line 562-567: Make both C-ABI diagnostics non-panicking by replacing
the eprintln! calls in crates/perry-runtime/src/gc/roots/shadow_stack.rs:562-567
and crates/perry-runtime/src/gc/roots/temp_roots.rs:77-81 with writeln! to a
locked stderr handle, explicitly discarding any write error. Preserve the
existing diagnostic messages and behavior otherwise.
In `@crates/perry-runtime/src/gc/tests/oldgen.rs`:
- Around line 754-757: Update the test around ConservativeScanDisabledGuard and
collect_minor_trace to reload child_header after collection from a rewritten
root or the relocated parent slot before performing the mark assertion. Do not
dereference the pre-collection child_header after relocation; preserve the
existing root-scanning setup and assertion behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ce8ac6ae-01ca-4de6-adf1-0c8b447c2597
📒 Files selected for processing (7)
changelog.d/7147-gc-test-build-production-scan-mode.mdcrates/perry-codegen/src/linker.rscrates/perry-runtime/src/gc/roots.rscrates/perry-runtime/src/gc/roots/shadow_stack.rscrates/perry-runtime/src/gc/roots/temp_roots.rscrates/perry-runtime/src/gc/tests/oldgen.rscrates/perry-runtime/src/gc/tests/support.rs
Comment on lines
+253
to
+277
| // ONE default for every build, tests included. | ||
| // | ||
| // The test build used to default to `Full` on the grounds that unit tests | ||
| // root GC-managed pointers as raw locals on the *native* (Rust) stack | ||
| // rather than the shadow stack, so the conservative scan was what kept them | ||
| // alive. That is a description of how some tests were written, not a | ||
| // requirement of testing, and it had a cost that outweighed the | ||
| // convenience: it made the test build a DIFFERENT GC configuration from | ||
| // production. A missing precise root — the #7055 shape — was rescued by the | ||
| // native scan under test and was a live wrong answer in production, so the | ||
| // suite filtered out precisely the failure class it exists to catch. It | ||
| // also made the copying minor ineligible under test | ||
| // (`CopiedMinorFallbackReason::ConservativeStack`), so relocation was | ||
| // largely unexercised. | ||
| // | ||
| // The correct mechanism already existed and the tests already used it: | ||
| // `RuntimeHandleScope` / `gc_temp_root_*`, plus the isolation guards, which | ||
| // pin `Auto` themselves — 351 of the 531 gc tests were already running this | ||
| // way. Flipping the default cost exactly one test conversion | ||
| // (`test_minor_preserves_old_to_young_edge_across_minors`, which read a | ||
| // relocated child through a stale pre-collection local). | ||
| // | ||
| // A test that needs the scan *provably* off — not merely resolved off by | ||
| // today's `Auto` policy — should pin `ConservativeScanDisabledGuard`. | ||
| ConservativeStackScanMode::Auto |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the related test-helper comment.
ScopedRootScannerRegistryGuard::new still says that its Auto override opts out of a test-only Full default. This change removes that default. Keep the override if it establishes test isolation, but describe its current purpose.
🤖 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-runtime/src/gc/roots.rs` around lines 253 - 277, Update the
comment for ScopedRootScannerRegistryGuard::new to remove the claim that its
Auto override opts out of a test-only Full default. Keep the override if it
preserves test isolation, and describe that current isolation purpose instead.
Comment on lines
+562
to
+567
| eprintln!( | ||
| "[perry-gc] shadow-stack pop past end: frame handle {frame_handle:#x} is out of \ | ||
| range; the pop was SKIPPED (memory-safe, over-approximates the root set). This \ | ||
| usually means a caller threaded a NaN-boxed value into js_shadow_frame_pop \ | ||
| instead of the index js_shadow_frame_push returned. Reported once per process." | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Rust's eprintln! macro panic if writing to stderr fails? Cite Rust standard-library documentation or source.
💡 Result:
Yes, Rust's eprintln! macro panics if writing to standard error (stderr) fails [1][2]. According to the official Rust standard library documentation, this behavior occurs because the macro is designed for reporting error and progress messages, where it is generally considered appropriate to crash if the output mechanism itself fails [1][3]. Specifically, writing to non-blocking stderr can result in an error that leads the macro to panic [1][2].
Citations:
- 1: https://doc.rust-lang.org/std/macro.eprintln.html
- 2: https://doc.rust-lang.org/stable/std/macro.eprintln.html
- 3: https://doc.rust-lang.org/1.74.0/std/macro.eprintln.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## locate files"
fd -a 'shadow_stack\.rs|temp_roots\.rs' . || true
echo
echo "## relevant sections"
for f in crates/perry-runtime/src/gc/roots/shadow_stack.rs crates/perry-runtime/src/gc/roots/temp_roots.rs; do
if [ -f "$f" ]; then
echo
echo "--- $f ---"
nl -ba "$f" | sed -n '1,120p'
echo
nl -ba "$f" | sed -n '530,590p'
fi
done
echo
echo "## extern C / c_unwind / panic settings references"
rg -n 'extern\s+"C"|c_unwind|cfg_attr\s*\(|catch_unwind|should_panic|test_no_safety' crates/perry-runtime/src/gc crates/perry-runtime/src -g '*.rs' | head -200
echo
echo "## module/crate panic behavior"
rg -n 'panic!|expect\(|unwrap\(|assert!|catch_unwind|unwind|panic=' Cargo.toml crates/perry-runtime -g 'Cargo.toml' -g '*.rs' | head -250Repository: PerryTS/perry
Length of output: 492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## relevant sections with cat -n"
for f in crates/perry-runtime/src/gc/roots/shadow_stack.rs crates/perry-runtime/src/gc/roots/temp_roots.rs; do
if [ -f "$f" ]; then
echo
echo "--- $f ---"
cat -n "$f" | sed -n '1,120p'
cat -n "$f" | sed -n '530,590p'
fi
done
echo
echo "## extern C / ABI panic references"
rg -n 'extern\s+"C"|extern "C"|c_unwind|cfg_attr\s*\(|catch_unwind|should_panic|test_no_safety' crates/perry-runtime/src/gc crates/perry-runtime/src -g '*.rs' | head -250 || true
echo
echo "## panic-related references"
rg -n 'panic!|expect\(|unwrap \(\)|assert!|catch_unwind|unwind|panic=' Cargo.toml crates/perry-runtime -g 'Cargo.toml' -g '*.rs' | head -300 || true
echo
echo "## locate C function exports"
rg -n '#\[no_mangle\]|\bjs_.*\(.*\)\s*{|pub extern "C"\s*fn|pub unsafe extern "C"\s*fn' crates/perry-runtime/src/gc crates/perry-runtime/src -g '*.rs' | head -300 || true
echo
echo "## deterministic read: source lines for print macros in target files"
python3 - <<'PY'
from pathlib import Path
for name in ["crates/perry-runtime/src/gc/roots/shadow_stack.rs", "crates/perry-runtime/src/gc/roots/temp_roots.rs"]:
p = Path(name)
print(f"\n--- {name} ---")
for i,line in enumerate(p.read_text(errors="replace").splitlines(), start=1):
if "eprintln!" in line or "extern" in line.lower() or "no_mangle" in line:
print(f"{i}: {line}")
PYRepository: PerryTS/perry
Length of output: 50371
🌐 Web query:
Rust std macro eprintln stderr write failure panic source
💡 Result:
The Rust eprintln! macro is explicitly designed to panic if it encounters an error while writing to io::stderr [1][2][3]. The official documentation for eprintln! explicitly states this behavior under its "Panics" section [1][2]: "Panics if writing to io::stderr fails." This behavior is particularly relevant when writing to non-blocking stderr streams, where an error can occur if the write operation cannot complete immediately, leading the macro to trigger a panic [1][4]. From an implementation perspective, the eprintln! macro is defined in the Rust standard library (library/std/src/macros.rs) as follows [5][6]: macro_rules! eprintln { => { $crate::eprint!("\n") }; ($($arg:tt)) => {{ $crate::io::_eprint($crate::format_args_nl!($($arg))); }}; } It delegates to io::_eprint, which internally handles the writing process to the standard error stream [5][7][8]. The design choice to panic ensures that error messages intended for stderr are treated as critical, reflecting the assumption that failures in reporting errors should not be silently ignored [1][2].
Citations:
- 1: https://doc.rust-lang.org/std/macro.eprintln.html
- 2: https://doc.rust-lang.org/stable/std/macro.eprintln.html
- 3: https://doc.rust-lang.org/nightly/std/macro.eprintln.html
- 4: https://dev-doc.rust-lang.org/std/macro.eprintln.html
- 5: https://github.com/rust-lang/rust/blob/master/library/std/src/macros.rs
- 6: https://github.com/rust-lang/rust/blob/1.58.1/library/std/src/macros.rs
- 7: https://github.com/rust-lang/rust/blob/770b9e3012bd58bdf6046d328dabfd57df163eb6/src/libstd/io/stdio.rs
- 8: https://doc.rust-lang.org/1.64.0/src/std/macros.rs.html
Make both C-ABI diagnostics non-panicking.
eprintln! panics when writing to io::stderr fails. These helpers are reachable from extern "C" functions, where a panic cannot unwind normally and can abort the process. Use writeln! on a locked stderr handle and discard any write error.
crates/perry-runtime/src/gc/roots/shadow_stack.rs:562-567: replaceeprintln!with a fallible stderr write that ignores write errors.crates/perry-runtime/src/gc/roots/temp_roots.rs:77-81: replaceeprintln!with the same non-panicking write pattern.
📍 Affects 2 files
crates/perry-runtime/src/gc/roots/shadow_stack.rs#L562-L567(this comment)crates/perry-runtime/src/gc/roots/temp_roots.rs#L77-L81
🤖 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-runtime/src/gc/roots/shadow_stack.rs` around lines 562 - 567,
Make both C-ABI diagnostics non-panicking by replacing the eprintln! calls in
crates/perry-runtime/src/gc/roots/shadow_stack.rs:562-567 and
crates/perry-runtime/src/gc/roots/temp_roots.rs:77-81 with writeln! to a locked
stderr handle, explicitly discarding any write error. Preserve the existing
diagnostic messages and behavior otherwise.
Comment on lines
+754
to
+757
| // This test verifies objects it holds only as native-stack locals are | ||
| // COLLECTED, so the native scan must not rescue them. `Disabled` says that | ||
| // unconditionally; the default `Auto` merely resolves that way today. | ||
| let _scan = ConservativeScanDisabledGuard::new(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reload child_header after the minor collection.
This guard enables relocation. The test dereferences child_header after collect_minor_trace, but it derived that pointer before collection. TemporaryCopyOnlyRootScanner::rust_bits(&[ptr_bits(child)]) can keep the child live, but it cannot rewrite the local child_header.
Reload the child through a rewritten root or the relocated parent slot before the mark assertion. Otherwise the test can inspect reclaimed from-space.
🤖 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-runtime/src/gc/tests/oldgen.rs` around lines 754 - 757, Update
the test around ConservativeScanDisabledGuard and collect_minor_trace to reload
child_header after collection from a rewritten root or the relocated parent slot
before performing the mark assertion. Do not dereference the pre-collection
child_header after relocation; preserve the existing root-scanning setup and
assertion behavior.
Source: Learnings
Contributor
Author
|
#7145 acceptance criterion verified end-to-end (Mac mini, darwin-arm64,
That is the whole point of the issue: a profile of the runtime suite that could not complete, invisible to a release-only CI gate. Production diff is provably empty for the scan-mode change. In a non-test build the old code already selected so the shipped runtime is unchanged by it. The only production-reachable change in this PR is the two |
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.
What
Makes the perry-runtime unit-test build run production's GC root
configuration, so the suite can see the bug class it exists to catch.
gc::roots::conservative_stack_scan_mode()defaulted the test build toConservativeStackScanMode::Fullwhile production resolvesAuto -> SkipDisabled. Three consequences, all in the direction that hidesdefects:
Exactly gc: the relocating minor produces a deterministic wrong answer in the SHIPPED default on an ordinary event-loop workload #7055's shape. The suite filtered out the failure class it exists
to catch.
(
CopiedMinorFallbackReason::ConservativeStack), so relocation went largelyunexercised.
the wrong direction for "this object should have been collected" — the
tests most dependent on precision are the ones it degraded most.
The comment justifying it described how some tests were written, not a
requirement of testing.
Why it was cheap
The correct mechanism already existed and the tests already used it
(
RuntimeHandleScope/gc_temp_root_*, plus the isolation guards, which pinAutothemselves). 351 of the 531 gc tests were already running inproduction's mode. Flipping the default for the rest cost exactly one test
conversion.
The one conversion
test_minor_preserves_old_to_young_edge_across_minorsasserted the parent'sslot still held
ptr_bits(child)— the address captured before the firstminor. That only held because
Fullmade the minor non-moving. With preciseroots the copying minor relocates the child and correctly rewrites the slot:
Reading a raw local across a relocating collection is the defect class this
suite exists to catch, so the test now does what generated code does and
re-reads the child out of the parent's slot.
Probe measurement (scan off): cycles 0–2 run
copied=1 promoted=0with the RSedge intact; cycle 3 reports
promoted=1and the child leaves the nursery, atwhich point the edge is old→old and the remembered set correctly retires it. The
loop now handles that explicitly instead of asserting an RS entry that should
not exist, with an
rs_covered_cycles >= 2floor so it cannot go vacuous.Also fixes #7145
js_shadow_frame_pop's corrupted-handle guard useddebug_assert!(false, …)inside an
extern "C"fn. In a debug build the assert fires, the panic cannotunwind across the boundary, and the process aborts — so
cargo test -p perry-runtime --libin the dev profile SIGABRTed the wholetest binary. CI runs
--release, wheredebug_assert!compiles out, so thegate was structurally blind to it: an entire profile of the runtime suite was
un-runnable and no gate could go red.
Both that site and
js_gc_temp_root_push's identical (unreachable, thereforenever-caught) overflow guard now use the
report_growth_stub_skipped_below_heap_minpattern —#[cold], one line onstderr, once per process, so it cannot perturb a stdout parity comparison. Skip
behaviour is unchanged in both profiles.
Evidence (Mac mini, darwin-arm64, release,
--test-threads=1)PERRY_CONSERVATIVE_STACK_SCAN=0The red-then-green is the point: the scan-off arm failed before the conversion
and passes after.
Sabotage-verified both directions — injecting
remembered_set_clear()after theminor (simulating the #6181 dropped edge) turns the converted test red in both
scan modes, so it still catches the bug it was written for.
ConservativeScanAutoGuardis deleted: with the default alreadyAutoit setthe value it was going to get anyway and could no longer fail. Per CLAUDE.md's
kill-policy a mode that cannot be exercised is deleted, not left for a future
bisect to trust. Its single user moves to
ConservativeScanDisabledGuard, whichasserts the stronger property it actually needs.
Unrelated, folded in because it blocks every PR
crates/perry-codegen/src/linker.rs:96landed with a rustfmt violation in#7135, so
cargo fmt --all -- --checkis red onmainas of a3b31c0 andlintfails on every open PR. One rustfmt-only commit here unblocks it.Not in this PR
Removing
ConservativeStackScanMode::Full, the sixforce_full_scan()sitesand the
PERRY_CONSERVATIVE_STACK_SCANknob is a production behaviourchange and is deliberately separate — see the follow-up. This PR is test-build
only.
Worth recording here:
PERRY_CONSERVATIVE_STACK_SCAN=fullfails 134 of 1574runtime tests on
maintoday (verified against pristinemainroots.rs, sonot caused by this PR). The env value beats the per-thread override, so the
isolation guards cannot opt back out. That is an untested shipped
configuration — and the
gc_ratchetbaseline is measured under it.Closes #7145
Summary by CodeRabbit
Bug Fixes
Tests
Maintenance