gc: whole-heap from-space scan, independent of the rewrite pass (#7035) - #7041
Merged
Merged
Conversation
PERRY_GC_VERIFY_EVACUATION walks the same surfaces the rewrite pass walks, so it can only check that the rewrite pass agreed with itself: a holder the rewrite pass never enumerated is invisible to both. On the #7022 reproducer it reports clean while the program dies of malloc-heap corruption. Add PERRY_GC_FROMSPACE_SCAN, which asks a question that depends on no root enumeration: after the copying minor rewrite pass and before from-space is reset, does any live object OUTSIDE from-space still contain a word that decodes to a from-space address? It walks the arena census plus the malloc-tracked registry and decodes every payload word through decode_root_word, the decoder the mark and rewrite paths already share. From-space objects are skipped -- a dead object legitimately still points at its dead peers. Offenders are split along two axes with different fixes: missing rewrite (target is FORWARDED -- it moved and the reference was not updated) vs dangling (target was never evacuated); and remembered-set coverage of the slot page -- never_dirty (a store path skipped the barrier), lost_dirty (the edge was recorded then lost), dirty_but_missed. The coverage split reuses the existing EVER_DIRTY_OLD_PAGES tracking, now enabled by this scan too. Debug-only and off by default (O(live heap) per collection). PERRY_GC_FROMSPACE_SCAN_ABORT=1 aborts on the first offender. Teeth verified by sabotage: blinding the from-space predicate turns two of the three unit tests red with the exact planted=0 signature. Claude-Session: https://claude.ai/code/session_01G4k1vE6PVb53m2dRZ2aDtv
📝 WalkthroughWalkthroughAdds an opt-in whole-heap from-space scan to the copying minor GC, including remembered-set diagnostics, offender reporting, abort support, integration before from-space reset, documentation, and targeted tests. ChangesFrom-space scan verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CopyingMinorGC
participant FromSpaceScan
participant BarrierTracking
participant HeapObjects
CopyingMinorGC->>FromSpaceScan: run_fromspace_scan()
FromSpaceScan->>HeapObjects: scan arena and malloc-tracked objects
HeapObjects-->>FromSpaceScan: payload words
FromSpaceScan->>BarrierTracking: query dirty-page coverage
FromSpaceScan-->>CopyingMinorGC: emit clean or OFFENDERS report
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 |
proggeramlug
force-pushed
the
fix/7022-moving-minor-precise-root-gap
branch
from
July 30, 2026 05:03
6f7918c to
3ea0380
Compare
![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 `@crates/perry-runtime/src/gc/fromspace_scan.rs`:
- Around line 267-273: Update report_and_abort to call std::process::abort()
after emit_report(report, "abort") instead of panicking, ensuring
PERRY_GC_FROMSPACE_SCAN_ABORT always terminates unconditionally.
🪄 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: 4160590e-7105-4e20-82da-541af9a5b654
📒 Files selected for processing (7)
changelog.d/7041-gc-whole-heap-fromspace-scan.mdcrates/perry-runtime/src/gc/barrier.rscrates/perry-runtime/src/gc/copying.rscrates/perry-runtime/src/gc/fromspace_scan.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/tests/fromspace_scan.rscrates/perry-runtime/src/gc/tests/mod.rs
Comment on lines
+267
to
+273
| fn report_and_abort(report: &FromSpaceScanReport) -> ! { | ||
| emit_report(report, "abort"); | ||
| panic!( | ||
| "gc from-space scan: {} missing rewrite(s), {} dangling reference(s) survived the rewrite pass", | ||
| report.missing_rewrites, report.dangling | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether this GC cycle can run underneath a catch_unwind boundary,
# and whether the workspace uses panic="abort" anywhere.
rg -n 'panic\s*=\s*"abort"' -g '*.toml'
rg -n 'catch_unwind' --type rustRepository: PerryTS/perry
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -u
echo "Tracked GC fromspace_scan files:"
git ls-files | rg '(^|/)fromspace_scan\.rs$|(^|/)change|CHANGE|changelog|from-space|from_space' || true
echo
echo "File size and relevant context:"
wc -l crates/perry-runtime/src/gc/fromspace_scan.rs
sed -n '1,80p' crates/perry-runtime/src/gc/fromspace_scan.rs
sed -n '240,290p' crates/perry-runtime/src/gc/fromspace_scan.rs
echo
echo "Search for reported symbol/context:"
rg -n 'report_and_abort|FromSpaceScanReport|missing_rewrites|dangling|PERRY_GC_FROMSPACE_SCAN_ABORT|from-space scan|from_space_scan|emit_report|std::process::abort|panic!' crates/perry-runtime/src --type rust || trueRepository: PerryTS/perry
Length of output: 27242
🏁 Script executed:
#!/bin/bash
set -u
echo "Relevant from-space scan call site:"
sed -n '180,210p' crates/perry-runtime/src/gc/fromspace_scan.rs
sed -n '1180,1210p' crates/perry-runtime/src/gc/copying.rs
sed -n '70,90p' crates/perry-runtime/src/gc/mod.rs
echo
echo "Runtime panic settings across toml files:"
rg -n 'panic\s*=' . --glob '*.toml' || true
echo
echo "panic behavior in the codebase (excluding generated/dist/build likely dirs):"
rg -n 'std::process::abort\(|panic!' crates/perry-runtime/src --type rust | rg 'perry-runtime/src/(gc|exception|process|typed_feedback|object/field_get_set|process/env_misc|abi_trampoline).*:|^crates/perry-runtime/src/gc/fromspace|crates/perry-runtime/src/gc/copying:|^crates/perry-runtime/src/gc/verify:' || true
echo
echo "Focused Rust unwind/catch_unwind search in runtime:"
rg -n 'catch_unwind|AssertUnwindSafe|LocalHook|panic|Panic' crates/perry-runtime/src --type rust || trueRepository: PerryTS/perry
Length of output: 24545
🏁 Script executed:
#!/bin/bash
set -u
echo "Top-level Cargo profile settings:"
sed -n '80,105p' Cargo.toml
sed -n '215,230p' crates/perry-runtime/Cargo.toml
echo
echo "gc policy panic settings around comment:"
sed -n '300,325p' crates/perry-runtime/src/gc/mod.rs
echo
echo "scan_heap_for_fromspace_refs callers:"
rg -n 'scan_heap_for_fromspace_refs|run_fromspace_scan|PERRY_GC_FROMSPACE_SCAN' --type rust .Repository: PerryTS/perry
Length of output: 5346
Use an unconditional abort for PERRY_GC_FROMSPACE_SCAN_ABORT.
report_and_abort() calls panic!() while the GC docs say this flag should always cause a debugger/crash-reportable abort. Since the release profile uses panic = "unwind", any unwind boundary above this diagnostics path can swallow the panic; use std::process::abort() after emitting the report to preserve the intended behavior.
🛠️ Proposed fix
fn report_and_abort(report: &FromSpaceScanReport) -> ! {
emit_report(report, "abort");
- panic!(
+ eprintln!(
"gc from-space scan: {} missing rewrite(s), {} dangling reference(s) survived the rewrite pass",
report.missing_rewrites, report.dangling
);
+ std::process::abort();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn report_and_abort(report: &FromSpaceScanReport) -> ! { | |
| emit_report(report, "abort"); | |
| panic!( | |
| "gc from-space scan: {} missing rewrite(s), {} dangling reference(s) survived the rewrite pass", | |
| report.missing_rewrites, report.dangling | |
| ); | |
| } | |
| fn report_and_abort(report: &FromSpaceScanReport) -> ! { | |
| emit_report(report, "abort"); | |
| eprintln!( | |
| "gc from-space scan: {} missing rewrite(s), {} dangling reference(s) survived the rewrite pass", | |
| report.missing_rewrites, report.dangling | |
| ); | |
| std::process::abort(); | |
| } |
🤖 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/fromspace_scan.rs` around lines 267 - 273, Update
report_and_abort to call std::process::abort() after emit_report(report,
"abort") instead of panicking, ensuring PERRY_GC_FROMSPACE_SCAN_ABORT always
terminates unconditionally.
proggeramlug
added a commit
that referenced
this pull request
Jul 30, 2026
…ce offenders by snapshot coverage (#7043) Two diagnostics salvaged from the #7022 investigation, both landable on their own. 1. js_array_grow skips installing the growth forwarding stub when the array sits below the platform heap floor. That is an address-conditional silent divergence: behaviour depends on where the allocator happened to place memory, with no signal. Now a debug_assert plus a once-per-process stderr line. This has already cost debugging time. An experiment replacing arena blocks with mmap'd guard-paged blocks used a NULL hint, landed below macOS's 2 TB floor, silently disabled every growth stub, and came back falsely clean -- it was only caught by re-running with a high MAP_FIXED hint. 2. The from-space scan (#7041) now splits offenders by whether their page was in this cycle's dirty snapshot: not_in_snapshot -- the in-cycle remembered-set scan never looked at the page in_snapshot -- the scan looked and still did not rewrite the slot That split is what established #7022's failure as in-cycle, rather than an edge recorded and later dropped. Refs #7022, #7041. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Contributor
Author
|
Follow-up from #7022 (fixed in #7050): on that reproducer the scan's offender population is ~99% structural noise, and the I extended
Suggested change: keep the whole-heap walk (that part works and is the point of #7035), but
With that split, the population that is a defect — live owner, enumerated slot — reads zero on this reproducer both before and after #7050, which is the measurement that eliminated the missing-rewrite hypothesis and sent the investigation to Deliberately not folded into #7050: it would have invalidated that PR's matrix run, and a measurement-tool change does not belong in the same diff as a collector fix. |
This was referenced Aug 1, 2026
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
Adds
PERRY_GC_FROMSPACE_SCAN— a debug-only, whole-heap verification pass that answers, after the copying minor's rewrite pass and before from-space is reset:This lands on its own merit, independent of #7022. It is not a fix for that crash and does not depend on it.
Why — we now have four measurement instruments with documented holes
PERRY_GC_TRACEgc_repsel_matrix.sh --pressuremoved=counterPERRY_GC_VERIFY_EVACUATIONThe last one is what this PR addresses. The existing verifier walks the same surfaces the rewrite pass walks —
visit_shadow_stack_root_slots,visit_global_root_slots, and the registered side-table scanners. Both derive from one enumeration, so a holder the rewrite pass never knew about is invisible to the verifier too. It is a self-consistency check, not independent evidence.That is not hypothetical: on the #7022 reproducer the verifier reports clean while the program dies of malloc-heap corruption. I read "verifier clean" as evidence against a missing rewrite and spent several bisect cycles on other hypotheses because of it. A verification tool that silently under-reports is worse than one that is absent.
How
Walks the whole heap — the arena census (
Address-orderArenaObjectCursor, covering Eden / both survivor semispaces / old / longlived) plus the malloc-tracked registry — and decodes every payload word throughdecode_root_word, the decoder the mark and rewrite paths already share (#6910). Objects that are themselves in from-space are skipped: a dead object legitimately still points at its dead peers, and on the #7022 reproducer from-space holds tens of thousands of them.Offenders are classified along two axes, because they have different fixes:
GC_FLAG_FORWARDED— it moved and this reference was not updated) vs dangling (target was never evacuated and is about to be recycled);never_dirty(a store path skipped the write barrier),lost_dirty(the edge was recorded and then lost),dirty_but_missed(the page is dirty and the slot was still missed). This reuses the existingEVER_DIRTY_OLD_PAGEStracking, whose own comment already calls this "the decisive split for the missing old→young edge bug"; it is now enabled by this scan as well as byPERRY_GC_VERIFY_EVACUATION.PERRY_GC_FROMSPACE_SCAN_ABORT=1aborts on the first offender so a crash report captures the offending cycle rather than the downstream corruption.O(live heap) per collection, so it is strictly opt-in and off by default. New module rather than an addition to
copying.rs, which is close to the 2000-linecheck_file_size.shcap.It works
On the #7022 reproducer it is clean on the first collection and then names the holders:
i.e. old-gen arrays (
GC_TYPE_ARRAY) whose element slots still point at evacuated nursery objects, withlost_dirtydominating — old→young edges that were recorded and then dropped. The evacuation verifier reports clean on the same runs.Corroboration:
gc::tests::copying::large_object_old_born_array_slot_write_keeps_young_child_aliveis already red onmainand asserts exactly this invariant.Validation
cargo test -p perry-runtime fromspace_scan— 3 passed.cargo test -p perry-runtime gc::— no new failures. Two pre-existing failures verified red at pristinemainin the same worktree first:copying::large_object_old_born_array_slot_write_keeps_young_child_alive(real, and related — see above) and theteardown::map_set_*family (flaky; a different subset fails per run).cargo fmt -p perry-runtimeclean. (cargo fmt --all --checkis red onmainforperry-codegen/src/loop_purity.rs, untouched here.)Teeth, verified in both directions
Three unit tests: the scan finds a planted un-rewritten old→young reference; it stops reporting once the reference is removed; it separates dangling from missing-rewrite; and it ignores references held by from-space objects. Assertions are deltas, since a test process shares one thread-local heap.
Teeth verified by sabotage rather than assumed — blinding the from-space predicate turns two of the three red with the exact signature the real defect would produce:
The third asserts absence and correctly still passes under sabotage.
Refs #7035, #7022.
Summary by CodeRabbit
New Features
Tests