Skip to content

GC: the remaining unrooted-alloca hazards after #7207 — class-keys pointer caches, interleaved staging arrays, inlined-callee param slots #7210

Closed

Description

@proggeramlug

Summary

The remaining #7202-class hazards after PR #7207, from the same bare-alloca enumeration (140 sites, 23 HAZARD / 117 SAFE). #7207 fixed the inline-constructor this slot; #7208 and #7209 own the closure this/new.target slots and the catch parameter. This tracks the rest so they stop being invisible.

1. An unrooted cached copy of a value whose original is a registered root (3 sites)

  • function.rs:521 (entry_init_load_global) — caches the @perry_class_keys_* pointer in an entry alloca for the whole function. codegen/string_pool.rs:464-477 (GC: old-page defrag unsound on conservative-scan cycles — un-rewritten referrer of moved old objects (class-keys global suspected) #5042) explicitly says that array "lives in the longlived (old-gen) arena … so old-page defrag (C4b) can relocate it", which is why the global was registered with js_gc_register_global_root. This second copy silently opts out. Split by consumer: new.rs:621, new.rs:705, scalar_method.rs:553 dereference it (passing it as keys to js_object_alloc_class_inline_keys) — real hazard; method_override.rs:213 and property_get/dynamic_dispatch.rs:105 only icmp eq it as a shape token — fail-safe.

    This is the one violation gc_root_dominance_check.py --unrooted-allocas still reports after fix(gc): close the three residual root-store holes — the PutValueSet key, js_object_assign_one, and the inline-ctor this slot #7207 on its reproducer corpus (not moving-reachable there, because js_array_alloc is the only call in that window).

  • stmt/loops.rs:1505 — a double cache of a module global aliased into ctx.locals[gid] for both copies of a packed-f64 range loop.

  • stmt/loops.rs:4766 — a hoisted arr.length; narrow, but the hoist classifier imposes no type gate, so a Named-typed receiver whose length is a string lands a pointer here.

2. Staging arrays filled interleaved with lowering (9 sites)

Shape: for a in args { let v = lower_expr(ctx, a)?; store v into buf[i] } — argument i's NaN-boxed pointer is already sitting in the unrooted buffer while argument i+1 is lowered, and lowering allocates.

  • ★ codegen/helpers.rs:1275 — vals_buf stages every exported binding's value for js_create_namespace. js_closure_alloc_singleton (:1300, :1341) and an arbitrary user getter (:1315) run inside the loop; N GC pointers accumulate across N allocating calls, then js_create_namespace (:1366) allocates while reading all N.
  • expr/call_spread.rs:473, expr/instance_misc1.rs:1035, lower_call/extern_func.rs:1161/:1197/:1249, lower_call/namespace_call.rs:68/:107/:141.

setTimeout(resolve, d, someObject) with two or more heap arguments is the reachable shape. expr::temp_root::lower_exprs_rooted (#6951) exists for exactly this and has ~7 callers repo-wide.

3. Staging arrays with a call between the stores and the consumer (3 sites)

  • lower_call/property_get/dynamic_dispatch.rs:1315 — js_object_get_own_field_or_undef (:1351, a prototype/shape-walking read) sits between the stores and js_native_call_value/js_native_call_method. Widest blast radius: the codegen: split large modules into parallel codegen units (bound clang peak memory; remove the single-TU wall) #5391 collapsed instance-dispatch outline.
  • lower_call/console_promise.rs:1336 — js_closure_unbox_callee_checked_rebind (:1358), which rebinds a closure's baked this capture slot.
  • lower_call/early_branches.rs:244 — unbox_str_handle (:262-265), string-key branch only.

4. Inlined-callee locals — structurally unrootable today (4 sites)

lower_call/new_ctor_args.rs:54, stmt/let_stmt.rs:974/:997/:367. These hold arbitrary JS values and go straight into ctx.locals, but their HIR ids belong to a different function, so collect_pointer_typed_locals — which walks only this frame's params and Stmt::Lets — never assigns them a slot. Nothing can bind them even in principle; closing these needs a representation change, not a call.

#6969's temp-root scope keeps the arguments alive across construction, so this is not a premature-sweep bug — it is the "rooted slot, stale copy" split. refresh_rooted_args (new.rs:199-217) does exactly the needed re-read for lowered_args; nothing does it for the param slots.

5. Conditionally safe, worth a standing note (4 sites)

codegen/function.rs:928/:991, expr/buffer_views.rs:272, stmt/let_buffer_views.rs:54 cache handle+8/handle+16 data pointers for a whole function. The in-tree argument (function.rs:962-976) is liveness via the caller's rooted binding plus address stability, because typed arrays are old-arena GC_FLAG_TENURED and gc_type_is_movable(GC_TYPE_TYPED_ARRAY) == false. Sound as written. If any admitted construction form ever nursery-allocates, all four become hazards silently — that invariant deserves an assertion rather than a comment.

Why this matters

Together with #7208 and #7209 these are the reason the #7161 revert cannot be called safe on the strength of #7192/#7198/#7207 alone: they have never been exercised under a moving minor, and gc_root_dominance_check.py --unrooted-allocas (added in #7207) is the instrument that can now find them without waiting for a crash.

Refs #7202, #7207, #7208, #7209, #7154, #7161, #6951, #6968, #6969.

Activity

  1. proggeramlug commented on Aug 2, 2026

    @proggeramlug
    ContributorAuthor

    The 66 triaged — all of them are false positives, and the reason is one line in the checker

    gc_root_dominance_check.py --unrooted-allocas --moving-only over a freshly generated 128-module corpus at 7d1dc9ca2 reports 66, every one moving-reachable. Classified by the provenance of the stored value (script: re-run check_func_unrooted_allocas and resolve each store's provenance() back to its heap source):

    n heap source verdict
    64 load i64, ptr @perry_class_keys_* SAFE — object cannot move
    2 call @js_box_alloc_bits SAFE — object is outside the GC heap

    43 of the 66 are in main; the rest are spread over 20 functions. There is no third class: #7210's §2/§3/§4 sites do not appear here at all (they are excluded by the checker's escaped rule at gc_root_dominance_check.py:1488-1495, because a staging buffer's address is passed to its consuming call).

    The 64: @perry_class_keys_* cannot move under any shipped or matrix configuration

    #7210 §1 calls these "real hazard". They are not, today, and the argument is short:

    • js_build_class_keys_array allocates the array with js_array_alloc_with_length_longlived (object/alloc.rs:337, :319) — the old/longlived arena, never the nursery. The nursery copying minor only relocates nursery objects.
    • The one thing that relocates an old-arena object is old-page defrag, and defrag is off: select_old_page_defrag_pages returns default() unless PERRY_GC_OLD_DEFRAG=1 (gc/oldgen.rs:229-259), disabled under GC evacuation leaves a stale reference to a moved array element (wild-pointer crash / corrupt cached value) #6206 pending a rewrite-contract fix. Not one matrix arm sets it.

    The #5042 comment that motivated registering the global as a root ("old-page defrag (C4b) can relocate it") is still the right reason to keep that registration — it is future-proofing against #6206 being resolved. But it does not make the cached copy a live hazard, and rooting 64 of them today buys a shadow slot per (class, function) for zero behaviour change.

    Condition, stated so it can be re-checked: if PERRY_GC_OLD_DEFRAG ever becomes the default, or js_build_class_keys_array ever stops using the _longlived allocator, all 64 become real and entry_init_load_global's slot must be bound (expr::root_entry_alloca works on it — gc/roots.rs:1344-1349 records that both root-slot kinds "accept a heap reference either NaN-boxed or bare", so a raw i64 keys pointer binds correctly).

    The 2: a box is never relocated

    js_box_alloc_bits uses std::alloc::alloc (box.rs:69-88), the box is never freed, and scan_box_roots_mut rewrites the JSValue inside the box, not the box's address. expr/literals_vars.rs:706-717 already states this invariant in prose for a sibling site.

    What the checker should learn from this

    _is_heap_source (gc_root_dominance_check.py:1456) conflates two different things: "the collector rewrites this location" and "the object this names can move." REWRITTEN_LOAD_RE includes perry_class_keys_\w+ because the global is rewritten — but the array it points at is old-arena and immovable, so the copy can never go stale. Until that distinction exists in the tool, --unrooted-allocas --moving-only cannot reach 0, and #7198's "promote the gate to required after a clean week" clock cannot start. That is the actionable item here, not 66 roots.


    §2/§3/§4, which the checker cannot see

    Audited separately at source level. The headline: the issue predicted staleness and the reproducer produced a crash, for a different operand than the one named.

    • §2 timers/nextTick — real, and the callback register is the live defect, not (only) the staging buffer. cb_box was lowered first and read at js_timer_validate_callback after every argument's lower_expr. setTimeout(cb, 0, {…}, churn()) is red 3/3 at base under PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_GC_HEAP_LIMIT=8:
      TypeError [ERR_INVALID_ARG_TYPE]: The "callback" argument must be of type function. Received an instance of Object.
      The buffer is the second window and is worse in kind — a premature sweep, not staleness. Fixed in fix(gc): root the callback and every staged argument of the timer family #7230 with a witness. setImmediate, process.nextTick and the spread-call buffer were probed with the same shape and are green at base — 1 of 4 reproduced, so the static "9 HAZARDs" reading was over-called.
    • §3 — dynamic_dispatch.rs: the cited intervening call js_object_get_own_field_or_undef is a pure own-keys walk that does not allocate, so the stated claim is refuted; a genuinely unprotected sibling loop exists one frame up (for a in args { push(lower_expr(ctx, a)?) }). console_promise.rs: already fixed by fix(codegen): root the callee, this and every argument of js_closure_callN #7214's RootedOperands. early_branches.rs: confirmed, unbox_str_handle → js_get_string_pointer_unified allocates on the SSO path.
    • §4 — "structurally unrootable today ... needs a representation change, not a call" is wrong. root_entry_alloca does not go through collect_pointer_typed_locals; it calls ctx.func.reserve_shadow_slot(), which takes no HIR id and grows the frame on demand at the store site (function.rs:341-376). GC: heap values in plain alloca_entry slots are neither marked nor rewritten (inline-ctor this_slot, closure-capture staging array) #7202 already used it on the sibling slot in the same inlined-constructor window (lower_call/new.rs:1164). All four are ordinary fixable sites. let_stmt.rs:367 additionally has no "different function" problem at all — it is this function's own Stmt::Let, in a branch that early-returns past the normal bind.

    Deferred from #7230 and worth their own change: the §4 ctor-param slots, call_spread.rs's regular-arg buffer (needs the callee lowered inside the rooted group without disturbing evaluation order), and codegen/helpers.rs's vals_buf.

  2. proggeramlug commented on Aug 2, 2026

    @proggeramlug
    ContributorAuthor

    All 66 are false positives, and the tool's own predicate is the actionable finding.

    Triaged over a fresh 128-module corpus at 7d1dc9ca2, classified by each store's heap-source provenance:

    n source verdict
    64 load i64, ptr @perry_class_keys_* SAFE — js_build_class_keys_array uses js_array_alloc_with_length_longlived (old arena, never the nursery). Only old-page defrag relocates those, and select_old_page_defrag_pages returns empty unless PERRY_GC_OLD_DEFRAG=1 (off under #6206; no matrix arm sets it).
    2 call @js_box_alloc_bits SAFE — std::alloc::alloc, never freed; scan_box_roots_mut rewrites the JSValue inside the box, not its address.

    §1's "real hazard" label does not survive contact with the allocator. §2/§3/§4 don't appear in the 66 at all (excluded by the checker's escaped rule) and were audited separately: §3's dynamic_dispatch claim is refuted (js_object_get_own_field_or_undef does not allocate); console_promise was already fixed by #7214; §4's "structurally unrootable, needs a representation change" is wrong — root_entry_alloca bypasses collect_pointer_typed_locals via reserve_shadow_slot, and #7202 already used exactly that on the sibling slot in the same window.

    ★ The actionable item

    One line in scripts/gc_root_dominance_check.py: _is_heap_source conflates "the collector rewrites this location" with "the object this names can move." Until that distinction exists, --unrooted-allocas --moving-only cannot reach 0, so #7198's promote-to-required clock cannot start. Fixing the predicate — not rooting 66 sites — is what closes this issue.

    Two conditions worth re-checking if either changes: the 64 become real hazards if PERRY_GC_OLD_DEFRAG=1 ever ships on, and the 2 if boxes ever become GC-heap allocations.

  3. proggeramlug commented on Aug 2, 2026

    @proggeramlug
    ContributorAuthor

    Re-verified on main = d4ac65f21 as part of the sweep in #7262. Still open, with one correction to §1.

    Confirmed still open

    §2's flagship site is unchanged: codegen/helpers.rs:1275 is still a bare blk.emit_raw(format!("{} = alloca [{} x double]", ...)), still filled interleaved with lower_expr and still consumed by js_create_namespace at :1374. expr::temp_root::lower_exprs_rooted is not applied to it.

    Correction to §1: the class-keys bullet is now a known FALSE POSITIVE class, not a hazard

    §1 leads with function.rs:521 (entry_init_load_global caching the @perry_class_keys_* pointer) and calls it "the one violation --unrooted-allocas still reports". That is no longer the reading. On a fresh 138-module corpus the mode reports:

    === files: 138   gc-capable allocas: 5779   unrooted-alloca violations: 0   (moving-minor reachable: 0)
    === suppressed by an IMMOVABLE_SOURCES exemption (#7210: rewritten location, immovable object):
          95  class-keys  (--assume-old-defrag re-reports these)
           3  box  (--assume-boxes-in-gc-heap re-reports these)
    

    The 95 class-keys reports are suppressed by an exemption that cites this issue by number, on the ground #7239 established independently: CLASS_KEYS_BY_ID is safe because every write site uses a _longlived allocator. So the cached copy cannot go stale under the collector as it exists today.

    That is a narrower claim than "not a hazard", and the exemption is honest about it — --assume-old-defrag re-reports all 95. The residual exposure is precisely old-page defrag (C4b) relocating the long-lived keys array, which is the scenario codegen/string_pool.rs:464-477 warned about and which motivated registering the global as a root. If C4b defrag is ever enabled for that arena, these 95 sites become live hazards in one step — which is worth an assertion rather than an exemption comment, in the same spirit as §5's standing note about the typed-array data pointers.

    Net: §1's first bullet should be re-scoped from "real hazard, dereferenced at new.rs:621/:705/scalar_method.rs:553" to "conditionally exempt, contingent on _longlived allocation and on C4b defrag staying off". §§2–4 are untouched by this and remain open.

    Not fixing here — read-only sweep.

  4. proggeramlug commented on Aug 9, 2026

    @proggeramlug
    ContributorAuthor

    Re-adjudication against 8d27032c7 — every cited line number was stale

    Source-only. Per-site verdict; paths relative to the repo root.

    grp site (current) verdict evidence
    1a codegen/function.rs:528 entry_init_load_global; derefs lower_call/new_alloc.rs:249,:333, lower_call/scalar_method.rs:553; icmp-only method_override.rs:212, property_get/dynamic_dispatch.rs:105 ADJUDICATED — NOT A HAZARD (also MOVED: new.rs:621/:705 → new_alloc.rs:249/:333) Still a cached alloca copy, but gc_root_dominance_check.py:2638 IMMOVABLE_SOURCES["class-keys"] suppresses it behind 3 machine probes, re-enabled by --assume-old-defrag, audited in CI at gc-root-dominance.yml:185 and :451. §1's own ask — "an assertion rather than an exemption comment" — is already satisfied.
    1b stmt/loops.rs:1505 STILL PRESENT alloca_entry(DOUBLE) caching load double, ptr @perry_global_* into ctx.locals[gid]. Producer packed_f64_range_loop_invariant_global_reads (:1001-1031) tests only module_globals.contains_key(id) — no type gate.
    1c stmt/loops.rs:4777 STILL PRESENT Hoisted arr.length. classify_for_length_hoist_impl (:5530-5564) matches property == "length" syntactically; array_length_receiver_is_loop_local (:5714-5720) checks slot/box/global/scalar-replacement only — still no type gate.
    2a codegen/helpers.rs:1470 vals_buf, consumed at :1565 STILL PRESENT — the flagship, unchanged alloca [N x double] filled at :1551-1553 in a loop calling js_closure_alloc_singleton (:1494, :1535) and an arbitrary user getter (:1516). No crate::rooting anywhere in the function.
    2b expr/call_spread.rs regular-arg buffer STILL PRESENT (and wider than filed) alloca_entry_array(DOUBLE, regular_count) filled per lower_expr. Plus cb_box held across the whole marshal, plus the raw acc_handle accumulator loop. The accumulator half is fixed by #7696; the buffer and cb_box are not.
    2c expr/instance_misc1.rs:1138 FIXED (#7230, migrated #7615) rooting::with_operands_rooted; buffer filled in a second lowering-free pass. Comment at :1130 cites this issue.
    2d–f MOVED lower_call/extern_func.rs → lower_call/extern_timers.rs:213,:245,:283 (shared fill_arg_buffer :73) FIXED (#7230, #7615) All inside with_operands_rooted. The migration also found and fixed the 2-argument siblings #7210's own fix missed (extern_timers.rs:20-40).
    2g–i lower_call/namespace_call.rs:113,:146,:178 FIXED (#7230, #7615) with_operands_rooted + fill_arg_buffer; the unbox_to_i64 now sits below the group.
    3a property_get/dynamic_dispatch.rs:1341 ADJUDICATED — PREMISE REFUTED js_object_get_own_field_or_undef (object/object_ops/accessors.rs:109-181) is a pure own-keys walk — js_array_length, js_array_get, js_string_key_matches_bytes (byte compare, string/compare.rs:245-272), js_object_get_field. Zero allocations, so there is no collection point between the stores and the consumer.
    3a′ property_get/dynamic_dispatch.rs:275,285-288 STILL PRESENT — the real defect in this file recv_box = lower_expr(object) then for a in args { static_user_args.push(lower_expr(a)?) } — receiver and each arg in bare registers across every later arg's lowering.
    3b lower_call/console_promise.rs:1382 FIXED (#7214, #7615) with_rooted_group(ctx, args.len() + 2, …); comment at :1361-1370 names js_closure_unbox_callee_checked_rebind → clone_closure_rebind_this and re-reads below it.
    3c lower_call/early_branches.rs:244 STILL PRESENT — confirmed live Bare alloca_entry_array filled :245-250; unbox_str_handle (:264) is js_get_string_pointer_unified, which the checker explicitly declines to call non-collecting because its SSO branch calls js_string_materialize_to_heap (gc_root_dominance_check.py:2170). Also unprotected arg loop :236-238 and bare recv_box/key_box :234-235.
    4a–d lower_call/new_ctor_args.rs:52-54; stmt/let_stmt.rs:978, :1001, :371 STILL PRESENT, and §4's "structurally unrootable" claim is REFUTED see below
    5a–d codegen/function.rs:932, :995; expr/buffer_views.rs:267; stmt/let_buffer_views.rs:54 STILL PRESENT (conditionally safe); the asked-for assertion still does not exist see below

    §4 is closable today, and the source says so in its own words

    The claim was that these locals' HIR ids belong to a different function so collect_pointer_typed_locals can never slot them. But reserve_shadow_slot (codegen/function.rs:352-377) takes no HIR id and grows the frame at the store site, and root_entry_alloca (expr/scalar_slot_root.rs:102-142) keys on the alloca's LLVM name string (scalar_slot_shadow_slots: HashMap<String, u32>). let_stmt.rs:472 and :861 already call root_scalar_replaced_slot on id-less slots in the same function, and :861 is inside the same if branch as sites 4b/4c.

    Site 4d is not even an inlined-callee local: id is this frame's own Stmt::Let, so collect_pointer_typed_locals does reserve it a slot — the POD-record branch just return Ok(())s at :432 past the bind at :1810, while storing a NaN-boxed js_object_alloc handle (expr/pod_record.rs:360). One line: emit_shadow_slot_bind_for_local(ctx, id) after the store.

    The only per-site wrinkle is 4a's boxed_param arm, which stores an I64 box pointer and wants the box-slot treatment rather than the DOUBLE one.

    §5's premise holds — but not for the reason the in-tree comment gives

    gc_type_is_movable (gc/types.rs:619) does return false for GC_TYPE_TYPED_ARRAY/GC_TYPE_BUFFER, but types.rs:517 states that flag only gates old-page defrag — the nursery copying minor evacuates eden objects regardless. The whole load is carried by old-arena residency, defended by exactly two choke points: typed_array_alloc (typedarray/mod.rs:~97) and buffer_alloc (buffer/header.rs:478), both arena_alloc_gc_old(...) followed by a hand-written gc_flags |= GC_FLAG_TENURED. All ~147 construction sites funnel through those two, and nothing nursery-allocates today.

    Two fragilities worth the assertion §5 asks for: arena_alloc_gc_old does not itself set GC_FLAG_TENURED (a purpose-built arena_alloc_gc_born_tenured exists and is unused), and every old-gen residency test is on the large-object path — no test asserts a small typed array is old-gen. IMMOVABLE_SOURCES has exactly two entries (class-keys, box); there is no typed-array entry, and typed arrays are structurally outside the checker's reach anyway (getelementptr is absent from TRANSPARENT_OPS and typed_array_alloc\w* was deliberately removed from ALLOC_RE). So the assertion exists for §1 and does not exist for §5.

    Note on the 0/0 --unrooted-allocas --moving-only reading

    It is not evidence for §§2–4: those sites are excluded by the checker's escaped rule (a staging buffer's address is passed to its consumer), and §4's are unreached because the corpus does not route a movable heap value through an inlined-ctor param under --moving-only.

  5. added 3 commits that reference this issue on Aug 9, 2026
    0ec8588
    b636f67
    0c4d65b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions