diff --git a/.github/workflows/gc-root-dominance.yml b/.github/workflows/gc-root-dominance.yml index 977d002790..acee34c5b9 100644 --- a/.github/workflows/gc-root-dominance.yml +++ b/.github/workflows/gc-root-dominance.yml @@ -92,6 +92,22 @@ jobs: - name: Checker self-test (can this gate still fail?) run: python3 scripts/gc_root_dominance_check.py --self-test + # The checker only reports a stale register when it recognises what + # MATERIALIZED the value, and that recognition is one regex. An + # alternative in it that matches no real symbol reads as coverage and is + # not -- the gate-can't-fail pattern in regex form. It has shipped nine + # times across two rounds: four (`regexp_alloc\w*`, `promise_alloc\w*`, + # `bigint_alloc\w*`, `typed_array_alloc\w*`) found only because + # `js_regexp_new` cost #7154 a whole investigation round, and five more + # introduced by the change that removed those four. A prose audit does not + # survive its own next edit, so it is a gate now: every alternative must + # match at least one `extern "C" fn js_*` the runtime actually exports. + # + # Static and instant — no toolchain, no corpus — so it runs before the + # build with the self-test. + - name: ALLOC_RE alternatives must match real runtime symbols + run: python3 scripts/gc_root_dominance_check.py --audit-alloc-re + - name: Install Rust toolchain uses: dtolnay/rust-toolchain@stable diff --git a/changelog.d/7227-regexp-receiver-rooting-and-alloc-re-audit.md b/changelog.d/7227-regexp-receiver-rooting-and-alloc-re-audit.md new file mode 100644 index 0000000000..2a66dd37ce --- /dev/null +++ b/changelog.d/7227-regexp-receiver-rooting-and-alloc-re-audit.md @@ -0,0 +1,201 @@ +### Fixed + +- **`codegen`: the receiver of `re.test(s)` / `re.exec(s)` is rooted across the + `ToString(s)` coercion the lowering emits below it** (#7154). Both + `Expr::RegExpTest` and `Expr::RegExpExec` lowered the receiver first, unboxed + it to a raw `RegExpHeader*` in a bare SSA register, and only then emitted + `js_jsvalue_to_string_coerce`. That coerce is not a bystander: it allocates, + and on an object argument it dispatches a user `[Symbol.toPrimitive]` / + `toString` / `valueOf`, which is arbitrary JS with its own loop back-edge + polls. Under `PERRY_GC_MOVING_LOOP_POLLS=1` one of those polls runs an + evacuating minor while the regexp is live only in that register. + + This is the residual #7226 measured and named rather than fixed. In the + `sfw-registry` reproducer it is `src/lib/api/shared.ts:67`, + `/\[[a-zA-Z]+\]/.test(url)`, faulting at + `perry_fn_src_lib_api_shared_ts__defineApiCall + 404`: + + ```asm + bl js_regexp_new ; ALLOCATES + and x20, x0, #0xffffffffffff ; raw regexp pointer -> bare register + ldr d0, [sp, #0x28] + bl js_jsvalue_to_string_coerce ; ALLOCATES, runs user toString + mov x0, x20 ; STALE + bl js_regexp_test ; faults here + ``` + + The receiver now takes the established `guard_store_operand_across` / + `reread_store_operand` pair, and the unbox moves BELOW the coerce — unboxing + above it is what parked the pre-move address in a register in the first + place. `RegExpExec` had the identical defect and is fixed with it. + +### Changed + +- **`scripts/gc_root_dominance_check.py`: `ALLOC_RE` audited against the + runtime's real symbol table instead of an assumed naming convention.** This + is the more valuable half of the change, because the miss above was the + *second* allocator to escape this pattern (`js_implicit_this_set` was the + first, #7226) and each one has cost a full investigation round. + + The old pattern carried an alternative spelled `regexp_alloc\w*`. **No such + symbol has ever existed.** It was not a typo — it was an extrapolation: + whoever wrote it knew a RegExp allocates and inferred the `_alloc` suffix + from its neighbours. Reconciling every alternative against + `extern "C" fn js_\w+` over perry-runtime + perry-stdlib, intersected with + the names perry-codegen actually declares, found **four alternatives matching + nothing at all** — `regexp_alloc\w*`, `promise_alloc\w*`, `bigint_alloc\w*` + and `typed_array_alloc\w*`. A quarter of the pattern was decorative. + + The root cause is that the runtime materializes fresh GC objects under + **three** naming conventions and the pattern modelled one: + + | convention | examples | matched before | + |---|---|---| + | `*_alloc*` | `js_object_alloc`, `js_array_alloc`, `js_closure_alloc`, `js_uint8array_alloc`, `js_inline_arena_slow_alloc` | yes | + | `*_new*` | `js_regexp_new`, `js_promise_new`, `js_symbol_new`, `js_date_new`, `js_error_new`, `js_typed_array_new`, `js_weakmap_new`, `js_url_new`, `js_boxed_string_new`, ~140 more | **no** | + | `*_create*` | `js_object_create`, `js_array_create`, `js_vm_create_context`, `js_crypto_create_hash`, ~40 more | only `object_create*` | + + All three are now matched as conventions, and the constructors that use none + of them are enumerated explicitly: the `_construct*` ctor forms, the fresh + string producers (`string_coerce`, `jsvalue_to_string*`, `string_slice`, + `string_to_*_case`, `string_pad_*`, `string_trim*`, …), the copy-on-read and + ES2023 change-by-copy array family (`array_to_sorted*`, `array_to_spliced`, + `array_with`, `array_flat*`, `array_like_to_array`, `iterator_to_array`, …), + the whole-object producers (`object_keys*`, `object_entries*`, + `object_from_entries`, `object_get_own_property_descriptor*`, + `structured_clone*`, the Set-methods family), the namespace/class-shape + helpers, and BigInt's `bigint_from*` (which has neither `_alloc` nor `_new`). + + Widening is safe in the checker's one-sided direction: a name that turns out + not to allocate costs a false positive to triage, while a missing one costs a + shipped use-after-free plus the round it takes to find by hand. The file now + says so, so the next person extends it rather than guessing. + +- **`scripts/gc_root_dominance_check.py`: the ToPrimitive / ToString / ToNumber + coercion family is `POLL_CAPABLE_RUNTIME`.** This is the second half of the + same blind spot and it is the half that matters for CI, because + `--moving-only` is the mode `gc-root-dominance.yml` gates on. With `ALLOC_RE` + widened but the coercions unmodelled, the `/re/.test(s)` site was reported by + the raw `--stale-registers` count and **still invisible to `--moving-only`**: + nothing in its window was classified as reaching a moving minor. A coercion + does not look like a call into user code, but ToPrimitive is exactly that — + and `js_string_coerce`'s own doc comment already said so ("a `POINTER_TAG` + object routes through `js_jsvalue_to_string`, which can invoke a user + `toString` / `valueOf`"). The checker just never read it. + +- **`scripts/gc_root_dominance_check.py`: `js_regexp_test` / `js_regexp_exec` + are fatal sinks.** A stale `RegExpHeader*` is dereferenced immediately by + both, and this one faulted rather than merely answering wrong, so it belongs + in the `--fatal-sinks` ranking and not only in the raw count. + +## Verification + +Measured against the parent (`4e99c1bad`, #7226's head), built from this +worktree rather than borrowed from another one. + +The checker change is what makes the codegen change checkable, so it is +reported first. Over the gap-test IR for the new reproducer: + +| `--stale-registers` over `test_gap_gc_regexp_receiver_rooting.ts` | parent | this PR | +|---|---|---| +| base checker (`regexp_alloc\w*`) | **0 reported** | — | +| widened `ALLOC_RE`, raw count | 3 | **0** | +| widened + `--moving-only` (the gate's mode) | **3**, `MOVING: YES via js_jsvalue_to_string_coerce` | **0** | + +All three are named exactly: `source (alloc): call i64 @js_regexp_new`, +`stale use: call i32 @js_regexp_test` / `@js_regexp_exec`, with +`js_jsvalue_to_string_coerce` in the window. + +Over the 130-module / 2170-function gap corpus, both checker widenings +together: + +| `--moving-only` | parent | this PR | +|---|---|---| +| bind-anchored violations (**the gate**) | 0 | **0**, allowlist still empty | +| `--stale-registers`, total | 2730 | 2738 | +| `--stale-registers --moving-only` | 2 | **62** | +| `--stale-registers --fatal-sinks` | 279 | 282 | +| `--stale-registers --moving-only --fatal-sinks` | 0 | **0** | +| `--unrooted-allocas`, moving-reachable | 57 | 85 | + +The gate does not move. The 60 newly-*moving* stale-register leads were all +already in the 2730-entry diagnostic list; modelling the coercions is what +reclassified their windows. Triaged mechanically by the shape of the stale +use: + +| stale use | count | verdict | +|---|---|---| +| `lshr … , 48` | 37 | NaN-box **tag** read. Relocation rewrites the low 48 bits; the tag is unchanged. Not a bug. | +| `fadd double` | 3 | float arithmetic on a value the mode could not prove non-pointer. Not a bug. | +| `getelementptr i8, …, 32` | 15 | direct field access in the `*__pshape` pointer-shape specializations. A real dereference shape and a real population — **left for its own PR**, since it is the `PERRY_PTR_SHAPE_LOCALS` family and wants its own measured count. | +| call argument | 7 | typed-feedback array receivers held across `js_number_coerce`. Same call: real shape, own population, own PR. | + +Nothing in the newly-visible set is a fatal sink, which is why the fatal count +moves only by the three regexp entries this PR then fixes. + +Gap test — `test_gap_gc_regexp_receiver_rooting.ts`, compiled **and** run with +`PERRY_GC_MOVING_LOOP_POLLS=1`: + +| | parent | this PR | +|---|---|---| +| `POLLS=1` + `PERRY_GC_ZEAL=1` | **0/10 — SIGSEGV/SIGBUS every run** | `bad 0` **10/10** | +| `POLLS=1` + zeal + `PERRY_GEN_GC=0` | `bad 0` | `bad 0` | + +The parent arm is a hard fault rather than a nonzero `bad`, and that is the +honest signature: with a regex **literal** receiver — the registry's shape — +`js_regexp_new`'s result is held only in the register, so the evacuating minor +retires the block under it and the deref lands in from-space. The +`PERRY_GEN_GC=0` arm proves the test tracks collector mode rather than being +flaky. Zeal is required for the same structural reason #7226 recorded for +`prev_this`: the window is a user call, so only a *moving* collection exploits +it, and allocation-triggered collections take +`ManualGcScanGuard::force_full_scan`, which makes the copying minor ineligible. + +## `sfw-registry` moves, and does NOT reach 30/30 + +`sfw-registry --help`, 141 modules, `PERRY_FORCE_WELL_KNOWN=iovalkey`, compiled +**and** run with `PERRY_GC_MOVING_LOOP_POLLS=1`: + +| | parent (`4e99c1bad`) | this PR | +|---|---|---| +| `POLLS=1`, **30 runs** | **0/30** | **28/30** | + +Both arms use the same runtime archives (the codegen fix is the only +difference) and the same firewall tree, so the comparison is like-for-like. +The parent's 30 failures are **not** crashes — every one is a deterministic +`TypeError: Cannot convert undefined or null to object`, which is what a stale +regexp receiver produces here: `defineApiCall` computes +`urlRequiresInterpolation = /\[[a-zA-Z]+\]/.test(url)` at definition time, the +stale read returns the wrong boolean, and the wrong branch hands `undefined` +to a downstream `Object` operation. The fix removes that failure mode +completely. + +Note that #7226 reported 26/30 for this same parent commit. That measurement +was taken against a different firewall checkout; on the tree measured here the +parent is 0/30. The delta this PR is responsible for is the one measured above, +on one tree, with one runtime. + +**This does not close #7154 and #7161's stopgap stays.** Two runs in thirty +still SIGSEGV, and the residual is a *different* object from the one this PR +fixes. Under `PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_GC_PROTECT_FROMSPACE=1 +PERRY_GC_PROTECT_FROMSPACE_DEPTH=800` it is deterministic — **40/40** — and the +reporter names it: + +``` +[gc-fromspace-protect] FAULT: signal 10 at 0x…5d5c + block=0x…20000 +220508 retired_bytes=253416 retired_by_minor=#155 + last-known object: user_ptr=0x…5d58 obj_type=3 size=80 +``` + +`obj_type=3` is a **string**, not a `RegExpHeader`, so it is not the receiver +this PR rooted. Disassembling the faulting frame confirms it: the return +address is `defineApiCall + 428`, and `+424` is `bl js_regexp_test` — the same +call site, twenty bytes further along, which is exactly the size of the +`js_gc_temp_root_push` / `js_gc_temp_root_get` pair this PR inserts. The +receiver operand is now correct; the surviving stale value is a string reaching +the same call. + +That the protected arm is 40/40 while the unprotected arm is 2/30 is the useful +part: the next round has a deterministic reproducer instead of a 7 % one. It is +**not** fixed speculatively here — no edit ships without a test that can fail +without it. diff --git a/crates/perry-codegen/src/expr/instance_misc1.rs b/crates/perry-codegen/src/expr/instance_misc1.rs index 0a206a77ab..c68ca6b52d 100644 --- a/crates/perry-codegen/src/expr/instance_misc1.rs +++ b/crates/perry-codegen/src/expr/instance_misc1.rs @@ -1064,22 +1064,44 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // a NaN-tagged string. Both must be unboxed before the call. Expr::RegExpTest { regex, string } => { let regex_box = lower_expr(ctx, regex)?; + // #7154: the receiver is live across BOTH the string operand's own + // lowering and the `js_jsvalue_to_string_coerce` below it, and the + // coerce is unconditional — it allocates, and on an object argument + // it runs a user `toString`, which is arbitrary JS with its own + // back-edge polls. So the window always exists and `collects` is + // `true` rather than a `expr_may_trigger_gc(string)` test. + // + // This is the site the registry reproducer faults at + // (`defineApiCall + 404`, `obj_type=3 size=80`): `js_regexp_new`'s + // raw result went into a bare `x20`, the coerce drove an evacuating + // minor that moved it, and `js_regexp_test` dereferenced from-space. + // The static checker could not see it because `ALLOC_RE` spelled the + // allocator `regexp_alloc\w*` and the call is `js_regexp_new`. + let guard = super::temp_root::guard_store_operand_across(ctx, regex, ®ex_box, true); let str_box = lower_expr(ctx, string)?; - let blk = ctx.block(); - let regex_handle = unbox_to_i64(blk, ®ex_box); // Per spec `RegExp.prototype.test` does `ToString(argument)`, so a // String wrapper (`re.test(new String("x"))`), a number // (`re.test(123)`), or an object with a custom `toString` must be // coerced — and a throwing `toString`/`valueOf` must propagate. // `js_get_string_pointer_unified` only unwraps real strings, so use // the coercing ToString that dispatches `toString` on objects. - let str_handle = blk.call(I64, "js_jsvalue_to_string_coerce", &[(DOUBLE, &str_box)]); + let str_handle = + ctx.block() + .call(I64, "js_jsvalue_to_string_coerce", &[(DOUBLE, &str_box)]); + // Re-read BELOW the coerce, then unbox. Unboxing above it is what + // parked the pre-move address in a register in the first place. + let regex_box = super::temp_root::reread_store_operand(ctx, &guard, regex, ®ex_box)?; + let blk = ctx.block(); + let regex_handle = unbox_to_i64(blk, ®ex_box); let i32_v = blk.call( I32, "js_regexp_test", &[(I64, ®ex_handle), (I64, &str_handle)], ); - Ok(i32_bool_to_nanbox(blk, &i32_v)) + let out = i32_bool_to_nanbox(ctx.block(), &i32_v); + // After the call: `js_regexp_test` allocates while reading these. + super::temp_root::release_store_operand(ctx, guard); + Ok(out) } Expr::RegExpExec { regex, string } => { // Returns ArrayHeader* or null. For a null (0) result we must @@ -1088,18 +1110,28 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // non-null pointer value that compares unequal to null, causing // infinite loops + segfaults when callers IndexGet on the result. let regex_box = lower_expr(ctx, regex)?; + // #7154, identical shape to `RegExpTest` above and found with it: + // the receiver is live across the string operand's lowering and + // across the unconditional coerce, which allocates and can run a + // user `toString`. + let guard = super::temp_root::guard_store_operand_across(ctx, regex, ®ex_box, true); let str_box = lower_expr(ctx, string)?; - let blk = ctx.block(); - let regex_handle = unbox_to_i64(blk, ®ex_box); // `RegExp.prototype.exec` does `ToString(argument)` — coerce String // wrappers / numbers / objects (and propagate a throwing toString) // rather than only unwrapping real strings (see RegExpTest above). - let str_handle = blk.call(I64, "js_jsvalue_to_string_coerce", &[(DOUBLE, &str_box)]); + let str_handle = + ctx.block() + .call(I64, "js_jsvalue_to_string_coerce", &[(DOUBLE, &str_box)]); + let regex_box = super::temp_root::reread_store_operand(ctx, &guard, regex, ®ex_box)?; + let blk = ctx.block(); + let regex_handle = unbox_to_i64(blk, ®ex_box); let result = blk.call( I64, "js_regexp_exec", &[(I64, ®ex_handle), (I64, &str_handle)], ); + super::temp_root::release_store_operand(ctx, guard); + let blk = ctx.block(); // Branch on result == 0 → TAG_NULL; else NaN-box as pointer. let is_null = blk.icmp_eq(I64, &result, "0"); let ptr_boxed = nanbox_pointer_inline(ctx.block(), &result); diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index 5c75ba8cb5..b68843ec9b 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -282,16 +282,207 @@ def build_cfg(f): MOVING_POLL = "js_gc_loop_safepoint" # Result-producing calls that materialize a fresh GC object. +# +# ------------------------------------------------------------------ THE AUDIT +# This list is enumerated from the runtime's actual exported entry points, NOT +# guessed from a naming convention, because guessing a convention is precisely +# how it has failed twice: +# +# * `js_implicit_this_set` (#7226) -- a root READ that was not modelled, so +# `prev_this` survived two PRs that were looking straight at it; +# * `js_regexp_new` (#7154, this change) -- the regex carried an alternative +# spelled `regexp_alloc\w*`, and **no such symbol has ever existed**. The +# `/re/.test(s)` lowering holds `js_regexp_new`'s raw result in a register +# across `js_jsvalue_to_string_coerce`, and the checker reported nothing +# because the register had no recognised heap-value source. +# +# The second one is the instructive one. `regexp_alloc\w*` was not a typo, it +# was an ASSUMED convention: whoever wrote it knew a RegExp allocates and +# extrapolated the `_alloc` suffix from its neighbours. Reconciling every +# alternative below against the real symbol table (`grep -rhoE 'extern "C" fn +# js_\w+'` over perry-runtime + perry-stdlib, intersected with the names +# perry-codegen actually declares) found FOUR alternatives in the same state -- +# `regexp_alloc\w*`, `promise_alloc\w*`, `bigint_alloc\w*` and +# `typed_array_alloc\w*` matched nothing whatsoever. A quarter of the pattern +# was decorative. +# +# The root cause is that the runtime materializes fresh GC objects under THREE +# naming conventions and the old pattern modelled one: +# +# `_alloc*` js_object_alloc, js_array_alloc, js_closure_alloc, js_box_alloc, +# js_map_alloc, js_set_alloc, js_buffer_alloc, js_uint8array_alloc, +# js_arguments_object_alloc, js_inline_arena_slow_alloc, ... +# `_new*` js_regexp_new, js_promise_new, js_symbol_new, js_date_new, +# js_error_new, js_typed_array_new, js_weakmap_new, js_weakset_new, +# js_url_new, js_boxed_string_new, js_array_buffer_new, ~140 more +# `_create*` js_object_create, js_array_create, js_vm_create_context, +# js_crypto_create_hash, js_readline_create_interface, ~40 more +# +# So the three conventions are matched as conventions, and the constructors +# that use none of them are enumerated explicitly below. Widening is SAFE in +# the checker's one-sided direction: a name that is in fact not an allocation +# costs a false positive to triage, while a missing one costs a shipped +# use-after-free plus the investigation round it takes to find it by hand. +# When in doubt, add it. ALLOC_RE = re.compile( r"^js_(" - r"object_alloc\w*|array_alloc\w*|closure_alloc\w*|box_alloc\w*|" - r"string_alloc\w*|string_concat\w*|string_coerce|string_from\w*|" - r"map_alloc\w*|set_alloc\w*|promise_alloc\w*|bigint_alloc\w*|" - r"typed_array_alloc\w*|buffer_alloc\w*|regexp_alloc\w*|" - r"object_create\w*|array_from\w*|build_class_keys_array" + # -- convention 1: `*_alloc*`, including the arena's own slow path. + r"\w*_alloc\w*|" + # -- convention 2: `*_new*`. `js_regexp_new` is the #7154 residual. + r"\w*_new\w*|" + # -- convention 3: `*_create*`. + r"\w*_create\w*|create_\w+|" + # -- constructors using none of the three conventions ------------------- + # `new X(...)` / `X(...)` forms folded at HIR into a direct ctor call. + r"\w*_construct|\w*_construct_call|\w*_construct_apply|" + r"reflect_construct|new_function_construct\w*|super_construct_apply|" + # fresh strings. Every one of these returns a *mut StringHeader the + # caller holds raw; `string_coerce` and `jsvalue_to_string_coerce` are + # the two the `/re/` lowerings feed. + r"string_concat\w*|string_coerce|string_from\w*|string_append|" + r"jsvalue_to_string\w*|value_to_string\w*|number_to_string\w*|" + r"string_repeat|string_slice|string_substring|string_substr|" + r"string_pad_\w+|string_trim\w*|string_to_\w+_case|string_replace\w*|" + r"string_split|string_normalize|string_at|string_char_at|" + r"string_index_get_boxed|boxed_string\w*|" + # fresh arrays: the copy-on-read Array.prototype methods, the ES2023 + # change-by-copy family, and the iterable/array-like converters. + r"array_from\w*|array_clone\w*|array_concat\w*|array_slice|array_splice|" + r"array_to_spliced|array_to_sorted\w*|array_to_reversed|array_with|" + r"array_flat\w*|array_map|array_filter|array_like_to_array|" + r"iterator_to_array|" + # fresh objects/collections handed back as a whole + r"object_keys\w*|object_values\w*|object_entries\w*|object_from_entries|" + r"object_assign\w*|object_group_by|object_coerce|" + r"object_get_own_property_descriptor\w*|object_get_own_property_names|" + r"object_get_own_property_symbols|" + r"map_from_iterable|set_from_iterable|map_group_by|" + r"set_union|set_intersection|set_difference|set_symmetric_difference|" + r"structured_clone\w*|" + # module namespace objects, class-shape side tables, generator plumbing + r"build_class_keys_array|create_namespace|create_native_module_namespace|" + r"generator_attach_prototype|proxy_revocable|" + # BigInt: no `_alloc` and no `_new`; the constructors are `_from_*`. + # + # NOT COVERED, AND DELIBERATELY LEFT UNCOVERED HERE: BigInt *arithmetic* + # (`js_bigint_add`, `js_bigint_and`, `js_bigint_mul`, ...) allocates a fresh + # BigInt and matches none of the three conventions. An alternative spelled + # `bigint_\w+_op` used to sit here and matched nothing -- the same + # extrapolated-suffix mistake as `regexp_alloc\w*`, in the same list, added + # by the change that removed the first four. It is deleted rather than + # widened because widening it is a coverage decision with its own false + # positives (`js_bigint_equals` returns a bool), and that belongs in a + # change that can measure the new hits. Tracked in the PR thread. + r"bigint_from\w*|" + # Buffers / typed arrays that spell it neither way. `array_buffer_slice` + # and `typed_array_from\w*` were removed for the same reason: no such + # symbols. The real one, `js_buffer_from_arraybuffer_slice`, is already + # matched by `buffer_from\w*`. + r"buffer_from\w*" r")$" ) +# --------------------------------------------------------------------- AUDIT +# +# A regex alternative that matches nothing is the gate-can't-fail pattern in +# regex form: it reads as coverage, it costs nothing to keep, and it is +# indistinguishable from real coverage until a bug slips through the hole it +# was supposed to close. This has now happened twice in this one pattern: +# +# * `regexp_alloc\w*`, `promise_alloc\w*`, `bigint_alloc\w*`, +# `typed_array_alloc\w*` -- four dead alternatives, found only because +# `js_regexp_new` cost #7154 a full investigation round; +# * `array_of`, `array_group_by`, `bigint_\w+_op`, `typed_array_from\w*`, +# `array_buffer_slice` -- five MORE, introduced by the change that removed +# the first four, and found by running this auditor against it. +# +# The second round is the argument for automating it. A prose audit does not +# survive its own next edit; the enumeration below is checked by CI instead +# (`--audit-alloc-re`), so an alternative cannot go dead without going red. +# +# Deleting a dead alternative changes NOTHING about what the checker matches -- +# that is what "matches no symbol" means -- so this is not a narrowing. + +_EXTERN_C_FN_RE = re.compile(r'extern\s+"C"\s+fn\s+(js_\w+)') + +# The runtime crates that export the C-ABI surface perry-codegen calls. +SYMBOL_ROOTS = ("crates/perry-runtime/src", "crates/perry-stdlib/src") + + +def runtime_symbols(roots=SYMBOL_ROOTS): + """Every `extern "C" fn js_*` the runtime actually exports.""" + syms = set() + for root in roots: + if not os.path.isdir(root): + continue + for dirpath, _dirs, files in os.walk(root): + for name in files: + if not name.endswith(".rs"): + continue + with open(os.path.join(dirpath, name), + encoding="utf-8", errors="replace") as fh: + syms.update(_EXTERN_C_FN_RE.findall(fh.read())) + return syms + + +def alloc_re_alternatives(): + """The top-level alternatives inside ALLOC_RE's `js_(...)` group. + + Asserts the pattern's shape rather than assuming it: if ALLOC_RE is ever + rewritten into a form this cannot decompose, that must be a loud failure, + not an audit that silently checks zero alternatives. + """ + pat = ALLOC_RE.pattern + if not (pat.startswith("^js_(") and pat.endswith(")$")): + raise MalformedIR( + "ALLOC_RE is no longer of the form ^js_(...)$; the alternative " + "audit cannot decompose it. Update alloc_re_alternatives() rather " + "than leaving the audit silently vacuous.") + inner = pat[len("^js_("):-len(")$")] + if "(" in inner: + raise MalformedIR( + "ALLOC_RE now contains a nested group; splitting on '|' would " + "produce bogus alternatives. Update alloc_re_alternatives().") + return [a for a in inner.split("|") if a] + + +def dead_alloc_alternatives(symbols): + """ALLOC_RE alternatives matching none of `symbols`, in pattern order.""" + dead = [] + for alt in alloc_re_alternatives(): + probe = re.compile("^js_(?:%s)$" % alt) + if not any(probe.match(s) for s in symbols): + dead.append(alt) + return dead + + +def audit_alloc_re(roots=SYMBOL_ROOTS): + """Exit status for `--audit-alloc-re`. 0 clean, 2 on a dead alternative.""" + syms = runtime_symbols(roots) + # Non-vacuity first: an empty symbol set would report every alternative + # dead, and a tiny one would report a plausible-looking few. Either way the + # verdict would be about the scan, not about the pattern. + 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 ALLOC_RE. Run it from the repository root.", file=sys.stderr) + return 2 + dead = dead_alloc_alternatives(syms) + print(f"=== ALLOC_RE: {len(alloc_re_alternatives())} alternatives vs " + f"{len(syms)} exported js_* symbols") + if dead: + print("error: ALLOC_RE alternatives that match no runtime symbol:", + file=sys.stderr) + for a in dead: + print(f" {a}", file=sys.stderr) + print("An alternative that matches nothing reads as coverage and is " + "not. Either it is misspelled -- check the real name in the " + "runtime -- or the symbol was renamed or removed and the " + "alternative should go with it.", file=sys.stderr) + return 2 + print("=== every alternative matches at least one exported 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") @@ -362,6 +553,21 @@ def is_collecting(callee): # ------------------------------------------------- interprocedural poll reach # Runtime helpers that re-enter compiled JS (and therefore its back-edge polls). +# +# The COERCION family is the half of this set that is easy to leave out, and +# leaving it out is what kept `--moving-only` blind to #7154's `/re/.test(s)` +# residual even once `ALLOC_RE` had been widened to recognise `js_regexp_new`. +# A coercion does not *look* like a call into user code, but ToPrimitive is +# exactly that: `js_jsvalue_to_string_coerce` runs `to_string_method_impl(…, +# skip_to_primitive = false)`, which consults `[Symbol.toPrimitive]`, then +# `toString`, then `valueOf` — arbitrary JS, with its own loop back-edge polls. +# `js_string_coerce`'s own doc comment already said so ("a `POINTER_TAG` object +# routes through `js_jsvalue_to_string`, which can invoke a user `toString` / +# `valueOf`"); the checker just never read it. +# +# 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. POLL_CAPABLE_RUNTIME = { "js_call_function", "js_call_closure", "js_invoke_closure", "js_call_value", "js_apply_function", "js_function_call", @@ -370,6 +576,13 @@ def is_collecting(callee): "js_array_sort", "js_array_map", "js_array_filter", "js_array_for_each", "js_array_reduce", "js_json_stringify", "js_string_replace", "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. + "js_to_primitive", + "js_jsvalue_to_string", "js_jsvalue_to_string_coerce", + "js_jsvalue_to_string_method", "js_jsvalue_to_string_radix", + "js_string_coerce", "js_string_coerce_method_this", + "js_number_coerce", "js_object_coerce", } @@ -1198,7 +1411,11 @@ def seeded_violation_test(paths, moving_only, anchor, want_sites, verbose=False) r"object_get_property|object_set_property|put_value_set_dyn_ic|" r"get_value_dyn_ic|closure_call\w*|call_closure|call_function|" r"call_value|invoke_closure|apply_function|" - r"array_\w+|map_\w+|set_\w+|typed_feedback_\w*call\w*" + r"array_\w+|map_\w+|set_\w+|typed_feedback_\w*call\w*|" + # A stale RegExpHeader* is dereferenced immediately by both of these — + # this is #7154's residual, and it faulted rather than merely answering + # wrong, so it belongs in the fatal ranking and not just the raw count. + r"regexp_test|regexp_exec|regexp_match\w*|regexp_replace\w*" r")$" ) @@ -1805,6 +2022,41 @@ def self_test(): "breadth floor. A guard that rejects every corpus is a " "gate that always fails.", file=sys.stderr) ok = False + # --- the ALLOC_RE alternative audit, both directions ---------------- + # + # The auditor is itself a gate, so it gets the same treatment as the + # rest: prove it reports a planted dead alternative AND clears a live + # one. Driven off a synthetic symbol set so the arm does not depend on + # the repository's current contents -- an arm that only passes because + # today's runtime happens to export the right names would go quiet the + # moment someone renamed a symbol, which is the case it exists for. + live_alts = [a for a in alloc_re_alternatives() + if re.compile("^js_(?:%s)$" % a).match("js_object_alloc") + or re.compile("^js_(?:%s)$" % a).match("js_regexp_new")] + if not live_alts: + print("self-test FAIL: neither js_object_alloc nor js_regexp_new " + "matches any ALLOC_RE alternative; the audit arm below would " + "be testing nothing", file=sys.stderr) + ok = False + if dead_alloc_alternatives({"js_object_alloc", "js_regexp_new"}) == []: + print("self-test FAIL: audited against a two-symbol table, almost " + "every ALLOC_RE alternative must be reported dead. An empty " + "result means the auditor cannot detect a dead alternative " + "at all.", file=sys.stderr) + ok = False + # And it must clear an alternative that IS covered: build a symbol set + # from one probe per alternative, and require a clean verdict. + synthetic = set() + for a in alloc_re_alternatives(): + # `\w*` -> `x`, `\w+` -> `x`: a concrete name the alternative matches. + synthetic.add("js_" + a.replace(r"\w*", "x").replace(r"\w+", "x")) + residue = dead_alloc_alternatives(synthetic) + if residue: + print(f"self-test FAIL: the auditor reported {len(residue)} " + "alternative(s) dead against a symbol set synthesised from " + f"the alternatives themselves: {residue}. That is a bug in " + "the auditor, not in ALLOC_RE.", 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: @@ -2041,8 +2293,19 @@ def main(): "real corpus IR and require every one to be reported. " "Proves the checker can still fail against the IR perry " "actually emits, not just against frozen fixtures.") + ap.add_argument("--audit-alloc-re", action="store_true", + help="check every ALLOC_RE alternative against the runtime's " + "real `extern \"C\" fn js_*` symbols and fail on any " + "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.") ns = ap.parse_args() + # Standalone static audit: it reads the crates, not an IR corpus, so it is + # checked before the corpus arguments are. + if ns.audit_alloc_re: + return audit_alloc_re() + # A knob that is silently ignored is a disarmed knob: `--max-stale 0` # without `--stale-registers` would run the bind-anchored check and look # like it enforced a budget, and `--fatal-sinks` alone would look like it diff --git a/test-files/test_gap_gc_regexp_receiver_rooting.ts b/test-files/test_gap_gc_regexp_receiver_rooting.ts new file mode 100644 index 0000000000..c30e12b737 --- /dev/null +++ b/test-files/test_gap_gc_regexp_receiver_rooting.ts @@ -0,0 +1,80 @@ +// #7154: the RECEIVER of `re.test(s)` / `re.exec(s)` must be rooted across the +// `ToString(s)` coercion the lowering emits below it. +// +// `Expr::RegExpTest` / `Expr::RegExpExec` lowered the receiver first, unboxed it +// to a raw `RegExpHeader*` in a bare SSA register, and only THEN emitted +// `js_jsvalue_to_string_coerce`. That coerce is not a bystander: it allocates, +// and on an object argument it dispatches a user `toString`, which is arbitrary +// JS with its own loop back-edge polls. Under `PERRY_GC_MOVING_LOOP_POLLS=1` one +// of those polls runs an evacuating minor while the regexp is live only in that +// register, and `js_regexp_test` then dereferences abandoned from-space memory. +// +// This is the residual #7226 measured and named rather than fixed. In the +// `sfw-registry` reproducer it is `src/lib/api/shared.ts:67`, +// `/\[[a-zA-Z]+\]/.test(url)`, faulting at +// `perry_fn_src_lib_api_shared_ts__defineApiCall + 404`: +// +// bl js_regexp_new ; ALLOCATES +// and x20, x0, #0xffffffffffff ; raw regexp pointer -> bare register +// bl js_jsvalue_to_string_coerce ; ALLOCATES, runs user toString +// mov x0, x20 ; STALE +// bl js_regexp_test ; faults here +// +// The static checker could not see it, and that is the other half of the bug: +// `ALLOC_RE` carried an alternative spelled `regexp_alloc\w*` while the call is +// named `js_regexp_new`, so the register had no recognised heap-value source. +// No such symbol as `js_regexp_alloc` has ever existed. +// +// LIVE BY CONSTRUCTION. Both arms use a regex LITERAL receiver, which is the +// registry's shape and the strongest one: `js_regexp_new`'s result is held +// ONLY in the register, so nothing else keeps it alive across the coerce. The +// coercion allocates long enough that the minor runs early inside it and the +// abandoned bytes are then reused by the rest of the coercion's own work. The +// answers are checked against known-correct booleans and match text, so a stale +// read is observable rather than latent. Clean under `PERRY_GEN_GC=0`, so the +// evacuating arms are the ones that bite. + +// The loop is what matters, not its trip count: under `PERRY_GC_ZEAL=1` the +// FIRST back-edge poll inside it already runs an evacuating minor, which is +// the collection the receiver has to survive. The count is kept modest on +// purpose — zeal collects at every safepoint, so a 4000-trip churn (what the +// sibling #7154 tests use, where the collection has to arrive on its own +// budget) turns this file into a multi-hour run for no extra coverage. +function churn(tag: string): string { + const bits: any[] = []; + for (let i = 0; i < 120; i++) { + bits.push({ i: i, s: "x", pad: [i, i + 1, i + 2] }); + } + return bits.length === 120 ? tag : "unreachable"; +} + +class Coercer { + tag: string; + constructor(tag: string) { + this.tag = tag; + } + toString(): string { + return churn(this.tag); + } +} + +function run(): number { + let bad = 0; + for (let r = 0; r < 40; r++) { + // `.test` — the exact registry site. + if (!/^tag-[0-9]+$/.test(new Coercer("tag-" + r) as any)) { + bad++; + } + if (/^tag-[0-9]+$/.test(new Coercer("nope-" + r) as any)) { + bad++; + } + // `.exec` — the same lowering with the same defect, fixed with it. + const m = /^tag-([0-9]+)$/.exec(new Coercer("tag-" + r) as any); + if (m === null || m[1] !== String(r)) { + bad++; + } + } + return bad; +} + +console.log("bad", run()); diff --git a/test-parity/gc_repsel_corpus.txt b/test-parity/gc_repsel_corpus.txt index 9991520d51..ff1164da4f 100644 --- a/test-parity/gc_repsel_corpus.txt +++ b/test-parity/gc_repsel_corpus.txt @@ -317,3 +317,25 @@ test_gap_gc_assign_string_source_rooting # make a typeof cache or an implicit-this restore pass. test_gap_gc_typeof_string_cache_rooting test_gap_gc_closure_call_prev_this_rooting +# --- The regexp receiver across ToString (#7154 residual) -------------------- +# `re.test(s)` / `re.exec(s)` unboxed the receiver to a raw `RegExpHeader*` and +# only THEN emitted `js_jsvalue_to_string_coerce`, which allocates and can +# dispatch a user `toString` -- arbitrary JS with its own back-edge polls. #7226 +# measured this residual and named it rather than claiming closure; this file is +# it, and it is the last thing between `sfw-registry --help` and 30/30 under a +# genuine polls build. +# +# It is also the witness for the CHECKER half of the same bug. `ALLOC_RE` spelled +# the allocator `regexp_alloc\w*` while the real call is `js_regexp_new`, so the +# static pass had no recognised heap-value source for the register and reported +# nothing at all. That hole is gated now (`--audit-alloc-re`, which requires +# every alternative to match a real exported symbol), but a static gate cannot +# prove the FIX -- only running the program under a moving collector does, and +# only the `requires=move` arms do that. +# +# Measured on this branch, release, `--arms loop_polls --filter test_gap_gc_`: +# PASS on `loop_polls` 8/8 (`bad 0`, matching the oracle), and exit=139 on all +# TEN allocation-point arms, also deterministic. See gc_repsel_triage.txt: the +# fix in this PR is verified on the safepoint route and is NOT claimed on the +# allocation-point route, which is #7217's open defect class at a second site. +test_gap_gc_regexp_receiver_rooting diff --git a/test-parity/gc_repsel_triage.txt b/test-parity/gc_repsel_triage.txt index 75fdba403f..c7d2574114 100644 --- a/test-parity/gc_repsel_triage.txt +++ b/test-parity/gc_repsel_triage.txt @@ -65,3 +65,39 @@ test_gap_gc_assign_string_source_rooting | rep_ptr_shape_off | #7217 -- %E% a test_gap_gc_assign_string_source_rooting | rep_ptr_numarray_off | #7217 -- %E% allocation-point window; see rep_i32_off. test_gap_gc_assign_string_source_rooting | rep_spec_abi_off | #7217 -- %E% allocation-point window; see rep_i32_off. test_gap_gc_assign_string_source_rooting | rep_int_valued_off | #7217 -- %E% allocation-point window; see rep_i32_off. +# --- The regexp receiver on the ALLOCATION-POINT arms (#7217 class) ---------- +# `test_gap_gc_regexp_receiver_rooting` is a HARD GATE on `loop_polls`, where the +# fix it ships with is claimed and verified: `bad 0`, 8/8, byte-exact against the +# oracle, with the copying minor live. These ten entries cover the arms where no +# fix is claimed -- the ones that force the collection at the register-imprecise +# ALLOCATION point (`%E%` without the compile-time PERRY_GC_MOVING_LOOP_POLLS=1) +# rather than at a loop safepoint. +# +# Measured on this branch, release, oracle node 26.5.1 (`bad 0`): +# PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0 PERRY_CONSERVATIVE_STACK_SCAN=off +# -> exit=139, no output, 8/8 deterministic +# compiled+run PERRY_GC_MOVING_LOOP_POLLS=1 -> `bad 0`, 8/8 +# shipped default -> `bad 0` +# `--arms all` reports the SAME `exit=139 cycles=1 scavenged=14` on all ten, and +# PASS on `loop_polls`. The split is the route, not the arm's other knobs. +# +# THIS IS THE SECOND FILE WITH EXACTLY THIS SIGNATURE. #7216's +# `test_gap_gc_assign_string_source_rooting` behaves the same way and is triaged +# to #7217 for the same reason. Two independent sites where a rooting fix that +# holds at a safepoint does not hold when the collection is forced inside the +# allocating helper is a statement about the route, not about either fix -- and +# it is what #7217 says in words about `object_assign_set_string_key`'s interning +# and keys-array growth. Whoever closes #7217 should check this file too; if it +# turns out to need its own fix, split these entries onto their own issue. +# +# DELETE ALL TEN when the allocation-point route is fixed. +test_gap_gc_regexp_receiver_rooting | evac_minor | #7217 -- allocation-point relocation; the fix is verified on the safepoint route (loop_polls, 8/8) and not claimed here. exit=139, deterministic. +test_gap_gc_regexp_receiver_rooting | force_evac | #7217 -- same allocation-point window, with force-evacuate on top. +test_gap_gc_regexp_receiver_rooting | force_verify | #7217 -- same allocation-point window, force + verify. +test_gap_gc_regexp_receiver_rooting | rep_i32_off | #7217 -- %E% allocation-point window; the repsel knob is not the discriminator (identical evidence on all ten). +test_gap_gc_regexp_receiver_rooting | rep_str_off | #7217 -- %E% allocation-point window; see rep_i32_off. +test_gap_gc_regexp_receiver_rooting | rep_str_static_off | #7217 -- %E% allocation-point window; see rep_i32_off. +test_gap_gc_regexp_receiver_rooting | rep_ptr_shape_off | #7217 -- %E% allocation-point window; see rep_i32_off. +test_gap_gc_regexp_receiver_rooting | rep_ptr_numarray_off | #7217 -- %E% allocation-point window; see rep_i32_off. +test_gap_gc_regexp_receiver_rooting | rep_spec_abi_off | #7217 -- %E% allocation-point window; see rep_i32_off. +test_gap_gc_regexp_receiver_rooting | rep_int_valued_off | #7217 -- %E% allocation-point window; see rep_i32_off.