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? ------------------------------------------