From 235c66d856b12e82755b5bd71cbcaf3cbf7d5d8c Mon Sep 17 00:00:00 2001 From: jdalton Date: Mon, 3 Aug 2026 01:37:44 -0400 Subject: [PATCH] fix(gc): classify the emitted property-GET dispatch as poll-capable (#7154) `--stale-registers` and `PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800` disagreed at zod's `clone`: the protector faulted deterministically, the checker classified the window `MOVING: no`, and the `--moving-only` arm `gc-root-dominance.yml` gates on dropped it. The checker was wrong. `POLL_CAPABLE_RUNTIME` is an exact-symbol set. It carried `js_object_get_field_by_name` next to `js_object_set_field_by_name`, which reads as symmetric coverage of property access and is not: codegen emits the SET verbatim and never emits the GET, lowering every read to `js_object_get_field_by_name_f64` / `js_object_get_field_ic_miss` / `js_typed_feedback_object_get_field_by_name_f64` (1324 / 532 / 556 calls in the gate corpus), none of which were in the set. 31 stale uses with a GET helper in their window classified `MOVING: no`. Auditing the set the way #7227 audits ALLOC_RE found ten of twenty-eight entries naming symbols that do not exist, four of them four different spellings of "call a JS closure". Each is replaced by the emitted symbol carrying its premise rather than deleted. `--audit-poll-capable` is the gate for that, and a new `--self-test` fixture pair pins the property-GET window in both directions. Both audits and both `--moving-only` gate arms are green; the raw `--stale-registers` count is unchanged at 4860 and `--moving-only` moves 65 -> 115. The `gc-root-dominance.yml` step that runs `--audit-poll-capable` is held back to a follow-up commit: pushing a workflow change needs an OAuth token scope this session does not have. --- .github/workflows/gc-root-dominance.yml | 21 ++ ...284-poll-capable-property-get-and-audit.md | 188 ++++++++++ docs/src/internals/gc-rooting-invariant.md | 76 +++- scripts/gc_root_dominance_check.py | 333 +++++++++++++++++- 4 files changed, 607 insertions(+), 11 deletions(-) create mode 100644 changelog.d/7284-poll-capable-property-get-and-audit.md diff --git a/.github/workflows/gc-root-dominance.yml b/.github/workflows/gc-root-dominance.yml index e54815f4b1..6b8f51c4cb 100644 --- a/.github/workflows/gc-root-dominance.yml +++ b/.github/workflows/gc-root-dominance.yml @@ -116,6 +116,27 @@ jobs: - name: ALLOC_RE alternatives must match real runtime symbols run: python3 scripts/gc_root_dominance_check.py --audit-alloc-re + # The same hazard on the other axis, and a sharper one. `ALLOC_RE` + # decides whether a register has a heap-value source; `POLL_CAPABLE_RUNTIME` + # decides whether the window around it is MOVING -- which is the arm the + # two `--moving-only` steps below actually gate on. An entry naming a + # symbol that does not exist can never classify a window, so it reads as + # coverage of an operation the gate is in fact blind to, and unlike a dead + # regex alternative it survives review because the misspelling is a + # PLAUSIBLE name for a real operation. + # + # Ten of twenty-eight entries were phantoms when this step was added, + # including four separate spellings of "call a JS closure" (`js_call_closure`, + # `js_invoke_closure`, `js_function_call`, `js_apply_function`) where the + # real symbols are `js_closure_callN`. The consequence was measured at + # zod's `clone`: the from-space protector faulted deterministically on a + # window that `--stale-registers` classified `MOVING: no`, so the + # `--moving-only` arm could not see it. + # + # Static and instant, like the audit above, so it runs before the build. + - name: POLL_CAPABLE_RUNTIME entries must name real runtime symbols + run: python3 scripts/gc_root_dominance_check.py --audit-poll-capable + # `--unrooted-allocas` exempts two heap-value sources whose objects can # neither move nor be reclaimed (#7210): the old-arena class-keys array, # and `js_box_alloc*`'s Box, which is not a GC-heap allocation at all. diff --git a/changelog.d/7284-poll-capable-property-get-and-audit.md b/changelog.d/7284-poll-capable-property-get-and-audit.md new file mode 100644 index 0000000000..dbbae22ac1 --- /dev/null +++ b/changelog.d/7284-poll-capable-property-get-and-audit.md @@ -0,0 +1,188 @@ +### Fixed + +- **`scripts/gc_root_dominance_check.py`: the property-GET dispatch is + `POLL_CAPABLE_RUNTIME`, so `--moving-only` can finally see the GET half of + property access** (#7154). Two of the effort's instruments disagreed at zod's + `clone`: `PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800` + faulted deterministically, while `--stale-registers` classified the same + window `MOVING: no` — so the arm `gc-root-dominance.yml` gates on could not + see it. **The checker was the wrong one.** + + Membership in `POLL_CAPABLE_RUNTIME` is by exact emitted symbol. The set + carried `js_object_get_field_by_name` next to `js_object_set_field_by_name`, + which reads as symmetric coverage of property access and is not: codegen + emits the SET verbatim (27 call sites in perry-codegen, 205 in the gate + corpus) and **never emits the GET at all**. Every property read lowers to + `js_object_get_field_by_name_f64`, `js_object_get_field_ic_miss` or + `js_typed_feedback_object_get_field_by_name_f64` — 1324 / 532 / 556 calls in + the same corpus — and not one of the three was in the set. Property sets + classified `MOVING: YES`; property gets classified `MOVING: no`. That + asymmetry is what put "all five violations the gate reports are `MOVING: YES + via js_object_set_field_by_name`" in the rooting-invariant doc, and it left + the whole GET side unread. + + The premise for each addition is the one the set already granted the name it + could not match: `js_object_get_field_by_name` resolves a user getter through + a transmuted function pointer + (`object/field_get_set/get_field_by_name.rs:1064`) and routes a registered + Proxy to `js_proxy_get` (`:84`) — arbitrary JS with its own back-edge polls. + `js_object_get_field_by_name_f64` (`ic_miss.rs:29`), + `js_object_get_field_by_name_boxed` (`:81`), + `js_object_get_field_by_property_id_f64` (`:123`), + `js_object_get_field_ic_miss` (`:228`, `:275`, and `:252` to `js_proxy_get`), + `js_object_get_field_ic` (`:526`, `:544`, `:561`) and + `js_typed_feedback_object_get_field_by_name_f64` (`typed_feedback.rs:992`) + all reach it. + +- **`scripts/gc_root_dominance_check.py`: ten `POLL_CAPABLE_RUNTIME` entries + named symbols that do not exist.** Auditing the set the way #7227 audited + `ALLOC_RE` found that TEN of its twenty-eight entries matched no + `extern "C" fn js_*` anywhere in perry-runtime or perry-stdlib: + `js_apply_function`, `js_array_for_each`, `js_array_sort`, + `js_call_closure`, `js_call_value`, `js_function_call`, `js_invoke_closure`, + `js_object_get_property`, `js_object_set_property`, `js_string_replace`. + + Four of the ten were four different spellings of "call a JS closure", so the + most obviously poll-capable operation in the language was covered zero times + — while `RECEIVER_SINKS`, three hundred lines away in the same file, already + spelled it `closure_call\w*`. Each phantom is **replaced** by the emitted + symbol carrying its premise, never merely deleted (deleting turns the audit + green and leaves the hole): `js_closure_call0`–`16`, + `js_closure_call_array`, `js_closure_call_apply_with_spread` + (`closure/dispatch/calln.rs:33`); `js_native_call_value` + (`closure/dispatch/value_call.rs:17`); `js_array_sort_default` / + `js_array_sort_with_comparator` (`array/sort.rs:542,635`); + `js_typed_array_for_each` (`typedarray/iterate.rs:137`); + `js_object_get_property_key` / `js_object_set_property_key` / + `js_object_set_property_key_method` (`object/property_key.rs:154,210,257`); + and the four `js_string_replace_*_fn` variants that take a user replacer + callback. The `_dyn` and plain `string_replace` variants are a separate + coverage decision with their own hit count and are deliberately left out, + same reasoning as `ALLOC_RE`'s deleted `bigint_\w+_op`. + +### Added + +- **`scripts/gc_root_dominance_check.py --audit-poll-capable`, and a + `gc-root-dominance.yml` step for it.** `--audit-alloc-re` (#7227) exists + because a regex alternative matching nothing reads as coverage. This is the + same hazard on the other axis and a sharper one: `ALLOC_RE` decides whether a + register *has* a heap-value source, `POLL_CAPABLE_RUNTIME` decides whether + the window around it is *moving*, and the moving classification is what the + two `--moving-only` gate arms actually key on. A phantom entry is invisible + even to a careful reader, because the misspelling is a plausible name for a + real operation. The audit fails on any entry that names no exported + `extern "C" fn js_*`, is static and instant, and runs before the build next + to the ALLOC_RE audit. It is itself self-tested in both directions, and the + self-test additionally pins the three GET-family names in place by name. + +- **A `--self-test` fixture pair for the property-GET window.** Reduced from + the real emitted IR (`property_get/generic_dispatch.rs`'s `pget.recv_ok` + block, and the full-outline `js_object_get_field_ic` at + `object/field_get_set/ic_miss.rs:544`): a bound shadow slot is loaded into a + bare register, the generic GET dispatch runs, and the register is + dereferenced below it. The control re-reads the slot below the dispatch and + must report nothing. The arm asserts the raw report *and* survival under + `--moving-only`, because the raw arm was never the broken one. + +## Verification + +No Rust changed — `git diff --stat` is three files, all Python / YAML / +Markdown — so `cargo test` is untouched by construction rather than by +measurement. + +Corpus: 141 `.ll` files, 2321 functions, 3068 root stores, emitted by +`./scripts/gc_root_dominance_corpus.sh` from a build of this worktree. + +### The adjudication, measured + +| over the gate corpus | parent (`97c69211d`) | this PR | +|---|---|---| +| compiled functions in `poll_reaching` | 473 | **671** | +| `--stale-registers`, raw | 4860 | 4860 | +| `--stale-registers --moving-only` | 65 | **115** | +| `--stale-registers --fatal-sinks` | 1055 | 1055 | +| `--stale-registers --moving-only --fatal-sinks` | 1 | **13** | +| bind-anchored `--moving-only` (**the gate**) | 0 | **0**, allowlist still empty | +| `--unrooted-allocas --moving-only` (**the gate**) | 0 | **0** | + +The raw counts do not move, which is the point: this change reclassifies +windows, it does not widen what counts as a heap-value source. 55 +`--stale-registers` uses have an emitted property-GET helper in their window +and 31 of them classified `MOVING: no` before this change, so `--moving-only` +dropped all 31. + +The twelve newly moving-*and*-fatal leads, triaged by shape (diagnostic mode, +not gated): + +| sink | count | classified moving via | +|---|---|---| +| `js_object_set_field_by_name` | 6 | `js_object_get_field_by_name_f64` | +| `js_set_function_prototype` | 3 | `js_object_get_field_by_name_f64`, `js_object_get_field_ic_miss` | +| `js_array_push_f64` | 1 | `js_object_get_field_by_name_f64` | +| `js_closure_call1` | 1 | `js_closure_call1` | +| `js_typed_feedback_closure_direct_call_guard` | 1 | `js_closure_call1` | + +### The gate can now fail on the shape + +The self-test arm goes red against the parent's `POLL_CAPABLE_RUNTIME` and +green with this one: + +``` +$ python3 --self-test +self-test FAIL: the generic property-GET dispatch resolves a user getter +(object/field_get_set/get_field_by_name.rs:1064) and routes a Proxy to +js_proxy_get (:84), so a register held across it MUST classify MOVING and +survive --moving-only. Got {} ... +exit 1 +``` + +And the gate's own command, over a corpus mutant that splices exactly one +`js_object_get_field_ic_miss` into one real allocation-to-root-store gap — +nothing else changed: + +| `--moving-only --allowlist … --seeded-violations 40` over the mutant corpus | parent | this PR | +|---|---|---| +| violations | 0 | **1** | +| exit status | **0 (green — blind)** | **1 (red)** | + +``` +test_gap_array_iterator_next…::perry_closure_…__1 + alloc : %r15 = call i64 @js_closure_alloc_singleton(...) + store : store double %r17, ptr %r14 + bind : slot 3 call void @js_shadow_slot_bind(i32 3, ptr %r14) + between: js_object_get_field_ic_miss + MOVING : YES via js_object_get_field_ic_miss +``` + +Complementary direction, on the unmutated corpus: reseeding +`--seeded-violations`' spliced collection point with +`js_object_get_field_ic_miss` instead of `js_call_function` gives +`40 planted, 0 caught, 40 MISSED` on the parent and `40 planted, 40 caught` +here. + +### The two hypotheses that were NOT the answer + +Both were investigated before editing, and both are recorded because ruling +them out is most of the adjudication. + +- **"Attribution mismatch — the register goes stale in a caller and arrives as + an argument."** `heap_source_kind` genuinely has no incoming-parameter source + kind, so a stale *parameter* register would be invisible in the callee. It is + a real gap and it is not this one: perry spills and binds parameters + (`store double %argN, ptr %rM` + `js_shadow_slot_bind`), which makes the + re-read a `slotload` source the checker already models. Measured over the + corpus, 1708 parameters are spilled and 256 are not; of the 256, exactly 4 + are used below a collecting call and all 4 are `double`/`i64` arithmetic in + typed-f64 specialisations, none of them a pointer. That gap needs its own + change with its own measured hit count; the new fixture passes its receiver + and key as bare parameters so the shape is at least on the record. + +- **"Protector overreach — DEPTH=800 proves staleness against *some* prior + collection, not this window."** Structurally impossible. A page-set enters + the quarantine only because an evacuating minor retired it as from-space + (the knob gates `copying_reset_from_spaces_and_flip` and nothing else, + `arena/quarantine.rs`), so any fault on a quarantined address is a genuine + stale read at any depth. Eviction hands blocks **back to Eden**, where the + same read silently succeeds — so depth removes false negatives and cannot + manufacture a false positive. "It only faults at DEPTH=800" is never grounds + to doubt the protector. The rooting-invariant doc now says so. diff --git a/docs/src/internals/gc-rooting-invariant.md b/docs/src/internals/gc-rooting-invariant.md index 0f3b12bd06..cba7a0d0dc 100644 --- a/docs/src/internals/gc-rooting-invariant.md +++ b/docs/src/internals/gc-rooting-invariant.md @@ -168,16 +168,57 @@ 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 +(`gc_root_dominance_check.py`, the `movers` property on `Violation`, `StaleUse` +and `UnrootedAlloca`) 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. +`js_object_get_field_ic_miss` and `js_closure_call1`. Those are moving with no +poll anywhere near them. So: turn the knob on, because it widens what the corpus can express, but do not read a poll-free function as safe. +### `POLL_CAPABLE_RUNTIME` is by EXACT emitted symbol, and that has bitten twice + +`movers` is a set-membership test on the callee name, so an entry that names a +symbol codegen does not emit classifies nothing, forever, and looks exactly +like coverage while doing it. Two rounds of this have now been measured: + +1. **A real symbol codegen never emits.** The set carried + `js_object_get_field_by_name` next to `js_object_set_field_by_name`, which + reads as symmetric coverage of property access. It is not: codegen emits the + SET verbatim but lowers every GET to `js_object_get_field_by_name_f64`, + `js_object_get_field_ic_miss` or + `js_typed_feedback_object_get_field_by_name_f64`, none of which were in the + set. Property sets classified `MOVING: YES`, property gets classified + `MOVING: no`, and 31 `--stale-registers` hits on the gate corpus were dropped + by `--moving-only` as a result — including the shape that faults + deterministically under `PERRY_GC_PROTECT_FROMSPACE=1 + PERRY_GC_PROTECT_FROMSPACE_DEPTH=800` at zod's `clone`. The protector and the + checker disagreed; the checker was wrong. +2. **Ten names that were not symbols at all.** `js_apply_function`, + `js_array_for_each`, `js_array_sort`, `js_call_closure`, `js_call_value`, + `js_function_call`, `js_invoke_closure`, `js_object_get_property`, + `js_object_set_property`, `js_string_replace` — extrapolated spellings, none + of them an `extern "C" fn` anywhere in the runtime. Four of the ten were four + different ways of saying "call a JS closure", so the single most obviously + poll-capable operation in the language was covered zero times; the real + entry points are `js_closure_callN`, which `RECEIVER_SINKS` in the same file + already spelled correctly. + +`--audit-poll-capable` is the gate for this, and `gc-root-dominance.yml` runs it +alongside `--audit-alloc-re` before the build. It fails on any entry that names +no exported `extern "C" fn js_*`. When it goes red, **replace** the phantom with +the symbol codegen actually emits rather than deleting it — deleting turns the +audit green and leaves the hole. + +Checking a *plausible* name is not enough. Confirm against emitted IR: + +```bash +grep -ho 'call [^@]*@js_[A-Za-z0-9_.$]*(' ir-corpus/*.ll \ + | sed -E 's/.*@([A-Za-z0-9_.$]+)\($/\1/' | sort | uniq -c | sort -rn +``` + `PERRY_INLINE_SHADOW_SLOT=0` makes every root store the `js_shadow_slot_bind` call form the checker anchors on. @@ -220,6 +261,23 @@ From #7196: > holding the block your stale pointer is in by the time it is dereferenced. > **Use 800.** A clean run at the default depth means nothing. +Depth is a **detection-window** knob, not a sensitivity knob, and the +difference matters when the two instruments disagree. A page-set enters the +quarantine only because an evacuating minor actually retired it as from-space +(`arena/quarantine.rs`, and the knob gates only +`copying_reset_from_spaces_and_flip`), so *any* fault on a quarantined address +is a genuine stale read no matter how deep the ring is. Raising the depth +removes false NEGATIVES — evicting a set hands its blocks back to Eden, where +the same read silently succeeds — and cannot manufacture a false positive. So +"the protector faulted, but only at DEPTH=800" is never grounds to doubt the +protector; when it disagrees with the static checker, look at the checker first. +That is how the zod `clone` disagreement was settled. + +And when a fault does fire, **walk UP the stack**. The reporter names the frame +that DEREFERENCED the stale value, which is usually not the frame that owns the +register — the value commonly arrives as an argument from a caller that let it +go stale. + And remember the ceiling on all of these: if the collection happens while the only copy is in a register, there is nothing at that moment for any runtime probe to notice. These instruments catch the *consequence*, later. The static @@ -243,7 +301,13 @@ It is built to be able to fail, against all four hazards in CLAUDE.md: - `--self-test` proves it still fires on planted fixtures, and `--seeded-violations 40` splices collection points into the **real** corpus IR and requires all 40 to be reported — that is the arm that catches the checker - silently losing the ability to read perry's output. + silently losing the ability to read perry's output; +- `--audit-alloc-re` and `--audit-poll-capable` refuse a name that matches no + exported runtime symbol, in the two tables that decide *whether a register has + a heap-value source* and *whether the window around it is moving*. Both run + before the build, because both are static and instant and both have shipped + dead entries: nine in `ALLOC_RE` across two rounds, ten in + `POLL_CAPABLE_RUNTIME`. ### The allowlist, and why it is not a number diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index af13eb6481..6ff59e6297 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -483,6 +483,59 @@ def audit_alloc_re(roots=SYMBOL_ROOTS): print("=== every alternative matches at least one exported symbol") return 0 + +def dead_poll_capable(symbols): + """`POLL_CAPABLE_RUNTIME` entries that name no exported symbol, sorted. + + No exceptions, deliberately — `MOVING_POLL` itself is a real export + (`gc/policy.rs:1766`), so carving one out would be an unearned hole in the + very audit that exists because unearned holes are the recurring bug here. + """ + return sorted(n for n in POLL_CAPABLE_RUNTIME if n not in symbols) + + +def audit_poll_capable(roots=SYMBOL_ROOTS): + """Exit status for `--audit-poll-capable`. 0 clean, 2 on a phantom entry. + + Same hazard as `--audit-alloc-re`, in the other direction. `ALLOC_RE` + decides whether a register HAS a heap-value source; `POLL_CAPABLE_RUNTIME` + decides whether the window around it is MOVING, which is what + `gc-root-dominance.yml`'s `--moving-only` arms gate on. An entry that names + nothing reads as coverage and suppresses nothing, and unlike a dead + ALLOC_RE alternative it is invisible even to a careful reader, because the + misspelling is usually a *plausible* name for a real operation. + + Measured when this auditor was written: TEN of twenty-eight entries were + phantoms, including four separate spellings of "call a JS closure", none of + which existed. See the block comment on `POLL_CAPABLE_RUNTIME`. + """ + syms = runtime_symbols(roots) + # Non-vacuity first, for the same reason `audit_alloc_re` does it: an empty + # or tiny symbol table would report every entry dead and the verdict would + # be about the scan. + if len(syms) < 500: + print(f"error: found only {len(syms)} `extern \"C\" fn js_*` symbols " + f"under {', '.join(roots)}. The audit is measuring its own scan, " + "not POLL_CAPABLE_RUNTIME. Run it from the repository root.", + file=sys.stderr) + return 2 + dead = dead_poll_capable(syms) + print(f"=== POLL_CAPABLE_RUNTIME: {len(POLL_CAPABLE_RUNTIME)} entries vs " + f"{len(syms)} exported js_* symbols") + if dead: + print("error: POLL_CAPABLE_RUNTIME entries that name no runtime symbol:", + file=sys.stderr) + for n in dead: + print(f" {n}", file=sys.stderr) + print("An entry that matches nothing cannot classify any window as " + "MOVING, so it reads as coverage of an operation the " + "`--moving-only` gate is in fact blind to. Replace it with the " + "symbol codegen actually emits -- do not just delete it, or the " + "audit goes green and the hole stays.", file=sys.stderr) + return 2 + print("=== every entry names an exported runtime symbol") + return 0 + # Bit-level / identity producers a heap address flows through unchanged. TRANSPARENT_OPS = ("or i64", "and i64", "bitcast", "inttoptr", "ptrtoint", "select", "phi", "add i64", "sub i64") @@ -568,13 +621,118 @@ def is_collecting(callee): # This matters more than the raw-count modes suggest, because `--moving-only` # is the mode the `gc-root-dominance.yml` gate runs. A source the gate cannot # classify as reaching a moving minor is a source the gate cannot fail on. +# +# ## The property-GET hole (#7154 follow-up) +# +# Membership here is by EXACT emitted symbol, and that is the trap. The set +# carried `js_object_get_field_by_name` and `js_object_set_field_by_name` as a +# matched pair, which reads like symmetric coverage of property access. It is +# not. Codegen emits `js_object_set_field_by_name` verbatim (27 call sites in +# perry-codegen, 205 in the gate's own corpus) but never emits +# `js_object_get_field_by_name` at all — the GET side lowers to +# `js_object_get_field_by_name_f64`, `js_object_get_field_ic_miss` and +# `js_typed_feedback_object_get_field_by_name_f64` (1324 / 532 / 556 calls in +# the same corpus), and not one of those three was in this set. So every +# property SET classified `MOVING: YES` and every property GET classified +# `MOVING: no`, which is the asymmetry that put "as of this writing all five +# violations the gate reports are `MOVING: YES via js_object_set_field_by_name`" +# in docs/src/internals/gc-rooting-invariant.md and left the GET half unread. +# +# Measured on the gate corpus at the commit that added these six names: 55 +# `--stale-registers` uses have an emitted property-GET helper in their window +# and 31 of them classified `MOVING: no`, so `--moving-only` dropped all 31 — +# including the shape that faults deterministically under +# `PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800` at zod's +# `clone`. The protector and the checker disagreed and the checker was wrong. +# +# The premise for each addition is the SAME premise the set already grants +# `js_object_get_field_by_name`: it runs a user getter through a transmuted +# function pointer (`get_field_by_name.rs:1064-1067`) and routes a registered +# Proxy to `js_proxy_get` (`get_field_by_name.rs:84`) — arbitrary JS, with its +# own back-edge polls. Each name below is an entry point that reaches it: +# +# js_object_get_field_by_name_f64 ic_miss.rs:29 -> _by_name +# js_object_get_field_by_name_boxed ic_miss.rs:81,86 -> _f64 +# js_object_get_field_by_property_id_f64 ic_miss.rs:123 -> _f64 +# js_object_get_field_ic_miss ic_miss.rs:228,275 -> _by_name +# ic_miss.rs:252 -> js_proxy_get +# js_object_get_field_ic ic_miss.rs:526,561 -> _f64, +# ic_miss.rs:544 -> _ic_miss +# js_typed_feedback_object_get_field_by_name_f64 +# typed_feedback.rs:992 -> _f64 +# +# `js_object_get_field_by_name` STAYS. It is a real exported symbol +# (`--audit-poll-capable` checks that), the runtime calls it internally, and a +# future lowering may emit it directly; removing it would be a narrowing bought +# for nothing. The bug was never that it was listed — it was that listing it +# read as covering the GET side when nothing in the corpus matched it. +# +# ## The ten names that were not symbols at all +# +# Auditing the set the way `--audit-alloc-re` audits ALLOC_RE found that TEN of +# its twenty-eight entries matched no `extern "C" fn js_*` anywhere in +# perry-runtime or perry-stdlib: `js_apply_function`, `js_array_for_each`, +# `js_array_sort`, `js_call_closure`, `js_call_value`, `js_function_call`, +# `js_invoke_closure`, `js_object_get_property`, `js_object_set_property`, +# `js_string_replace`. Extrapolated spellings, exactly like `regexp_alloc\w*` +# and the eight other dead ALLOC_RE alternatives, and with the same effect: the +# set LOOKED like it covered "call a JS closure" behind four separate entries +# and covered it zero times. The real closure-call entry points +# (`js_closure_callN`, 186 call sites in the gate corpus) were never in it, so +# a value held live across a plain JS function call classified `MOVING: no` — +# the most obviously poll-capable operation there is. `RECEIVER_SINKS` in this +# same file already spelled it `closure_call\w*`, so the correct name was three +# hundred lines away the whole time. +# +# Each phantom is replaced by the emitted symbol that carries the premise the +# phantom asserted, never deleted outright — dropping the fake name without the +# real one would keep the audit green and leave the hole: +# +# js_call_closure / js_invoke_closure / js_function_call / js_apply_function +# -> js_closure_call0..16, js_closure_call_array, +# js_closure_call_apply_with_spread (closure/dispatch/calln.rs:33 — +# invokes a JS closure; nothing is more poll-capable than this) +# js_call_value -> js_native_call_value (closure/dispatch/value_call.rs:17 +# — calls a value as a function, incl. the Proxy `apply` trap) +# js_array_sort -> js_array_sort_default, js_array_sort_with_comparator +# (array/sort.rs:542,635 — the comparator is user JS) +# js_array_for_each -> js_typed_array_for_each (typedarray/iterate.rs:137) +# js_object_get_property / js_object_set_property +# -> js_object_get_property_key, js_object_set_property_key, +# js_object_set_property_key_method (object/property_key.rs:154,210,257 +# — ToPropertyKey coercion, then `js_object_get_field_by_name`) +# js_string_replace -> the four `_fn` variants, which take a user replacer +# callback (regex/replace_expand.rs:277 and siblings). The `_dyn` and +# plain variants are a separate coverage decision with their own hit +# count and are deliberately NOT folded in here — same reasoning as +# ALLOC_RE's deleted `bigint_\w+_op`. POLL_CAPABLE_RUNTIME = { - "js_call_function", "js_call_closure", "js_invoke_closure", - "js_call_value", "js_apply_function", "js_function_call", - "js_object_get_property", "js_object_set_property", + "js_call_function", + # Calling a JS closure. The four names this replaces + # (`js_call_closure`, `js_invoke_closure`, `js_function_call`, + # `js_apply_function`) were not symbols; these are. + "js_closure_call0", "js_closure_call1", "js_closure_call2", + "js_closure_call3", "js_closure_call4", "js_closure_call5", + "js_closure_call6", "js_closure_call7", "js_closure_call8", + "js_closure_call9", "js_closure_call10", "js_closure_call11", + "js_closure_call12", "js_closure_call13", "js_closure_call14", + "js_closure_call15", "js_closure_call16", + "js_closure_call_array", "js_closure_call_apply_with_spread", + "js_native_call_value", + "js_object_get_property_key", "js_object_set_property_key", + "js_object_set_property_key_method", "js_object_get_field_by_name", "js_object_set_field_by_name", - "js_array_sort", "js_array_map", "js_array_filter", "js_array_for_each", - "js_array_reduce", "js_json_stringify", "js_string_replace", + # The property-GET dispatch codegen ACTUALLY emits. See the block comment + # above for the reach proof behind each one. + "js_object_get_field_by_name_f64", "js_object_get_field_by_name_boxed", + "js_object_get_field_by_property_id_f64", + "js_object_get_field_ic", "js_object_get_field_ic_miss", + "js_typed_feedback_object_get_field_by_name_f64", + "js_array_sort_default", "js_array_sort_with_comparator", + "js_array_map", "js_array_filter", "js_typed_array_for_each", + "js_array_reduce", "js_json_stringify", + "js_string_replace_regex_fn", "js_string_replace_string_fn", + "js_string_replace_all_regex_fn", "js_string_replace_all_string_fn", "js_promise_run_microtasks", "js_gc_loop_safepoint", # ToPrimitive / ToString / ToNumber: every one of these dispatches a user # `[Symbol.toPrimitive]` / `toString` / `valueOf` on an object operand. @@ -2330,6 +2488,77 @@ def window_hits(A, B): """ +# The property-GET window, both directions. +# +# This is the shape at which `--stale-registers` and +# `PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800` +# disagreed at zod's `clone(schema, def, params)`: the protector faulted +# deterministically, the checker classified the window `MOVING: no`, and +# `--moving-only` — the arm `gc-root-dominance.yml` gates on — dropped it. The +# checker was the wrong one. `POLL_CAPABLE_RUNTIME` held +# `js_object_get_field_by_name`, which codegen never emits, and none of the +# three GET entry points it DOES emit. +# +# Reduced from the real emitted IR (perry-codegen `property_get/ +# generic_dispatch.rs`'s `pget.recv_ok` block, and the full-outline +# `js_object_get_field_ic` in `object/field_get_set/ic_miss.rs:544`): +# parameter #1 is spilled and bound, loaded into a bare register, and then a +# property read on the RECEIVER runs the generic GET dispatch — which resolves +# a user getter through a transmuted function pointer and can therefore run an +# evacuating minor — before that register is dereferenced. +# +# The bind is what makes this a `slotload` source rather than an invisible one: +# the slot IS rewritten by evacuation, so re-reading it is the fix and holding +# the pre-call register is the bug. Nothing in the fixture is an allocation, so +# the whole verdict rests on the window classification. +# +# The receiver and the key arrive as bare function PARAMETERS on purpose. That +# keeps the fixture to exactly one recognised source — `%def` — so the arms +# below assert one number about one register rather than a total that moves for +# any reason. It also records a second, separate gap honestly: `heap_source_kind` +# has no "incoming pointer parameter" kind, so `%a1` and `%a3` are invisible +# here and would be invisible in a real callee too. That gap is REAL but it is +# not what the zod adjudication turned on — measured over the gate corpus, only +# 4 unspilled parameters are used below a collecting call and all 4 are +# `double`/`i64` arithmetic in typed-f64 specialisations, none a pointer. It +# needs its own change with its own measured hit count. +_SELFTEST_PROPERTY_GET_WINDOW = """\ +define double @perry_fn_selftest__clone(double %a1, double %a2, i64 %a3) { +entry.0: + %d = alloca double + store double %a2, ptr %d + call void @js_shadow_slot_bind(i32 1, ptr %d) + %def = load double, ptr %d + %rb = bitcast double %a1 to i64 + %rh = and i64 %rb, 281474976710655 + call void @js_typed_feedback_observe_property_get(i64 4242, i64 %rh, i64 %a3) + %inner = call double @js_object_get_field_ic_miss(i64 %rh, i64 %a3, ptr @perry_ic_1) + %out = call double @perry_fn_other__callee(double %def, double %inner) + ret double %out +} +""" + +# The fix's shape: `%def` is re-read from its shadow slot BELOW the GET +# dispatch, so it observes the address evacuation wrote back. Identical in +# every other respect — if the control reports anything, the check is firing on +# the code shape rather than on the staleness. +_SELFTEST_PROPERTY_GET_RELOADED = """\ +define double @perry_fn_selftest__clone_reloaded(double %a1, double %a2, i64 %a3) { +entry.0: + %d = alloca double + store double %a2, ptr %d + call void @js_shadow_slot_bind(i32 1, ptr %d) + %rb = bitcast double %a1 to i64 + %rh = and i64 %rb, 281474976710655 + call void @js_typed_feedback_observe_property_get(i64 4242, i64 %rh, i64 %a3) + %inner = call double @js_object_get_field_ic_miss(i64 %rh, i64 %a3, ptr @perry_ic_1) + %def = load double, ptr %d + %out = call double @perry_fn_other__callee(double %def, double %inner) + ret double %out +} +""" + + def _scan_unrooted(paths, moving_only=False, **source_opts): """(violations, n_gc_capable_allocas) over `paths`.""" parsed = [(os.path.basename(p), parse_file(p)) for p in sorted(paths)] @@ -2580,6 +2809,47 @@ def self_test(): f"the alternatives themselves: {residue}. That is a bug in " "the auditor, not in ALLOC_RE.", file=sys.stderr) ok = False + # --- the POLL_CAPABLE_RUNTIME audit, both directions ----------------- + # + # Same treatment as the ALLOC_RE auditor above, and for a sharper + # reason: this set decides the MOVING classification, so a phantom + # entry is a hole in the arm the CI gate actually runs. Ten of + # twenty-eight entries were phantoms when the auditor was written, and + # four of those ten were four different misspellings of "call a JS + # closure". Driven off synthetic symbol tables so the arm does not pass + # merely because today's runtime happens to export the right names. + if dead_poll_capable({"js_gc_loop_safepoint"}) == []: + print("self-test FAIL: audited against a one-symbol table, almost " + "every POLL_CAPABLE_RUNTIME entry must be reported dead. An " + "empty result means the auditor cannot detect a phantom " + "entry at all.", file=sys.stderr) + ok = False + if dead_poll_capable(set(POLL_CAPABLE_RUNTIME)) != []: + print("self-test FAIL: the auditor reported entries dead against a " + "symbol table that is POLL_CAPABLE_RUNTIME itself. That is a " + "bug in the auditor, not in the set.", file=sys.stderr) + ok = False + if dead_poll_capable(set(POLL_CAPABLE_RUNTIME) | + {"js_extra"}) != []: + print("self-test FAIL: a symbol table that is a strict SUPERSET of " + "the set must also come back clean; the auditor is asking " + "the wrong containment question.", file=sys.stderr) + ok = False + # And the property-GET family specifically: the entries this whole + # change is about must be in the set, or the fixture arms below are + # asserting something the classification no longer does. + _pget_family = {"js_object_get_field_by_name_f64", + "js_object_get_field_ic_miss", + "js_typed_feedback_object_get_field_by_name_f64"} + if not _pget_family <= POLL_CAPABLE_RUNTIME: + print("self-test FAIL: the emitted property-GET dispatch " + f"{sorted(_pget_family - POLL_CAPABLE_RUNTIME)} must stay in " + "POLL_CAPABLE_RUNTIME. Codegen emits these and never emits " + "js_object_get_field_by_name, so dropping them puts the GET " + "half of property access back outside --moving-only.", + file=sys.stderr) + ok = False + # A knob that is silently ignored is a disarmed knob -- the same rule # `--max-stale` and `--fatal-sinks` already carry, read the other way. if _main_probe(["--stale-registers", "--any-def", planted]) != 2: @@ -2635,6 +2905,49 @@ def self_test(): "broken.", file=sys.stderr) ok = False + # --- the property-GET window, both directions ----------------------- + # + # The zod `clone` adjudication. `--stale-registers` said `MOVING: no` + # and the from-space protector faulted deterministically on the same + # site; the checker was wrong, because `POLL_CAPABLE_RUNTIME` named the + # one GET symbol codegen never emits and none of the three it does. + # + # The RAW arm is not the interesting one and never was — the window + # already reported, it just reported as non-moving, and `--moving-only` + # is the arm `gc-root-dominance.yml` gates on. So both are asserted, + # and the second is the one that was red. + pget = os.path.join(td, "property_get_window.ll") + pget_fixed = os.path.join(td, "property_get_reloaded.ll") + for p, text in ((pget, _SELFTEST_PROPERTY_GET_WINDOW), + (pget_fixed, _SELFTEST_PROPERTY_GET_RELOADED)): + with open(p, "w") as fh: + fh.write(text) + + if _stale_kinds_probe(pget).get("slotload", 0) != 1: + print("self-test FAIL: --stale-registers must report the shadow-slot " + "load held across the generic property-GET dispatch as " + f"source=slotload. Got {_stale_kinds_probe(pget)!r}.", + file=sys.stderr) + ok = False + pget_moving = _stale_kinds_probe(pget, moving_only=True) + if pget_moving.get("slotload", 0) != 1: + print("self-test FAIL: the generic property-GET dispatch resolves a " + "user getter (object/field_get_set/get_field_by_name.rs:1064) " + "and routes a Proxy to js_proxy_get (:84), so a register held " + "across it MUST classify MOVING and survive --moving-only. " + f"Got {pget_moving!r}. This is the shape that faults under " + "PERRY_GC_PROTECT_FROMSPACE_DEPTH=800 at zod's `clone` while " + "the checker called the window non-moving; a gate that cannot " + "see a live bug is the failure mode this whole file exists " + "to avoid.", file=sys.stderr) + ok = False + if _stale_kinds_probe(pget_fixed).get("slotload", 0) != 0: + print("self-test FAIL: re-reading the shadow slot BELOW the GET " + "dispatch is the fix, so the control fixture must report no " + "slotload use. A non-zero count means the check fires on the " + "code shape rather than on the staleness.", file=sys.stderr) + ok = False + try: _scan([broken], False, "alloc") except MalformedIR: @@ -2980,6 +3293,14 @@ def main(): "that matches nothing. A dead alternative reads as " "coverage and is not -- nine of them have shipped in " "this pattern across two rounds. Takes no corpus.") + ap.add_argument("--audit-poll-capable", action="store_true", + help="check every POLL_CAPABLE_RUNTIME entry against the " + "runtime's real `extern \"C\" fn js_*` symbols and " + "fail on any that names nothing. This set decides the " + "MOVING classification the --moving-only gate runs on, " + "so a phantom entry is a hole the gate cannot fail " + "through -- ten of twenty-eight were phantoms when " + "this was added. Takes no corpus.") ap.add_argument("--audit-immovable-sources", action="store_true", help="re-check the PREMISES of every --unrooted-allocas " "exemption against the runtime source (#7210): the " @@ -3005,6 +3326,8 @@ def main(): # are checked before the corpus arguments are. if ns.audit_alloc_re: return audit_alloc_re() + if ns.audit_poll_capable: + return audit_poll_capable() if ns.audit_immovable_sources: return audit_immovable_sources()