fix(gc): split the dominance checker's heap-source predicate by movability (#7210) - #7235
Conversation
…ility `_is_heap_source` answered "does the collector REWRITE this location?" when the reportable question is "can the OBJECT this register names go bad while it sits in unrooted memory across a collection?". #7210 measured the consequence: every one of the 66 `--unrooted-allocas --moving-only` hits was a false positive, so the population could never reach 0 and #7198's promote-to-required clock could not start. The split models the two independent ways a held address goes bad -- the object MOVES, or the object is RECLAIMED -- and exempts a source only when both are false. Two exemptions, each with its premises machine-checked by a new `--audit-immovable-sources` and each reversible by its own flag so #7210's counterfactuals stay one command away instead of becoming tribal knowledge: class-keys `@perry_class_keys_*` / `js_build_class_keys_array`. Not movable: allocated through `js_array_alloc_with_length_longlived` (old arena), and the only thing that relocates an old-arena object is old-page defrag, which `select_old_page_defrag_pages` short-circuits off (#6206). Not reclaimable: the global is registered with `js_gc_register_global_root`. Re-report with `--assume-old-defrag`. box `js_box_alloc*`. Not movable: `std::alloc::alloc`, outside the GC heap -- `scan_box_roots_mut` rewrites the JSValue INSIDE the box, never the box's address. Not reclaimable: no dealloc, and `BOX_REGISTRY` is monotonic. Re-report with `--assume-boxes-in-gc-heap`. The exemptions are one-sided in the UNSAFE direction: an unlisted source keeps the old conservative classification, so forgetting one costs a false positive rather than a missed bug. Because an exemption is a suppression, it gets the strictest treatment in the file. `--self-test` now asserts, for each: it fires; its knob exactly reverses it; and -- the arm that matters -- a structurally IDENTICAL fixture whose only difference is a NURSERY allocator is still reported, under `--moving-only` and under both knobs. If a future widening ever swallows that, the exemption has stopped being about the allocator and become a rule about code shape. `--unrooted-allocas` also prints what each exemption suppressed, so "the corpus is clean" and "the corpus is entirely exempted" no longer print the same line. Measured over a freshly generated 134-file corpus (116 sources) at c9cd73b, `--unrooted-allocas --moving-only`: 98 before, 2 after, 96 suppressed (93 class-keys + 3 box). The count is above #7210's 66 because both the corpus and `ALLOC_RE` grew since -- #7227 added the `*_new*` convention, which is what makes the 2 residuals visible at all. Refs #7210, #7198, #7202, #7154.
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 selected for processing (1)
📝 WalkthroughWalkthroughThe checker now models heap-source movability and reclamation, validates immovable-source exemptions, supports audit and counterfactual modes, reports suppressed findings, and runs exemption audits in the GC workflow. ChangesGC source exemption analysis
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GCWorkflow
participant CLI
participant ImmovableSourceAudit
participant UnrootedScan
GCWorkflow->>CLI: run --audit-immovable-sources
CLI->>ImmovableSourceAudit: validate exemption premises
CLI->>UnrootedScan: pass source options
UnrootedScan-->>CLI: return findings and exemption counts
CLI-->>GCWorkflow: report audit or scan status
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/gc_root_dominance_check.py (1)
2112-2122: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAn exempted store suppresses every later store into the same alloca.
The exempted branch sets
reported = True. At Line 2121 the outer loop then breaks out offor st in stores[reg]. So if one store into%slotcomes from an exempt source and a later store into the same%slotcomes from a genuine hazardous source, the hazardous store is never examined.The existing fixtures each hold a single store, so the self-tests do not cover this shape. Use a separate flag for the exempted case so the store loop continues.
🐛 Proposed fix: stop the loads loop without ending the store loop
if not hazardous: if exempt_counts is not None: for k in sorted(set(exemptions)): exempt_counts[k] += 1 - reported = True break out.append(v) reported = True breakConsider adding a fixture with two stores into one slot — one exempt, one nursery-allocated — so the self-test covers it.
🤖 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 `@scripts/gc_root_dominance_check.py` around lines 2112 - 2122, Update the store-processing logic around the hazardous/exempt branch to use a separate flag for stopping the loads loop without setting the outer store-loop termination flag. Exempt stores should still update exempt_counts and stop processing their current load path, but the loop over stores[reg] must continue so later hazardous stores are examined. Add a regression fixture covering two stores to the same alloca, with one exempt source followed by one nursery-allocated source.
🧹 Nitpick comments (3)
scripts/gc_root_dominance_check.py (3)
2596-2603: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the knob set instead of restating it.
knobsrepeats the keys of theassumeddict inclassify_heap_source(Lines 1963-1964). Two literals must stay in sync. Extract one module-level constant and use it in both places, so a new knob cannot be declared in one and missed in the other.♻️ Proposed change: single source for the knob names
+ASSUMPTION_KNOBS = ("assume_old_defrag", "assume_boxes_in_gc_heap")- knobs = {"assume_old_defrag", "assume_boxes_in_gc_heap"} + knobs = set(ASSUMPTION_KNOBS)🤖 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 `@scripts/gc_root_dominance_check.py` around lines 2596 - 2603, Extract the knob names currently defined in classify_heap_source’s assumed dict into a shared module-level constant, then use that constant both to build assumed and in the self-test loop around IMMOVABLE_SOURCES. Remove the duplicated local knobs set so adding a knob in one place automatically keeps classification and exemption validation synchronized.
2817-2854: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe audit short-circuit bypasses the disarmed-knob checks.
--audit-immovable-sourcesreturns at Line 2822, above the validation at Lines 2850-2854. So--audit-immovable-sources --assume-old-defragruns the audit and ignores the assume flag. That is the exact case the file rejects everywhere else. Move the two assume-flag checks above the standalone audit dispatch, or addns.audit_immovable_sourcesto the rejected combinations.🤖 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 `@scripts/gc_root_dominance_check.py` around lines 2817 - 2854, The standalone audit dispatch in the argument-validation flow returns before validating the disarmed assume flags. Move the --assume-old-defrag and --assume-boxes-in-gc-heap checks above the audit_alloc_re/audit_immovable_sources short-circuit, or include audit_immovable_sources in their rejection condition, so those flags are rejected unless --unrooted-allocas is active.
2216-2216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe exemption-count path is not exercised by any self-test.
_scan_unrootednever passesexempt_counts, socheck_func_unrooted_allocasalways takes theexempt_counts is not Nonefalse branch during--self-test. The counting code at Lines 2113-2115 and the reporting block at Lines 2999-3006 run only in production. The report is the thing that separates "clean corpus" from "fully exempted corpus", so it deserves an arm.♻️ Proposed change: thread the counters through the scan helper
-def _scan_unrooted(paths, moving_only=False, **source_opts): +def _scan_unrooted(paths, moving_only=False, exempt_counts=None, **source_opts):for v in check_func_unrooted_allocas(mod, f, moving_only, poll_reaching, - source_opts) + source_opts, exempt_counts)Then assert in
self_test()that the class-keys fixture yields{"class-keys": 1}and the box fixture yields{"box": 1}.Also applies to: 2234-2235
🤖 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 `@scripts/gc_root_dominance_check.py` at line 2216, Update _scan_unrooted to accept and pass through an exempt_counts mapping to check_func_unrooted_allocas, then update self_test() to provide counters and assert the class-keys fixture yields {"class-keys": 1} while the box fixture yields {"box": 1}, exercising both counting and reporting paths.
🤖 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.
Outside diff comments:
In `@scripts/gc_root_dominance_check.py`:
- Around line 2112-2122: Update the store-processing logic around the
hazardous/exempt branch to use a separate flag for stopping the loads loop
without setting the outer store-loop termination flag. Exempt stores should
still update exempt_counts and stop processing their current load path, but the
loop over stores[reg] must continue so later hazardous stores are examined. Add
a regression fixture covering two stores to the same alloca, with one exempt
source followed by one nursery-allocated source.
---
Nitpick comments:
In `@scripts/gc_root_dominance_check.py`:
- Around line 2596-2603: Extract the knob names currently defined in
classify_heap_source’s assumed dict into a shared module-level constant, then
use that constant both to build assumed and in the self-test loop around
IMMOVABLE_SOURCES. Remove the duplicated local knobs set so adding a knob in one
place automatically keeps classification and exemption validation synchronized.
- Around line 2817-2854: The standalone audit dispatch in the
argument-validation flow returns before validating the disarmed assume flags.
Move the --assume-old-defrag and --assume-boxes-in-gc-heap checks above the
audit_alloc_re/audit_immovable_sources short-circuit, or include
audit_immovable_sources in their rejection condition, so those flags are
rejected unless --unrooted-allocas is active.
- Line 2216: Update _scan_unrooted to accept and pass through an exempt_counts
mapping to check_func_unrooted_allocas, then update self_test() to provide
counters and assert the class-keys fixture yields {"class-keys": 1} while the
box fixture yields {"box": 1}, exercising both counting and reporting paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d3aaa2d-5483-4fbb-84a7-90d551395560
📒 Files selected for processing (3)
.github/workflows/gc-root-dominance.ymlchangelog.d/7235-gc-heap-source-movability.mdscripts/gc_root_dominance_check.py
|
Filed the one residual as #7236 — |
Review of my own change: `check_func_unrooted_allocas` counted an exemption and then set `reported`, which ends the store loop for that alloca. One slot written from an exempt source AND from a nursery allocator would therefore have its real store never examined -- the accounting turning into a missed hazard, which is exactly the failure mode the exemption machinery exists to avoid. The tally is now collected per alloca and applied after the loop, so it never competes with a report. `--self-test` gains the arm that catches it: one slot, `@perry_class_keys_*` store first, `js_array_alloc_with_length` store second, both across a moving poll. Sabotage-checked in both directions -- reinstating the old control flow makes that arm, and only that arm, fail (exit 1); restoring it clears (exit 0). Corpus numbers unchanged (2 violations, 93 class-keys + 3 box suppressed), and the partition is now exact: `--assume-old-defrag` alone reports 95, `--assume-boxes-in-gc-heap` alone 5, both together **98** -- byte-identical to what origin/main's predicate reports over the same corpus.
Follow-up measurement + one self-review fixSelf-review fix (pushed). The exemption tally was setting
★ The split is an exact partitionRe-measured over a freshly regenerated 134-file / 116-source corpus at
|
…ow slot (#7236) (#7243) * fix(gc): a Symbol-typed local is a GC-heap reference, and gets a shadow slot (#7236) `collectors/pointer_locals.rs` classified `Type::Symbol` as a non-pointer, so a `Symbol`-typed local never got a shadow-stack slot and sat in a plain `alloca` across every collection point in its scope. `alloc_symbol` is `gc_malloc(size_of::<SymbolHeader>(), GC_TYPE_STRING)` and `js_symbol_new` returns it POINTER_TAG-boxed; nothing else holds a fresh symbol, so with no slot the malloc sweep inside the copying minor frees one that is still live. The drift had already happened: `typed_shape::type_is_pointer_bearing` — an exhaustive match, and the function that lays out the GC's own field masks — answered `true` for `Symbol` while this predicate answered "non-pointer". That is exactly what the doc comment over `is_definitely_non_pointer_type` predicted would cost a use-after-move. So the three copies of the question are now one exhaustive definition, and the other two delegate. Third site, a separate hazard from the missing slot: the same `Symbol` entry in `expr/shadow_slot.rs`'s `expr_is_known_non_pointer_shadow_value` suppressed temp-root protection for a symbol operand held across an allocating call. * test(gc): witness for the unrooted Symbol local (#7236) Red at base (`A 30 B 20` on loop_polls AND on the shipped default, 3/3 deterministic), green after (`A 0 B 0`), byte-exact vs node 26.5.1. Registered in test-parity/gc_repsel_corpus.txt, which #7228's gc-moving-witnesses arm gates and which rejects UNVER as hard as FAIL. * ci(gc): gate the unrooted-alloca mode, now that it reads 0 (#7236) --unrooted-allocas --moving-only was 98 before #7235, 2 after, and 0 once Type::Symbol stopped being classified as an immediate. That number is the stated condition for promoting gc-root-dominance to a required context (#7198); a number that is an acceptance criterion and is checked by nothing regresses silently. Verified it can fail: exit 1 at f8f1e71, exit 0 here. * docs(changelog): fragment for #7236 (PR #7243) * test(gc): narrow the Symbol witness to the defect #7236 fixes The first shape mixed two defects. A symbol WITH a description stayed red 2/20 after the fix on every PERRY_GC_INCREMENTAL=0 + PERRY_CONSERVATIVE_STACK_SCAN=off arm, because GC_TYPE_STRING is a pointer-free Leaf and a symbol's description StringHeader is never traced -- filed as #7246. A descriptionless Symbol() has no such pointer, so what the file measures is exactly the lifetime of the symbol object, which is what a shadow slot decides. --arms all --pressure 8: base 21/21 cells FAIL (0/21 byte-exact, 5 exit=1), with the fix 0 FAIL and 21/21 byte-exact (PASS=17 UNVER=4). --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Closes the actionable item from #7210's triage:
_is_heap_sourceconflates "the collector rewrites this location" with "the object this names can move."That is not a nit. It is why
--unrooted-allocas --moving-onlycould not reach 0, and therefore why #7198's promote-to-required clock could not start. #7210 measured the whole population and every single hit was a false positive.The split
A register naming a heap object can go bad in exactly two independent ways, and the old predicate modelled neither directly:
--moving-onlyAn exemption must assert both are false. Two qualify:
class-keys—@perry_class_keys_*andjs_build_class_keys_array.Not movable:
js_build_class_keys_arrayallocates throughjs_array_alloc_with_length_longlived(object/alloc.rs:320,337) — the old arena. The nursery copying minor relocates nursery objects only, and the one thing that relocates an old-arena object is old-page defrag, whichselect_old_page_defrag_pagesshort-circuits off (#6206). No shipped configuration and nogc_repsel_matrixarm setsPERRY_GC_OLD_DEFRAG=1.Not reclaimable: the global is registered with
js_gc_register_global_root(codegen/string_pool.rs:477), so the array is root-reachable for the life of the process. #5042 registered it for the defrag rewrite; the reachability is the side effect that closes this half.box—js_box_alloc*.Not movable:
std::alloc::alloc(box.rs:69-88) — outside the GC heap. What the collector touches is the JSValue inside the box, whichscan_box_roots_mutrewrites in place; the box's own address never changes.Not reclaimable: no
deallocinbox.rs, andBOX_REGISTRYis monotonic per thread — an invariantjs_box_is_box's membership test already depends on (box.rs:429).The exemptions are one-sided in the unsafe direction: a source that is not listed keeps the old, conservative classification. Forgetting one costs a false positive, never a missed bug — the same asymmetry
NONCOLLECTINGandALLOC_REalready carry.★ An exemption is a suppression, so it gets the strictest treatment in the file
Three mechanisms, because "we triaged those" is exactly the kind of claim that rots:
1.
--audit-immovable-sources(new, wired into the workflow next to--audit-alloc-re). Every premise above is a machine-checked probe over the runtime source, not a paragraph:If
js_build_class_keys_arrayever picks up a nursery allocator, orselect_old_page_defrag_pagesstops short-circuiting, orbox.rsgrows adealloc, the audit goes red and says which exemption is void. An exemption whose premise has quietly lapsed reads as a triaged false positive and is a live hazard — strictly worse than no exemption at all.2. Both of #7210's counterfactuals are one flag away.
--assume-old-defragand--assume-boxes-in-gc-heaprestore the reports, so "what does this look like if old-defrag ships on?" is a command rather than tribal knowledge. Each is a usage error without--unrooted-allocas, following the file's existing disarmed-knob rule.3. ★ The planted genuine hazard.
--self-testgains a fixture that is structurally identical to the class-keys one — samei64slot, same load below the same collection point, same consumer (js_object_alloc_class_inline_keys'skeysargument) — differing only in the allocator:js_array_alloc_with_length, the nursery. It must still be reported, under--moving-onlyand under both knobs. If a future widening ever swallows it, the exemption has stopped being about the allocator and become a rule about code shape, which is the failure this whole split exists to prevent.The self-test also asserts, per exemption: it fires; its knob exactly reverses it; it declares a knob
classify_heap_sourceactually reads; it has premise probes; and it states all three of "cannot move", "cannot be reclaimed" and "becomes real when". A future exemption cannot be added without those.4.
--unrooted-allocasnow prints what each exemption suppressed. Without it, "the corpus is clean" and "the corpus is entirely exempted" print the same line — CLAUDE.md hazard 4 applied to the suppression rather than to the check.Measured
Freshly generated corpus at
c9cd73ba5(134.llfiles, 116 sources, 5654 gc-capable allocas), same corpus for both arms,--unrooted-allocas --moving-only:origin/main's checker--self-test OK.--audit-immovable-sourcesclean,--audit-alloc-reclean. No production code changed — this PR is one file,scripts/gc_root_dominance_check.py.The 98 is above #7210's 66 because both the corpus and
ALLOC_REgrew since that measurement: #7227 added the*_new*convention. That widening is also what makes the 2 residuals visible at all.Both are in
test_gap_class_forward_capture_6523.ts, and they are one bug:const s = Symbol("x")— aSymbol-typed local held in a plainallocaacross ~250 instructions of allocating calls. The cause is one entry in a list:alloc_symbol(symbol.rs:370) isgc_malloc(size_of::<SymbolHeader>(), GC_TYPE_STRING)— a real, movable GC-heap allocation — and its own comment says fresh symbols are held alive "not at all". So aSymbol-typed local gets no shadow slot, exactly theMap/Setdefect (#7019) documented eight lines above it in the same function.Deliberately not fixed here. It is a codegen change that resizes every frame holding a
Symbollocal, it wants its own before/after and its own runtime witness, andpointer_locals.rsis live ground for other in-flight work. Filed separately; this PR reports it rather than allowlisting it, because a real hazard suppressed by an allowlist is the thing #7210 just spent a round proving is expensive.So: 98 → 2, the remaining 2 have a named single cause, and
gc-root-dominancebecomes promotable to a required context the moment that one-line classification is fixed and the corpus re-runs at 0. That is a much shorter statement of what remains than "66 false positives nobody has triaged".Refs #7210, #7198, #7202, #7154, #7227, #7230.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores