From 3bdc1a8ab8c9088802c0fde9f0bcdec4a50e039b Mon Sep 17 00:00:00 2001 From: jdalton Date: Sat, 1 Aug 2026 18:55:53 -0400 Subject: [PATCH] docs(gc): correct the dominance direction, the shape summary, and the allowlist fingerprint; restore the workspace version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-ups to the review threads on #7196 and #7212, both of which merged before the findings were worked through. Cargo.toml — #7196 set `[workspace.package].version` back to 0.5.1277, undoing the 0.5.1278 bump #7199 had landed four commits earlier. Nothing since has touched the line, so main is currently shipping a version number it already released. Restored. docs/src/internals/gc-rooting-invariant.md — the checker description read "reports any root store that does not dominate a preceding collection point", which is vacuous: a store can never dominate anything that precedes it, so the sentence is true of every store in the program. What the checker actually reports is the collection point — `window_hits(origin, bind)` collects the calls that can run between the instruction producing a GC value and the `js_shadow_slot_bind` that publishes it. Reworded to name the collection point as the reported object and to keep the true relation (the root store must dominate it), which is the rule stated at the top of the same page. Also noted that the gate command shown does not pass `--stale-registers`, so cases 3 and 4 only surface when it is run by hand. CLAUDE.md — the shape summary claimed all three failures present as "a rooted slot holding a dangling pointer", contradicting its own two preceding clauses: #7184's bind is a silent no-op so nothing is bound, and the `alloca_entry` shape is never a slot at all. Split per shape. It also said the class "only bites under PERRY_GC_MOVING_LOOP_POLLS=1"; the checker's own POLL_CAPABLE_RUNTIME set treats js_object_set_field_by_name, js_object_get_property and js_call_function as moving-capable with no poll involved, and #7211's allowlist entry is exactly such a window. Polls widen in-loop coverage; they are not a precondition. docs/src/internals/rfc-rooting-by-construction.md — `Plain(String)` cannot be `Copy`; the migration section costed it as if it were. It is `Clone`, which still imposes nothing on the caller. Added the emitter/frame branding gap to "What it cannot catch": PhantomData<&'e Emitter> records a lifetime, not an instance, and `Rooted` carries a bare SlotIdx, so the design as written catches ordering mistakes but not provenance ones. scripts/gc_root_dominance_allowlist.json — the fingerprint format line said "". It is `sorted(set(callees))[0]`, the alphabetically first, not the first in program order. A hand-derived entry that guesses program order matches nothing, and an entry that matches nothing fails the build — so the misleading line pointed straight at a red gate. scripts/gc_root_dominance_check.py, three surgical changes: * the self-test failure text described sinking the root store; `_mutate` splices _SEED_CALL above the store and moves nothing. * `--stale-registers` returned before the `--unrooted-allocas` block, so passing both ran one pass and silently skipped the other. Now an argparse error, matching the --max-stale and --fatal-sinks guards directly above it. * the stale-allowlist-entry report returned 2 before the uncovered- violation report could run. Fix one violation and introduce another in the same PR and the log showed only the bookkeeping problem. Both reports now print; the exit code is unchanged (2 when an entry is stale, 1 when only uncovered violations remain). Verified: `--self-test` OK. Against the parent, a corpus with one stale entry and two uncovered violations printed only the stale entry and hid both violations; it now prints all three and still exits 2. Refs #7196, #7212, #7199, #7211. --- docs/src/internals/gc-rooting-invariant.md | 44 ++++++++++++++----- docs/src/internals/memory-model.md | 4 +- .../internals/rfc-rooting-by-construction.md | 24 ++++++++-- scripts/gc_root_dominance_allowlist.json | 10 ++++- scripts/gc_root_dominance_check.py | 23 ++++++++-- 5 files changed, 84 insertions(+), 21 deletions(-) diff --git a/docs/src/internals/gc-rooting-invariant.md b/docs/src/internals/gc-rooting-invariant.md index df8ee4beda..1502597ae1 100644 --- a/docs/src/internals/gc-rooting-invariant.md +++ b/docs/src/internals/gc-rooting-invariant.md @@ -140,10 +140,14 @@ python3 scripts/gc_root_dominance_check.py ir-corpus --moving-only \ ``` It parses the emitted LLVM IR, builds per-function CFGs, computes real -Cooper/Harvey/Kennedy dominance, and reports any root store that does not -dominate a preceding collection point. It is one-sided: an unrecognised call -counts as collecting, so a gap in its model costs a false positive, never a -missed bug. +Cooper/Harvey/Kennedy dominance, and reports every **collection point** that can +run between the instruction producing a GC value and the root store that +publishes it — that is, a collection point the value's root store does **not** +dominate, which is exactly the rule at the top of this page. Dominance is what +makes the report sound in both directions: the producing instruction must +dominate the bind, so the register being rooted really is the one that +instruction produced on every path. It is one-sided: an unrecognised call counts +as collecting, so a gap in its model costs a false positive, never a missed bug. For a single file you are iterating on: @@ -153,15 +157,35 @@ PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_INLINE_SHADOW_SLOT=0 \ python3 scripts/gc_root_dominance_check.py .perry-trace/llvm -v ``` -Both env knobs matter. `PERRY_GC_MOVING_LOOP_POLLS=1` is what puts -`js_gc_loop_safepoint` in the IR, which the `MOVING` classification keys on; -without it the corpus **cannot express the bug**. `PERRY_INLINE_SHADOW_SLOT=0` -makes every root store the `js_shadow_slot_bind` call form the checker anchors -on. +Both env knobs matter, for different reasons. + +`PERRY_GC_MOVING_LOOP_POLLS=1` is what puts `js_gc_loop_safepoint` in the IR. It +is the only collection point a **back edge itself** introduces — a loop whose +body calls nothing that collects still collects, once per iteration, and only +with this on. So a bug that needs a collection between two points of an +otherwise inert loop body cannot appear in the corpus without it. + +It is **not** what makes the `MOVING` classification work, and it is not the +only collection point that can run inside a loop — a `POLL_CAPABLE_RUNTIME` +helper called from a loop body is in-loop too. `movers` +(`gc_root_dominance_check.py:576-579`) counts `js_gc_loop_safepoint`, anything +in `poll_reaching`, **and** anything in `POLL_CAPABLE_RUNTIME` — the runtime +helpers that can re-enter JS, such as `js_object_set_field_by_name`, +`js_object_get_property` and `js_call_function`. Those are moving with no poll +anywhere near them. As of this writing all five violations the gate reports are +`MOVING: YES via js_object_set_field_by_name`; not one of them needs a poll. + +So: turn the knob on, because it widens what the corpus can express, but do not +read a poll-free function as safe. + +`PERRY_INLINE_SHADOW_SLOT=0` makes every root store the `js_shadow_slot_bind` +call form the checker anchors on. `--stale-registers` (#7206) additionally catches values that are *never* rooted — read out of a root and held in a register across a collection point. That is -the mode that found cases 3 and 4. +the mode that found cases 3 and 4. It ships and works, but **the gate command +above does not pass it**: the bind-anchored scan is the arm that is baselined by +the allowlist, so cases 3 and 4 only surface when you run this mode by hand. `--unrooted-allocas` (#7207) covers the remaining shape, and is the one the bind-anchored check is structurally blind to: the value lives in a plain diff --git a/docs/src/internals/memory-model.md b/docs/src/internals/memory-model.md index 66f2f02098..e0398f84f5 100644 --- a/docs/src/internals/memory-model.md +++ b/docs/src/internals/memory-model.md @@ -136,8 +136,8 @@ to collapse that detection latency. **All are default-off and inert when off.** | `PERRY_GC_ZEAL=1` | Force an evacuating minor at **every GC safepoint** — loop back-edge polls and the outermost microtask-pump boundary — instead of only when nursery pressure is due. Implies `PERRY_GC_FORCE_EVACUATE`, so survivors actually move — but an explicit `PERRY_GEN_GC_EVACUATE=0` still wins, and with it set zeal moves nothing and therefore surfaces nothing. Zeal also does **not** bypass `gc_safepoint_moving_minor`'s entry guards (in-allocation, suppressed, unsafe FFI zone, non-zero root-lock depth, budgeted cycle): a safepoint reached in any of those states still declines to collect. Modelled on V8 `--stress-scavenge` / SpiderMonkey `gcZeal`. Composes with the two above; that pairing is what turns a rooting bug into an immediate precise fault. | | `PERRY_GC_FROMSPACE_SCAN_ABORT=1` | Abort on the **first** offending slot the whole-heap from-space scan finds, printing slot, holder, target (including the target's `obj_type`) and a collector backtrace. Now implies `PERRY_GC_FROMSPACE_SCAN=1`; previously it was silently inert on its own. | -Two caveats these instruments are explicit about, because both have burned -prior investigations: +These instruments have explicit caveats, because each has burned a prior +investigation: - `PERRY_GC_PROTECT_FROMSPACE` gates **only** the copying minor's from-space reset. A run with the knob on and zero copying minors protects nothing. Check diff --git a/docs/src/internals/rfc-rooting-by-construction.md b/docs/src/internals/rfc-rooting-by-construction.md index 3132f9d891..8f3c28c1e7 100644 --- a/docs/src/internals/rfc-rooting-by-construction.md +++ b/docs/src/internals/rfc-rooting-by-construction.md @@ -73,7 +73,8 @@ pub struct Rooted { } /// A register holding something the GC does not manage: i32, double, bool, -/// a slot index. Freely copyable, no lifetime. +/// a slot index. Freely clonable, no lifetime, no borrow of the emitter. +#[derive(Clone)] pub struct Plain(String); ``` @@ -160,9 +161,11 @@ The honest number is large but the distribution is favourable. - **~2500 builder call sites**, 35 files. Most are *not* GC-managed: loop counters, `double` arithmetic, NaN-box bit twiddling, slot indices. Those - become `Plain`, which is `Copy` and imposes nothing. A rough read of the call - sites suggests **300–500 genuinely handle GC pointers** — the ones in - `expr/`, `lower_call/`, and the object/array/closure literal paths. + become `Plain`, which is `Clone` and imposes nothing — it holds a register + name, so it cannot be `Copy`, but it borrows nothing and outlives every `&mut` + emit. A rough read of the call sites suggests **300–500 genuinely handle GC + pointers** — the ones in `expr/`, `lower_call/`, and the object/array/closure + literal paths. - **8 `rooted_handle_begin` sites** exist today, so the *explicit* rooting surface is currently tiny. That is the point: the sites that need rooting and do not have it are the bugs. @@ -216,6 +219,19 @@ than one known to be partial: table is trusted input. This is why the checker must stay: it derives its verdict from the emitted IR, so the two failure modes are not correlated. - **The escape hatch**, for as long as any caller uses it. +- **Confusion of two emitters, or of two shadow frames.** `PhantomData<&'e + Emitter>` records a *lifetime*, not an *instance*, and `&'e T` makes `Raw<'e>` + **covariant** in `'e` — a longer-lived `Raw` shortens to any compatible `'e`. + Nothing in the type names *which* emitter it came from, so a `Raw` minted by + one emitter type-checks against a different `&mut Emitter` whose borrow + region fits. The same hole exists for `Rooted`, which carries a bare + `SlotIdx` and so does not name the `ShadowFrame` that allocated it; a + `Rooted` outliving its frame's pop, or read against a sibling frame, is + accepted. Closing this needs invariant branding (a generic `Id` parameter + over an invariant lifetime, `GhostToken`-style) rather than `PhantomData` + alone, and `Rooted` needs to borrow its frame. That is a real cost to price + into step 1 — as written, the design catches *ordering* mistakes, not + *provenance* ones. - **Runtime-side rooting.** `RuntimeHandleScope` in `perry-runtime` is a separate discipline over hand-written Rust; nothing here touches it. - **Anything interprocedural.** A lowering that returns a `Raw` to a caller that diff --git a/scripts/gc_root_dominance_allowlist.json b/scripts/gc_root_dominance_allowlist.json index 936b642598..fba4bcbd84 100644 --- a/scripts/gc_root_dominance_allowlist.json +++ b/scripts/gc_root_dominance_allowlist.json @@ -19,8 +19,14 @@ "green is the thing this file exists to prevent -- if the count went up, a", "new violation was introduced.", "", - "Fingerprint format: .ll::::->", - "Get it from the checker's own output (`-v` prints one per violation)." + "Fingerprint format:", + " .ll::::->", + "The collector is `sorted(set(callees))[0]` over EVERY collector in the", + "window -- the alphabetically first, NOT the first one in program order.", + "Do not hand-write it from reading the IR; copy it out of the checker's own", + "output (`-v` prints one per violation). A hand-derived entry that guesses", + "program order matches nothing, and an entry that matches nothing is a", + "build failure." ], "entries": [ { diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index a6214460d4..6c1d61e85d 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -1875,8 +1875,8 @@ def _write_allowlist(entries): fh.write("\n".join(_mutate(clean_lines, sites[0])) + "\n") got, _ = _scan([mutant], False, "alloc") if not got: - print("self-test FAIL: sinking the root store below the " - "collecting call in the CLEAN fixture must produce a " + print("self-test FAIL: splicing a collecting call above the " + "root store in the CLEAN fixture must produce a " "violation; the mutator or the checker is broken", file=sys.stderr) ok = False @@ -1948,6 +1948,12 @@ def main(): ap.error("--max-stale requires --stale-registers") if ns.fatal_sinks and not ns.stale_registers: ap.error("--fatal-sinks requires --stale-registers") + # Same rule: --stale-registers returns before the --unrooted-allocas block, + # so passing both would run the stale scan and silently skip the alloca one + # while the command line claims both. + if ns.stale_registers and ns.unrooted_allocas: + ap.error("--stale-registers and --unrooted-allocas are separate " + "passes; run them one at a time") if ns.self_test: return self_test() @@ -2108,6 +2114,12 @@ def render(v): return 2 # --- allowlist hygiene -------------------------------------------------- + # + # Both reports are printed before either return. Fixing one violation and + # introducing a different one in the same PR makes an entry stale AND + # produces an uncovered violation; returning on the stale entry first would + # print only the bookkeeping problem and hide the actual new bug -- the one + # finding this gate exists for. stale = stale_entries(allowlist) if stale: print("error: allowlist entries matched nothing:", file=sys.stderr) @@ -2117,7 +2129,6 @@ def render(v): "PR, that is the ratchet — or the corpus shrank and no longer " "contains the module it names, which means this run checked less " "than it claims to.", file=sys.stderr) - return 2 if remaining: if not verbose: @@ -2129,6 +2140,12 @@ def render(v): "gc-rooting-invariant.md. If this is genuinely known and tracked, " "add an entry with an issue and a justification — never a count " "bump.", file=sys.stderr) + + # A stale entry is the more specific diagnosis (the allowlist itself is + # wrong), so it keeps its exit code where both fire. + if stale: + return 2 + if remaining: return 1 # --- can this gate still fail? ------------------------------------------