diff --git a/.github/workflows/gc-root-dominance.yml b/.github/workflows/gc-root-dominance.yml index 1b7b0e0205..d58cde4df8 100644 --- a/.github/workflows/gc-root-dominance.yml +++ b/.github/workflows/gc-root-dominance.yml @@ -548,8 +548,53 @@ jobs: # residual above, newly visible because `js_new_function_construct` # is now classified as a mover. # + # ── 8 here. `expr/call_spread.rs` roots the argument-bundle + # accumulator, which closes all three `unrooted:alloc` hits. Measured + # on this corpus, same binary both arms, 11 -> 8 with the other eight + # byte-identical and `stale` still 0. + # + # ★ THE FOUR `unmasked` HITS ARE CHECKER FALSE POSITIVES, and the + # description three paragraphs up ("the reload has to go in the + # PREDECESSOR, on the edge") is solving a problem that is not there. + # `"phi"` is in `TRANSPARENT_OPS`, so taint flows from a phi OPERAND to + # the phi RESULT and the use is located at the phi's block — but a phi + # operand is used on ITS INCOMING EDGE, not at the join. All four have + # the identical shape, e.g. `readCtx`: + # + # entry.0: %r2 = + # br i1 %r4, label %then, label %merge + # merge: %r87 = phi double [ %r2, %entry.0 ], + # [ %r86, %pget.recv_merge ] + # ret double %r87 + # + # The safepoints are all on the `%then` path, where the phi selects + # `%r86`. On the edge that carries `%r2` nothing collects between its + # definition and the join. Verified register-by-register on all four + # (`readCtx`, `__closure_5`, `Readable`, `__obj_method_toLocaleString_3` + # -- every one a `logical.merge` join of an `&&`). + # + # So the residual population is 4, not 8, and it splits: + # + # 2 global (`@perry_global_*`), test_gap_arraybuffer_transfer::main. + # Real; needs ROOTING, per the note above. + # 2 capture. `test_gap_class_expr_dynamic_parent_ctor::__closure_21` + # is real and checked by hand: `%r7 = bitcast %r61` (capture 0, + # the dynamic parent class) is read at the top of the closure and + # passed to `js_new_function_construct` ~60 lines below, across + # `js_object_alloc_class_inline_keys` AND a user constructor, and + # appears in no `gc-live` bundle anywhere in the function. It is + # NOT reloadable the way a strhandle is: the recipe would have to + # re-derive the closure pointer from `%this_closure`, an i64 + # PARAMETER that RS4GC does not relocate, so the re-read would + # address the pre-move closure. That population needs the callee's + # own closure pointer to be a tracked root first. + # # #7664 stays open as this budget's referent: a number with nothing - # behind it is the thing CLAUDE.md warns a threshold decays into. + # behind it is the thing CLAUDE.md warns a threshold decays into. It + # cannot go below 4 without either the two rooting fixes or the + # edge-sensitive phi rule, and the phi rule must arrive with a + # sabotage arm proving it still reports a phi operand that IS live + # across a safepoint on its own edge. # # `stale` (the object survives and is relocated, but a raw copy of # its pre-move address is used below) reads 0 today and is held @@ -569,7 +614,7 @@ jobs: --min-statepoints 15000 \ --min-live-bundles 8000 \ --min-relocates 20000 \ - --max-unrooted 11 \ + --max-unrooted 8 \ --max-stale 0 \ --allowlist scripts/gc_root_dominance_allowlist.json \ --seeded-violations 40 \ diff --git a/CLAUDE.md b/CLAUDE.md index 20d4a80853..938d6bf0ab 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,7 +8,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co Perry is a native TypeScript compiler written in Rust that compiles TypeScript source code directly to native executables. It uses SWC for TypeScript parsing and LLVM for code generation. -**Current Version:** 0.5.1395 +**Current Version:** 0.5.1396 ## TypeScript Parity Status diff --git a/Cargo.lock b/Cargo.lock index 12cd207708..6ad6e538f0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5547,7 +5547,7 @@ checksum = "9b4f627cb1b25917193a259e49bdad08f671f8d9708acfd5fe0a8c1455d87220" [[package]] name = "perry" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "anyhow", "base64", @@ -5607,14 +5607,14 @@ dependencies = [ [[package]] name = "perry-api-manifest" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "serde", ] [[package]] name = "perry-audio-miniaudio" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "cc", "libc", @@ -5622,7 +5622,7 @@ dependencies = [ [[package]] name = "perry-codegen" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "anyhow", "inkwell", @@ -5639,7 +5639,7 @@ dependencies = [ [[package]] name = "perry-codegen-arkts" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "anyhow", "perry-hir", @@ -5647,7 +5647,7 @@ dependencies = [ [[package]] name = "perry-codegen-glance" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "anyhow", "perry-hir", @@ -5655,7 +5655,7 @@ dependencies = [ [[package]] name = "perry-codegen-js" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "anyhow", "perry-dispatch", @@ -5664,7 +5664,7 @@ dependencies = [ [[package]] name = "perry-codegen-swiftui" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "anyhow", "perry-hir", @@ -5672,7 +5672,7 @@ dependencies = [ [[package]] name = "perry-codegen-wasm" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "anyhow", "base64", @@ -5684,7 +5684,7 @@ dependencies = [ [[package]] name = "perry-codegen-wear-tiles" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "anyhow", "perry-hir", @@ -5692,7 +5692,7 @@ dependencies = [ [[package]] name = "perry-container-compose" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "anyhow", "async-trait", @@ -5721,14 +5721,14 @@ dependencies = [ [[package]] name = "perry-container-e2e" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "anyhow", ] [[package]] name = "perry-diagnostics" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "serde", "serde_json", @@ -5736,7 +5736,7 @@ dependencies = [ [[package]] name = "perry-dispatch" -version = "0.5.1395" +version = "0.5.1396" [[package]] name = "perry-doc-fixture-my-bindings" @@ -5747,7 +5747,7 @@ dependencies = [ [[package]] name = "perry-doc-tests" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "anyhow", "clap", @@ -5762,7 +5762,7 @@ dependencies = [ [[package]] name = "perry-ext-ads" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "block2", "objc2", @@ -5772,7 +5772,7 @@ dependencies = [ [[package]] name = "perry-ext-argon2" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "argon2", "perry-ffi", @@ -5780,7 +5780,7 @@ dependencies = [ [[package]] name = "perry-ext-axios" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-ffi", "reqwest", @@ -5789,7 +5789,7 @@ dependencies = [ [[package]] name = "perry-ext-bcrypt" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "bcrypt", "perry-ffi", @@ -5797,7 +5797,7 @@ dependencies = [ [[package]] name = "perry-ext-better-sqlite3" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-ffi", "rusqlite", @@ -5805,7 +5805,7 @@ dependencies = [ [[package]] name = "perry-ext-cheerio" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-ffi", "scraper", @@ -5813,7 +5813,7 @@ dependencies = [ [[package]] name = "perry-ext-commander" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-ffi", "perry-runtime", @@ -5821,7 +5821,7 @@ dependencies = [ [[package]] name = "perry-ext-cron" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "chrono", "cron", @@ -5831,7 +5831,7 @@ dependencies = [ [[package]] name = "perry-ext-dayjs" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "chrono", "perry-ffi", @@ -5839,7 +5839,7 @@ dependencies = [ [[package]] name = "perry-ext-decimal" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-ffi", "rust_decimal", @@ -5847,7 +5847,7 @@ dependencies = [ [[package]] name = "perry-ext-dotenv" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-ffi", "serde_json", @@ -5855,7 +5855,7 @@ dependencies = [ [[package]] name = "perry-ext-ethers" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-ffi", "rand 0.10.1", @@ -5863,7 +5863,7 @@ dependencies = [ [[package]] name = "perry-ext-events" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-ffi", "perry-runtime", @@ -5871,14 +5871,14 @@ dependencies = [ [[package]] name = "perry-ext-exponential-backoff" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-ffi", ] [[package]] name = "perry-ext-fastify" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "bytes", "http-body-util", @@ -5896,7 +5896,7 @@ dependencies = [ [[package]] name = "perry-ext-fetch" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "bytes", "lazy_static", @@ -5909,7 +5909,7 @@ dependencies = [ [[package]] name = "perry-ext-http" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "bytes", "h2", @@ -5933,7 +5933,7 @@ dependencies = [ [[package]] name = "perry-ext-ioredis" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "lazy_static", "perry-ffi", @@ -5943,7 +5943,7 @@ dependencies = [ [[package]] name = "perry-ext-jsonwebtoken" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "base64", "jsonwebtoken", @@ -5954,7 +5954,7 @@ dependencies = [ [[package]] name = "perry-ext-lru-cache" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "lru", "perry-ffi", @@ -5963,7 +5963,7 @@ dependencies = [ [[package]] name = "perry-ext-moment" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "chrono", "perry-ffi", @@ -5971,7 +5971,7 @@ dependencies = [ [[package]] name = "perry-ext-mongodb" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "bson", "futures-util", @@ -5983,7 +5983,7 @@ dependencies = [ [[package]] name = "perry-ext-mysql2" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "chrono", "perry-ffi", @@ -5993,7 +5993,7 @@ dependencies = [ [[package]] name = "perry-ext-nanoid" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "nanoid", "perry-ffi", @@ -6002,7 +6002,7 @@ dependencies = [ [[package]] name = "perry-ext-net" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "bytes", "perry-ffi", @@ -6015,7 +6015,7 @@ dependencies = [ [[package]] name = "perry-ext-node-forge" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "const-oid 0.9.6", "der 0.7.10", @@ -6034,7 +6034,7 @@ dependencies = [ [[package]] name = "perry-ext-nodemailer" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "lettre", "perry-ffi", @@ -6044,7 +6044,7 @@ dependencies = [ [[package]] name = "perry-ext-pdf" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-ffi", "printpdf", @@ -6052,7 +6052,7 @@ dependencies = [ [[package]] name = "perry-ext-pg" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-ffi", "sqlx", @@ -6061,7 +6061,7 @@ dependencies = [ [[package]] name = "perry-ext-ratelimit" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "governor", "perry-ffi", @@ -6069,7 +6069,7 @@ dependencies = [ [[package]] name = "perry-ext-sharp" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "fast_image_resize", "image", @@ -6079,14 +6079,14 @@ dependencies = [ [[package]] name = "perry-ext-slugify" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-ffi", ] [[package]] name = "perry-ext-streams" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "lazy_static", "perry-ffi", @@ -6095,7 +6095,7 @@ dependencies = [ [[package]] name = "perry-ext-undici" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-ffi", "perry-runtime", @@ -6104,7 +6104,7 @@ dependencies = [ [[package]] name = "perry-ext-uuid" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-ffi", "uuid", @@ -6112,7 +6112,7 @@ dependencies = [ [[package]] name = "perry-ext-validator" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-ffi", "regex", @@ -6122,7 +6122,7 @@ dependencies = [ [[package]] name = "perry-ext-ws" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "futures-util", "lazy_static", @@ -6135,7 +6135,7 @@ dependencies = [ [[package]] name = "perry-ext-zlib" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "brotli", "flate2", @@ -6145,7 +6145,7 @@ dependencies = [ [[package]] name = "perry-ffi" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "dashmap", "once_cell", @@ -6154,7 +6154,7 @@ dependencies = [ [[package]] name = "perry-hir" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "anyhow", "perry-api-manifest", @@ -6172,7 +6172,7 @@ dependencies = [ [[package]] name = "perry-parser" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "anyhow", "perry-diagnostics", @@ -6184,7 +6184,7 @@ dependencies = [ [[package]] name = "perry-runtime" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "anyhow", "base64", @@ -6226,14 +6226,14 @@ dependencies = [ [[package]] name = "perry-runtime-static" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-runtime", ] [[package]] name = "perry-stdlib" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "aes 0.8.4", "aes 0.9.1", @@ -6328,14 +6328,14 @@ dependencies = [ [[package]] name = "perry-stdlib-static" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-stdlib", ] [[package]] name = "perry-transform" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "anyhow", "perry-hir", @@ -6344,14 +6344,14 @@ dependencies = [ [[package]] name = "perry-ui" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-ui-model", ] [[package]] name = "perry-ui-android" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "base64", "itoa", @@ -6368,7 +6368,7 @@ dependencies = [ [[package]] name = "perry-ui-geisterhand" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "rand 0.10.1", "serde", @@ -6378,7 +6378,7 @@ dependencies = [ [[package]] name = "perry-ui-gtk4" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "base64", "cairo-rs 0.22.0", @@ -6401,7 +6401,7 @@ dependencies = [ [[package]] name = "perry-ui-ios" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "base64", "block2", @@ -6417,7 +6417,7 @@ dependencies = [ [[package]] name = "perry-ui-macos" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "base64", "block2", @@ -6432,7 +6432,7 @@ dependencies = [ [[package]] name = "perry-ui-model" -version = "0.5.1395" +version = "0.5.1396" [[package]] name = "perry-ui-test" @@ -6443,11 +6443,11 @@ dependencies = [ [[package]] name = "perry-ui-testkit" -version = "0.5.1395" +version = "0.5.1396" [[package]] name = "perry-ui-tvos" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "base64", "block2", @@ -6463,7 +6463,7 @@ dependencies = [ [[package]] name = "perry-ui-visionos" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "base64", "block2", @@ -6479,7 +6479,7 @@ dependencies = [ [[package]] name = "perry-ui-watchos" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "block2", "libc", @@ -6492,7 +6492,7 @@ dependencies = [ [[package]] name = "perry-ui-windows" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "base64", "libc", @@ -6509,14 +6509,14 @@ dependencies = [ [[package]] name = "perry-ui-windows-winui" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "perry-ui-windows", ] [[package]] name = "perry-updater" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "anyhow", "base64", @@ -6532,7 +6532,7 @@ dependencies = [ [[package]] name = "perry-wasm-host" -version = "0.5.1395" +version = "0.5.1396" dependencies = [ "wasmi", ] diff --git a/Cargo.toml b/Cargo.toml index 7c7a39f516..201efddd39 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -315,7 +315,7 @@ codegen-units = 16 codegen-units = 16 [workspace.package] -version = "0.5.1395" +version = "0.5.1396" edition = "2021" license = "MIT" repository = "https://github.com/PerryTS/perry" diff --git a/changelog.d/7696-spread-accumulator-rooting.md b/changelog.d/7696-spread-accumulator-rooting.md new file mode 100644 index 0000000000..b4bd2f2121 --- /dev/null +++ b/changelog.d/7696-spread-accumulator-rooting.md @@ -0,0 +1,88 @@ +### gc: the spread argument-bundle accumulator is rooted, and four of the residual hazards turn out to be checker false positives (#7664, #7453) + +Six arms of `expr/call_spread.rs` bundle every argument — regular and spread, in +source order — into one JS array before dispatching (`console.*` spread, the +`recv.m(...)` and `recv[k](...)` method-apply arms, the namespace REST export, +and the closure-callee path's interleaved and multi-spread arms). All six wrote +the same loop and all six held the half-built array in a bare `i64` SSA register +across it: + +```llvm +%acc = call i64 @js_array_alloc(i32 0) +%box = call double @perry_fn_…(…) ; arbitrary user code +%part = call i64 @js_array_like_to_array(double %box) ; ALLOCATES +%acc2 = call i64 @js_array_concat(i64 %acc, i64 %part) ; %acc is stale +``` + +`--statepoints --moving-only` reports that as `unrooted:alloc`: nothing in the +register's cast chain appears in the `js_array_like_to_array` safepoint's live +bundle, so an evacuating minor there neither marks nor rewrites the array and +`js_array_concat` reads from-space. It is #7453's shape — a fresh heap value in a +raw register across an allocating helper — in the spread lowering rather than the +URL one, and it is reachable from `a.splice(1, 0, ...src)`. + +**The window is never empty here**, which is why the six sites share one helper +(`bundle_args_rooted`, a `rooting::with_rooted_accumulator` with the two fold +steps this lowering needs) rather than each asking `operand_protection` +separately. An `Expr::CallSpread` has at least one spread source by construction +and `js_array_like_to_array` allocates unconditionally, so `f(...[1, 2])` — every +operand an inert literal — is already the bug, and an operands-only "can anything +here collect?" predicate answers `false` for it. The `protect` predicate disjoins +the spread-present test for exactly that reason. + +**Measured** on the native corpus (150 modules, 2538 functions, 32689 +statepoints, 19593 live bundles), the same binary in both arms: **11 → 8** +`unrooted` hazards, `stale` still 0. The three that disappear are exactly the +three `unrooted:alloc` hits in `test_gap_array_splice_spread::main`; the other +eight are byte-identical. `gc-root-dominance-statepoints`'s `--max-unrooted` +drops to 8 in the same commit. All 71 gap tests containing a spread call are +byte-identical to the pinned Node 26.5.1 oracle. + +The tests assert **ordering, never slot counts**, and walk *every* +`js_array_concat` rather than the first: on the default build the pooled-alloca +lowering emits a plain `store`/`load` and no `js_gc_temp_root_*` call at all, so +a `temp_root_calls(ir) > 0` assertion reads zero and passes vacuously; and a +bundle emits one fold per spread source, so checking only the first would leave a +partially-fixed loop green. The sabotage arm (`protect = false`, which reproduces +the pre-fix IR exactly) takes the suite to `0 passed; 3 failed`. + +### ★ Four of the remaining eight hazards are checker false positives + +The gate's comment described them as *"4 unmasked, all PHI-MEDIATED … the reload +has to go in the PREDECESSOR, on the edge, which is a different insertion +model"*. That is a fix for a problem that is not there, and the comment is +corrected rather than the code. + +`"phi"` is in `TRANSPARENT_OPS`, so taint flows from a phi *operand* to the phi +*result* and the use is located at the join — but a phi operand is used on **its +own incoming edge**. All four hits are the same `&&` join, e.g. `readCtx`: + +```llvm +entry.0: %r2 = + br i1 %r4, label %logical.then.1, label %logical.merge.2 +logical.merge.2: + %r87 = phi double [ %r2, %entry.0 ], [ %r86, %pget.recv_merge.7 ] + ret double %r87 +``` + +Every safepoint is on the `%logical.then.1` path, where the phi selects `%r86`; +on the edge that carries `%r2` nothing collects between its definition and the +join. Verified register-by-register on all four (`readCtx`, `__closure_5`, +`Readable`, `__obj_method_toLocaleString_3`). + +So the real residual is **4**: two `@perry_global_*` reads +(`test_gap_arraybuffer_transfer::main`) that need rooting rather than reloading, +and two capture reads. The capture one in +`test_gap_class_expr_dynamic_parent_ctor::__closure_21` was checked by hand and +is real — capture 0 (the dynamic parent class) is read at the top of the closure +and passed to `js_new_function_construct` ~60 lines below, across +`js_object_alloc_class_inline_keys` **and** a user constructor, appearing in no +`gc-live` bundle anywhere in the function. It is **not** reloadable the way a +string-handle global is: the recipe would have to re-derive the closure pointer +from `%this_closure`, an `i64` *parameter* that RS4GC does not relocate, so the +re-read would address the pre-move closure. That population needs the callee's +own closure pointer to be a tracked root first. + +An edge-sensitive phi rule is deliberately not included: it *lowers* a reported +count, so it must arrive with a sabotage arm proving it still reports a phi +operand that **is** live across a safepoint on its own edge. diff --git a/crates/perry-codegen/src/expr/call_spread.rs b/crates/perry-codegen/src/expr/call_spread.rs index 58aaaee961..9fe206a9f0 100644 --- a/crates/perry-codegen/src/expr/call_spread.rs +++ b/crates/perry-codegen/src/expr/call_spread.rs @@ -3,21 +3,118 @@ //! Extracted from `expr/mod.rs` to keep that file under the 2000-line cap. //! Pure mechanical move — match arm bodies are verbatim copies, called from //! `lower_expr`'s outer dispatch. +//! +//! # The argument-bundle accumulator (#7664) +//! +//! Six arms here bundle every argument — regular and spread, in source order — +//! into one JS array before dispatching. All six wrote the same loop, and all +//! six held the half-built array in a bare `i64` SSA register across it: +//! +//! ```llvm +//! %acc = call i64 @js_array_alloc(i32 0) +//! %box = call double @perry_fn_…(…) ; arbitrary user code +//! %part = call i64 @js_array_like_to_array(double %box) ; ALLOCATES +//! %acc2 = call i64 @js_array_concat(i64 %acc, i64 %part) ; %acc is stale +//! ``` +//! +//! `--statepoints --moving-only` reports that as `unrooted:alloc` — the fresh +//! array is in no safepoint's live bundle, so an evacuating minor inside +//! `js_array_like_to_array` neither marks nor rewrites it and `js_array_concat` +//! reads from-space. It is #7453's shape (a fresh heap value in a raw register +//! across an allocating helper) in the spread lowering rather than the URL one, +//! and it is reachable from `a.splice(1, 0, ...src)`. +//! +//! **The window is never empty here**, which is why the six sites share one +//! helper rather than each answering `operand_protection` separately: an +//! `Expr::CallSpread` has at least one spread argument by construction, and +//! `js_array_like_to_array` allocates unconditionally. So the accumulator is at +//! risk even when every operand is an inert literal — `f(...[1, 2])` is the +//! smallest reproducer, and an operands-only predicate answers `false` for it. +//! +//! [`bundle_args_rooted`] is the whole fix. It is `with_rooted_accumulator` +//! with the two fold steps this lowering needs (`js_array_push_f64` for a +//! regular argument, `js_array_concat` for a spread source), so the accumulator +//! never exists as a register the loop holds across an emission. use anyhow::Result; -use perry_hir::Expr; +use perry_hir::{CallArg, Expr}; use crate::nanbox::double_literal; use crate::native_value::MaterializationReason; +use crate::rooting::{self, Arg, Repr}; use crate::type_analysis::receiver_class_name; use crate::types::{DOUBLE, I32, I64}; use super::{downgrade_buffer_aliases_in_expr, lower_expr, nanbox_pointer_inline, FnCtx}; +/// The expression a call argument carries, whatever its spread-ness. +fn call_arg_expr(a: &CallArg) -> &Expr { + match a { + CallArg::Expr(e) | CallArg::Spread(e) => e, + } +} + +/// Bundle `args` into one source-ordered JS array with the accumulator rooted +/// for the whole loop, then hand the finished array to `finish` (#7664). +/// +/// `select` decides which arguments participate: the multi-spread marshalling +/// arm folds only the spread sources, every other arm folds all of them. +/// +/// `finish` runs BELOW the last collection point and ABOVE the release, so the +/// register it receives is the only one that ever escapes — the same split +/// [`rooting::with_rooted_accumulator`] imposes everywhere else. +fn bundle_args_rooted<'f, R>( + ctx: &mut FnCtx<'f>, + args: &[CallArg], + spread_only: bool, + finish: impl FnOnce(&mut FnCtx<'f>, &str) -> Result, +) -> Result { + let acc0 = ctx.block().call(I64, "js_array_alloc", &[(I32, "0")]); + // One-sided, and `true` in practice at every caller — see the module + // header. It is still computed rather than hardcoded so that a future arm + // with no spread source (and therefore no `js_array_like_to_array`) gets + // the un-rooted IR byte for byte instead of paying for a window it does + // not have. + let protect = args.iter().any(|a| matches!(a, CallArg::Spread(_))) + || rooting::any_operand_may_collect(ctx, args.iter().map(call_arg_expr)); + rooting::with_rooted_accumulator( + ctx, + Repr::Ptr, + &acc0, + protect, + |ctx, acc| { + for a in args { + match a { + CallArg::Expr(e) => { + if spread_only { + continue; + } + let v = lower_expr(ctx, e)?; + acc.advance(ctx, "js_array_push_f64", &[Arg::Plain(DOUBLE, &v)]); + } + CallArg::Spread(e) => { + let part_box = lower_expr(ctx, e)?; + // `js_array_like_to_array` allocates, so the re-read + // `advance` fuses to the `concat` has to sit BELOW it. + // That ordering is the fix; a re-read above this call + // would root an already-stale pointer, which is #7192's + // shape. + let part = + ctx.block() + .call(I64, "js_array_like_to_array", &[(DOUBLE, &part_box)]); + acc.advance(ctx, "js_array_concat", &[Arg::Plain(I64, &part)]); + } + } + } + Ok(()) + }, + finish, + ) +} + pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { match expr { Expr::CallSpread { callee, args, .. } => { - use perry_hir::CallArg; let spread_count = args .iter() .filter(|a| matches!(a, CallArg::Spread(_))) @@ -56,30 +153,6 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { "log" | "info" | "warn" | "error" | "debug" ) { - let mut acc_handle = ctx.block().call(I64, "js_array_alloc", &[(I32, "0")]); - for a in args { - match a { - CallArg::Expr(e) => { - let v = lower_expr(ctx, e)?; - acc_handle = ctx.block().call( - I64, - "js_array_push_f64", - &[(I64, &acc_handle), (DOUBLE, &v)], - ); - } - CallArg::Spread(e) => { - let part_box = lower_expr(ctx, e)?; - let blk = ctx.block(); - let part_handle = - blk.call(I64, "js_array_like_to_array", &[(DOUBLE, &part_box)]); - acc_handle = ctx.block().call( - I64, - "js_array_concat", - &[(I64, &acc_handle), (I64, &part_handle)], - ); - } - } - } let runtime_fn = match property.as_str() { "info" => "js_console_info_spread", "debug" => "js_console_debug_spread", @@ -87,8 +160,10 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { "error" => "js_console_error_spread", _ => "js_console_log_spread", }; - ctx.block().call_void(runtime_fn, &[(I64, &acc_handle)]); - return Ok(double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED))); + return bundle_args_rooted(ctx, args, false, |ctx, acc| { + ctx.block().call_void(runtime_fn, &[(I64, acc)]); + Ok(double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED))) + }); } } @@ -235,41 +310,28 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { if !skip { let recv_box = lower_expr(ctx, object)?; // Build a single JS array containing every arg in order. - let mut acc_handle = ctx.block().call(I64, "js_array_alloc", &[(I32, "0")]); - for a in args { - match a { - CallArg::Expr(e) => { - let v = lower_expr(ctx, e)?; - acc_handle = ctx.block().call( - I64, - "js_array_push_f64", - &[(I64, &acc_handle), (DOUBLE, &v)], - ); - } - CallArg::Spread(e) => { - let part_box = lower_expr(ctx, e)?; - let part_handle = ctx.block().call( - I64, - "js_array_like_to_array", - &[(DOUBLE, &part_box)], - ); - acc_handle = ctx.block().call( - I64, - "js_array_concat", - &[(I64, &acc_handle), (I64, &part_handle)], - ); - } - } - } - let key_idx = ctx.strings.intern(property); - let dispatch_global = ctx.strings.static_dispatch_global(key_idx); - let method_id = - crate::strings::emit_static_dispatch_id(ctx.block(), &dispatch_global); - return Ok(ctx.block().call( - DOUBLE, - "js_native_call_method_apply_by_id", - &[(DOUBLE, &recv_box), (I64, &method_id), (I64, &acc_handle)], - )); + // + // ★ `recv_box` is ALSO held across this loop and is NOT + // rooted here. That is a second, distinct window — an + // operand rather than the accumulator — and it is #7640's + // population, not this one. It is left alone deliberately + // rather than half-fixed: rooting it needs the receiver and + // the array in ONE `RootedGroup` scope so the release + // post-dominates the dispatch, which is a different change + // with a different acceptance test. + return bundle_args_rooted(ctx, args, false, |ctx, acc| { + let key_idx = ctx.strings.intern(property); + let dispatch_global = ctx.strings.static_dispatch_global(key_idx); + // Pure: `ptrtoint` + `or`, no collection point between + // the accumulator's final read and the dispatch. + let method_id = + crate::strings::emit_static_dispatch_id(ctx.block(), &dispatch_global); + Ok(ctx.block().call( + DOUBLE, + "js_native_call_method_apply_by_id", + &[(DOUBLE, &recv_box), (I64, &method_id), (I64, acc)], + )) + }); } } @@ -294,37 +356,16 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { if !(crate::type_analysis::is_numeric_expr(ctx, index) && !object_is_class_ref) { let recv_box = lower_expr(ctx, object)?; let key_box = lower_expr(ctx, index)?; - let mut acc_handle = ctx.block().call(I64, "js_array_alloc", &[(I32, "0")]); - for a in args { - match a { - CallArg::Expr(e) => { - let v = lower_expr(ctx, e)?; - acc_handle = ctx.block().call( - I64, - "js_array_push_f64", - &[(I64, &acc_handle), (DOUBLE, &v)], - ); - } - CallArg::Spread(e) => { - let part_box = lower_expr(ctx, e)?; - let part_handle = ctx.block().call( - I64, - "js_array_like_to_array", - &[(DOUBLE, &part_box)], - ); - acc_handle = ctx.block().call( - I64, - "js_array_concat", - &[(I64, &acc_handle), (I64, &part_handle)], - ); - } - } - } - return Ok(ctx.block().call( - DOUBLE, - "js_native_call_method_value_apply", - &[(DOUBLE, &recv_box), (DOUBLE, &key_box), (I64, &acc_handle)], - )); + // Same second window as the `PropertyGet` arm above: + // `recv_box` and `key_box` are unrooted across the bundle. + // #7640, not this change. + return bundle_args_rooted(ctx, args, false, |ctx, acc| { + Ok(ctx.block().call( + DOUBLE, + "js_native_call_method_value_apply", + &[(DOUBLE, &recv_box), (DOUBLE, &key_box), (I64, acc)], + )) + }); } } @@ -372,36 +413,15 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let symbol = format!("perry_fn_{}__{}", source_prefix, origin_suffix); // Bundle every arg (regular + spread) into the single // rest array, in source order. - let mut acc = ctx.block().call(I64, "js_array_alloc", &[(I32, "0")]); - for a in args { - match a { - CallArg::Expr(e) => { - let v = lower_expr(ctx, e)?; - acc = ctx.block().call( - I64, - "js_array_push_f64", - &[(I64, &acc), (DOUBLE, &v)], - ); - } - CallArg::Spread(e) => { - let part_box = lower_expr(ctx, e)?; - let part = ctx.block().call( - I64, - "js_array_like_to_array", - &[(DOUBLE, &part_box)], - ); - acc = ctx.block().call( - I64, - "js_array_concat", - &[(I64, &acc), (I64, &part)], - ); - } - } - } - let rest_box = nanbox_pointer_inline(ctx.block(), &acc); ctx.pending_declares .push((symbol.clone(), DOUBLE, vec![DOUBLE])); - return Ok(ctx.block().call(DOUBLE, &symbol, &[(DOUBLE, &rest_box)])); + return bundle_args_rooted(ctx, args, false, |ctx, acc| { + // `nanbox_pointer_inline` is an `or` with the + // POINTER_TAG constant — pure, so the final read + // above it still dominates the call. + let rest_box = nanbox_pointer_inline(ctx.block(), acc); + Ok(ctx.block().call(DOUBLE, &symbol, &[(DOUBLE, &rest_box)])) + }); } } } @@ -438,32 +458,14 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let (regs_ptr, regs_len, spread_handle) = if interleaved { // Build one array containing every arg in source order, then // apply it as the entire argument list. - let mut acc_handle = ctx.block().call(I64, "js_array_alloc", &[(I32, "0")]); - for a in args { - match a { - CallArg::Expr(e) => { - let v = lower_expr(ctx, e)?; - acc_handle = ctx.block().call( - I64, - "js_array_push_f64", - &[(I64, &acc_handle), (DOUBLE, &v)], - ); - } - CallArg::Spread(e) => { - let part_box = lower_expr(ctx, e)?; - let part_handle = ctx.block().call( - I64, - "js_array_like_to_array", - &[(DOUBLE, &part_box)], - ); - acc_handle = ctx.block().call( - I64, - "js_array_concat", - &[(I64, &acc_handle), (I64, &part_handle)], - ); - } - } - } + // + // The escaped register is safe HERE and only here: the accumulator's + // final read is the last emission before + // `js_closure_call_apply_with_spread`, which is the consuming call + // itself, so no collection point separates them. Anything inserted + // between this `if` and that call has to root the handle again. + let acc_handle = + bundle_args_rooted(ctx, args, false, |_ctx, acc| Ok(acc.to_string()))?; ("null".to_string(), "0".to_string(), acc_handle) } else { // Marshal regular args into a stack buffer (or null/0 if none). @@ -506,23 +508,10 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let blk = ctx.block(); blk.call(I64, "js_array_like_to_array", &[(DOUBLE, &arr_box)]) } else { - // Concat all spread sources into a fresh array. - let acc = ctx.block().call(I64, "js_array_alloc", &[(I32, "0")]); - let mut acc_handle = acc; - for a in args { - if let CallArg::Spread(e) = a { - let part_box = lower_expr(ctx, e)?; - let blk = ctx.block(); - let part_handle = - blk.call(I64, "js_array_like_to_array", &[(DOUBLE, &part_box)]); - acc_handle = ctx.block().call( - I64, - "js_array_concat", - &[(I64, &acc_handle), (I64, &part_handle)], - ); - } - } - acc_handle + // Concat all spread sources into a fresh array. Same + // escaped-register argument as the interleaved arm above: + // the consuming call is the next emission. + bundle_args_rooted(ctx, args, true, |_ctx, acc| Ok(acc.to_string()))? }; (regs_ptr, regs_len, spread_handle) }; diff --git a/crates/perry-codegen/src/expr/call_spread_rooting_tests.rs b/crates/perry-codegen/src/expr/call_spread_rooting_tests.rs new file mode 100644 index 0000000000..9bcd3aac5d --- /dev/null +++ b/crates/perry-codegen/src/expr/call_spread_rooting_tests.rs @@ -0,0 +1,204 @@ +//! Rooting coverage for the spread argument-bundle accumulator (#7664). +//! +//! # What is asserted, and why it cannot pass vacuously +//! +//! **Ordering, never slot counts.** The harness is slice 7's and the reason is +//! slice 8's, restated because it is the trap this file would otherwise fall +//! into: on the DEFAULT build `reserve_shadow_slot` hands back a stack-map +//! index, no `js_shadow_frame_enter` is emitted, and **no `js_gc_temp_root_*` +//! call is emitted either** — the pooled-alloca lowering is a plain +//! `store`/`load` pair. So `temp_root_calls(ir) > 0` reads **zero** here and a +//! test built on it would assert nothing at all. The property that is visible +//! in all three lowerings (pooled alloca, shadow frame, FFI fallback) is +//! **where the accumulator operand is defined**: below the allocating call, or +//! above it. +//! +//! **Every fold is checked, not the first one.** `require_call_line` returns +//! the first match; a bundle emits one `js_array_concat` per spread source, and +//! checking only the first would leave a partially-fixed loop green. [`assert_every_fold_rereads_the_accumulator`] +//! walks them all. +//! +//! # The window, and why it is never empty +//! +//! `js_array_like_to_array` allocates unconditionally. An `Expr::CallSpread` +//! has at least one spread source by construction, so every bundle has at least +//! one allocating call between `js_array_alloc` and the `js_array_concat` that +//! consumes the accumulator. That is why these tests use INERT operands +//! (`Expr::Number`) in one arm: the hazard does not need a user callback to +//! exist, and an operands-only "can this collect?" predicate answers `false` +//! for `f(...[1, 2])` while the accumulator is still at risk. + +use perry_hir::types::Type; +use perry_hir::{CallArg, Expr, Function, Module as HirModule, Stmt}; + +use super::slice7_rooting_tests::allocating; + +/// Compile a one-function module whose body is `stmts` and return its LLVM IR. +/// +/// A local copy rather than a re-export: `compile_body` is private to slice 7 +/// and duplicating six fields is cheaper than widening its visibility for one +/// caller. +fn compile_body(name: &str, body: Vec) -> String { + let mut hir = HirModule::new(name); + hir.functions.push(Function { + id: 0, + name: "build".to_string(), + type_params: Vec::new(), + params: Vec::new(), + return_type: Type::Any, + body, + is_async: false, + is_generator: false, + is_strict: true, + is_exported: false, + captures: Vec::new(), + decorators: Vec::new(), + was_plain_async: false, + was_unrolled: false, + }); + let opts = crate::CompileOptions { + emit_ir_only: true, + ..Default::default() + }; + let bytes = crate::compile_module(&hir, opts).expect("test module compiles"); + String::from_utf8(bytes).expect("LLVM IR is UTF-8") +} + +/// Line indices of every non-`declare` call to `callee`. +fn call_lines(ir: &str, callee: &str) -> Vec { + let needle = format!("@{callee}("); + ir.lines() + .enumerate() + .filter(|(_, l)| l.contains(&needle) && !l.trim_start().starts_with("declare")) + .map(|(i, _)| i) + .collect() +} + +/// The register passed as argument 0 of the call on `line`. +fn first_operand(ir: &str, line: usize) -> String { + let text = ir.lines().nth(line).expect("line index came from this IR"); + let args = text + .rsplit_once('(') + .unwrap_or_else(|| panic!("call has no argument list: {text}")) + .1; + args.split(',') + .next() + .expect("a call has at least one argument") + .trim() + .rsplit(' ') + .next() + .expect("an operand is a type followed by a register") + .trim_end_matches(')') + .to_string() +} + +fn definition_line(ir: &str, reg: &str) -> Option { + let prefix = format!("{reg} = "); + ir.lines().position(|l| l.trim_start().starts_with(&prefix)) +} + +/// Every `js_array_concat` in `ir` must read an accumulator register defined +/// BELOW the `js_array_like_to_array` immediately above it. +/// +/// That is the whole invariant. `js_array_like_to_array` allocates; a register +/// defined above it and used below it is the `unrooted:alloc` hazard +/// `--statepoints --moving-only` reports, because nothing in its cast chain +/// appears in that safepoint's live bundle. +fn assert_every_fold_rereads_the_accumulator(ir: &str, what: &str) { + let folds = call_lines(ir, "js_array_concat"); + assert!( + !folds.is_empty(), + "{what}: no js_array_concat in the emitted IR, so this test asserts \ + nothing about the bundle it was written for. The lowering changed \ + shape — re-aim the test rather than deleting it.\n{ir}" + ); + let windows = call_lines(ir, "js_array_like_to_array"); + assert!( + !windows.is_empty(), + "{what}: no js_array_like_to_array, so there is no window and the \ + assertion below is vacuous.\n{ir}" + ); + for fold in folds { + let acc = first_operand(ir, fold); + let def = definition_line(ir, &acc) + .unwrap_or_else(|| panic!("{what}: no definition for the accumulator {acc} in:\n{ir}")); + let window = windows + .iter() + .copied() + .filter(|&w| w < fold) + .next_back() + .unwrap_or_else(|| { + panic!("{what}: js_array_concat at line {fold} has no allocating conversion above it\n{ir}") + }); + assert!( + def > window, + "{what}: js_array_concat at line {fold} reads {acc}, defined at line {def}, \ + ABOVE the js_array_like_to_array at line {window}. That conversion allocates, \ + so an evacuating minor inside it relocates the half-built argument array and \ + this concat reads from-space. The accumulator must be rooted above the window \ + and re-read below it.\n{ir}" + ); + } +} + +/// `cb(...spread, regular)` — a regular argument AFTER a spread, which forces +/// the source-ordered single-array path (`interleaved`). +#[test] +fn an_interleaved_spread_bundle_rereads_its_accumulator_below_the_array_conversion() { + let ir = compile_body( + "interleaved_spread", + vec![Stmt::Expr(Expr::CallSpread { + callee: Box::new(allocating("cb")), + args: vec![ + CallArg::Spread(allocating("a")), + CallArg::Expr(allocating("b")), + CallArg::Spread(allocating("c")), + ], + type_args: Vec::new(), + })], + ); + assert_every_fold_rereads_the_accumulator(&ir, "interleaved spread bundle"); +} + +/// `cb(...a, ...b)` — two spread sources and no regular argument, which takes +/// the multi-spread marshalling arm instead. +#[test] +fn a_multi_spread_bundle_rereads_its_accumulator_below_the_array_conversion() { + let ir = compile_body( + "multi_spread", + vec![Stmt::Expr(Expr::CallSpread { + callee: Box::new(allocating("cb")), + args: vec![ + CallArg::Spread(allocating("a")), + CallArg::Spread(allocating("b")), + ], + type_args: Vec::new(), + })], + ); + assert_every_fold_rereads_the_accumulator(&ir, "multi-spread bundle"); +} + +/// ★ The operands are INERT — `[1, 2]` and `3` cannot run user code — and the +/// accumulator still has to be rooted, because `js_array_like_to_array` itself +/// allocates. +/// +/// This is the arm that pins the `protect` predicate. An operands-only "can +/// anything here collect?" test answers `false` for this program, which is the +/// answer `rooting::any_operand_may_collect` gives on its own; the bundle +/// disjoins the spread-present test precisely so this case still roots. +#[test] +fn an_all_inert_spread_bundle_still_roots_its_accumulator() { + let ir = compile_body( + "inert_spread", + vec![Stmt::Expr(Expr::CallSpread { + callee: Box::new(allocating("cb")), + args: vec![ + CallArg::Spread(Expr::Array(vec![Expr::Number(1.0), Expr::Number(2.0)])), + CallArg::Expr(Expr::Number(3.0)), + CallArg::Spread(Expr::Array(vec![Expr::Number(4.0)])), + ], + type_args: Vec::new(), + })], + ); + assert_every_fold_rereads_the_accumulator(&ir, "all-inert spread bundle"); +} diff --git a/crates/perry-codegen/src/expr/mod.rs b/crates/perry-codegen/src/expr/mod.rs index 7cb7d8916a..4a64446e5f 100644 --- a/crates/perry-codegen/src/expr/mod.rs +++ b/crates/perry-codegen/src/expr/mod.rs @@ -139,6 +139,8 @@ pub(crate) mod shadow_inline; // `pub(crate)` since #7615 slice 8: `rooting/temp_root.rs` binds a pooled // temp alloca through the same shadow-slot emission every named local uses, // and it now lives outside `crate::expr`. +#[cfg(test)] +mod call_spread_rooting_tests; pub(crate) mod shadow_slot; #[cfg(test)] mod slice7_rooting_tests;