feat(gc): RuntimeHandle::across_* and a raw-handle debt ratchet - #7389
Merged
Conversation
added 2 commits
August 4, 2026 19:14
The layer-3 half of the rooting-by-construction RFC. A RuntimeHandleScope gives an object liveness -- the collector marks it and rewrites the slot -- and does nothing for a raw pointer already read out of that slot. That copy is invisible to the collector, and if the object moves it names from-space. Every rooting bug fixed in the #7341 sweep had rooting ALREADY. What was missing each time was ordering the re-read against the collection point. across_* runs the allocating call and returns the post-collection address in one step, so the pre-call pointer is never bound. The .size arm of js_object_get_field_by_name is converted as the worked example. This is a debt counter, NOT a soundness proof, and the script says so. Rust has no effect system to mark "may allocate", so no signature can reject holding a stale copy across such a call, and a &mut Heap token cannot cross extern "C". The ratchet instead makes the unconverted count visible and monotonically decreasing: 1006 sites across 110 files, wired into test.yml beside the address-classification audit. Checked against negative controls, per the four-ways-a-gate-cannot-fail rule: sabotaging across_mut to return the pre-call pointer fails the test with the intended message; the ratchet fails on a rise and refuses to raise its own baseline; and --self-test asserts the matcher still fires on the shapes it counts and still ignores across_*, so a broken matcher cannot report zero and pass forever. Also drops a redundant republish in the .size arm that rustc flagged as assigned-never-read (introduced by #7385, superseded by the arm-level one).
📝 WalkthroughWalkthroughThe change adds ChangesGC-safe runtime handles
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant RuntimeHandle
participant MovingGC
Caller->>RuntimeHandle: invoke across_const
RuntimeHandle->>Caller: run own_key_present
Caller->>MovingGC: trigger allocation and collection
MovingGC-->>RuntimeHandle: update rooted receiver
RuntimeHandle-->>Caller: return result and refreshed pointer
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 1
🧹 Nitpick comments (1)
crates/perry-runtime/src/gc/tests/copying.rs (1)
1087-1092: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the refreshed handle value.
assert_ne!only proves thatfreshdiffers fromstale. A changed but unrelated value also passes. Comparefresh.to_bits()withhandle.get_nanbox_f64().to_bits().Proposed test strengthening
assert_ne!( fresh.to_bits(), stale, "across_nanbox returned the stale value -- the combinator is not re-reading" ); + assert_eq!( + fresh.to_bits(), + handle.get_nanbox_f64().to_bits(), + "across_nanbox must agree with a fresh read of the same handle" + );🤖 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/copying.rs` around lines 1087 - 1092, Update the assertion in the copying test around assert_copied_minor_trace to compare fresh.to_bits() directly with handle.get_nanbox_f64().to_bits(), replacing the weaker stale-value inequality while preserving the existing diagnostic context.
🤖 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 @.github/workflows/test.yml:
- Around line 198-200: Update the workflow step invoking
scripts/raw_handle_debt.py to compare scripts/raw_handle_debt_baseline.txt
against the PR base revision’s baseline, rather than only the checked-out PR
value. Fail CI when the current baseline is higher, while allowing unchanged or
reduced values; preserve the existing self-test and debt-count checks.
---
Nitpick comments:
In `@crates/perry-runtime/src/gc/tests/copying.rs`:
- Around line 1087-1092: Update the assertion in the copying test around
assert_copied_minor_trace to compare fresh.to_bits() directly with
handle.get_nanbox_f64().to_bits(), replacing the weaker stale-value inequality
while preserving the existing diagnostic context.
🪄 Autofix
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: 825d8fe4-a2f7-4c90-ab15-3d4424f37fe2
📒 Files selected for processing (7)
.github/workflows/test.ymlchangelog.d/7389-raw-handle-across.mdcrates/perry-runtime/src/gc/roots/runtime_handles.rscrates/perry-runtime/src/gc/tests/copying.rscrates/perry-runtime/src/object/field_get_set/get_field_by_name.rsscripts/raw_handle_debt.pyscripts/raw_handle_debt_baseline.txt
Comment on lines
+198
to
+200
| run: | | ||
| python3 scripts/raw_handle_debt.py --self-test | ||
| python3 scripts/raw_handle_debt.py |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject direct increases to the committed baseline.
Line 200 compares the current count only with the baseline from the same PR checkout. A PR can add get_raw_*_ptr reads and change scripts/raw_handle_debt_baseline.txt from 1006 to the new total. CI then passes without calling the guarded --update path.
Compare the current baseline with the PR base revision. Fail if it increased. Allow only an unchanged or lower baseline.
🤖 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 @.github/workflows/test.yml around lines 198 - 200, Update the workflow step
invoking scripts/raw_handle_debt.py to compare
scripts/raw_handle_debt_baseline.txt against the PR base revision’s baseline,
rather than only the checked-out PR value. Fail CI when the current baseline is
higher, while allowing unchanged or reduced values; preserve the existing
self-test and debt-count checks.
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
* docs(plan): fold in the 2026-08-04 findings Two things this plan treated as measured were not. Statepoints could not compile on aarch64-ELF at all -- a hard failure on a default-on path, from two stacked bugs (#7390: the compact stack-map parser did not model GNU-as `sym = expr`, emitted only at -O3 and only on ELF; and the assembler was not told the -mcpu the code generator was told, so Graviton-emitted SVE was rejected) behind two toolchain ones (#7384, #7388). And three of the four RS4GC matrix arms had NEVER executed, in any run, for want of a concurrency group (#7393). Every "the ELF arm is the only one red" conclusion rested on arms that never reached a runner. That is a fifth way a gate cannot fail, and it is now written down. Also folded in: nine Layer 3 rooting fixes and the rule they share (ordering, not missing roots; a fault that MOVES is a real fix, one that does not move by a byte was already dead before you rooted it); #7380's type confusion and the `gc_type == GC_TYPE_OBJECT` generalisation; RSS -69% (#7377); and the first honest performance measurement -- two benchmarks that measure nothing (#7395) and the array-store guard's siting cost (#7396). The Layer 1 framing is corrected: lower_exprs_rooted already implements the RFC's proposal for codegen operands, gated on any_later_ref_may_trigger_gc, and all four arms of func_ref.rs use it. So the gap is Layer 3, where #7389 supplies the first structural answer. * docs: name the fragment for its real PR (#7397) --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
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.
The layer-3 half of the argument in
docs/src/internals/rfc-rooting-by-construction.md(#7378), which found that three of four sampled bugs lived inperry-runtimewhereRuntimeHandleScopeexists but is optional.The defect shape this targets
A
RuntimeHandleScopegives an object liveness — the collector marks it and rewrites the slot. It does nothing for a raw pointer already read out of that slot. That copy is invisible to the collector, and if the object moves it names from-space.Every rooting bug fixed in the #7341 sweep — #7373, #7374, #7375, #7376, #7381, #7383, #7385 — had rooting already. What was missing every time was ordering the re-read against the collection point:
The defect is not a missing root. It is that
objis still nameable after the call.The
.sizearm ofjs_object_get_field_by_nameis converted as the worked example.What this is not
Not a soundness proof, and the script says so in its own docstring. Rust has no effect system to mark "this call may allocate", so no signature can reject holding a stale copy across one — and a
&mut Heaptoken cannot be threaded acrossextern "C"boundaries. Making the accessorsunsafewould add no friction either, since runtime code is alreadyunsafe-heavy.What is achievable is making the correct shape shorter than the incorrect one, and making the remaining count visible and monotonically decreasing: 1006 sites across 110 files, wired into
test.ymlbeside the address-classification audit.Checked against its own negative controls
Per CLAUDE.md's four-ways-a-gate-cannot-fail rule, each assertion was verified to be capable of failing:
across_mutre-reads::error::--updatewith a rise--self-testacross_*That last one matters most: a ratchet whose regex stops matching reports 0 and passes forever.
Also
Drops a redundant republish in the
.sizearm that rustc flagged as assigned-never-read — I introduced it in #7385 and the arm-level republish supersedes it.Summary by CodeRabbit
Bug Fixes
.sizeaccess for Map and Set subclasses in cases involving memory allocation.Tests
Documentation
Chores