gc: close the strhandle, derived-mask and new.target unrooted hazards (#7664)

`gc-root-dominance-statepoints`' `--max-unrooted` ratchet goes 21 -> 7.

#7663 pointed the root-dominance rule at the NATIVE root lowering -- the one
that ships since #7370 -- and reported 21 `unrooted` hazards. Fourteen were
shapes `root_reload.rs` looked straight through, because its rule is stated
over the load's own register and in both shapes the value at risk lives
somewhere else.

  1. The root is a GLOBAL, not an alloca (10 hits). A string literal lowers to
     `load double, ptr @<mod>_.str.N.handle`; the handle global is a registered
     root, so the string is never swept, and an evacuating cycle REWRITES the
     global while a register loaded beforehand keeps the pre-move address.
     #7240's shape, whose fix covered call operands only.

  2. The stale register is DERIVED from the load (3 of 7 unmasked receivers).
     `this.count++` holds the unmasked receiver across the property GET; the
     load's only use is the bitcast ABOVE the collecting call, so the window
     was empty and the function took zero reloads.

  3. `new.target`'s saved previous value (1 hit). `new.rs` saved
     `js_new_target_get()` in a bare register across the whole constructor
     body; the cell is a registered mutable root, so the restore publishes a
     pre-move address back INTO a root the collector scans. #7226's
     `prev_this` bug for `new.target`.

The reload rule is restated over the value's derivation rather than its
register: for a value read out of a collector-rewritten location -- a shadow
slot or a string-handle global -- and any value derived from it by pure bit
ops, every use a collection point can reach re-materialises the whole
derivation. A recipe is extended only through ops that are pure functions of
their operands and whose every register operand is already in the same single
root's recipe, which makes it self-contained and materialisable anywhere. Each
value's window is anchored at its own defining instruction, not at the root
load.

`new.target` gets `new_target_save`/`new_target_restore` in `crate::rooting`,
structurally `implicit_this_save`/`implicit_this_restore`. Re-reading the cell
would be the wrong repair: `js_new_target_set` has already overwritten it.

Measured on `Counter__increment`: before, all three statepoints carried an
EMPTY live set, so the receiver was marked by nothing; after, each carries a
"gc-live" bundle and a `gc.relocate`, and the SET reads a mask re-derived from
the relocated pointer plus a fresh load of the handle global.

Remaining 7, each its own slice: 4 unmasked are phi-mediated (the reload has to
go in the predecessor, on the edge); 2 `@perry_global_*` are module-level
variables the program assigns, so they need rooting rather than reloading
(pinned by `a_module_global_is_not_a_reload_source`); 1 capture read. #7664
stays open as the budget's referent.

Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix

Author: Ralph Küpper

.github/workflows/gc-root-dominance.yml

@@ -464,17 +464,35 @@ jobs:
           #
           # BUDGETS. `unrooted` (the object is in NO live bundle at all, so
           # nothing marks or rewrites it) is the serious class, and it is a
-          # RATCHET at its measured value rather than a calibrated zero: this
-          # is a new instrument pointed at a lowering nothing has ever checked
-          # statically, and 21 is a population under triage, not a number
-          # anyone has adjudicated. It is a budget rather than allowlist
-          # entries for the reason `--stale-registers` records — 21 tombstones
-          # with no issue numbers would be worse documentation than one number
-          # that can only go down. Lower it as sites are fixed.
+          # RATCHET rather than a calibrated zero: this is a young instrument
+          # pointed at a lowering nothing had ever checked statically, and the
+          # remainder is a population under triage rather than a number anyone
+          # has adjudicated. It is a budget rather than allowlist entries for
+          # the reason `--stale-registers` records — tombstones with no issue
+          # numbers would be worse documentation than one number that can only
+          # go down. Lower it as sites are fixed.
           #
-          # The 21 are enumerated by shape in #7664, which is this budget's
-          # referent: a number with nothing behind it is the thing CLAUDE.md
-          # warns a threshold decays into.
+          # 21 at #7663. #7664 fixed 14 of them in `root_reload.rs` and
+          # `lower_call/new.rs` — the whole `strhandle` population (10), the
+          # `js_new_target_get` save/restore (1), and 3 of the 7 unmasked
+          # receivers — so the budget is 7. What remains, and why each is its
+          # own slice rather than a widening of the same fix:
+          #
+          #   4  unmasked, all PHI-MEDIATED. The stale value reaches its use
+          #      through a `phi`, and `root_reload` cannot insert above a phi;
+          #      the reload has to go in the PREDECESSOR, on the edge, which is
+          #      a different insertion model.
+          #   2  global (`@perry_global_*`). NOT reloadable: a module-level
+          #      variable is one the program assigns, so a re-read can observe
+          #      a later assignment instead of the value the call was given
+          #      (`operand_needs_root`). That population needs ROOTING, and
+          #      `a_module_global_is_not_a_reload_source` in root_reload.rs
+          #      pins the distinction so it cannot be widened away by accident.
+          #   1  capture, a `js_closure_get_capture_bits` read held across
+          #      `js_number_coerce`.
+          #
+          # #7664 stays open as this budget's referent: a number with nothing
+          # behind it is the thing CLAUDE.md warns a threshold decays into.
           #
           # `stale` (the object survives and is relocated, but a raw copy of
           # its pre-move address is used below) reads 0 today and is held
@@ -494,7 +512,7 @@ jobs:
             --min-statepoints 15000 \
             --min-live-bundles 8000 \
             --min-relocates 20000 \
-            --max-unrooted 21 \
+            --max-unrooted 7 \
             --max-stale 0 \
             --allowlist scripts/gc_root_dominance_allowlist.json \
             --seeded-violations 40 \

changelog.d/7667-native-lowering-unrooted-hazards.md

@@ -0,0 +1,97 @@
+### The reload rule now covers global roots and derived values (native lowering)
+
+`gc-root-dominance-statepoints`' `--max-unrooted` ratchet goes **21 → 7**.
+
+#7663 pointed the root-dominance rule at the **native** root lowering — the one
+that ships on every target whose frames the runtime can walk since #7370 — and
+reported 21 `unrooted` hazards, enumerated by shape in #7664. Fourteen of them
+were shapes `root_reload.rs` (#7280) looked straight through, and for one
+reason: its rule is stated over *the load's own register*, and in both shapes
+the value at risk lives somewhere else.
+
+**1. The root is a global, not an alloca — 10 hits.** A string literal lowers to
+`load double, ptr @<mod>_.str.N.handle`. The handle global **is** a registered
+root (`js_gc_register_global_root`, `codegen/string_pool.rs`), so the string is
+never swept — and an evacuating cycle **rewrites the global** while a register
+loaded beforehand keeps the pre-move address. That is #7240's shape, whose fix
+(`OperandProtection::Reload`) covers call *operands* and never reached the ~194
+codegen sites that load a handle global directly.
+
+**2. The stale register is DERIVED from the load — 3 of 7 unmasked receivers.**
+`this.count++` lowered to a slot load, a `bitcast`, an `and` mask, the property
+GET, and then the SET re-using the *pre-GET* mask. The load's only use is the
+`bitcast`, which sits **above** the collecting call, so the window was empty and
+the function took zero reloads; the value that actually crosses the GET is the
+mask. Under the native lowering the same shape reads
+`ptrtoint ptr addrspace(1) %s to i64` — LLVM relocates the `addrspace(1)`
+pointer and rewrites its uses, but it cannot touch an `i64` copy, so the unmask
+is where the value leaves the tracked domain for good. #7280's zod-`clone`
+shape and #7240's literal shape, meeting in one function.
+
+**3. `new.target`'s saved previous value — 1 hit.** `lower_call/new.rs` saved
+`js_new_target_get()` into a bare SSA register across the **whole constructor
+body**. The cell is a registered mutable root
+(`scan_current_new_target_root_mut`), so evacuation rewrites it and the restore
+publishes a pre-move address back *into* a root the collector scans. The
+runtime's own construct paths have always rooted their `prev_new_target`
+(`scope.root_nanbox_f64`); generated code did not. This is #7226's `prev_this`
+bug for `new.target`, and it is fixed the same way — a
+`new_target_save`/`new_target_restore` pair in `crate::rooting`, structurally
+`implicit_this_save`/`implicit_this_restore`.
+
+#### The restated rule
+
+> For a value read out of a collector-rewritten location — a shadow slot **or a
+> string-handle global** — and any value **derived from it by pure bit ops**,
+> every use that a collection point can reach re-materialises the whole
+> derivation instead.
+
+A derivation ("recipe") is extended only through instructions that are pure
+functions of their operands (`and`/`or`/`xor`, `bitcast`/`ptrtoint`/`inttoptr`/
+`trunc`/`zext`/`sext`) **and** whose every register operand is already in the
+same single root's recipe. That makes a recipe self-contained — a load plus bit
+ops on constants — so it materialises at any point in the function with no
+dominance question, and re-executing it is by construction the same function of
+the same root evaluated against the address the collector wrote back. A phi, a
+call, or an operand from a second root is not extended through. Each reloadable
+value's window is anchored at **its own defining instruction**, not at the root
+load: a derivation built *below* a collecting call is already fresh.
+
+Re-reading a *handle global* is sound for the reason `operand_is_reloadable`
+gives: the only writer in generated code is `__perry_init_strings_*`, so a
+re-read cannot observe a later assignment. That function is excluded by the
+analysis rather than by the argument — the same store side-condition that
+protects a reassigned slot covers a stored-to global.
+
+#### Measured
+
+`test_gap_closures.ts`'s `Counter__increment`, native corpus. **Before**, all
+three statepoints carried an empty live set: the receiver was in no bundle at
+all, so nothing marked or rewrote it, and the SET read the mask computed before
+the GET. **After**, every statepoint carries a `"gc-live"` bundle and emits a
+`gc.relocate`, and the SET reads a mask re-derived from the relocated pointer
+plus a fresh load of the handle global. The re-derivation is what puts the
+receiver in the bundle: it extends the tracked pointer's live range past the
+safepoints, so `rewrite-statepoints-for-gc` must report and relocate it.
+
+#### What remains
+
+`--max-unrooted 7`, and each remainder is its own slice rather than a widening
+of this fix: **4 unmasked** are phi-mediated (no instruction can be inserted
+above a phi — the reload has to go in the predecessor, on the edge); **2
+global** are `@perry_global_*`, module-level variables the program assigns, so a
+re-read can observe a later assignment instead of the value the call was given
+(`operand_needs_root`) — that population needs rooting, not reloading; **1
+capture** is a `js_closure_get_capture_bits` read held across
+`js_number_coerce`. #7664 stays open as the budget's referent.
+
+The `global` exclusion is pinned by `a_module_global_is_not_a_reload_source`, so
+widening `is_string_handle_global` to swallow it is a test failure rather than a
+silent decision. Five more unit tests cover the two new shapes, the
+`__perry_init_strings_*` store side-condition, the no-collection-point control
+(the derivation closure must not turn every mask in the program into three extra
+instructions), and the refusal to extend a derivation through a call.
+
+Also corrected: #7664's shape-1 heading says nine hits; its own list has ten,
+and the checker reports ten. The census of the 21 is 10 `strhandle` / 7
+`unmasked` / 2 `global` / 1 `rootread` / 1 `capture`.

crates/perry-codegen/src/inst.rs

@@ -15,6 +15,7 @@
 
 use crate::types::LlvmType;
 
+#[derive(Clone)]
 pub enum LoadFlavor {
     Plain,
     Aligned(u32),
@@ -24,6 +25,11 @@ pub enum LoadFlavor {
     Invariant,
 }
 
+/// `Clone` is what lets `root_reload.rs` carry a value's derivation RECIPE —
+/// the root load plus the pure bit ops above it — to the stale use and
+/// re-materialise it there (#7664). Cloning an instruction is cloning its
+/// operand tokens; nothing in a variant is an identity.
+#[derive(Clone)]
 pub enum LlInst {
     /// Pre-rendered instruction line, two-space indent included.
     Raw(String),

crates/perry-codegen/src/lower_call/new.rs

@@ -1014,15 +1014,15 @@ fn lower_new_impl_inner(
         // ponytail: a throw inside the ctor skips the restore, leaving the cell
         // set — same edge case the runtime construct paths already have; fix
         // holistically if it bites.
+        // #7664: `prev` is saved across the WHOLE constructor body, and the
+        // cell it comes out of is a registered mutable root that evacuation
+        // rewrites — so it goes in a temp root, not a bare register.
         let saved_new_target = if ctor_chain_uses_new_target(ctx, class) {
-            ctx.class_ids.get(class_name).map(|&cid| {
-                let prev = ctx.block().call(DOUBLE, "js_new_target_get", &[]);
+            ctx.class_ids.get(class_name).copied().map(|cid| {
                 let class_ref = double_literal(f64::from_bits(
                     crate::nanbox::INT32_TAG | (cid as u64 & 0xFFFF_FFFF),
                 ));
-                ctx.block()
-                    .call(DOUBLE, "js_new_target_set", &[(DOUBLE, &class_ref)]);
-                prev
+                crate::rooting::new_target_save(ctx, &class_ref)
             })
         } else {
             None
@@ -1034,9 +1034,8 @@ fn lower_new_impl_inner(
             &lowered_args,
             caps_absent_from_args,
         ) {
-            if let Some(prev) = &saved_new_target {
-                ctx.block()
-                    .call(DOUBLE, "js_new_target_set", &[(DOUBLE, prev)]);
+            if let Some(save) = &saved_new_target {
+                crate::rooting::new_target_restore(ctx, save);
             }
             // #7154: the constructor body has run, so every register holding
             // the instance is potentially pre-move. Re-read it from its root
@@ -1081,9 +1080,8 @@ fn lower_new_impl_inner(
             );
             return Ok(final_box);
         }
-        if let Some(prev) = &saved_new_target {
-            ctx.block()
-                .call(DOUBLE, "js_new_target_set", &[(DOUBLE, prev)]);
+        if let Some(save) = &saved_new_target {
+            crate::rooting::new_target_restore(ctx, save);
         }
         // #6921: `call_local_constructor_symbol` returned `None` — this module
         // has no `<Class>_constructor` entry, so no constructor ran and the
@@ -1760,13 +1758,10 @@ fn lower_new_impl_inner(
                 // 'type')`, or silently set `type = undefined` → the auth error
                 // was mis-categorized and the login redirect fell back to
                 // `?error=Configuration`.
-                let nt_prev = ctx.block().call(DOUBLE, "js_new_target_get", &[]);
                 let nt_ref = double_literal(f64::from_bits(new_target_bits));
-                ctx.block()
-                    .call(DOUBLE, "js_new_target_set", &[(DOUBLE, &nt_ref)]);
+                let nt_save = crate::rooting::new_target_save(ctx, &nt_ref);
                 let _ = ctx.block().call(DOUBLE, &ctor.symbol, &ctor_args);
-                ctx.block()
-                    .call(DOUBLE, "js_new_target_set", &[(DOUBLE, &nt_prev)]);
+                crate::rooting::new_target_restore(ctx, &nt_save);
             } else if let Some(ctor) = ctx.imported_class_ctors.get(class_name).cloned() {
                 // Pad missing optional args with TAG_UNDEFINED so the constructor
                 // doesn't read garbage from stale registers, and pack the rest
@@ -1798,13 +1793,10 @@ fn lower_new_impl_inner(
                 // new.target cross-module: bind the runtime cell to the leaf
                 // class ref around the imported ctor call (see the ANCESTOR arm
                 // above for why). This is the direct `new ImportedClass()` case.
-                let nt_prev = ctx.block().call(DOUBLE, "js_new_target_get", &[]);
                 let nt_ref = double_literal(f64::from_bits(new_target_bits));
-                ctx.block()
-                    .call(DOUBLE, "js_new_target_set", &[(DOUBLE, &nt_ref)]);
+                let nt_save = crate::rooting::new_target_save(ctx, &nt_ref);
                 let ctor_ret = ctx.block().call(DOUBLE, &ctor.symbol, &ctor_args);
-                ctx.block()
-                    .call(DOUBLE, "js_new_target_set", &[(DOUBLE, &nt_prev)]);
+                crate::rooting::new_target_restore(ctx, &nt_save);
                 ctx.block().store(DOUBLE, &ctor_ret, &ctor_result_slot);
                 found_inherited_ctor = true;
             }