fix(gc): pin long-lived and malloc-resident objects without the space classifier (#7650) - #7655
Merged
Merged
Conversation
added 2 commits
August 8, 2026 19:30
… classifier #7650 routed every GC_FLAG_PINNED write through gc::pin_object, which reaches arena::classify_heap_space. That new edge kept a reference chain alive that -Wl,-dead_strip had been removing, and five perry-ext-* crates stopped linking: Undefined symbols for architecture arm64: _js_blob_new, _js_fetch_with_options, _js_fetch_notify_signal_aborted perry-ext-{pdf,lru-cache,node-forge,mongodb,http} all failed. Bisected: the commit before #7650 builds them clean, #7650 does not, and reverting just the two perry-runtime call sites restores the link. perry-stdlib's async_bridge keeps pin_object -- its promises really are Eden-resident and must arm the latch. The two reverted sites are documented by #7650 itself as long-lived and malloc-resident, so they never needed the classifier. pin_object_non_young does the flag write directly, debug_asserts the claim, and has a unit test asserting it for each real call site plus a control proving the predicate is not vacuously false. Not visible per-PR: cargo-test scopes to the changed crates' reverse-dependency closure, and the full workspace runs on tags and nightly only. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
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
pushed a commit
that referenced
this pull request
Aug 8, 2026
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
proggeramlug
pushed a commit
that referenced
this pull request
Sep 29, 2026
…on-young pin stays arena-free The pinned-summary block walk no longer casts headers itself: the linear block iteration the arena walkers share is one function in arena/walk.rs (for_each_block_header), used by the filtered and block-index walkers and by the pinned walk. The addr_class allowlist entry for arena/pinned.rs is gone. pin_object_non_young must stay as light as #7655 made it (#7650: it is kept by the feature-stripped perry-ext-* links). It no longer reaches note_pinned_arena_header: a non-leaf tenured arena pin made through it sets a leaf thread-local (TENURED_PIN_UNPLACED), and the next full root scan walks every tenured block once, which places the pin in its block summary. A minor leaves the bit set; it does not act on tenured objects. Test: full_mark_traces_through_every_non_young_pin (born-tenured, old, malloc parents pinned through pin_user_ptr_non_young); red with the placement sabotaged.
proggeramlug
added a commit
that referenced
this pull request
Sep 29, 2026
…after-free in cross-thread promises) (#11664) * gc: a pinned object is a root, marked and traced like any other Every mark entry treated GC_FLAG_PINNED as already marked, so a pinned object was never traced and a child reachable only through it was freed: js_promise_new_cross_thread lost its reaction closure after one full. A pin now means only no move, no sweep. Pinned objects are rooted by a scanner that finds them through the header bit plus a per-block pinned_summary (arena) and a malloc-registry summary, set only by the pin setters. Block persistence no longer counts a pinned header as live. * gc: the copying minor traces a pinned malloc parent; pinned-root matrix complete A pinned gc_malloc parent lost its young child under a copying minor and a forced evacuation: the copying minor marked malloc and long-lived objects only when neither MARKED nor PINNED was set, so the pinned parent reached through the pin scanner was never scanned. A malloc parent is never remembered by the write barrier, so that scan was its only cover. The same PINNED-as-marked early-out is removed from the incremental mark barrier and the budgeted cycle block persistence. The born-tenured and old controls that failed under a minor were a test bug: the copying-nursery isolation guard empties the scanner registry, and without the shape table scanner an old parent keeps its forwarded slot but loses the young keys array that names it. The pinned-root tests now register it, cover every birth under full, minor and forced evacuation, assert a pinned parent is never moved, and check the full trace exactly by running its root scan and mark worklist (sabotage of either arm turns it red). * gc_runtime_root_holders: re-audit PASS1_MARKED after the block-persistence pin change * addr_class allowlist: the pinned-summary block walk reads headers by linear block iteration * gc: pinned-root walk reads headers through the shared block walker; non-young pin stays arena-free The pinned-summary block walk no longer casts headers itself: the linear block iteration the arena walkers share is one function in arena/walk.rs (for_each_block_header), used by the filtered and block-index walkers and by the pinned walk. The addr_class allowlist entry for arena/pinned.rs is gone. pin_object_non_young must stay as light as #7655 made it (#7650: it is kept by the feature-stripped perry-ext-* links). It no longer reaches note_pinned_arena_header: a non-leaf tenured arena pin made through it sets a leaf thread-local (TENURED_PIN_UNPLACED), and the next full root scan walks every tenured block once, which places the pin in its block summary. A minor leaves the bit set; it does not act on tenured objects. Test: full_mark_traces_through_every_non_young_pin (born-tenured, old, malloc parents pinned through pin_user_ptr_non_young); red with the placement sabotaged. * changelog: name the fragment after #11664 --------- Co-authored-by: Ralph Küpper <ralph3@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.
fix(gc): pin long-lived and malloc-resident objects without the space classifier (#7650 follow-up).#7650 routed every
GC_FLAG_PINNEDwrite throughgc::pin_object, which reachesarena::classify_heap_space. That new edge kept a reference chain alive that-Wl,-dead_striphad been removing, and fiveperry-ext-*crates stoppedlinking:
perry-ext-{pdf,lru-cache,node-forge,mongodb,http}. They link a feature-strippedruntime through
perry-ffi'sruntime-link, so those thunks have no definitionand only survived because the stripper removed them.
Bisected rather than guessed: the commit before #7650 builds all five clean,
#7650 does not, and reverting only the two
perry-runtimecall sitesrestores the link.
perry-stdlib'sasync_bridgekeepspin_object— itsjs_promise_new()promises really are Eden-resident and must arm the young-pinlatch.
pin_object_non_youngdoes the flag write directly for the sites #7650's owncomments already document as long-lived (
string/format.rs, the interned formatbuffer) and malloc-resident (
thread.rs, the spawn promise and its handle) —they never needed the classifier. Making
pin_objectconservative instead(arming for any
GC_FLAG_ARENAobject) would also remove the edge, but it wouldarm on exactly these long-lived pins and throw away the preflight skip #7645
bought.
The claim is checked, not asserted.
debug_assertcatches a young object intest builds, and
pin_object_non_young_call_sites_are_never_young(
gc/tests/copying/latch.rs) asserts non-youngness for each real call site plusa control proving the predicate is not vacuously false for everything. Sabotage:
forcing the predicate false reddens the control (0 compile errors, test binary
reached).
Why no gate caught it.
cargo-testscopes per-PR runs to the changed crates'reverse-dependency closure (
scripts/ci_test_scope.py); the full workspace runson tags and nightly only.
perry-ext-*is outside the closure of aperry-runtimeGC change, so this could only have surfaced at the next tag.Found by running
cargo test --release --workspaceby hand againstmain.