diff --git a/.github/workflows/gc-root-dominance.yml b/.github/workflows/gc-root-dominance.yml index d58cde4df8..fe3279a753 100644 --- a/.github/workflows/gc-root-dominance.yml +++ b/.github/workflows/gc-root-dominance.yml @@ -517,49 +517,28 @@ jobs: # 21 at #7663. #7664 fixed 14 of them in `root_reload.rs` and # `lower_call/new.rs` — the whole `strhandle` population (10), the # `js_new_target_get` save/restore (1), and 3 of the 7 unmasked - # receivers — so the budget is 7. What remains, and why each is its - # own slice rather than a widening of the same fix: + # receivers — so the budget is 7. #7616 (widening POLL_CAPABLE_RUNTIME) + # and #7690 (rooting the spread argument-bundle accumulator) moved it + # 7 -> 11 -> 8, tracked in the two changelog fragments for those PRs. # - # 4 unmasked, all PHI-MEDIATED. The stale value reaches its use - # through a `phi`, and `root_reload` cannot insert above a phi; - # the reload has to go in the PREDECESSOR, on the edge, which is - # a different insertion model. - # 2 global (`@perry_global_*`). NOT reloadable: a module-level - # variable is one the program assigns, so a re-read can observe - # a later assignment instead of the value the call was given - # (`operand_needs_root`). That population needs ROOTING, and - # `a_module_global_is_not_a_reload_source` in root_reload.rs - # pins the distinction so it cannot be widened away by accident. - # 1 capture, a `js_closure_get_capture_bits` read held across - # `js_number_coerce`. + # ★ RE-VERIFIED AT #7664's SECOND PASS: the 8 above was already stale + # by the time this ran. A fresh corpus read 9, not 8 — a static-method + # receiver hazard in `test_gap_static_method_value_name_collision` + # (added after the "8" snapshot, by the #7689/#7691 fix that armed a + # receiver-sensitive static `this`) had joined the population without + # anyone lowering — er, RAISING — the budget to match. That in itself + # is the thing CLAUDE.md's hazard-4 corollary warns about: a budget + # nobody re-measures silently absorbs the next hazard. # - # 11 at #7616, and the four new ones are the POINT of that change - # rather than a regression: widening POLL_CAPABLE_RUNTIME by the 77 - # symbols `--audit-poll-reach` found makes windows MOVING that the - # filter previously dropped. Measured on this corpus, 7 -> 11, with - # no hit disappearing: - # - # 3 test_gap_array_splice_spread::main, `unrooted:alloc`. A fresh - # array held in a raw i64 across `js_array_like_to_array` and - # consumed by `js_array_concat` -- #7453's shape exactly, in the - # spread lowering instead of the URL one. - # 1 test_gap_class_expr_dynamic_parent_ctor, `unrooted:capture`. - # The same `js_closure_get_capture_bits` population as the - # 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`: + # Of the 9: 4 were CHECKER FALSE POSITIVES, not real hits. `chain` + # (the untracked cast-closure a stale use is searched in) treated + # `phi` as unconditionally transparent — one tainted incoming edge + # blanket-tainted the phi's RESULT, and a downstream use of that + # result was checked against ANY CFG path from source to use + # (`between_blocks` is deliberately path-insensitive, which is sound + # for an ordinary register but not a phi, whose dynamic value depends + # on which edge was actually taken). All four were the identical + # shape, e.g. `readCtx`: # # entry.0: %r2 = # br i1 %r4, label %then, label %merge @@ -569,32 +548,76 @@ jobs: # # 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 `&&`). + # definition and the join — the checker was reporting the OTHER + # edge's safepoint against THIS edge's value. Verified register-by- + # register on all four (`readCtx`, `__closure_5`, `Readable`, + # `__obj_method_toLocaleString_3` — every one a `logical.merge` join + # of an `&&`). + # + # Fixed: `_cast_closure` gained `phi_all_edges` — a phi joins `chain` + # only once EVERY incoming edge is independently in it, closing the + # false positive. That deliberately gives up the case of a SINGLE + # tainted edge with its own intervening safepoint before its own + # predecessor's terminator; `_phi_edge_hazard` covers that separately, + # checking each edge's window on its own. Two sabotage-tested + # self-test fixtures (`phi_safe_edge` / `phi_hazard_edge`) pin both + # directions. + # + # The remaining 5 were real, and split two ways: # - # So the residual population is 4, not 8, and it splits: + # 3 unrooted:global. `test_gap_arraybuffer_transfer::main` (2, + # `new DataView`/`new Uint8Array` reading a module-global + # receiver, then holding it across a sibling `{ valueOf() {...} }` + # argument's allocation) — fixed upstream by #7719, which covers + # the identical shape across all 30 `lower_call/builtin.rs` ctor + # arms via a shared `RootedGroup`. `test_gap_static_method_value_ + # name_collision::main` (1, `(Lexer as any).lex(...)`'s receiver + # held across arg-bundling that always allocates for a rest + # param) — fixed here, in + # `lower_call/property_get/static_dispatch.rs`, the same + # `RootedGroup::adopt`/`reread` shape on the receiver instead of + # a constructor argument. + # 2 unrooted:capture. `js_closure_get_capture_bits` returns a raw + # `i64` that may be a NaN-boxed heap value, and unlike + # `%this_closure` itself (which codegen already re-enters into + # the `ptr addrspace(1)` tracked domain), its generic "read a + # captured value" call sites never re-enter it into either that + # domain or a temp root. + # `test_gap_class_expr_dynamic_parent_ctor::__closure_21`: a + # captured dynamic-parent-class reference read at the top of the + # synthesized implicit ctor closure, used ~60 lines later as + # `js_new_function_construct`'s callee, across + # `js_object_alloc_class_inline_keys` AND the class's own user + # constructor. + # `test_gap_computed_key_method_nested_this::__closure_9`: a + # captured numeric local read, then `total + x`-style `fadd` + # after `js_number_coerce` (which can run a user + # `Symbol.toPrimitive`) intervenes. NOT reloadable the way a + # strhandle global is (re-deriving from `%this_closure` reads + # the pre-move closure — RS4GC does not relocate a raw `i64` + # parameter) — but IS reloadable the way OTHER root-derived + # values are: re-calling `js_closure_get_capture_bits` with the + # same closure pointer and index re-reads the same slot, sound + # under the identical store side-condition + # (`js_closure_set_capture_bits` to that index) `root_reload.rs` + # already tracks for shadow slots and handle globals. That needs + # `root_reload.rs`'s `Facts` to model a CALL as a reloadable + # source, not just a load — a real slice of work, tracked in its + # own issue rather than rushed into this one. # - # 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. + # `--max-unrooted` is 2 now, exactly the two `unrooted:capture` hits + # above — measured on the native corpus after the checker fix and the + # static-dispatch fix, both arms of `--moving-only`, `stale` still 0. + # (The corpus build that produced that reading predates rebasing this + # branch onto #7719; #7719 fixes the identical `unrooted:global` shape + # via the same `RootedGroup` mechanism across a superset of + # `lower_call/builtin.rs`'s arms, so the reading is not expected to + # change, but the exact post-rebase commit has not itself been run + # through the corpus — this job's own execution on the PR is that + # confirmation.) # - # #7664 stays open as this budget's referent: a number with nothing - # 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. + # #7725 is this budget's referent, tracking the `unrooted:capture` + # follow-up (split out of #7664, which #7724 otherwise closed). # # `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 @@ -614,7 +637,7 @@ jobs: --min-statepoints 15000 \ --min-live-bundles 8000 \ --min-relocates 20000 \ - --max-unrooted 8 \ + --max-unrooted 2 \ --max-stale 0 \ --allowlist scripts/gc_root_dominance_allowlist.json \ --seeded-violations 40 \ diff --git a/CLAUDE.md b/CLAUDE.md index 1e4a6ca565..f5c50cda45 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.1419 +**Current Version:** 0.5.1420 ## TypeScript Parity Status diff --git a/Cargo.lock b/Cargo.lock index 9275ca78d0..eb8203a8e9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5547,7 +5547,7 @@ checksum = "9b4f627cb1b25917193a259e49bdad08f671f8d9708acfd5fe0a8c1455d87220" [[package]] name = "perry" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "anyhow", "base64", @@ -5607,14 +5607,14 @@ dependencies = [ [[package]] name = "perry-api-manifest" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "serde", ] [[package]] name = "perry-audio-miniaudio" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "cc", "libc", @@ -5622,7 +5622,7 @@ dependencies = [ [[package]] name = "perry-codegen" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "anyhow", "inkwell", @@ -5639,7 +5639,7 @@ dependencies = [ [[package]] name = "perry-codegen-arkts" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "anyhow", "perry-hir", @@ -5647,7 +5647,7 @@ dependencies = [ [[package]] name = "perry-codegen-glance" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "anyhow", "perry-hir", @@ -5655,7 +5655,7 @@ dependencies = [ [[package]] name = "perry-codegen-js" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "anyhow", "perry-dispatch", @@ -5664,7 +5664,7 @@ dependencies = [ [[package]] name = "perry-codegen-swiftui" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "anyhow", "perry-hir", @@ -5672,7 +5672,7 @@ dependencies = [ [[package]] name = "perry-codegen-wasm" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "anyhow", "base64", @@ -5684,7 +5684,7 @@ dependencies = [ [[package]] name = "perry-codegen-wear-tiles" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "anyhow", "perry-hir", @@ -5692,7 +5692,7 @@ dependencies = [ [[package]] name = "perry-container-compose" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "anyhow", "async-trait", @@ -5721,14 +5721,14 @@ dependencies = [ [[package]] name = "perry-container-e2e" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "anyhow", ] [[package]] name = "perry-diagnostics" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "serde", "serde_json", @@ -5736,7 +5736,7 @@ dependencies = [ [[package]] name = "perry-dispatch" -version = "0.5.1419" +version = "0.5.1420" [[package]] name = "perry-doc-fixture-my-bindings" @@ -5747,7 +5747,7 @@ dependencies = [ [[package]] name = "perry-doc-tests" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "anyhow", "clap", @@ -5762,7 +5762,7 @@ dependencies = [ [[package]] name = "perry-ext-ads" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "block2", "objc2", @@ -5772,7 +5772,7 @@ dependencies = [ [[package]] name = "perry-ext-argon2" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "argon2", "perry-ffi", @@ -5780,7 +5780,7 @@ dependencies = [ [[package]] name = "perry-ext-axios" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-ffi", "reqwest", @@ -5789,7 +5789,7 @@ dependencies = [ [[package]] name = "perry-ext-bcrypt" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "bcrypt", "perry-ffi", @@ -5797,7 +5797,7 @@ dependencies = [ [[package]] name = "perry-ext-better-sqlite3" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-ffi", "rusqlite", @@ -5805,7 +5805,7 @@ dependencies = [ [[package]] name = "perry-ext-cheerio" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-ffi", "scraper", @@ -5813,7 +5813,7 @@ dependencies = [ [[package]] name = "perry-ext-commander" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-ffi", "perry-runtime", @@ -5821,7 +5821,7 @@ dependencies = [ [[package]] name = "perry-ext-cron" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "chrono", "cron", @@ -5831,7 +5831,7 @@ dependencies = [ [[package]] name = "perry-ext-dayjs" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "chrono", "perry-ffi", @@ -5839,7 +5839,7 @@ dependencies = [ [[package]] name = "perry-ext-decimal" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-ffi", "rust_decimal", @@ -5847,7 +5847,7 @@ dependencies = [ [[package]] name = "perry-ext-dotenv" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-ffi", "serde_json", @@ -5855,7 +5855,7 @@ dependencies = [ [[package]] name = "perry-ext-ethers" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-ffi", "rand 0.10.1", @@ -5863,7 +5863,7 @@ dependencies = [ [[package]] name = "perry-ext-events" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-ffi", "perry-runtime", @@ -5871,14 +5871,14 @@ dependencies = [ [[package]] name = "perry-ext-exponential-backoff" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-ffi", ] [[package]] name = "perry-ext-fastify" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "bytes", "http-body-util", @@ -5896,7 +5896,7 @@ dependencies = [ [[package]] name = "perry-ext-fetch" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "bytes", "lazy_static", @@ -5909,7 +5909,7 @@ dependencies = [ [[package]] name = "perry-ext-http" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "bytes", "h2", @@ -5933,7 +5933,7 @@ dependencies = [ [[package]] name = "perry-ext-ioredis" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "lazy_static", "perry-ffi", @@ -5943,7 +5943,7 @@ dependencies = [ [[package]] name = "perry-ext-jsonwebtoken" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "base64", "jsonwebtoken", @@ -5954,7 +5954,7 @@ dependencies = [ [[package]] name = "perry-ext-lru-cache" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "lru", "perry-ffi", @@ -5963,7 +5963,7 @@ dependencies = [ [[package]] name = "perry-ext-moment" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "chrono", "perry-ffi", @@ -5971,7 +5971,7 @@ dependencies = [ [[package]] name = "perry-ext-mongodb" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "bson", "futures-util", @@ -5983,7 +5983,7 @@ dependencies = [ [[package]] name = "perry-ext-mysql2" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "chrono", "perry-ffi", @@ -5993,7 +5993,7 @@ dependencies = [ [[package]] name = "perry-ext-nanoid" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "nanoid", "perry-ffi", @@ -6002,7 +6002,7 @@ dependencies = [ [[package]] name = "perry-ext-net" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "bytes", "perry-ffi", @@ -6015,7 +6015,7 @@ dependencies = [ [[package]] name = "perry-ext-node-forge" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "const-oid 0.9.6", "der 0.7.10", @@ -6034,7 +6034,7 @@ dependencies = [ [[package]] name = "perry-ext-nodemailer" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "lettre", "perry-ffi", @@ -6044,7 +6044,7 @@ dependencies = [ [[package]] name = "perry-ext-pdf" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-ffi", "printpdf", @@ -6052,7 +6052,7 @@ dependencies = [ [[package]] name = "perry-ext-pg" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-ffi", "sqlx", @@ -6061,7 +6061,7 @@ dependencies = [ [[package]] name = "perry-ext-ratelimit" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "governor", "perry-ffi", @@ -6069,7 +6069,7 @@ dependencies = [ [[package]] name = "perry-ext-sharp" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "fast_image_resize", "image", @@ -6079,14 +6079,14 @@ dependencies = [ [[package]] name = "perry-ext-slugify" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-ffi", ] [[package]] name = "perry-ext-streams" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "lazy_static", "perry-ffi", @@ -6095,7 +6095,7 @@ dependencies = [ [[package]] name = "perry-ext-undici" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-ffi", "perry-runtime", @@ -6104,7 +6104,7 @@ dependencies = [ [[package]] name = "perry-ext-uuid" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-ffi", "uuid", @@ -6112,7 +6112,7 @@ dependencies = [ [[package]] name = "perry-ext-validator" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-ffi", "regex", @@ -6122,7 +6122,7 @@ dependencies = [ [[package]] name = "perry-ext-ws" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "futures-util", "lazy_static", @@ -6135,7 +6135,7 @@ dependencies = [ [[package]] name = "perry-ext-zlib" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "brotli", "flate2", @@ -6145,7 +6145,7 @@ dependencies = [ [[package]] name = "perry-ffi" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "dashmap", "once_cell", @@ -6154,7 +6154,7 @@ dependencies = [ [[package]] name = "perry-hir" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "anyhow", "perry-api-manifest", @@ -6172,7 +6172,7 @@ dependencies = [ [[package]] name = "perry-parser" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "anyhow", "perry-diagnostics", @@ -6184,7 +6184,7 @@ dependencies = [ [[package]] name = "perry-runtime" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "anyhow", "base64", @@ -6226,14 +6226,14 @@ dependencies = [ [[package]] name = "perry-runtime-static" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-runtime", ] [[package]] name = "perry-stdlib" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "aes 0.8.4", "aes 0.9.1", @@ -6328,14 +6328,14 @@ dependencies = [ [[package]] name = "perry-stdlib-static" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-stdlib", ] [[package]] name = "perry-transform" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "anyhow", "perry-hir", @@ -6344,14 +6344,14 @@ dependencies = [ [[package]] name = "perry-ui" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-ui-model", ] [[package]] name = "perry-ui-android" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "base64", "itoa", @@ -6368,7 +6368,7 @@ dependencies = [ [[package]] name = "perry-ui-geisterhand" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "rand 0.10.1", "serde", @@ -6378,7 +6378,7 @@ dependencies = [ [[package]] name = "perry-ui-gtk4" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "base64", "cairo-rs 0.22.0", @@ -6401,7 +6401,7 @@ dependencies = [ [[package]] name = "perry-ui-ios" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "base64", "block2", @@ -6417,7 +6417,7 @@ dependencies = [ [[package]] name = "perry-ui-macos" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "base64", "block2", @@ -6432,7 +6432,7 @@ dependencies = [ [[package]] name = "perry-ui-model" -version = "0.5.1419" +version = "0.5.1420" [[package]] name = "perry-ui-test" @@ -6443,11 +6443,11 @@ dependencies = [ [[package]] name = "perry-ui-testkit" -version = "0.5.1419" +version = "0.5.1420" [[package]] name = "perry-ui-tvos" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "base64", "block2", @@ -6463,7 +6463,7 @@ dependencies = [ [[package]] name = "perry-ui-visionos" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "base64", "block2", @@ -6479,7 +6479,7 @@ dependencies = [ [[package]] name = "perry-ui-watchos" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "block2", "libc", @@ -6492,7 +6492,7 @@ dependencies = [ [[package]] name = "perry-ui-windows" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "base64", "libc", @@ -6509,14 +6509,14 @@ dependencies = [ [[package]] name = "perry-ui-windows-winui" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "perry-ui-windows", ] [[package]] name = "perry-updater" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "anyhow", "base64", @@ -6532,7 +6532,7 @@ dependencies = [ [[package]] name = "perry-wasm-host" -version = "0.5.1419" +version = "0.5.1420" dependencies = [ "wasmi", ] diff --git a/Cargo.toml b/Cargo.toml index 99c01e1c13..fdff82de8d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -315,7 +315,7 @@ codegen-units = 16 codegen-units = 16 [workspace.package] -version = "0.5.1419" +version = "0.5.1420" edition = "2021" license = "MIT" repository = "https://github.com/PerryTS/perry" diff --git a/changelog.d/7724-native-unrooted-residue.md b/changelog.d/7724-native-unrooted-residue.md new file mode 100644 index 0000000000..fa3f4b8379 --- /dev/null +++ b/changelog.d/7724-native-unrooted-residue.md @@ -0,0 +1,86 @@ +### gc-root-dominance-statepoints: the phi false positives are gone, and the real residual needed one more hit than the last snapshot said + +`--max-unrooted` goes **8 → 2**. #7664 asked for two things: fix the +checker's four phi-mediated `unmasked` false positives, and fix whatever real +hits remained underneath them. Re-verifying found a fifth real hit the prior +triage comment didn't have — `test_gap_static_method_value_name_collision`, +whose receiver-arming code landed with #7691 after that triage ran — so the +honest floor going into this PR was **9 hits total**, not the 8 the budget +still said: 4 phi false positives, 3 `unrooted:global`, 2 `unrooted:capture`. + +#### The checker: a phi operand's window is its own edge, not the join + +`scripts/gc_root_dominance_check.py`'s `chain` (the untracked cast-closure a +stale use is searched in) treated `phi` as unconditionally transparent: one +tainted incoming edge blanket-tainted the phi's *result*, and any downstream +use of that result was then checked for a safepoint on *any* CFG path between +the source and the use — `between_blocks` is deliberately path-insensitive, +which is sound for ordinary registers but not for a phi, where the dynamic +value at the join depends on which edge was actually taken. All four false +positives were the same `&&`/`||` short-circuit join: the tainted operand's +own edge never crosses a safepoint; the *other* edge does, and the checker +was reporting that. + +Fix: `_cast_closure` gained `phi_all_edges` — a phi only joins `chain` once +*every* incoming edge is independently in it (the worklist retries a +partially-satisfied phi from each operand's own arrival, so admission order +doesn't matter). That closes the false positive, and deliberately gives up +catching a hazard on a *single* tainted edge with its own intervening +safepoint — `_phi_edge_hazard` covers that case separately, checking each +edge's window against its own predecessor's terminator instead of the join. +Two new self-test fixtures pin both directions: `phi_safe_edge` (the false +positive shape, must report 0) and `phi_hazard_edge` — byte-identical except +the safepoint moves onto the tainted edge — which must report exactly 1. Each +was verified against a sabotaged copy of the checker to confirm it can still +fail (disabling `_phi_edge_hazard` fails `phi_hazard_edge`; reverting +`phi_all_edges` fails `phi_safe_edge`). + +#### Fixed: 3 `unrooted:global` + +`(Lexer as any).lex(...)` (marked's `Lexer.lex`/`Parser.parse` static-value +collision shape, `lower_call/property_get/static_dispatch.rs`) computed its +receiver once, then held it raw across arg-bundling logic that always +allocates when the resolved method has a synthesized rest param, and can +allocate for arbitrary argument expressions otherwise — +`implicit_this_save`/`js_static_this_arm_value` then read the stale copy. +Wrapped the receiver in `RootedGroup::adopt`/`reread` (the "root a value the +caller already computed" combinator), with `collects` derived from the same +predicate `operand_protection` uses elsewhere rather than hardcoded true — a +plain zero/literal-arg static call still emits no rooting traffic. + +The other two (`new DataView`/`new Uint8Array` reading a module-global +receiver, then holding it across a sibling `{ valueOf() {...} }` argument's +allocation, in `test_gap_arraybuffer_transfer`) turned out to already be +fixed on `main` by the time this branch rebased: #7719 landed the identical +rooting shape across all 30 of `lower_call/builtin.rs`'s constructor arms via +a shared `RootedGroup`, a superset of what this PR would otherwise have +needed to add there. + +#### Open: 2 `unrooted:capture` — `js_closure_get_capture_bits`'s result is never re-entered into either protected domain + +Diagnosed further than the prior triage's "signature/ABI change" note, and +it's narrower than that reads: `js_closure_get_capture_bits` returns a raw +`i64` that may be a NaN-boxed heap value. When the caller re-enters it into +the RS4GC-tracked `ptr addrspace(1)` domain (the closure-pointer masking +idiom `codegen/closure.rs` already uses for `%this_closure` itself) or into a +temp root, it's fine. The generic "read a captured value for arithmetic / +forwarding" call sites (`expr/mod.rs`, `literals_vars.rs`, `array_push.rs`, +and the `js_new_function_construct` callee case in the synthesized +dynamic-parent implicit constructor) do neither — the value is used as a bare +`double`/`i64` and stays that way across whatever the rest of the function +does. `test_gap_class_expr_dynamic_parent_ctor::__closure_21` and +`test_gap_computed_key_method_nested_this::__closure_9` are both this shape, +confirmed register-by-register against the corpus IR. + +The natural fix mirrors the string-handle precedent already in +`root_reload.rs`: treat a `js_closure_get_capture_bits` call as a +*reloadable* source (re-emit the same call rather than a load — the same +closure's same capture index, provably unmodified as long as no +`js_closure_set_capture_bits` to that index intervenes, the same store +side-condition the existing pass already tracks for shadow slots and handle +globals). That's a real engineering slice — `root_reload.rs`'s `Facts` +currently only models *loads* as reloadable sources, not calls — not a quick +follow-up, so it stays open. `--max-unrooted 2` is these two hits exactly. + +`gc-root-dominance` (shadow-stack) is unaffected — none of this touches the +shadow lowering's own corpus or `root_reload.rs`'s existing reload rule. diff --git a/crates/perry-codegen/src/lower_call/property_get/static_dispatch.rs b/crates/perry-codegen/src/lower_call/property_get/static_dispatch.rs index 7d200bbb73..c05fac8970 100644 --- a/crates/perry-codegen/src/lower_call/property_get/static_dispatch.rs +++ b/crates/perry-codegen/src/lower_call/property_get/static_dispatch.rs @@ -74,174 +74,211 @@ pub(crate) fn try_lower_static_dispatch( .and_then(|cc| cc.extends_name.clone()); } if let Some((fn_name, _is_static, declared, has_rest, is_synth_args)) = resolved { - // Receiver-box selection (`this` inside the static body): - // - `ClassRef`: `lower_expr` already yields the - // INT32-NaN-boxed class id; `this === ClassRef`. - // - `Call` (factory return): `lower_expr` returns the - // dynamic class produced by the factory, so each - // `Literal(value)` / `make(ast)` call carries - // unique static fields (`static literals = […]`, - // `static ast = …`). The static body reads those - // through `this.`, so passing the synthesized - // ClassRef would lose the per-call data — use the - // actual lowered call result instead. - // - Everything else (`LocalGet` after a - // `const Cls = make()` collapse, etc.): synthesize - // a fresh ClassRef NaN-box. The static body's - // `this.` then dispatches through the - // ClassRef's class-keys + class-field side-table, - // which is the post-#912 (gap 2) shape. - let recv_box = match object { - Expr::ClassRef(_) => lower_expr(ctx, object)?, - Expr::Call { .. } => lower_expr(ctx, object)?, - Expr::Sequence(_) => lower_expr(ctx, object)?, - // #1787: a class-expression value is a real heap class - // object whose per-evaluation static fields are OWN - // properties. Use the actual lowered object as `this` (NOT a - // synthesized ClassRef) so `this.ast` inside the static body - // reads this evaluation's own field rather than the shared - // template's static-field global. - Expr::ClassExprFresh { .. } => lower_expr(ctx, object)?, - // #1787: `const C = make(...); C.staticMethod()`. The local - // holds the class-expression's heap object (or, for a - // top-level-class alias like `const F = Foo`, the same - // INT32 ClassRef the synthesized fallback would produce). - // Loading the actual stored value preserves the - // per-evaluation own static fields a synthesized ClassRef - // would discard, and is value-identical for the ClassRef - // case — so `this.` resolves correctly either way. - Expr::LocalGet(_) => lower_expr(ctx, object)?, - _ => { - // Synthesize a ClassRef NaN-box from the resolved class. - let cid = ctx.class_ids.get(&cls_name).copied().unwrap_or(0); - let bits = crate::nanbox::INT32_TAG | (cid as u64 & 0xFFFF_FFFF); - crate::nanbox::double_literal(f64::from_bits(bits)) - } - }; - // Refs #915 (gap 3 / #321 follow-up): Effect's `class - // SchemaClass { static pipe() { ... arguments ... } }` - // factory returns an anon class whose `pipe` reads - // `arguments.length` to dispatch. The HIR appends a - // synthesized `arguments` rest param (#677 / #899). The - // direct-call dispatch here previously forwarded the - // call args 1:1 to the function whose only declared - // parameter is the rest array — so for - // `Cls.pipe(f1, f2)` the function got `arg0 = f1` (then - // read .length = "function" → undefined). Mirror the - // arg-bundling logic from the regular Call lowering - // (lines ~720–765) so the rest slot receives a real - // array of all call args, matching JS `arguments` - // semantics. The non-synthetic rest path (e.g. - // `static foo(a, ...rest)`) follows the same shape: - // pass the first `declared-1` positional args as-is, - // then bundle the trailing args into an Array. - let mut lowered: Vec = Vec::with_capacity(args.len()); - if has_rest && is_synth_args { - // Lower each call arg exactly ONCE (a value may have side - // effects), then reuse the SSA registers both for the leading - // real params and for the synthesized `arguments` object. - let mut vals: Vec = Vec::with_capacity(args.len()); - for a in args { - vals.push(lower_expr(ctx, a)?); - } - // #5703: the leading real params BEFORE the synth `arguments` - // slot (`static method(x, _ = 0) { … arguments … }` → - // params `[x, _, ]`) must receive their positional - // values, padded with `undefined` when under-supplied — exactly - // as the class-DECLARATION (StaticMethodCall) path does. - // Previously this branch pushed ONLY the arguments object, so a - // leading param like `x` received the (empty) arguments array - // instead of its argument / `undefined` (test262 - // `params-dflt-meth-static-args-unmapped`). - let undef = double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); - let fixed_count = declared.saturating_sub(1); - for i in 0..fixed_count { - lowered.push(vals.get(i).cloned().unwrap_or_else(|| undef.clone())); - } - // The synthesized `arguments` object holds ALL passed args. - let cap = (vals.len() as u32).to_string(); - let mut current = ctx.block().call(I64, "js_array_alloc", &[(I32, &cap)]); - for v in &vals { - let blk = ctx.block(); - current = blk.call(I64, "js_array_push_f64", &[(I64, ¤t), (DOUBLE, v)]); - } - current = - ctx.block() - .call(I64, "js_array_mark_arguments_object", &[(I64, ¤t)]); - let arguments_box = nanbox_pointer_inline(ctx.block(), ¤t); - lowered.push(arguments_box); - } else if has_rest { - let fixed_count = declared.saturating_sub(1); - for a in args.iter().take(fixed_count) { - lowered.push(lower_expr(ctx, a)?); - } - // #5703 (mirrors #235 in the StaticMethodCall path): when the - // caller under-supplies the fixed leading params, pad the - // missing slots with `undefined` BEFORE the rest array, so the - // callee's default-param prologue / destructuring fires instead - // of reading an uninitialized (0.0) parameter register. - let undef = double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); - while lowered.len() < fixed_count { - lowered.push(undef.clone()); - } - let rest_count = args.len().saturating_sub(fixed_count); - let cap = (rest_count as u32).to_string(); - let mut current = ctx.block().call(I64, "js_array_alloc", &[(I32, &cap)]); - for a in args.iter().skip(fixed_count) { - let v = lower_expr(ctx, a)?; - let blk = ctx.block(); - current = blk.call(I64, "js_array_push_f64", &[(I64, ¤t), (DOUBLE, &v)]); - } - let rest_box = nanbox_pointer_inline(ctx.block(), ¤t); - lowered.push(rest_box); - } else { - for a in args { - lowered.push(lower_expr(ctx, a)?); - } - // #5703: a static method of a class EXPRESSION called with fewer - // args than declared (`C.m()` for `static m(a = 1)` or - // `static m([x, y] = […])`) reaches this fused get-static-method - // +call path rather than the `StaticMethodCall` path used by - // class DECLARATIONS. That path pads missing slots with - // `undefined` (#235); this one did not, so the callee read an - // uninitialized (0.0) register — its default-param prologue - // (`if (p === undefined) p = …`) and array destructuring - // (`GetIterator(p)` → "is not iterable") never fired. Pad here - // too so both paths behave identically. - let undef = double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); - while lowered.len() < declared { - lowered.push(undef.clone()); + // #7664: `recv_box` (below) is frequently a module global — + // `(Lexer as any).lex(...)` reads the `Lexer` class value straight + // out of `@perry_global_*` once any closure in the module also + // reads it, exactly as marked's Lexer/Parser collision test does — + // and the runtime REWRITES that global's storage on an evacuating + // cycle without touching a register already loaded from it. Every + // arg-bundling arm below can allocate (an object-literal arg with + // a `valueOf` method allocates the object AND a closure for the + // method; the has-rest arms unconditionally allocate the + // synthesized array), so a naive `let recv_box = …; /* lower args + // */; use(recv_box)` holds the pre-collection copy across all of + // it — measured: `new DataView`/`Uint8Array`'s sibling-operand + // bug (`lower_call/builtin.rs`), here on the receiver instead of + // a constructor argument. `RootedGroup::adopt` protects an + // already-computed value the same way `with_operands_rooted` + // protects a lowered one — see its doc for why `object` being + // neither reloadable nor always literally `lower_expr(object)` + // (the `_` arm below synthesizes instead) is fine: the + // precondition is "not reloadable", not "value came from + // `lower_expr`". + return crate::rooting::with_rooted_group(ctx, 1, |ctx, group| { + // Receiver-box selection (`this` inside the static body): + // - `ClassRef`: `lower_expr` already yields the + // INT32-NaN-boxed class id; `this === ClassRef`. + // - `Call` (factory return): `lower_expr` returns the + // dynamic class produced by the factory, so each + // `Literal(value)` / `make(ast)` call carries + // unique static fields (`static literals = […]`, + // `static ast = …`). The static body reads those + // through `this.`, so passing the synthesized + // ClassRef would lose the per-call data — use the + // actual lowered call result instead. + // - Everything else (`LocalGet` after a + // `const Cls = make()` collapse, etc.): synthesize + // a fresh ClassRef NaN-box. The static body's + // `this.` then dispatches through the + // ClassRef's class-keys + class-field side-table, + // which is the post-#912 (gap 2) shape. + let recv_box = match object { + Expr::ClassRef(_) => lower_expr(ctx, object)?, + Expr::Call { .. } => lower_expr(ctx, object)?, + Expr::Sequence(_) => lower_expr(ctx, object)?, + // #1787: a class-expression value is a real heap class + // object whose per-evaluation static fields are OWN + // properties. Use the actual lowered object as `this` (NOT a + // synthesized ClassRef) so `this.ast` inside the static body + // reads this evaluation's own field rather than the shared + // template's static-field global. + Expr::ClassExprFresh { .. } => lower_expr(ctx, object)?, + // #1787: `const C = make(...); C.staticMethod()`. The local + // holds the class-expression's heap object (or, for a + // top-level-class alias like `const F = Foo`, the same + // INT32 ClassRef the synthesized fallback would produce). + // Loading the actual stored value preserves the + // per-evaluation own static fields a synthesized ClassRef + // would discard, and is value-identical for the ClassRef + // case — so `this.` resolves correctly either way. + Expr::LocalGet(_) => lower_expr(ctx, object)?, + _ => { + // Synthesize a ClassRef NaN-box from the resolved class. + let cid = ctx.class_ids.get(&cls_name).copied().unwrap_or(0); + let bits = crate::nanbox::INT32_TAG | (cid as u64 & 0xFFFF_FFFF); + crate::nanbox::double_literal(f64::from_bits(bits)) + } + }; + // `has_rest` unconditionally allocates the synthesized array + // below regardless of what `args` themselves do; otherwise + // defer to the real predicate so a plain zero/literal-arg + // static call (the common case) still emits no rooting + // traffic at all, matching its pre-#7664 IR exactly. + let collects = + has_rest || crate::rooting::any_operand_may_collect(ctx, args.iter()); + let recv_idx = group.adopt(ctx, object, &recv_box, collects); + + // Refs #915 (gap 3 / #321 follow-up): Effect's `class + // SchemaClass { static pipe() { ... arguments ... } }` + // factory returns an anon class whose `pipe` reads + // `arguments.length` to dispatch. The HIR appends a + // synthesized `arguments` rest param (#677 / #899). The + // direct-call dispatch here previously forwarded the + // call args 1:1 to the function whose only declared + // parameter is the rest array — so for + // `Cls.pipe(f1, f2)` the function got `arg0 = f1` (then + // read .length = "function" → undefined). Mirror the + // arg-bundling logic from the regular Call lowering + // (lines ~720–765) so the rest slot receives a real + // array of all call args, matching JS `arguments` + // semantics. The non-synthetic rest path (e.g. + // `static foo(a, ...rest)`) follows the same shape: + // pass the first `declared-1` positional args as-is, + // then bundle the trailing args into an Array. + let mut lowered: Vec = Vec::with_capacity(args.len()); + if has_rest && is_synth_args { + // Lower each call arg exactly ONCE (a value may have side + // effects), then reuse the SSA registers both for the leading + // real params and for the synthesized `arguments` object. + let mut vals: Vec = Vec::with_capacity(args.len()); + for a in args { + vals.push(lower_expr(ctx, a)?); + } + // #5703: the leading real params BEFORE the synth `arguments` + // slot (`static method(x, _ = 0) { … arguments … }` → + // params `[x, _, ]`) must receive their positional + // values, padded with `undefined` when under-supplied — exactly + // as the class-DECLARATION (StaticMethodCall) path does. + // Previously this branch pushed ONLY the arguments object, so a + // leading param like `x` received the (empty) arguments array + // instead of its argument / `undefined` (test262 + // `params-dflt-meth-static-args-unmapped`). + let undef = double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); + let fixed_count = declared.saturating_sub(1); + for i in 0..fixed_count { + lowered.push(vals.get(i).cloned().unwrap_or_else(|| undef.clone())); + } + // The synthesized `arguments` object holds ALL passed args. + let cap = (vals.len() as u32).to_string(); + let mut current = ctx.block().call(I64, "js_array_alloc", &[(I32, &cap)]); + for v in &vals { + let blk = ctx.block(); + current = + blk.call(I64, "js_array_push_f64", &[(I64, ¤t), (DOUBLE, v)]); + } + current = + ctx.block() + .call(I64, "js_array_mark_arguments_object", &[(I64, ¤t)]); + let arguments_box = nanbox_pointer_inline(ctx.block(), ¤t); + lowered.push(arguments_box); + } else if has_rest { + let fixed_count = declared.saturating_sub(1); + for a in args.iter().take(fixed_count) { + lowered.push(lower_expr(ctx, a)?); + } + // #5703 (mirrors #235 in the StaticMethodCall path): when the + // caller under-supplies the fixed leading params, pad the + // missing slots with `undefined` BEFORE the rest array, so the + // callee's default-param prologue / destructuring fires instead + // of reading an uninitialized (0.0) parameter register. + let undef = double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); + while lowered.len() < fixed_count { + lowered.push(undef.clone()); + } + let rest_count = args.len().saturating_sub(fixed_count); + let cap = (rest_count as u32).to_string(); + let mut current = ctx.block().call(I64, "js_array_alloc", &[(I32, &cap)]); + for a in args.iter().skip(fixed_count) { + let v = lower_expr(ctx, a)?; + let blk = ctx.block(); + current = + blk.call(I64, "js_array_push_f64", &[(I64, ¤t), (DOUBLE, &v)]); + } + let rest_box = nanbox_pointer_inline(ctx.block(), ¤t); + lowered.push(rest_box); + } else { + for a in args { + lowered.push(lower_expr(ctx, a)?); + } + // #5703: a static method of a class EXPRESSION called with fewer + // args than declared (`C.m()` for `static m(a = 1)` or + // `static m([x, y] = […])`) reaches this fused get-static-method + // +call path rather than the `StaticMethodCall` path used by + // class DECLARATIONS. That path pads missing slots with + // `undefined` (#235); this one did not, so the callee read an + // uninitialized (0.0) register — its default-param prologue + // (`if (p === undefined) p = …`) and array destructuring + // (`GetIterator(p)` → "is not iterable") never fired. Pad here + // too so both paths behave identically. + let undef = double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); + while lowered.len() < declared { + lowered.push(undef.clone()); + } } - } - // #7211: rooted save/restore — the displaced implicit `this` is - // live across the static method body below, which is user code. - let prev_this = crate::rooting::implicit_this_save(ctx, &recv_box); - // Receiver-sensitive static `this`: arm the one-shot override with - // the ACTUAL receiver box so the callee prologue's - // `js_static_this_resolve` binds `this` to it (spec - // OrdinaryCallBindThis). This must cover the dynamic-value receiver - // shapes too (ClassExprFresh / factory `Call` / `LocalGet`), not - // just plain class-refs: the prologue consumes the armed override - // or falls back to the LEXICAL class-ref — it never reads implicit - // `this` — so the previous implicit-this-only treatment of these - // shapes silently bound `this` to the shared template. A class - // EXPRESSION's per-evaluation statics are OWN properties of the - // fresh heap class object (never written to the template's - // static-field globals), so `this.` inside the static body - // read `undefined` (#1787 criterion 1: `make(a).viaThis()` / - // `const C = make(a); C.viaThis()`). For a local that holds a - // plain ClassRef value this arms exactly the prologue's default — - // no behavior change — and for fresh objects it restores the - // receiver, matching the runtime dispatch tower - // (`js_class_static_method_call`), which has armed its receiver - // since the static-private-brand work. - ctx.block() - .call_void("js_static_this_arm_value", &[(DOUBLE, &recv_box)]); - let arg_slices: Vec<(crate::types::LlvmType, &str)> = - lowered.iter().map(|s| (DOUBLE, s.as_str())).collect(); - let result = ctx.block().call(DOUBLE, &fn_name, &arg_slices); - crate::rooting::implicit_this_restore(ctx, prev_this); - return Ok(Some(result)); + // #7664: re-read below the arg lowering above, not the + // register captured before it — see the rationale on this + // closure's opening comment. + let recv_box = group.reread(ctx, recv_idx)?; + // #7211: rooted save/restore — the displaced implicit `this` is + // live across the static method body below, which is user code. + let prev_this = crate::rooting::implicit_this_save(ctx, &recv_box); + // Receiver-sensitive static `this`: arm the one-shot override with + // the ACTUAL receiver box so the callee prologue's + // `js_static_this_resolve` binds `this` to it (spec + // OrdinaryCallBindThis). This must cover the dynamic-value receiver + // shapes too (ClassExprFresh / factory `Call` / `LocalGet`), not + // just plain class-refs: the prologue consumes the armed override + // or falls back to the LEXICAL class-ref — it never reads implicit + // `this` — so the previous implicit-this-only treatment of these + // shapes silently bound `this` to the shared template. A class + // EXPRESSION's per-evaluation statics are OWN properties of the + // fresh heap class object (never written to the template's + // static-field globals), so `this.` inside the static body + // read `undefined` (#1787 criterion 1: `make(a).viaThis()` / + // `const C = make(a); C.viaThis()`). For a local that holds a + // plain ClassRef value this arms exactly the prologue's default — + // no behavior change — and for fresh objects it restores the + // receiver, matching the runtime dispatch tower + // (`js_class_static_method_call`), which has armed its receiver + // since the static-private-brand work. + ctx.block() + .call_void("js_static_this_arm_value", &[(DOUBLE, &recv_box)]); + let arg_slices: Vec<(crate::types::LlvmType, &str)> = + lowered.iter().map(|s| (DOUBLE, s.as_str())).collect(); + let result = ctx.block().call(DOUBLE, &fn_name, &arg_slices); + crate::rooting::implicit_this_restore(ctx, prev_this); + Ok(Some(result)) + }); } // #1787 / #321: the call target is a static FIELD holding a callable, // not a static METHOD — e.g. effect's diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index c5d8c7dc23..8afbeda67e 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -3368,6 +3368,26 @@ def _is_statepoint(ins): return ins.is_statepoint +# `rhs.starts_with("phi ")` is `root_reload.rs`'s own test (it needs the same +# fact: "nothing may be inserted above a phi"). Same split as `operand_regs`. +def _is_phi(ins): + if ins.callee is not None: + return False + body = ins.text.split(" = ", 1)[-1] if " = " in ins.text else ins.text + return body.strip().startswith("phi ") + + +_PHI_EDGE_RE = re.compile(r"\[\s*([^,\[\]]+?)\s*,\s*%([-\w.$]+)\s*\]") + + +def phi_incoming(ins): + """`[(operand_text, predecessor_block), ...]` for a `phi`'s incoming + edges, in LLVM's printed order. `operand_text` is `%reg` for a register + operand or the constant's own spelling (`0.000000e+00`, `null`, ...) — + never itself tainted, which is why callers filter on the leading `%`.""" + return _PHI_EDGE_RE.findall(ins.text) + + def transparent_use_graph(f): """`reg -> [Insn]` for transparent, result-producing users of `reg`. @@ -3388,12 +3408,39 @@ def transparent_use_graph(f): return graph -def _cast_closure(graph, seed, tracked, stop_at_tracked): +def _cast_closure(graph, seed, tracked, stop_at_tracked, phi_all_edges=False): """Forward closure of `seed` over bit-level/identity producers. With `stop_at_tracked`, a register LLVM tracks terminates the walk: it is relocated and its uses rewritten, so nothing derived from it below a safepoint is stale. + + `phi_all_edges` changes how a `phi` is admitted. Off (the default, used + for `reach`'s rooted/stale classification), a phi is transparent like any + other bit op: any tainted operand taints the result. On (used for + `chain`, the closure a use is searched in), a phi is admitted only once + EVERY incoming edge is independently in the closure. A phi with one + untainted edge does not carry the tainted value into the join on every + path, so blanket admission over-approximates in the loose direction and + reports a hazard on an edge the value never crosses — #7664's four + `unmasked` false positives, all the same `&&`/`||` short-circuit join + with one safe edge (verified by hand: the join's OTHER edge is a + safepoint-free straight line from the source to that edge's own + predecessor, so nothing is actually at risk on the path that carries it). + The worklist retries a partially-satisfied phi from each operand's own + arrival — `graph[r]` lists the phi under every one of its operand + registers — so admission order does not matter. + + This deliberately does not resolve a loop-carried self-referential phi + (an incoming edge that is the phi's own result, directly or through + another transparent op): that is a mutual dependency no forward fixpoint + from empty can break on its own, and it is why `phi_all_edges` is opt-in + rather than the new default. Measured against it: the native corpus's + `--statepoints` hit set changes by exactly the four named false + positives, nothing else — see `changelog.d/`. A single untainted edge + with its OWN intervening safepoint is still caught, just not through this + closure — see `_phi_edge_hazard`, which checks each edge's window + independently of what the others carry. """ chain = set(seed) q = deque(seed) @@ -3404,6 +3451,12 @@ def _cast_closure(graph, seed, tracked, stop_at_tracked): continue if stop_at_tracked and ins.result in tracked: continue + if phi_all_edges and _is_phi(ins): + edges = phi_incoming(ins) + if not edges or not all( + v.strip().startswith("%") and v.strip()[1:] in chain + for v, _p in edges): + continue chain.add(ins.result) q.append(ins.result) return chain @@ -3474,6 +3527,7 @@ def check_func_statepoints(module, f, want_moving_only=False, tracked = tracked_registers(f) use_graph = transparent_use_graph(f) idom = dominators(f) + phis = [ins for blk in f.blocks for ins in f.insns[blk] if _is_phi(ins)] out = [] for b in f.blocks: @@ -3496,22 +3550,26 @@ def check_func_statepoints(module, f, want_moving_only=False, # `ptr addrspace(1)`, because from there LLVM relocates it and # rewrites its uses. Anything derived from the tracked value BELOW # a safepoint is derived from the relocated one and is correct. + # `phi_all_edges=True` so a phi only joins it when every incoming + # edge is independently tainted — see `_cast_closure`. # # `reach` crosses that line in both directions and exists only to # answer "is the OBJECT in this safepoint's live bundle" — a bundle # can only ever name the `ptr addrspace(1)` register, so without # crossing, every hit would classify `unrooted` and the split would - # be decoration. + # be decoration. Left at the old blanket phi admission: a phi + # merging a rooted value with an unrooted one is still worth + # knowing is reachable from the tracked domain on SOME path. chain = _cast_closure(use_graph, {src.result}, tracked, - stop_at_tracked=True) + stop_at_tracked=True, phi_all_edges=True) reach = _cast_closure(use_graph, _back_closure(def_of, src.result), tracked, stop_at_tracked=False) as1_chain = reach & tracked + hit = None for bb in f.blocks: if not dominates(idom, src.block, bb): continue - hit = None for use in f.insns[bb]: if use.result in chain or is_transparent(use): continue @@ -3532,8 +3590,22 @@ def check_func_statepoints(module, f, want_moving_only=False, hit = v break if hit is not None: - out.append(hit) break + + # `chain`'s phi gate (above) requires EVERY edge to be tainted + # before treating a downstream use as reachable from `src` at + # all. The single-edge case that excludes on purpose — one + # tainted operand, with its own intervening safepoint before ITS + # predecessor's terminator — is still a real hazard, just not one + # the use-scan above can see once the phi itself is out of + # `chain`. Check it directly. + if hit is None: + hit = _phi_edge_hazard(f, idom, module, f.name, src, kind, + chain, as1_chain, phis, poll_reaching, + want_moving_only) + + if hit is not None: + out.append(hit) return out @@ -3558,6 +3630,53 @@ def scan(blk, lo, hi): for m_blk in between_blocks(f, A.block, B.block)) +def _phi_edge_hazard(f, idom, module, fname, src, kind, chain, as1_chain, + phis, poll_reaching, want_moving_only): + """The single-edge phi hazard `chain`'s `phi_all_edges` gate excludes on + purpose (#7664). + + A phi operand is used exactly once, on its own incoming edge — the + restated rule is that the window it is live across ends at that edge's + predecessor's TERMINATOR, not at the join and not at some downstream real + use of the merged result (a later use is reached on every OTHER edge + too, most of which never carried the tainted value at all). So each edge + is checked on its own, against its own window, independent of what the + phi's other incoming edges carry. + + `Insn("", pred_block, len(f.insns[pred_block]))` is a zero-cost sentinel + one past the predecessor's last instruction (its terminator, never + itself a statepoint) — it lets `window_hits_generic` and + `_protected_by_temp_root` do the actual window walk unmodified, the same + two functions the use-scan above calls, just aimed at the edge's end + instead of a downstream use. + """ + for phi in phis: + for val, pred_block in phi_incoming(phi): + val = val.strip() + if not val.startswith("%") or val[1:] not in chain: + continue + if not dominates(idom, src.block, pred_block): + # Not reachable from `src` on the path this edge represents — + # cannot happen for a genuine chain member (SSA use-dominance + # puts `val`'s definition, and therefore `src`'s block, above + # `pred_block`), but this reads printed IR, not a proof, so + # stay defensive rather than assume it. + continue + end = Insn("", pred_block, len(f.insns[pred_block])) + sps = window_hits_generic(f, src, end, pred=_is_statepoint) + if not sps: + continue + if _protected_by_temp_root(f, src, end, chain): + continue + rooted = any(as1_chain & set(sp.live) for sp in sps) + v = StatepointHazard(module, fname, src, kind, phi, sps, rooted, + poll_reaching) + if want_moving_only and not v.moving: + continue + return v + return None + + def statepoint_corpus_stats(parsed): """`(statepoints, live_bundles, relocates, live_roots)` over a corpus. @@ -4002,6 +4121,72 @@ def _sp(tok="tok", callee="js_gc_loop_safepoint", live=()): } """.replace("__SAFEPOINT__", _sp()) +# ★★ The phi-edge false-positive, and its sabotage twin (#7664). +# +# `readCtx`'s actual shape, minimised: an unmasked receiver feeds a +# `js_is_truthy` short-circuit test (which the native mode does NOT special- +# case — see `js_is_truthy` NOT being consulted anywhere in this mode — it +# is simply a plain `call`, never wrapped in a `gc.statepoint`, so it is not +# a collection point by construction, same as production output), then joins +# a value from the OTHER branch at a two-predecessor phi. +# +# SAFE: the safepoint sits on the untainted branch (`then.1`), never on the +# edge that carries `%r2` into the join. Before this fix, blanket phi +# admission taxed the phi's result into `chain` because ONE edge (`%r2`) was +# tainted, and then found `ret double %m` as a "use of chain" reachable via +# `between_blocks(entry.0, join.2)` — which includes `then.1`, the SAFEPOINT +# lives there, on a path the `%r2` edge never actually takes. Zero hazards is +# the whole point of this fixture. +_SELFTEST_SP_PHI_SAFE_EDGE = """\ +define double @perry_fn_selftest__sp_phi_safe_edge(double %a, double %b) gc "statepoint-example" { +entry.0: + %rs4gc.b1 = bitcast double %a to i64 + %rs4gc.s1 = inttoptr i64 %rs4gc.b1 to ptr addrspace(1) + %raw = ptrtoint ptr addrspace(1) %rs4gc.s1 to i64 + %r2 = bitcast i64 %raw to double + %c = call i32 @js_is_truthy(double %r2) + %cc = icmp ne i32 %c, 0 + br i1 %cc, label %then.1, label %join.2 + +then.1: +__SAFEPOINT__ + %other = call double @js_object_get_field_by_name_f64(i64 0, i64 0) + br label %join.2 + +join.2: + %m = phi double [ %r2, %entry.0 ], [ %other, %then.1 ] + ret double %m +} +""".replace("__SAFEPOINT__", _sp()) + +# HAZARD: byte-identical except the safepoint moves onto the TAINTED edge +# itself, between `%r2`'s definition and `entry.0`'s own terminator -- the +# window `_phi_edge_hazard` checks. This is the case the restated rule keeps: +# "operand defined, safepoint runs, then the predecessor's terminator is +# reached." One `unrooted` hazard, or the false-positive fix went too far +# and quietly stopped the mode from seeing a phi-mediated hazard at all. +_SELFTEST_SP_PHI_HAZARD_EDGE = """\ +define double @perry_fn_selftest__sp_phi_hazard_edge(double %a, double %b) gc "statepoint-example" { +entry.0: + %rs4gc.b1 = bitcast double %a to i64 + %rs4gc.s1 = inttoptr i64 %rs4gc.b1 to ptr addrspace(1) + %raw = ptrtoint ptr addrspace(1) %rs4gc.s1 to i64 + %r2 = bitcast i64 %raw to double +__SAFEPOINT__ + %c = call i32 @js_is_truthy(double %b) + %cc = icmp ne i32 %c, 0 + br i1 %cc, label %then.1, label %join.2 + +then.1: + %other = call double @js_object_get_field_by_name_f64(i64 0, i64 0) + br label %join.2 + +join.2: + %m = phi double [ %r2, %entry.0 ], [ %other, %then.1 ] + ret double %m +} +""".replace("__SAFEPOINT__", _sp()) + # A `define` whose name this parser cannot read must RAISE, not be skipped. _SELFTEST_SP_BAD_DEFINE = """\ define double @(double %a) { @@ -4096,7 +4281,9 @@ def statepoint_self_test(): ("reloaded", _SELFTEST_SP_RELOADED), ("invoke", _SELFTEST_SP_INVOKE), ("roundtrip", _SELFTEST_SP_TRACKED_ROUNDTRIP), - ("quoted", _SELFTEST_SP_QUOTED_NAME)): + ("quoted", _SELFTEST_SP_QUOTED_NAME), + ("phi_safe_edge", _SELFTEST_SP_PHI_SAFE_EDGE), + ("phi_hazard_edge", _SELFTEST_SP_PHI_HAZARD_EDGE)): p = os.path.join(td, f"sp_{name}.ll") with open(p, "w") as fh: fh.write(text) @@ -4161,6 +4348,39 @@ def statepoint_self_test(): file=sys.stderr) ok = False + # ★★ The phi-edge false positive (#7664) and its sabotage twin. Same + # two-predecessor join, same tainted register, same safepoint -- the + # ONLY difference between the fixtures is which edge the safepoint + # sits on. If both report 0, `_phi_edge_hazard` is dead code. If both + # report 1, the false-positive fix did not actually fix anything. + hits = _scan_statepoints([paths["phi_safe_edge"]], moving_only=True) + if hits: + print(f"self-test FAIL: the phi_safe_edge fixture's tainted " + f"register only reaches the join on an edge with NO " + f"intervening safepoint -- the other predecessor is where " + f"the safepoint lives, and the phi never carries the " + f"tainted value in on that edge. Must report 0, got " + f"{len(hits)}: {[(h.kind_class, h.kind) for h in hits]}. " + "This is #7664's four `unmasked` false positives: blanket " + "phi admission conflated 'tainted on one edge' with " + "'tainted on every path to every downstream use'.", + file=sys.stderr) + ok = False + hits = _scan_statepoints([paths["phi_hazard_edge"]], moving_only=True) + if len(hits) != 1 or hits[0].kind_class != "unrooted": + print(f"self-test FAIL: the phi_hazard_edge fixture moves the " + f"SAME safepoint onto the TAINTED edge, between the " + f"operand's definition and its own predecessor's " + f"terminator -- a real phi-mediated hazard the restated " + f"rule ('a phi operand's window ends at the terminator of " + f"its incoming predecessor block') must still catch. Must " + f"report exactly one unrooted hazard, got " + f"{[(h.kind_class, h.kind) for h in hits]}. If this is 0, " + "the false-positive fix above also deleted the mode's " + "ability to see a genuine phi-mediated hazard.", + file=sys.stderr) + ok = False + # --- LLVM's printed forms: continuation lines, hyphens, quotes ---- funcs = parse_file(paths["invoke"]) f = funcs[0]