diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index cd9ac0a5e8..819938292b 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -333,6 +333,27 @@ jobs: python3 scripts/check_test_registration.py --self-test python3 scripts/check_test_registration.py + # #7672: the GC test guards CLEAR ~20 process-global side tables from + # whatever libtest thread constructs a guard, and nothing requires a + # READER to take the clearing lock. Three flakes in two days came from + # that (#7665 x2, #7671), each exposed by an unrelated PR that changed the + # parallel schedule, so the author of the exposing PR paid the diagnosis. + # + # The class is fixed by storage, not by a lock: `per_test_global!` + # gives each thread its own table in a test build. This gate derives the + # clear list from the guards' own source and fails on any bare `static` + # left behind, so a NEW sink cannot be added quietly and a new READER + # never has to remember anything. Its allowlist entries each cite an + # issue, and an entry that matches nothing fails too. + # + # Pure text, ~1s, no compiler — in `lint` because `lint` is a required + # context (hazard 2), and `!cancelled()` for the reason given above. + - name: Per-test global sinks + if: ${{ !cancelled() }} + run: | + python3 scripts/global_sink_isolation.py --self-test + python3 scripts/global_sink_isolation.py + # --------------------------------------------------------------------------- # Clippy — enforces the deny-level lints in [workspace.lints] (root # Cargo.toml). `cargo clippy` exits nonzero only on `deny` lints, so diff --git a/CLAUDE.md b/CLAUDE.md index fe924e2522..fa43b90fdf 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.1386 +**Current Version:** 0.5.1387 ## TypeScript Parity Status diff --git a/Cargo.lock b/Cargo.lock index 0d37bf2099..89fab40384 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5547,7 +5547,7 @@ checksum = "9b4f627cb1b25917193a259e49bdad08f671f8d9708acfd5fe0a8c1455d87220" [[package]] name = "perry" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "anyhow", "base64", @@ -5607,14 +5607,14 @@ dependencies = [ [[package]] name = "perry-api-manifest" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "serde", ] [[package]] name = "perry-audio-miniaudio" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "cc", "libc", @@ -5622,7 +5622,7 @@ dependencies = [ [[package]] name = "perry-codegen" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "anyhow", "inkwell", @@ -5639,7 +5639,7 @@ dependencies = [ [[package]] name = "perry-codegen-arkts" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "anyhow", "perry-hir", @@ -5647,7 +5647,7 @@ dependencies = [ [[package]] name = "perry-codegen-glance" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "anyhow", "perry-hir", @@ -5655,7 +5655,7 @@ dependencies = [ [[package]] name = "perry-codegen-js" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "anyhow", "perry-dispatch", @@ -5664,7 +5664,7 @@ dependencies = [ [[package]] name = "perry-codegen-swiftui" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "anyhow", "perry-hir", @@ -5672,7 +5672,7 @@ dependencies = [ [[package]] name = "perry-codegen-wasm" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "anyhow", "base64", @@ -5684,7 +5684,7 @@ dependencies = [ [[package]] name = "perry-codegen-wear-tiles" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "anyhow", "perry-hir", @@ -5692,7 +5692,7 @@ dependencies = [ [[package]] name = "perry-container-compose" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "anyhow", "async-trait", @@ -5721,14 +5721,14 @@ dependencies = [ [[package]] name = "perry-container-e2e" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "anyhow", ] [[package]] name = "perry-diagnostics" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "serde", "serde_json", @@ -5736,7 +5736,7 @@ dependencies = [ [[package]] name = "perry-dispatch" -version = "0.5.1386" +version = "0.5.1387" [[package]] name = "perry-doc-fixture-my-bindings" @@ -5747,7 +5747,7 @@ dependencies = [ [[package]] name = "perry-doc-tests" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "anyhow", "clap", @@ -5762,7 +5762,7 @@ dependencies = [ [[package]] name = "perry-ext-ads" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "block2", "objc2", @@ -5772,7 +5772,7 @@ dependencies = [ [[package]] name = "perry-ext-argon2" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "argon2", "perry-ffi", @@ -5780,7 +5780,7 @@ dependencies = [ [[package]] name = "perry-ext-axios" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-ffi", "reqwest", @@ -5789,7 +5789,7 @@ dependencies = [ [[package]] name = "perry-ext-bcrypt" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "bcrypt", "perry-ffi", @@ -5797,7 +5797,7 @@ dependencies = [ [[package]] name = "perry-ext-better-sqlite3" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-ffi", "rusqlite", @@ -5805,7 +5805,7 @@ dependencies = [ [[package]] name = "perry-ext-cheerio" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-ffi", "scraper", @@ -5813,7 +5813,7 @@ dependencies = [ [[package]] name = "perry-ext-commander" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-ffi", "perry-runtime", @@ -5821,7 +5821,7 @@ dependencies = [ [[package]] name = "perry-ext-cron" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "chrono", "cron", @@ -5831,7 +5831,7 @@ dependencies = [ [[package]] name = "perry-ext-dayjs" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "chrono", "perry-ffi", @@ -5839,7 +5839,7 @@ dependencies = [ [[package]] name = "perry-ext-decimal" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-ffi", "rust_decimal", @@ -5847,7 +5847,7 @@ dependencies = [ [[package]] name = "perry-ext-dotenv" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-ffi", "serde_json", @@ -5855,7 +5855,7 @@ dependencies = [ [[package]] name = "perry-ext-ethers" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-ffi", "rand 0.10.1", @@ -5863,7 +5863,7 @@ dependencies = [ [[package]] name = "perry-ext-events" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-ffi", "perry-runtime", @@ -5871,14 +5871,14 @@ dependencies = [ [[package]] name = "perry-ext-exponential-backoff" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-ffi", ] [[package]] name = "perry-ext-fastify" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "bytes", "http-body-util", @@ -5896,7 +5896,7 @@ dependencies = [ [[package]] name = "perry-ext-fetch" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "bytes", "lazy_static", @@ -5909,7 +5909,7 @@ dependencies = [ [[package]] name = "perry-ext-http" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "bytes", "h2", @@ -5933,7 +5933,7 @@ dependencies = [ [[package]] name = "perry-ext-ioredis" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "lazy_static", "perry-ffi", @@ -5943,7 +5943,7 @@ dependencies = [ [[package]] name = "perry-ext-jsonwebtoken" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "base64", "jsonwebtoken", @@ -5954,7 +5954,7 @@ dependencies = [ [[package]] name = "perry-ext-lru-cache" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "lru", "perry-ffi", @@ -5963,7 +5963,7 @@ dependencies = [ [[package]] name = "perry-ext-moment" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "chrono", "perry-ffi", @@ -5971,7 +5971,7 @@ dependencies = [ [[package]] name = "perry-ext-mongodb" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "bson", "futures-util", @@ -5983,7 +5983,7 @@ dependencies = [ [[package]] name = "perry-ext-mysql2" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "chrono", "perry-ffi", @@ -5993,7 +5993,7 @@ dependencies = [ [[package]] name = "perry-ext-nanoid" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "nanoid", "perry-ffi", @@ -6002,7 +6002,7 @@ dependencies = [ [[package]] name = "perry-ext-net" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "bytes", "perry-ffi", @@ -6015,7 +6015,7 @@ dependencies = [ [[package]] name = "perry-ext-node-forge" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "const-oid 0.9.6", "der 0.7.10", @@ -6034,7 +6034,7 @@ dependencies = [ [[package]] name = "perry-ext-nodemailer" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "lettre", "perry-ffi", @@ -6044,7 +6044,7 @@ dependencies = [ [[package]] name = "perry-ext-pdf" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-ffi", "printpdf", @@ -6052,7 +6052,7 @@ dependencies = [ [[package]] name = "perry-ext-pg" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-ffi", "sqlx", @@ -6061,7 +6061,7 @@ dependencies = [ [[package]] name = "perry-ext-ratelimit" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "governor", "perry-ffi", @@ -6069,7 +6069,7 @@ dependencies = [ [[package]] name = "perry-ext-sharp" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "fast_image_resize", "image", @@ -6079,14 +6079,14 @@ dependencies = [ [[package]] name = "perry-ext-slugify" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-ffi", ] [[package]] name = "perry-ext-streams" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "lazy_static", "perry-ffi", @@ -6095,7 +6095,7 @@ dependencies = [ [[package]] name = "perry-ext-undici" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-ffi", "perry-runtime", @@ -6104,7 +6104,7 @@ dependencies = [ [[package]] name = "perry-ext-uuid" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-ffi", "uuid", @@ -6112,7 +6112,7 @@ dependencies = [ [[package]] name = "perry-ext-validator" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-ffi", "regex", @@ -6122,7 +6122,7 @@ dependencies = [ [[package]] name = "perry-ext-ws" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "futures-util", "lazy_static", @@ -6135,7 +6135,7 @@ dependencies = [ [[package]] name = "perry-ext-zlib" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "brotli", "flate2", @@ -6145,7 +6145,7 @@ dependencies = [ [[package]] name = "perry-ffi" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "dashmap", "once_cell", @@ -6154,7 +6154,7 @@ dependencies = [ [[package]] name = "perry-hir" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "anyhow", "perry-api-manifest", @@ -6172,7 +6172,7 @@ dependencies = [ [[package]] name = "perry-parser" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "anyhow", "perry-diagnostics", @@ -6184,7 +6184,7 @@ dependencies = [ [[package]] name = "perry-runtime" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "anyhow", "base64", @@ -6226,14 +6226,14 @@ dependencies = [ [[package]] name = "perry-runtime-static" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-runtime", ] [[package]] name = "perry-stdlib" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "aes 0.8.4", "aes 0.9.1", @@ -6328,14 +6328,14 @@ dependencies = [ [[package]] name = "perry-stdlib-static" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-stdlib", ] [[package]] name = "perry-transform" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "anyhow", "perry-hir", @@ -6344,14 +6344,14 @@ dependencies = [ [[package]] name = "perry-ui" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-ui-model", ] [[package]] name = "perry-ui-android" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "base64", "itoa", @@ -6368,7 +6368,7 @@ dependencies = [ [[package]] name = "perry-ui-geisterhand" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "rand 0.10.1", "serde", @@ -6378,7 +6378,7 @@ dependencies = [ [[package]] name = "perry-ui-gtk4" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "base64", "cairo-rs 0.22.0", @@ -6401,7 +6401,7 @@ dependencies = [ [[package]] name = "perry-ui-ios" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "base64", "block2", @@ -6417,7 +6417,7 @@ dependencies = [ [[package]] name = "perry-ui-macos" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "base64", "block2", @@ -6432,7 +6432,7 @@ dependencies = [ [[package]] name = "perry-ui-model" -version = "0.5.1386" +version = "0.5.1387" [[package]] name = "perry-ui-test" @@ -6443,11 +6443,11 @@ dependencies = [ [[package]] name = "perry-ui-testkit" -version = "0.5.1386" +version = "0.5.1387" [[package]] name = "perry-ui-tvos" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "base64", "block2", @@ -6463,7 +6463,7 @@ dependencies = [ [[package]] name = "perry-ui-visionos" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "base64", "block2", @@ -6479,7 +6479,7 @@ dependencies = [ [[package]] name = "perry-ui-watchos" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "block2", "libc", @@ -6492,7 +6492,7 @@ dependencies = [ [[package]] name = "perry-ui-windows" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "base64", "libc", @@ -6509,14 +6509,14 @@ dependencies = [ [[package]] name = "perry-ui-windows-winui" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "perry-ui-windows", ] [[package]] name = "perry-updater" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "anyhow", "base64", @@ -6532,7 +6532,7 @@ dependencies = [ [[package]] name = "perry-wasm-host" -version = "0.5.1386" +version = "0.5.1387" dependencies = [ "wasmi", ] diff --git a/Cargo.toml b/Cargo.toml index 3513254ea5..7ce89f6827 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -315,7 +315,7 @@ codegen-units = 16 codegen-units = 16 [workspace.package] -version = "0.5.1386" +version = "0.5.1387" edition = "2021" license = "MIT" repository = "https://github.com/PerryTS/perry" diff --git a/changelog.d/7674-per-test-global-sinks.md b/changelog.d/7674-per-test-global-sinks.md new file mode 100644 index 0000000000..3c3df8c340 --- /dev/null +++ b/changelog.d/7674-per-test-global-sinks.md @@ -0,0 +1,125 @@ +**Test isolation: the GC test guards' state reset can no longer reach another test's data (#7672).** + +`gc::tests::support::reset_copying_nursery_runtime_test_state()` runs from +`GcTestIsolationGuard` and `CopyingNurseryTestGuard`, on whatever libtest thread +constructs one, and calls 20 `test_clear_*` helpers that empty **process-global** +side tables. The guards serialize against each other and against the handful of +tests that remember to take `crate::gc::global_side_table_test_lock()`. Nothing +requires a *reader* to take it, so the defence was opt-in and the opt-in was +invisible at the read site. + +Three flakes in two days, each exposed by an unrelated PR that changed the +parallel schedule and each diagnosed from the wrong VALUE rather than the timing: + +| fixed in | table | presented as | +|---|---|---| +| #7665 | `opt_report`'s row sink | `rows.len() == 2` failing at **3** | +| #7665 | `ext_registry`'s `USED_PROVIDERS` | "empty" failing with `ioredis` present | +| #7671 | `closure`'s `CLOSURE_PROPS` | a static method read back as `TAG_UNDEFINED` | + +### The fix is storage, not a lock + +A lock cannot close this. The damage window is "between this test's write and +this test's read", and only the test knows that span — an accessor that takes a +shared lock for one call does not cover it, and a lock that does have to be taken +by the test, which is the opt-in the class is made of, on a reader population the +issue counts at ~180 and growing. + +`per_test_global!` expands to the plain `static` outside a test build (byte +for byte — `test_clear_*` is `#[cfg(test)]` and never runs in a shipped runtime) +and to a per-thread instance inside one. libtest runs one thread per test, so +"per thread" and "per test" coincide, and a guard on thread U empties U's +instance, which already holds only U's entries — exactly the isolation the clear +was reaching for. Call sites are unchanged: `PerThread` derefs to `T`, so +`SYMBOL_REGISTRY.lock()` and `CLASS_PROTOTYPE_OBJECTS.read()` keep working as +written, and this is a declaration-site change rather than a 300-site rewrite. + +**37 statics converted across 13 files**, covering every family the guards clear: +closure dynamic props (3), symbol side tables (5), the class-registry tables (9), +the timer queues (3), the object-constant caches and `GLOBAL_THIS_PTR` (9), +`geisterhand` (2), `ui_text_registry` (2), the `console.log` singleton — plus the +two the soak found (`tui::tree` 2, the write-barrier flag 1). + +### Both halves are shown able to fail + +`scripts/global_sink_isolation.py` runs in `lint` (a required context, ~1s, no +compiler). It derives the clear list from the reset function's own source, +follows one level of same-file accessors — `test_clear_closure_side_tables` names +no static at all, it goes through `get_closure_props()`, and a body-only scan +would have classified it "no storage" — resolves each identifier in its own +module first (`REGISTRY` exists in three files), and fails on any bare `static` +not allowlisted with an issue. An allowlist entry that matches nothing fails too, +so a fix must delete its line. `--self-test` has 9 checks, including running the +parsers against the real tree so a regex that stops matching cannot go green. + +`gc::tests::global_sink_isolation` plants the #7671 shape per clear helper — +write on this thread, run the guards' real clear on another, read back — without +taking the global lock, because taking it would test the opt-in rather than the +isolation. Each probe asserts its subject was live before the clear. A canary +declared as a plain `static` runs the identical procedure and **requires** the +wipe to be observed. + +Reverting the macro's `#[cfg(test)]` arm to the pre-fix bare static — one edit, +every table at once — fails **9 of the 11 tests**, each naming its table (0 +`error[` lines and `Running unittests` present on that run, so the sabotage +compiled and executed rather than failing to build). + +### Validation + +25 consecutive `cargo test -p perry-runtime --lib --no-fail-fast` runs, 25 green, +0 vacuous (every run checked for `Running unittests` and for `error[` before its +result was counted): 1926 passed, 0 failed, ~8.5s each. `cargo check +--all-targets` clean; all 24 `lint` commands green. + +`timer.rs` sat three lines under the 2000-line cap, so the macro takes an +optional trailing `;` and the three timer tables are declared on one line each. + +### Surveyed, not fixed here + +The same architecture, with the same split-lock-domain signature, exists off the +guards' clear path: `async_hooks`' `HOOKS`/`RESOURCES`/`NEXT_ASYNC_ID` under four +disjoint domains (and `gc/tests/alloc.rs:836` under none), `tui::state::SLOTS` +cleared under three different locks, and `agent_dispatch_tests.rs`'s private +`TIMER_QUEUE_TESTS` lock over the timer queues the guards also clear. + +### Two more found by soaking the fix, and the macro renamed for what it does + +A 22-run soak produced **two** reds, both the same class, neither reached by any +`test_clear_*` helper — so the clear list could not have found them, and neither +could a gate derived only from it: + +* **`gc::barrier::GENERATED_WRITE_BARRIERS_EMITTED`** is owned by TWO guards + under TWO DIFFERENT locks — `CopyingNurseryTestGuard` under the + copying-nursery isolation lock, `GeneratedWriteBarrierTestGuard` under + `GENERATED_BARRIER_TEST_LOCK` — and every runtime write barrier reads it + holding neither. `sabotaged_parent_gate_strands_a_young_child_the_shipped_gate_keeps` + failed with `missing_edges=1 ... slot_page_ever_dirty=false`: the barrier did + not fire, because another thread's guard had zeroed the flag mid-test. +* **`tui::tree`'s `NEXT_HANDLE` / `REGISTRY`** have no clear and no lock, and + `register_increments_handle` asserts `h2 == h1 + 1` plus an exact registry + length. Any concurrent `register()` breaks both. + +Both were diagnosed from the wrong VALUE, not the timing — a non-sequential +handle, and a barrier that did not dirty a page. + +The macro is therefore named `per_test_global!`, for what it does, rather than +`guard_cleared_global!`, for the sharpest instance of what it defends against; +two of its residents are not cleared by anything. The gate now audits the +guards' own module alongside the clear list, which is what makes the +write-barrier flag reachable by it at all. + +### Two ways this gate could have gone quiet, both closed + +Found while extending it, and both now self-tested: + +* The `per_test_global!(...)` **paren form** — used in `timer.rs` to stay under + the 2000-line cap — was invisible to a `{`-only matcher, which silently took + the three timer tables to "(no static storage)". A gate that stops matching + reports zero hazards and exits 0, which is indistinguishable from a clean tree. +* A **classified-statics floor** now fires when the matchers stop matching, so a + rotted regex fails loudly rather than passing vacuously. 93 classify today. + +A `Mutex<()>` serializer is classified as a lock and is never a hazard: making +one per-thread would turn it into a no-op, which is the opposite of the fix. +Reverting the delimiter fix fails the paren-form self-test case; reverting the +floor fails its own. diff --git a/crates/perry-runtime/src/builtins/console.rs b/crates/perry-runtime/src/builtins/console.rs index 04046fcfef..fb2c8ca763 100644 --- a/crates/perry-runtime/src/builtins/console.rs +++ b/crates/perry-runtime/src/builtins/console.rs @@ -123,11 +123,13 @@ extern "C" fn console_log_callable_thunk( } use std::sync::atomic::{AtomicI64, Ordering}; -/// Singleton closure pointer for `console.log` exposed as a value. -/// Allocated lazily by `js_console_log_as_closure`. Kept alive across GC -/// cycles by the `scan_console_log_singleton_roots` scanner registered in -/// `gc::gc_init`. -static CONSOLE_LOG_SINGLETON: AtomicI64 = AtomicI64::new(0); +per_test_global! { + /// Singleton closure pointer for `console.log` exposed as a value. + /// Allocated lazily by `js_console_log_as_closure`. Kept alive across GC + /// cycles by the `scan_console_log_singleton_roots` scanner registered in + /// `gc::gc_init`. + static CONSOLE_LOG_SINGLETON: AtomicI64 = AtomicI64::new(0); +} /// Returns a singleton ClosureHeader pointer that, when invoked through /// `js_closure_call1`, calls `console.log` on the argument. Used by codegen diff --git a/crates/perry-runtime/src/closure/dynamic_props.rs b/crates/perry-runtime/src/closure/dynamic_props.rs index 73a3d07244..aefbc6aabb 100644 --- a/crates/perry-runtime/src/closure/dynamic_props.rs +++ b/crates/perry-runtime/src/closure/dynamic_props.rs @@ -5,24 +5,28 @@ use super::*; use std::collections::{HashMap, HashSet}; use std::sync::{Mutex, OnceLock}; -static CLOSURE_PROPS: OnceLock>>> = OnceLock::new(); +per_test_global! { + static CLOSURE_PROPS: OnceLock>>> = OnceLock::new(); +} fn get_closure_props() -> &'static Mutex>> { CLOSURE_PROPS.get_or_init(|| Mutex::new(HashMap::new())) } -/// #3655: keys deleted off a closure via `delete fn.name` etc. -/// -/// Functions carry built-in own data properties (`name`, `length`, and — -/// for constructors — `prototype`) that aren't stored in `CLOSURE_PROPS`: -/// they're synthesized from the arity/name registries on read. Those -/// properties are spec'd `configurable: true`, so `delete fn.name` must make -/// them disappear from every subsequent `hasOwnProperty` / `getOwnProperty*` -/// / value read. We can't remove a synthesized slot, so we record the -/// deletion here and have every property-protocol site consult it. test262's -/// `verifyProperty` exercises exactly this (delete-then-`hasOwnProperty`) -/// when checking `configurable`. -static CLOSURE_DELETED_KEYS: OnceLock>>> = OnceLock::new(); +per_test_global! { + /// #3655: keys deleted off a closure via `delete fn.name` etc. + /// + /// Functions carry built-in own data properties (`name`, `length`, and — + /// for constructors — `prototype`) that aren't stored in `CLOSURE_PROPS`: + /// they're synthesized from the arity/name registries on read. Those + /// properties are spec'd `configurable: true`, so `delete fn.name` must make + /// them disappear from every subsequent `hasOwnProperty` / `getOwnProperty*` + /// / value read. We can't remove a synthesized slot, so we record the + /// deletion here and have every property-protocol site consult it. test262's + /// `verifyProperty` exercises exactly this (delete-then-`hasOwnProperty`) + /// when checking `configurable`. + static CLOSURE_DELETED_KEYS: OnceLock>>> = OnceLock::new(); +} fn get_closure_deleted_keys() -> &'static Mutex>> { CLOSURE_DELETED_KEYS.get_or_init(|| Mutex::new(HashMap::new())) @@ -62,17 +66,19 @@ pub fn closure_has_own_dynamic_prop(ptr: usize, prop: &str) -> bool { .unwrap_or(false) } -/// #36 / #321: `Object.setPrototypeOf(closure, protoObj)` side-table. -/// -/// Maps a closure pointer to the NaN-box bits of the object that was set as -/// its static prototype. effect's `Context.Tag(id)` returns a plain function -/// `TagClass` whose `_op: "Tag"`, `[TagTypeId]`, and `[EffectTypeId]` live on -/// `TagProto` (a regular object), wired by `Object.setPrototypeOf(TagClass, -/// TagProto)`. Perry bakes class IDs at allocation time so it can't mutate a -/// real prototype chain, but recording the (closure → proto) link here lets -/// string- and symbol-keyed property reads on the closure walk to the proto's -/// own properties — so `TagClass._op === "Tag"` and `isTag(TagClass)` hold. -static CLOSURE_STATIC_PROTOTYPES: OnceLock>> = OnceLock::new(); +per_test_global! { + /// #36 / #321: `Object.setPrototypeOf(closure, protoObj)` side-table. + /// + /// Maps a closure pointer to the NaN-box bits of the object that was set as + /// its static prototype. effect's `Context.Tag(id)` returns a plain function + /// `TagClass` whose `_op: "Tag"`, `[TagTypeId]`, and `[EffectTypeId]` live on + /// `TagProto` (a regular object), wired by `Object.setPrototypeOf(TagClass, + /// TagProto)`. Perry bakes class IDs at allocation time so it can't mutate a + /// real prototype chain, but recording the (closure → proto) link here lets + /// string- and symbol-keyed property reads on the closure walk to the proto's + /// own properties — so `TagClass._op === "Tag"` and `isTag(TagClass)` hold. + static CLOSURE_STATIC_PROTOTYPES: OnceLock>> = OnceLock::new(); +} fn get_closure_prototypes() -> &'static Mutex> { CLOSURE_STATIC_PROTOTYPES.get_or_init(|| Mutex::new(HashMap::new())) diff --git a/crates/perry-runtime/src/gc/barrier.rs b/crates/perry-runtime/src/gc/barrier.rs index e9e88b2a11..cb34ce3c86 100644 --- a/crates/perry-runtime/src/gc/barrier.rs +++ b/crates/perry-runtime/src/gc/barrier.rs @@ -657,7 +657,21 @@ thread_local! { const { Cell::new(BarrierTraceCounters::zero()) }; } -pub(super) static GENERATED_WRITE_BARRIERS_EMITTED: AtomicUsize = AtomicUsize::new(0); +per_test_global! { + /// #7672, fifth instance. Two test guards own this flag under two DIFFERENT + /// locks — `CopyingNurseryTestGuard` sets it to 1 under the copying-nursery + /// isolation lock, `GeneratedWriteBarrierTestGuard` swaps it under + /// `GENERATED_BARRIER_TEST_LOCK` — and `generated_write_barriers_active()` + /// is read by tests holding neither. A `GeneratedWriteBarrierTestGuard:: + /// inactive()` on one libtest thread therefore silences the runtime barrier + /// under another thread's test, whose store then dirties no page. + /// + /// Observed, not theoretical: `sabotaged_parent_gate_strands_a_young_child_ + /// the_shipped_gate_keeps` failed 1 run in 22 with + /// `missing_edges=1 ... slot_page_ever_dirty=false` — the barrier did not + /// fire, which is a wrong VALUE and not a timing symptom. + pub(super) static GENERATED_WRITE_BARRIERS_EMITTED: AtomicUsize = AtomicUsize::new(0); +} /// Number of threads whose incremental mark barrier is currently active. /// diff --git a/crates/perry-runtime/src/gc/tests/global_sink_isolation.rs b/crates/perry-runtime/src/gc/tests/global_sink_isolation.rs new file mode 100644 index 0000000000..fd4627ce59 --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/global_sink_isolation.rs @@ -0,0 +1,395 @@ +//! #7672: the GC test guards clear process-global side tables from whatever +//! libtest thread runs them, and no reader is required to take the clearing +//! lock. +//! +//! Each probe below plants the shape that produced the three flakes of +//! 2026-08-08/09 — write on THIS thread, run the guards' clear on ANOTHER +//! thread, read back — and asserts the entry survived. That is only meaningful +//! because the same harness is shown to catch a table that has *not* been +//! converted: `the_probe_catches_a_bare_process_global_sink` runs a canary +//! declared as a plain `static` and requires the wipe to be observed. A green +//! file therefore means the detector works, not that nothing was tried. +//! +//! The probes deliberately do NOT take `crate::gc::global_side_table_test_lock()`. +//! Taking it is what the ~180 unconverted readers are not required to do, and a +//! probe that took it would be testing the opt-in rather than the isolation. + +use std::collections::HashMap; +use std::sync::{Mutex, OnceLock}; + +/// Run one probe: install on this thread, clear on another, observe here. +/// +/// Returns `true` if the installed entry survived the foreign clear. +/// +/// Panics if `install` did not make `observe` true to begin with — a probe +/// whose subject was never live cannot distinguish "survived" from "was never +/// there", which is exactly the vacuity this file exists to avoid. +fn survives_a_foreign_clear( + label: &str, + install: impl FnOnce(), + observe: impl Fn() -> bool, + clear: fn(), +) -> bool { + install(); + assert!( + observe(), + "{label}: the probe installed nothing, so the survived-vs-wiped distinction \ + would be vacuous — fix the probe before reading its verdict" + ); + std::thread::spawn(clear) + .join() + .expect("the clearing thread panicked"); + observe() +} + +/// The guards' own state reset, run from another thread — verbatim what +/// `GcTestIsolationGuard::new` and `CopyingNurseryTestGuard::new/drop` do. +fn foreign_guard_clear() { + super::support::reset_copying_nursery_runtime_test_state(); +} + +// --------------------------------------------------------------------------- +// The canary: a bare process-global sink, and proof the harness catches it. +// --------------------------------------------------------------------------- + +/// Deliberately NOT `per_test_global!`. This is the pre-#7672 shape of +/// every table in this file, kept so the probe above is proven able to fail. +static CANARY_SINK: OnceLock>> = OnceLock::new(); + +fn canary() -> &'static Mutex> { + CANARY_SINK.get_or_init(|| Mutex::new(HashMap::new())) +} + +fn canary_clear() { + canary().lock().unwrap_or_else(|p| p.into_inner()).clear(); +} + +#[test] +fn the_probe_catches_a_bare_process_global_sink() { + let key = 0xCA_1A_0000_7672usize; + let survived = survives_a_foreign_clear( + "canary", + || { + canary() + .lock() + .unwrap_or_else(|p| p.into_inner()) + .insert(key, 0x7672); + }, + move || { + canary() + .lock() + .unwrap_or_else(|p| p.into_inner()) + .get(&key) + .copied() + == Some(0x7672) + }, + canary_clear, + ); + assert!( + !survived, + "the isolation probe reported a BARE process-global sink as surviving a \ + clear on another thread. The probe can no longer fail, so every other \ + test in this file is vacuous — fix the probe, do not delete this test." + ); +} + +// --------------------------------------------------------------------------- +// Converted tables. One test per `test_clear_*` helper the guards call. +// --------------------------------------------------------------------------- + +/// `closure::test_clear_closure_side_tables` — `CLOSURE_PROPS`, +/// `CLOSURE_STATIC_PROTOTYPES`, `CLOSURE_DELETED_KEYS`. +/// +/// This is the #7671 shape exactly: a value written through the closure +/// dynamic-property table and read back came out `TAG_UNDEFINED` because a GC +/// guard on another libtest thread had emptied the table in between. +#[test] +fn closure_side_tables_survive_a_guard_clear_on_another_thread() { + // A synthetic, per-test key: the tables are keyed by heap address but the + // accessors treat the key as an opaque integer for store/load. + let owner = 0x_C105_0000_7672_usize; + let survived = survives_a_foreign_clear( + "closure dynamic props", + || { + crate::closure::closure_set_dynamic_prop(owner, "probe7672", 42.5); + crate::closure::closure_set_static_prototype(owner, 0x7FFD_0000_0000_7672); + crate::closure::closure_mark_key_deleted(owner, "goneKey"); + }, + move || { + crate::closure::closure_get_own_dynamic_prop(owner, "probe7672") == Some(42.5) + && crate::closure::closure_has_own_dynamic_prop(owner, "probe7672") + && crate::closure::closure_static_prototype(owner) == Some(0x7FFD_0000_0000_7672) + && crate::closure::closure_is_key_deleted(owner, "goneKey") + }, + foreign_guard_clear, + ); + assert!( + survived, + "a closure dynamic property written on this thread was destroyed by the GC \ + test guards' state reset running on another thread (#7672 / #7671). The \ + table's `per_test_global!` declaration is what prevents this." + ); +} + +/// `symbol::test_clear_symbol_side_table_roots` — `SYMBOL_PROPERTIES`, +/// `SYMBOL_PROPERTY_ATTRS`, `CLASS_STATIC_SYMBOLS`, `SYMBOL_ACCESSOR_PROPERTIES` +/// and the `SYMBOL_POINTERS` rebuild. +/// +/// The largest reader cluster in the survey behind #7672: 14 tests populate one +/// of these and assert on it without taking the clearing lock. +#[test] +fn symbol_side_tables_survive_a_guard_clear_on_another_thread() { + let owner = 0x_5B01_0000_7672_usize; + let sym_key = 0x_5B01_0000_7673_usize; + let class_id = 0x7672_u32; + let survived = survives_a_foreign_clear( + "symbol side tables", + || { + crate::symbol::test_seed_symbol_property_root(owner, sym_key, 0x7FFD_0000_0000_7672); + crate::symbol::test_seed_class_static_symbol_root( + class_id, + sym_key, + 0x7FFD_0000_0000_7673, + ); + crate::symbol::test_seed_symbol_pointer_root(sym_key); + }, + move || { + crate::symbol::test_symbol_property_root_bits(owner, sym_key) + == Some(0x7FFD_0000_0000_7672) + && crate::symbol::test_class_static_symbol_root_bits(class_id, sym_key) + == Some(0x7FFD_0000_0000_7673) + && crate::symbol::test_symbol_pointer_root_contains(sym_key) + }, + foreign_guard_clear, + ); + assert!( + survived, + "a symbol side-table entry written on this thread was destroyed by the GC \ + test guards' state reset running on another thread (#7672)" + ); +} + +/// `timer::test_clear_all_timer_scanner_roots` — `TIMER_QUEUE`, +/// `CALLBACK_TIMERS`, `INTERVAL_TIMERS`. +#[test] +fn timer_queues_survive_a_guard_clear_on_another_thread() { + let survived = survives_a_foreign_clear( + "timer queues", + || crate::timer::test_seed_many_timeout_roots(&[f64::from_bits(0x7FFD_0000_0000_7672)]), + || crate::timer::test_timer_scanner_snapshot().timeout_value_bits == 0x7FFD_0000_0000_7672, + foreign_guard_clear, + ); + assert!( + survived, + "a queued timer written on this thread was destroyed by the GC test guards' \ + state reset running on another thread (#7672)" + ); +} + +/// `object::test_clear_class_side_table_roots` — the class-registry `RwLock` +/// tables (`CLASS_PROTOTYPE_OBJECTS`, `CLASS_PARENT_CLOSURES`, …), which the +/// guard sets to `None` wholesale rather than per class id. +#[test] +fn class_registry_tables_survive_a_guard_clear_on_another_thread() { + let proto_cid = 0x7672_u32; + let closure_cid = 0x7673_u32; + let survived = survives_a_foreign_clear( + "class registry", + || { + crate::object::test_seed_class_inheritance_roots(proto_cid, 0x7672_0000); + crate::object::test_seed_class_parent_closure_root(closure_cid, 0x7673_0000); + }, + move || { + crate::object::test_class_prototype_object_root(proto_cid) == 0x7672_0000 + && crate::object::test_class_parent_closure_root(closure_cid) == 0x7673_0000 + }, + foreign_guard_clear, + ); + assert!( + survived, + "a class-registry entry written on this thread was destroyed by the GC test \ + guards' state reset running on another thread (#7672)" + ); +} + +/// `object::test_clear_object_cache_roots` — the `AtomicU64` module-constant +/// caches and `GLOBAL_THIS_PTR`. These hold pointers into the *allocating +/// thread's* arena, so per-thread storage is also the only sound shape for +/// them; `array/tests.rs` carries 256-iteration retry loops written around +/// exactly this race. +#[test] +fn object_cache_roots_survive_a_guard_clear_on_another_thread() { + let bits: [u64; 7] = [ + 0x7FFD_0000_0000_7670, + 0x7FFD_0000_0000_7671, + 0x7FFD_0000_0000_7672, + 0x7FFD_0000_0000_7673, + 0x7FFD_0000_0000_7674, + 0x7FFD_0000_0000_7675, + 0x7FFD_0000_0000_7676, + ]; + let survived = survives_a_foreign_clear( + "object caches", + || crate::object::test_seed_object_cache_roots(bits, 0x7672_0000), + move || crate::object::test_object_cache_roots() == (bits, 0x7672_0000), + foreign_guard_clear, + ); + assert!( + survived, + "an object-cache root written on this thread was destroyed by the GC test \ + guards' state reset running on another thread (#7672)" + ); +} + +/// `geisterhand_registry::test_clear_geisterhand_roots` — `REGISTRY` and +/// `PENDING_ACTIONS`. +#[test] +fn geisterhand_registry_survives_a_guard_clear_on_another_thread() { + let closure = f64::from_bits(0x7FFD_0000_0000_7672); + let survived = survives_a_foreign_clear( + "geisterhand registry", + || crate::geisterhand_registry::test_seed_geisterhand_roots(closure, 1.0, 2.0), + move || { + crate::geisterhand_registry::test_geisterhand_roots_snapshot().0 + == 0x7FFD_0000_0000_7672 + }, + foreign_guard_clear, + ); + assert!( + survived, + "a geisterhand registry entry written on this thread was destroyed by the GC \ + test guards' state reset running on another thread (#7672)" + ); +} + +/// `ui_text_registry::test_clear_ui_text_registry_roots` — `STATE_VALUES` and +/// `FOREACH_REGISTRY`. +#[test] +fn ui_text_registry_survives_a_guard_clear_on_another_thread() { + let state = f64::from_bits(0x7FFD_0000_0000_7672); + let render = f64::from_bits(0x7FFD_0000_0000_7673); + let survived = survives_a_foreign_clear( + "ui text registry", + || crate::ui_text_registry::test_seed_ui_text_registry_roots(state, render), + move || { + crate::ui_text_registry::test_ui_text_registry_roots_snapshot() + == (0x7FFD_0000_0000_7672, 0x7FFD_0000_0000_7673) + }, + foreign_guard_clear, + ); + assert!( + survived, + "a UI text registry entry written on this thread was destroyed by the GC test \ + guards' state reset running on another thread (#7672)" + ); +} + +// --------------------------------------------------------------------------- +// The two instances that have no clear at all — found by soaking the fix for +// the ones that do, and both diagnosed from the wrong VALUE. +// --------------------------------------------------------------------------- + +/// `gc::barrier`'s `GENERATED_WRITE_BARRIERS_EMITTED` is owned by TWO guards +/// under TWO DIFFERENT locks — `CopyingNurseryTestGuard` sets it under the +/// copying-nursery isolation lock, `GeneratedWriteBarrierTestGuard` swaps it +/// under `GENERATED_BARRIER_TEST_LOCK` — and every runtime write barrier reads +/// it holding neither. +/// +/// Observed at 1 run in 22: +/// `sabotaged_parent_gate_strands_a_young_child_the_shipped_gate_keeps` failed +/// with `missing_edges=1 ... slot_page_ever_dirty=false`. The barrier did not +/// fire, because another thread's `GeneratedWriteBarrierTestGuard::inactive()` +/// had zeroed the flag mid-test. +#[test] +fn the_generated_write_barrier_flag_is_not_zeroed_by_another_thread() { + super::super::barrier::js_gc_write_barriers_emitted(1); + assert!( + super::super::barrier::generated_write_barriers_emitted(), + "the probe did not arm the barrier flag, so its verdict would be vacuous" + ); + // Exactly what `GeneratedWriteBarrierTestGuard::inactive()` does, on the + // thread the real guard would have run on. + std::thread::spawn(|| super::super::barrier::js_gc_write_barriers_emitted(0)) + .join() + .expect("the clearing thread panicked"); + let still_active = super::super::barrier::generated_write_barriers_emitted(); + super::super::barrier::js_gc_write_barriers_emitted(0); + assert!( + still_active, + "another thread's write-barrier guard silenced this thread's runtime \ + barrier (#7672). A store under a test that relies on the barrier then \ + dirties no page, and the failure reads as a missing remembered-set \ + edge rather than as a disabled barrier." + ); +} + +/// `tui::tree`'s `NEXT_HANDLE` / `REGISTRY` have no clear and no lock, and +/// `register_increments_handle` asserts `h2 == h1 + 1` plus an exact registry +/// length. Any concurrent `register()` breaks both. Observed at 1 run in 22. +#[test] +fn tui_tree_handles_are_not_perturbed_by_another_thread() { + fn text_node() -> crate::tui::tree::Node { + crate::tui::tree::Node::Text { + content: "probe7672".to_string(), + fg: crate::tui::color::Color::Default, + bg: crate::tui::color::Color::Default, + style: crate::tui::cell::Style::default(), + } + } + let h1 = crate::tui::tree::register(text_node()); + std::thread::spawn(|| { + for _ in 0..64 { + crate::tui::tree::register(text_node()); + } + }) + .join() + .expect("the interfering thread panicked"); + let h2 = crate::tui::tree::register(text_node()); + assert_eq!( + h2, + h1 + 1, + "another thread's `register()` consumed handles out of this test's \ + sequence (#7672): the tui handle table is process-global and nothing \ + serializes it." + ); +} + +// --------------------------------------------------------------------------- +// The probe list may not rot: every `clear_helper` named here must still be +// called by the guards' reset, and the reset's source is the authority. +// --------------------------------------------------------------------------- + +/// Every `test_clear_*` helper this file claims to cover. +const COVERED_CLEAR_HELPERS: &[&str] = &[ + "test_clear_closure_side_tables", + "test_clear_symbol_side_table_roots", + "test_clear_all_timer_scanner_roots", + "test_clear_class_side_table_roots", + "test_clear_object_cache_roots", + "test_clear_geisterhand_roots", + "test_clear_ui_text_registry_roots", +]; + +#[test] +fn every_covered_clear_helper_is_still_called_by_the_guards() { + // Compile-time include: a probe for a helper that no longer runs proves + // nothing, and would sit green forever. + let support = include_str!("support.rs"); + let body = support + .split_once("fn reset_copying_nursery_runtime_test_state()") + .expect("the guards' reset function was renamed — update this test") + .1; + let body = body.split_once("\n}\n").expect("unterminated reset fn").0; + assert!( + body.contains("test_clear_closure_side_tables"), + "sanity: the extracted reset body does not contain a call it certainly \ + makes, so the extraction is broken and this check is vacuous" + ); + for helper in COVERED_CLEAR_HELPERS { + assert!( + body.contains(helper), + "{helper} is probed in this file but is no longer called by \ + reset_copying_nursery_runtime_test_state — the probe has rotted" + ); + } +} diff --git a/crates/perry-runtime/src/gc/tests/mod.rs b/crates/perry-runtime/src/gc/tests/mod.rs index 9e75961edc..f33275515b 100644 --- a/crates/perry-runtime/src/gc/tests/mod.rs +++ b/crates/perry-runtime/src/gc/tests/mod.rs @@ -16,6 +16,7 @@ mod evacuation; mod fromspace_protect; mod fromspace_scan; mod global_bootstrap; +mod global_sink_isolation; mod helper_stores; mod host_safepoints; mod incremental_sweep_reclaim; diff --git a/crates/perry-runtime/src/gc/tests/support.rs b/crates/perry-runtime/src/gc/tests/support.rs index cbec3ba87f..4f35684a05 100644 --- a/crates/perry-runtime/src/gc/tests/support.rs +++ b/crates/perry-runtime/src/gc/tests/support.rs @@ -352,7 +352,7 @@ pub(super) struct CopyingNurseryTestGuard { _lock: std::sync::MutexGuard<'static, ()>, } -fn reset_copying_nursery_runtime_test_state() { +pub(super) fn reset_copying_nursery_runtime_test_state() { // Age-sensitive tests assume the power-on tenuring threshold (promote at // the 4th survival); pin it so a heavy-influx test earlier on the same // thread cannot leak a lowered adaptive threshold in. diff --git a/crates/perry-runtime/src/geisterhand_registry.rs b/crates/perry-runtime/src/geisterhand_registry.rs index 7e4172eb85..b19b32c315 100644 --- a/crates/perry-runtime/src/geisterhand_registry.rs +++ b/crates/perry-runtime/src/geisterhand_registry.rs @@ -86,8 +86,10 @@ pub const STYLE_PADDING_UNIFORM: u32 = 7; pub const STYLE_HIDDEN: u32 = 8; pub const STYLE_ENABLED: u32 = 9; -static REGISTRY: Mutex> = Mutex::new(Vec::new()); -static PENDING_ACTIONS: Mutex> = Mutex::new(Vec::new()); +per_test_global! { + static REGISTRY: Mutex> = Mutex::new(Vec::new()); + static PENDING_ACTIONS: Mutex> = Mutex::new(Vec::new()); +} /// Screenshot result buffer: shared between main thread (writer) and HTTP server (reader). /// The main thread captures the screenshot and writes PNG bytes here, then signals the condvar. diff --git a/crates/perry-runtime/src/lib.rs b/crates/perry-runtime/src/lib.rs index a1d3b268ee..79df150444 100644 --- a/crates/perry-runtime/src/lib.rs +++ b/crates/perry-runtime/src/lib.rs @@ -32,6 +32,11 @@ static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; #[global_allocator] static GLOBAL: std::alloc::System = std::alloc::System; +// Declared FIRST and with `#[macro_use]`: `per_test_global!` has to be in +// scope for every module below it. See its module docs for #7672. +#[macro_use] +pub mod per_test_global; + pub mod abi_trampoline; pub mod agent; #[cfg(test)] diff --git a/crates/perry-runtime/src/object/class_registry/prototype_methods.rs b/crates/perry-runtime/src/object/class_registry/prototype_methods.rs index 0361d0f4f9..5a12d4c07d 100644 --- a/crates/perry-runtime/src/object/class_registry/prototype_methods.rs +++ b/crates/perry-runtime/src/object/class_registry/prototype_methods.rs @@ -25,33 +25,35 @@ pub unsafe extern "C" fn js_class_register_static_field( class_dynamic_prop_root_store(class_id, name, value); } -/// Issue #838: JS-classic prototype method assignment. -/// -/// `Class.prototype.method = function() {…}` (and the aliased form -/// `var p = Class.prototype; p.method = function() {…}`) is a pre-ES6 -/// idiom dayjs, chalk, and a long tail of libraries still ship. -/// Pre-fix the assignment was lowered to a generic `PropertySet` whose -/// receiver evaluated to a class-prototype-shaped object that nothing -/// downstream consulted, so `(new Class()).method` came back as -/// `undefined`. -/// -/// The HIR-level fix routes recognised shapes to -/// `js_register_prototype_method(class_id, name, value)`, which stores -/// the closure value into a per-class side-table here. The dispatch -/// hot paths (`js_object_get_field_by_name` for `inst.method` reads -/// and `js_native_call_method` for `inst.method(...)` calls) consult -/// this table after the regular vtable / proto-object lookups miss, -/// invoking the closure with `this` bound to the receiver. -/// -/// Stored values use their full NaN-boxed bits (f64) — typically a -/// POINTER_TAG'd closure, but the dispatch path treats whatever is -/// stored as a callable value and routes it through -/// `js_native_call_value`, which itself accepts both closures and raw -/// `*ClosureHeader` shapes. -pub static CLASS_PROTOTYPE_METHODS: RwLock>>> = - RwLock::new(None); -pub(crate) static CLASS_PROTOTYPE_FAST_GUARDS_INVALIDATED: std::sync::atomic::AtomicBool = - std::sync::atomic::AtomicBool::new(false); +per_test_global! { + /// Issue #838: JS-classic prototype method assignment. + /// + /// `Class.prototype.method = function() {…}` (and the aliased form + /// `var p = Class.prototype; p.method = function() {…}`) is a pre-ES6 + /// idiom dayjs, chalk, and a long tail of libraries still ship. + /// Pre-fix the assignment was lowered to a generic `PropertySet` whose + /// receiver evaluated to a class-prototype-shaped object that nothing + /// downstream consulted, so `(new Class()).method` came back as + /// `undefined`. + /// + /// The HIR-level fix routes recognised shapes to + /// `js_register_prototype_method(class_id, name, value)`, which stores + /// the closure value into a per-class side-table here. The dispatch + /// hot paths (`js_object_get_field_by_name` for `inst.method` reads + /// and `js_native_call_method` for `inst.method(...)` calls) consult + /// this table after the regular vtable / proto-object lookups miss, + /// invoking the closure with `this` bound to the receiver. + /// + /// Stored values use their full NaN-boxed bits (f64) — typically a + /// POINTER_TAG'd closure, but the dispatch path treats whatever is + /// stored as a callable value and routes it through + /// `js_native_call_value`, which itself accepts both closures and raw + /// `*ClosureHeader` shapes. + pub static CLASS_PROTOTYPE_METHODS: RwLock>>> = + RwLock::new(None); + pub(crate) static CLASS_PROTOTYPE_FAST_GUARDS_INVALIDATED: std::sync::atomic::AtomicBool = + std::sync::atomic::AtomicBool::new(false); +} pub(crate) fn class_prototype_fast_guards_invalidated() -> bool { CLASS_PROTOTYPE_FAST_GUARDS_INVALIDATED.load(std::sync::atomic::Ordering::Acquire) diff --git a/crates/perry-runtime/src/object/class_registry/prototype_objects.rs b/crates/perry-runtime/src/object/class_registry/prototype_objects.rs index 92b066ed70..b18879bebd 100644 --- a/crates/perry-runtime/src/object/class_registry/prototype_objects.rs +++ b/crates/perry-runtime/src/object/class_registry/prototype_objects.rs @@ -116,13 +116,15 @@ pub(crate) fn ensure_function_prototype_object( proto } -/// Synthetic class id allocator for prototype-object classes. High bit -/// set (0x8000_0000+) to keep them separate from codegen-assigned ids -/// (which start from 1 and grow by module). u32 wraparound is not a -/// concern in practice — would require ~2 billion `Function.prototype = X` -/// statements at module init. -pub static NEXT_SYNTHETIC_CLASS_ID: std::sync::atomic::AtomicU32 = - std::sync::atomic::AtomicU32::new(0x8000_0000); +per_test_global! { + /// Synthetic class id allocator for prototype-object classes. High bit + /// set (0x8000_0000+) to keep them separate from codegen-assigned ids + /// (which start from 1 and grow by module). u32 wraparound is not a + /// concern in practice — would require ~2 billion `Function.prototype = X` + /// statements at module init. + pub static NEXT_SYNTHETIC_CLASS_ID: std::sync::atomic::AtomicU32 = + std::sync::atomic::AtomicU32::new(0x8000_0000); +} /// Register a function's prototype object. Called by codegen-emitted /// init code whenever the HIR detects `.prototype = ` at diff --git a/crates/perry-runtime/src/object/class_registry/state.rs b/crates/perry-runtime/src/object/class_registry/state.rs index f411054474..008dbc61e3 100644 --- a/crates/perry-runtime/src/object/class_registry/state.rs +++ b/crates/perry-runtime/src/object/class_registry/state.rs @@ -179,8 +179,10 @@ pub static CLASS_VTABLE_REGISTRY: RwLock>> = Rw pub static CLASS_STATIC_METHODS: RwLock>>> = RwLock::new(None); -pub static CLASS_STATIC_ACCESSORS: RwLock>>> = - RwLock::new(None); +per_test_global! { + pub static CLASS_STATIC_ACCESSORS: RwLock>>> = + RwLock::new(None); +} /// Spec `Function.prototype.length` per (class_id, method/accessor name) — the /// count of formal parameters before the first one with a default or a rest. @@ -199,40 +201,46 @@ pub static CLASS_METHOD_BIND_LENGTHS: RwLock> pub static CLASS_STATIC_METHOD_BIND_LENGTHS: RwLock>> = RwLock::new(None); -pub static CLASS_SYMBOL_METHODS: RwLock>> = - RwLock::new(None); +per_test_global! { + pub static CLASS_SYMBOL_METHODS: RwLock>> = + RwLock::new(None); -pub static CLASS_SYMBOL_ACCESSORS: RwLock>> = - RwLock::new(None); + pub static CLASS_SYMBOL_ACCESSORS: RwLock>> = + RwLock::new(None); +} /// Set of all registered class ids. Populated at module init by codegen /// emitting `js_register_class_id(cid)` for every user class — even /// classes without any methods. Refs #618 / #420 followup. pub static REGISTERED_CLASS_IDS: RwLock>> = RwLock::new(None); -/// Issue #711 part 2: `function Base() {}; Base.prototype = obj` pattern. -/// Effect's `internal/effectable.ts` declares classes via prototype -/// assignment on a plain function, not via `class` syntax. To make -/// `class Derived extends Base {}` walk into `obj`'s methods at dispatch -/// time, we model this as a synthetic class: -/// - `js_set_function_prototype(func, obj)` allocates a synthetic -/// class_id (high-bit-set to avoid collision with codegen-assigned -/// ids), stores `func_bits → synthetic_cid` in `FUNCTION_CLASS_IDS`, -/// and `synthetic_cid → obj_ptr` in `CLASS_PROTOTYPE_OBJECTS`. -/// - `js_register_class_parent_dynamic` extends to detect closure -/// parent values, looks up the synthetic class_id, and registers -/// the (child, synthetic) edge in CLASS_REGISTRY. -/// - The method-dispatch chain walk in `js_native_call_method` -/// consults `CLASS_PROTOTYPE_OBJECTS` when it reaches a synthetic -/// class_id: it resolves the method as a regular field lookup on -/// the prototype object and calls it with `this` bound to the -/// receiver. -pub static FUNCTION_CLASS_IDS: RwLock>> = RwLock::new(None); +per_test_global! { + /// Issue #711 part 2: `function Base() {}; Base.prototype = obj` pattern. + /// Effect's `internal/effectable.ts` declares classes via prototype + /// assignment on a plain function, not via `class` syntax. To make + /// `class Derived extends Base {}` walk into `obj`'s methods at dispatch + /// time, we model this as a synthetic class: + /// - `js_set_function_prototype(func, obj)` allocates a synthetic + /// class_id (high-bit-set to avoid collision with codegen-assigned + /// ids), stores `func_bits → synthetic_cid` in `FUNCTION_CLASS_IDS`, + /// and `synthetic_cid → obj_ptr` in `CLASS_PROTOTYPE_OBJECTS`. + /// - `js_register_class_parent_dynamic` extends to detect closure + /// parent values, looks up the synthetic class_id, and registers + /// the (child, synthetic) edge in CLASS_REGISTRY. + /// - The method-dispatch chain walk in `js_native_call_method` + /// consults `CLASS_PROTOTYPE_OBJECTS` when it reaches a synthetic + /// class_id: it resolves the method as a regular field lookup on + /// the prototype object and calls it with `this` bound to the + /// receiver. + pub static FUNCTION_CLASS_IDS: RwLock>> = RwLock::new(None); +} // Stored as `usize` (raw address) so the map is Send + Sync. The // pointer is always converted back to `*mut ObjectHeader` at call sites // (`class_prototype_object` / the dispatch walk) where single-threaded // usage is guaranteed. -pub static CLASS_PROTOTYPE_OBJECTS: RwLock>> = RwLock::new(None); +per_test_global! { + pub static CLASS_PROTOTYPE_OBJECTS: RwLock>> = RwLock::new(None); +} /// Lazily materialized `Class.prototype` objects for declared ES classes. /// These are separate from `CLASS_PROTOTYPE_OBJECTS`: that older table is @@ -285,17 +293,19 @@ pub(crate) fn class_prototype_method_is_enumerable(class_id: u32, name: &str) -> true } -/// #36 / #321: maps a child class_id to the raw address of a parent CLOSURE -/// (function value) when `class Child extends {}`. effect's -/// `class Svc extends Context.Tag("Svc")<...>() {}` extends the function -/// `TagClass` returned by `Tag(id)()`. In JS this sets `Svc.__proto__ = -/// TagClass` so static-property reads on `Svc` (`Svc.key`, `Svc._op`, -/// `Svc[TagTypeId]`) walk to the parent function's own props + ITS static -/// prototype. Perry's existing dynamic-parent path only models OBJECT parents -/// (class-expression values), so this records the closure-parent axis so the -/// class-ref static getters can reach the closure's props and proto chain. -/// Stored as `usize` (raw address) for Send + Sync; converted back at use. -pub static CLASS_PARENT_CLOSURES: RwLock>> = RwLock::new(None); +per_test_global! { + /// #36 / #321: maps a child class_id to the raw address of a parent CLOSURE + /// (function value) when `class Child extends {}`. effect's + /// `class Svc extends Context.Tag("Svc")<...>() {}` extends the function + /// `TagClass` returned by `Tag(id)()`. In JS this sets `Svc.__proto__ = + /// TagClass` so static-property reads on `Svc` (`Svc.key`, `Svc._op`, + /// `Svc[TagTypeId]`) walk to the parent function's own props + ITS static + /// prototype. Perry's existing dynamic-parent path only models OBJECT parents + /// (class-expression values), so this records the closure-parent axis so the + /// class-ref static getters can reach the closure's props and proto chain. + /// Stored as `usize` (raw address) for Send + Sync; converted back at use. + pub static CLASS_PARENT_CLOSURES: RwLock>> = RwLock::new(None); +} /// Maps a child class_id to the raw NaN-boxed bits of the parent constructor /// VALUE that `js_register_class_parent_dynamic` evaluated at class-definition diff --git a/crates/perry-runtime/src/object/mod.rs b/crates/perry-runtime/src/object/mod.rs index d7ff54d129..f5cdb6cc85 100644 --- a/crates/perry-runtime/src/object/mod.rs +++ b/crates/perry-runtime/src/object/mod.rs @@ -207,15 +207,17 @@ pub(crate) use this_binding::{ pub use to_string_tag::js_object_to_string; pub(crate) use to_string_tag::typed_array_to_string_tag_name; -static HTTP_METHODS_CACHE: AtomicU64 = AtomicU64::new(0); -static FS_CONSTANTS_CACHE: AtomicU64 = AtomicU64::new(0); -static OS_CONSTANTS_CACHE: AtomicU64 = AtomicU64::new(0); -static OS_CONSTANTS_SIGNALS_CACHE: AtomicU64 = AtomicU64::new(0); -static OS_CONSTANTS_ERRNO_CACHE: AtomicU64 = AtomicU64::new(0); -static OS_CONSTANTS_PRIORITY_CACHE: AtomicU64 = AtomicU64::new(0); -static OS_CONSTANTS_DLOPEN_CACHE: AtomicU64 = AtomicU64::new(0); -static GLOBAL_THIS_PTR: AtomicI64 = AtomicI64::new(0); -static GLOBAL_THIS_READY: AtomicBool = AtomicBool::new(false); +per_test_global! { + static HTTP_METHODS_CACHE: AtomicU64 = AtomicU64::new(0); + static FS_CONSTANTS_CACHE: AtomicU64 = AtomicU64::new(0); + static OS_CONSTANTS_CACHE: AtomicU64 = AtomicU64::new(0); + static OS_CONSTANTS_SIGNALS_CACHE: AtomicU64 = AtomicU64::new(0); + static OS_CONSTANTS_ERRNO_CACHE: AtomicU64 = AtomicU64::new(0); + static OS_CONSTANTS_PRIORITY_CACHE: AtomicU64 = AtomicU64::new(0); + static OS_CONSTANTS_DLOPEN_CACHE: AtomicU64 = AtomicU64::new(0); + static GLOBAL_THIS_PTR: AtomicI64 = AtomicI64::new(0); + static GLOBAL_THIS_READY: AtomicBool = AtomicBool::new(false); +} // `%TypedArray%` intrinsic constructor/prototype roots used by per-kind typed // array constructors and scanned by `scan_object_cache_roots_mut`. pub(crate) static TYPED_ARRAY_INTRINSIC_PTR: AtomicI64 = AtomicI64::new(0); diff --git a/crates/perry-runtime/src/per_test_global.rs b/crates/perry-runtime/src/per_test_global.rs new file mode 100644 index 0000000000..90d976443d --- /dev/null +++ b/crates/perry-runtime/src/per_test_global.rs @@ -0,0 +1,175 @@ +//! `per_test_global!` — a process-global table whose test-time isolation no +//! lock enforces, declared so that one test cannot reach another test's copy. +//! +//! The macro is named for what it *does* (a per-test instance in a test build) +//! rather than for the sharpest instance of the hazard (the GC guards' clear), +//! because two of its residents are not cleared by anything: +//! +//! * `tui::tree`'s `NEXT_HANDLE` / `REGISTRY` — no clear at all, just two +//! process-global counters and `assert_eq!(h2, h1 + 1)`. A concurrent +//! `register()` on any other test thread breaks it. +//! * `gc::barrier`'s `GENERATED_WRITE_BARRIERS_EMITTED` — owned by TWO guards +//! under TWO DIFFERENT locks, and read by tests holding neither. +//! +//! Both of those failed inside a 22-run soak of the fix for the first case, and +//! both were diagnosed from the wrong VALUE rather than the timing (a +//! non-sequential handle; `slot_page_ever_dirty=false`, i.e. the barrier did not +//! fire). Naming the macro after the clear would have made them look like +//! misfits rather than the same defect. +//! +//! # The defect this exists to make structurally impossible (#7672) +//! +//! `gc::tests::support::reset_copying_nursery_runtime_test_state()` runs from +//! `GcTestIsolationGuard` and `CopyingNurseryTestGuard`, and calls ~20 +//! `test_clear_*` helpers so the GC test that constructed the guard sees +//! exactly the roots it installs. Those helpers `clear()` PROCESS-global +//! tables, from whatever libtest thread happens to be running the guard. +//! +//! The guards serialize against *each other* (`global_side_table_test_lock`), +//! and against the handful of tests that remember to take that lock. Nothing +//! requires a *reader* to take it — the defence is opt-in and the opt-in is +//! invisible at the read site. So a test on thread T that writes an entry and +//! reads it back a few statements later has its entry deleted, mid-test, by a +//! guard on thread U, and observes a silently wrong value. +//! +//! Three flakes in two days came from exactly this, each diagnosed from the +//! wrong VALUE rather than the timing: +//! +//! | fixed in | table | presented as | +//! |---|---|---| +//! | #7665 | `opt_report`'s row sink | `rows.len() == 2` failing at **3** | +//! | #7665 | `ext_registry`'s `USED_PROVIDERS` | "empty" failing with `ioredis` present | +//! | #7671 | `closure`'s `CLOSURE_PROPS` | a static method read back as `TAG_UNDEFINED` | +//! +//! # Why the fix is per-thread storage and not a lock +//! +//! The damage window is "between this test's write and this test's read", and +//! only the test knows that span. An accessor that takes a shared lock for the +//! duration of one call does not cover it; a lock that covers it has to be +//! taken by the test, which is the opt-in the class is made of. Blanket-locking +//! the ~180 tests that read these tables serializes a large part of the suite +//! for a hazard whose incidence nobody can bound. +//! +//! So the storage moves instead. In a **test build** each thread gets its own +//! instance of the table, which is exactly the isolation the clear was reaching +//! for: `reset_copying_nursery_runtime_test_state()` on thread U empties U's +//! instance, which already contains only U's entries, and thread T's test is +//! structurally out of reach. libtest runs one thread per test, so "per thread" +//! and "per test" coincide. +//! +//! In a **non-test build** the macro expands to the plain `static` it replaced, +//! byte for byte. `test_clear_*` is `#[cfg(test)]` and never runs in a shipped +//! runtime, so the whole hazard — and the whole fix — is confined to tests. +//! +//! Call sites do not change: [`PerThread`] derefs to the table, so +//! `SYMBOL_REGISTRY.lock()` and `CLASS_PROTOTYPE_OBJECTS.read()` keep working +//! as written. +//! +//! # What holds the line +//! +//! * `scripts/global_sink_isolation.py` (runs in `lint`) derives the clear list +//! from `reset_copying_nursery_runtime_test_state`'s own source, resolves the +//! storage behind each helper, and FAILS on a bare `static` that is neither +//! thread-local nor declared here. A new sink cannot be added quietly, and a +//! new *reader* never has to remember anything. +//! * `gc::tests::global_sink_isolation` plants the #7671 shape — write on this +//! thread, run the guards' clear on another, read back — for each converted +//! table, and proves the harness still catches an unconverted one via a +//! deliberately bare canary. A green run means the detector works, not that +//! nothing was tried. + +/// Per-thread instance of a process-global side table, for test builds. +/// +/// Constructed only by [`per_test_global!`]. Derefs to `T`, so every call +/// site that used the `static` directly is unchanged. +/// +/// The instance is leaked (`Box::leak`) rather than dropped at thread exit: +/// `Deref::deref` must hand out a reference that outlives the borrow of the +/// `static`, and a thread-exit drop would dangle it. The cost is bounded by +/// (threads that touched the table) x (empty table size); a libtest thread that +/// never touches a table never allocates one. +#[cfg(test)] +pub struct PerThread { + init: fn() -> T, +} + +#[cfg(test)] +impl PerThread { + pub const fn new(init: fn() -> T) -> Self { + Self { init } + } + + fn instance(&self) -> &'static T { + let key = self as *const Self as usize; + // A thread-local map, not a global one: a global would reintroduce a + // process-wide lock on a path taken by every side-table access in the + // test build. + let cached = SLOTS.try_with(|slots| slots.borrow().get(&key).copied()); + if let Ok(Some(addr)) = cached { + // SAFETY: `addr` was produced below from a `Box::leak` of `T` on + // this thread and is never removed, so it is live for the process. + return unsafe { &*(addr as *const T) }; + } + let leaked: &'static T = Box::leak(Box::new((self.init)())); + // `try_with` fails only while this thread's TLS is being destroyed. A + // table touched from a TLS destructor gets a fresh instance rather than + // a panic; it is being torn down either way. + let _ = SLOTS.try_with(|slots| { + slots.borrow_mut().insert(key, leaked as *const T as usize); + }); + leaked + } +} + +#[cfg(test)] +impl std::ops::Deref for PerThread { + type Target = T; + + fn deref(&self) -> &T { + self.instance() + } +} + +// No `unsafe impl Sync` is needed and none is written: `PerThread` holds only +// a `fn() -> T`, so the auto impl already applies. Every thread observes only +// the instance it created and the instances are leaked, so no `T` crosses a +// thread boundary or is dropped through this type. `T: Send + Sync` is required +// regardless, because the non-test expansion is a plain `static`. + +#[cfg(test)] +thread_local! { + /// `&PerThread` address -> leaked `&'static T` address, for this thread. + static SLOTS: std::cell::RefCell> = + std::cell::RefCell::new(std::collections::HashMap::new()); +} + +/// Declare a process-global table that must be per-test in a test build. +/// +/// Expands to the plain `static` outside a test build, and to a +/// [`PerThread`] instance inside one. See the module docs for why. +/// +/// ```ignore +/// per_test_global! { +/// /// Doc comments and attributes pass through. +/// static SYMBOL_REGISTRY: Mutex>> = Mutex::new(None); +/// } +/// ``` +/// The trailing `;` is optional so a single-table declaration can be written +/// `per_test_global!(static X: T = init)` on one line — `timer.rs` sits +/// three lines under the 2000-line cap (`scripts/check_file_size.sh`) and the +/// block form would push it over. +macro_rules! per_test_global { + ($( + $(#[$attr:meta])* + $vis:vis static $name:ident : $ty:ty = $init:expr + );+ $(;)?) => {$( + #[cfg(not(test))] + $(#[$attr])* + $vis static $name: $ty = $init; + + #[cfg(test)] + $(#[$attr])* + $vis static $name: $crate::per_test_global::PerThread<$ty> = + $crate::per_test_global::PerThread::new(|| $init); + )+}; +} diff --git a/crates/perry-runtime/src/symbol.rs b/crates/perry-runtime/src/symbol.rs index 5b600847f9..938438f87c 100644 --- a/crates/perry-runtime/src/symbol.rs +++ b/crates/perry-runtime/src/symbol.rs @@ -115,7 +115,9 @@ static SYMBOL_REGISTRY: Mutex>> = Mutex::new(None) // `is_registered_symbol` so the runtime's property/method dispatch can // detect symbol pointers safely without reading the (possibly nonexistent) // GcHeader byte. -static SYMBOL_POINTERS: Mutex>> = Mutex::new(None); +per_test_global! { + static SYMBOL_POINTERS: Mutex>> = Mutex::new(None); +} /// Process-lifetime descriptions for registered (`Symbol.for`) and well-known /// symbols. These symbols are Box-leaked so they outlive every GC cycle, but @@ -287,13 +289,17 @@ pub(crate) fn is_global_registered_symbol(ptr: usize) -> bool { // Symbol-keyed property side tables. Object keys are metadata-only and get // rewritten when owners move; symbol keys and NaN-boxed values are GC roots. // Storage stays intentionally linear because per-object symbol keys are rare. -static SYMBOL_PROPERTIES: Mutex>>> = Mutex::new(None); +per_test_global! { + static SYMBOL_PROPERTIES: Mutex>>> = Mutex::new(None); +} // Descriptor attributes for symbol-keyed properties installed through // Object.defineProperty. Direct symbol assignment uses the normal data-property // defaults, so absence here means writable/enumerable/configurable are all true. -static SYMBOL_PROPERTY_ATTRS: Mutex>> = - Mutex::new(None); +per_test_global! { + static SYMBOL_PROPERTY_ATTRS: Mutex>> = + Mutex::new(None); +} /// Death pruning for the symbol-keyed property side tables (2026-07-09 GC /// audit wave 2). Both tables are PROCESS-global and owner-keyed; the values @@ -558,13 +564,15 @@ pub(crate) fn store_class_static_symbol_root(class_id: u32, sym_key: usize, valu publish_symbol_side_table_root_edges(sym_key, value_bits); } -/// Class-id-keyed side table for static Symbol-keyed properties. -/// drizzle's `static [entityKind] = "Table"` registers -/// (class_id, sym_ptr) → value here at module init via -/// `js_class_register_static_symbol`. Consulted by `js_object_has_own` -/// when the receiver is a class identifier (NaN-boxed INT32_TAG). -/// Refs #420. -static CLASS_STATIC_SYMBOLS: Mutex>> = Mutex::new(None); +per_test_global! { + /// Class-id-keyed side table for static Symbol-keyed properties. + /// drizzle's `static [entityKind] = "Table"` registers + /// (class_id, sym_ptr) → value here at module init via + /// `js_class_register_static_symbol`. Consulted by `js_object_has_own` + /// when the receiver is a class identifier (NaN-boxed INT32_TAG). + /// Refs #420. + static CLASS_STATIC_SYMBOLS: Mutex>> = Mutex::new(None); +} #[cfg(test)] mod wellknown_desc_tests { diff --git a/crates/perry-runtime/src/symbol/accessors.rs b/crates/perry-runtime/src/symbol/accessors.rs index 42105bc059..c31dbb84f7 100644 --- a/crates/perry-runtime/src/symbol/accessors.rs +++ b/crates/perry-runtime/src/symbol/accessors.rs @@ -12,9 +12,11 @@ pub(super) struct SymbolAccessorDescriptor { pub(super) set: u64, } -static SYMBOL_ACCESSOR_PROPERTIES: Mutex< - Option>, -> = Mutex::new(None); +per_test_global! { + static SYMBOL_ACCESSOR_PROPERTIES: Mutex< + Option>, + > = Mutex::new(None); +} pub(super) fn clear_symbol_accessor_property(obj_key: usize, sym_key: usize) { let mut guard = crate::gc::lock_gc_root_registry(&SYMBOL_ACCESSOR_PROPERTIES); diff --git a/crates/perry-runtime/src/timer.rs b/crates/perry-runtime/src/timer.rs index 7c84324fcd..aa9de333ff 100644 --- a/crates/perry-runtime/src/timer.rs +++ b/crates/perry-runtime/src/timer.rs @@ -42,7 +42,7 @@ struct Timer { unsafe impl Send for Timer {} // Global timer queues (Mutex-protected for cross-thread access) -static TIMER_QUEUE: Mutex> = Mutex::new(Vec::new()); +per_test_global!(static TIMER_QUEUE: Mutex> = Mutex::new(Vec::new())); static START_TIME: Mutex> = Mutex::new(None); /// Initialize the timer system (called once at startup) @@ -374,7 +374,7 @@ static MOCK_TIMERS: Mutex = Mutex::new(MockTimersState { intervals: Vec::new(), }); -static CALLBACK_TIMERS: Mutex> = Mutex::new(Vec::new()); +per_test_global!(static CALLBACK_TIMERS: Mutex> = Mutex::new(Vec::new())); // Shared id counter across callback timers AND intervals so a handle id is // globally unique. Node treats Timeout/Interval as the same internal Timer // type, so `clearTimeout(intervalHandle)` and `clearInterval(timeoutHandle)` @@ -1402,7 +1402,7 @@ struct IntervalTimer { // what makes the cross-thread pointers here sound. unsafe impl Send for IntervalTimer {} -static INTERVAL_TIMERS: Mutex> = Mutex::new(Vec::new()); +per_test_global!(static INTERVAL_TIMERS: Mutex> = Mutex::new(Vec::new())); /// JS-style setInterval that takes a callback function and interval /// The callback is a closure pointer that will be called repeatedly diff --git a/crates/perry-runtime/src/tui/tree.rs b/crates/perry-runtime/src/tui/tree.rs index 5f4b5da183..37025706a0 100644 --- a/crates/perry-runtime/src/tui/tree.rs +++ b/crates/perry-runtime/src/tui/tree.rs @@ -43,8 +43,10 @@ pub enum Node { /// through the table take the lock for the duration of a get/set. The /// lock isn't on the hot path (FFI calls fire on the main thread once /// per render) so a plain Mutex is fine. -static NEXT_HANDLE: AtomicI64 = AtomicI64::new(1); -static REGISTRY: Mutex> = Mutex::new(Vec::new()); +per_test_global! { + static NEXT_HANDLE: AtomicI64 = AtomicI64::new(1); + static REGISTRY: Mutex> = Mutex::new(Vec::new()); +} /// Register a freshly-built node and return its handle. pub fn register(node: Node) -> i64 { diff --git a/crates/perry-runtime/src/ui_text_registry.rs b/crates/perry-runtime/src/ui_text_registry.rs index bdd54eb261..38305f8ddc 100644 --- a/crates/perry-runtime/src/ui_text_registry.rs +++ b/crates/perry-runtime/src/ui_text_registry.rs @@ -231,7 +231,9 @@ pub extern "C" fn perry_arkts_set_text(id_handle: f64, val_handle: f64) { // Issue #535 — `perry/ui` `state` runtime registry. // ============================================================================= -static STATE_VALUES: Mutex>> = Mutex::new(None); +per_test_global! { + static STATE_VALUES: Mutex>> = Mutex::new(None); +} fn with_state_values(f: F) -> R where @@ -444,8 +446,10 @@ struct ForEachBinding { render_closure: f64, } -static FOREACH_REGISTRY: Mutex>>> = - Mutex::new(None); +per_test_global! { + static FOREACH_REGISTRY: Mutex>>> = + Mutex::new(None); +} /// GC root scanner for cross-platform UI state values and `ForEach` render /// callbacks held in Rust registries outside the managed heap. diff --git a/scripts/global_sink_isolation.py b/scripts/global_sink_isolation.py new file mode 100644 index 0000000000..0289c39f97 --- /dev/null +++ b/scripts/global_sink_isolation.py @@ -0,0 +1,445 @@ +#!/usr/bin/env python3 +"""Every table the GC test guards CLEAR must be out of reach of another test. + +WHY THIS EXISTS (#7672) +----------------------- +`gc::tests::support::reset_copying_nursery_runtime_test_state()` runs from +`GcTestIsolationGuard` and `CopyingNurseryTestGuard`, on whatever libtest thread +happens to construct one, and calls ~20 `test_clear_*` helpers that empty +PROCESS-global side tables. The guards serialize against each other, and against +the handful of tests that remember to take +`crate::gc::global_side_table_test_lock()`. Nothing requires a *reader* to take +it, so the defence is opt-in and the opt-in is invisible at the read site. + +Three flakes in two days came from exactly that, each exposed by an unrelated PR +that changed the parallel schedule, and each diagnosed from the wrong VALUE +rather than the timing: + + #7665 opt_report's row sink `rows.len() == 2` failed at 3 + #7665 ext_registry USED_PROVIDERS "empty" failed with `ioredis` present + #7671 closure CLOSURE_PROPS a static method read back TAG_UNDEFINED + +The fix is per-thread storage in test builds (`per_test_global!`), because +the damage window is "between this test's write and this test's read" and only +the test knows that span — a lock the accessor takes for one call does not cover +it, and a lock the test takes is the opt-in the class is made of. + +WHAT THIS SCRIPT ASSERTS +------------------------ +It derives the clear list from the guards' own source, resolves the storage +behind every helper, and classifies each `static` it writes to: + + * `thread_local!` -> safe by construction + * `per_test_global!` -> per-thread in test builds, safe + * a bare `static` -> HAZARD + +A hazard fails the build unless it is named in ALLOWLIST below with the issue +that blocks it — and an allowlist entry that matches nothing ALSO fails, so an +entry cannot outlive its cause. The burden lands on the ~20 table authors, who +are finite and gated, instead of on the ~180 readers, who are not. + +USAGE +----- + python3 scripts/global_sink_isolation.py # the gate + python3 scripts/global_sink_isolation.py --self-test # proves it can fail +""" + +from __future__ import annotations + +import argparse +import re +import sys +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +RUNTIME_SRC = REPO_ROOT / "crates" / "perry-runtime" / "src" +SUPPORT = RUNTIME_SRC / "gc" / "tests" / "support.rs" +RESET_FN = "reset_copying_nursery_runtime_test_state" +# 93 classify today; the floor only has to be high enough that a broken +# matcher cannot pass as a clean tree. +CLASSIFIED_FLOOR = 60 + +# name -> issue that blocks converting it. An entry matching nothing FAILS. +ALLOWLIST = { + # #7645 made this latch deliberately process-wide and monotone: one earlier + # pinning test must leave every later copying test on the same side of the + # preflight, or the skip path is masked entirely. It has no reader outside + # the guard, so it cannot damage another test's assertion. + "YOUNG_PIN_EVER": "#7645", + # Read, never written, by `test_clear_symbol_side_table_roots`: these two are + # the process-lifetime registries the per-thread `SYMBOL_POINTERS` rebuild is + # derived FROM. Their symbols are `Box::leak`ed, so a process-wide identity + # is the correct one — `Symbol.for("x") === Symbol.for("x")` depends on it. + "SYMBOL_REGISTRY": "#7672", + "WELL_KNOWN_SYMBOLS": "#7672", + # `plugin::REGISTRY` is the one table already defended the sound way, and + # the survey behind #7672 called it the model: the guard takes + # `PLUGIN_REGISTRY_TEST_LOCK` (support.rs) and every plugin test takes the + # same lock, so the clear and the readers share one lock domain. The split + # lock domain is the root cause in all three fixed flakes; this one is not + # split. + "REGISTRY": "#7672", + # A monotone unique-id source for test fixtures (`young_leaf_{id:x}` names, + # synthetic symbol ids). Nothing reads its VALUE back and nothing asserts on + # it; making it per-thread would only weaken the uniqueness it exists for. + "YOUNG_LEAF_COUNTER": "#7672", +} + + +class Violation(Exception): + pass + + +def rel(path) -> str: + """Repo-relative path, tolerant of the self-test's synthetic sources.""" + if path is None: + return "?" + try: + return str(Path(path).relative_to(REPO_ROOT)) + except ValueError: + return str(path) + + +PAIRS = {"{": "}", "(": ")", "[": "]"} + + +def brace_body(text: str, open_idx: int, opener: str = "{") -> str: + """Body of the block whose `opener` is at/after `open_idx`.""" + closer = PAIRS[opener] + start = text.index(opener, open_idx) + depth = 0 + for i in range(start, len(text)): + if text[i] == opener: + depth += 1 + elif text[i] == closer: + depth -= 1 + if depth == 0: + return text[start + 1 : i] + raise Violation("unterminated block") + + +def reset_body(support_text: str) -> str: + match = re.search(r"fn\s+%s\s*\(\s*\)" % RESET_FN, support_text) + if not match: + raise Violation( + "could not find `fn %s` in %s. The clear list is derived from that " + "function; if it was renamed this gate is reading nothing." + % (RESET_FN, SUPPORT.name) + ) + return brace_body(support_text, match.end()) + + +def clear_helpers(body: str) -> list: + """`test_clear_*` / `test_set_*` / `reset_*` calls the guards' reset makes.""" + names = re.findall(r"::((?:test_clear|test_set|test_reset|reset)_[a-z0-9_]+)\s*\(", body) + if not names: + raise Violation( + "parsed zero clear helpers out of %s's body — the extraction is broken, " + "and a gate over an empty list cannot fail." % RESET_FN + ) + return sorted(set(names)) + + +def rust_sources() -> dict: + return {p: p.read_text() for p in RUNTIME_SRC.rglob("*.rs")} + + +def find_fn_body(sources: dict, name: str): + """(path, body) of the first `fn (` definition found.""" + pattern = re.compile(r"\bfn\s+" + re.escape(name) + r"\s*(<[^>]*>)?\s*\(") + for path, text in sources.items(): + match = pattern.search(text) + if match: + return path, brace_body(text, match.end()) + return None, None + + +def declaration_kind(sources: dict, ident: str, prefer=None): + """'thread_local' | 'per_test' | 'static' | None, plus where. + + `prefer` is the file that mentioned `ident`; Rust resolves a bare name in + its own module first, and several of these names (`REGISTRY`) exist in more + than one file. Searching the whole crate first named the wrong file and, for + `per_test_global!`-converted tables, the wrong VERDICT. + """ + static_re = re.compile( + r"^([ \t]*)(?:pub(?:\([^)]*\))?\s+)?static\s+" + re.escape(ident) + r"\s*:\s*([^=]*)=", re.M + ) + tls_re = re.compile(r"\b(?:pub(?:\([^)]*\))?\s+)?static\s+" + re.escape(ident) + r"\s*:") + inline_static_re = re.compile( + r"\b(?:pub(?:\([^)]*\))?\s+)?static\s+" + re.escape(ident) + r"\s*:\s*[^=]*=" + ) + ordered = list(sources.items()) + if prefer is not None and prefer in sources: + ordered.sort(key=lambda kv: 0 if kv[0] == prefer else 1) + for path, text in ordered: + # thread_local! { ... IDENT: ... } + for tls in re.finditer(r"thread_local!\s*\{", text): + body = brace_body(text, tls.end() - 1) + if tls_re.search(body): + return "thread_local", path + # per_test_global! { ... } AND per_test_global!( ... ) — `timer.rs` uses + # the paren form to stay under the 2000-line cap, and a `{`-only regex + # silently dropped its three tables to "(no static storage)". A gate that + # stops matching is a gate that stops failing. + for g in re.finditer(r"per_test_global!\s*([\{\(\[])", text): + body = brace_body(text, g.end() - 1, g.group(1)) + if static_re.search(body) or inline_static_re.search(body): + return "per_test", path + found = static_re.search(text) + if found: + declared = found.group(2).strip() + # A LOCK IS NOT DATA. `static X: Mutex<()>` carries no state; making + # it per-thread would turn it into a no-op, which is the opposite of + # the fix. Same for the global allocator. + if re.fullmatch(r"(std::sync::)?(Mutex|RwLock)\s*<\s*\(\s*\)\s*>", declared): + return "lock", path + if "#[global_allocator]" in text[max(0, found.start() - 80) : found.start()]: + return "allocator", path + return "static", path + return None, None + + +def audit(sources: dict, support_text: str, allowlist: dict, out=sys.stdout, floor=None): + body = reset_body(support_text) + helpers = clear_helpers(body) + violations = [] + hazards = {} + seen_idents = set() + + # The guards' own module is audited as if it were a helper. #7672's FIFTH + # instance was `GENERATED_WRITE_BARRIERS_EMITTED`, which no `test_clear_*` + # touches: two guards in this very file own it under two DIFFERENT locks + # (`copying_nursery_isolation_lock` and `GENERATED_BARRIER_TEST_LOCK`) and + # every runtime write barrier reads it holding neither. Deriving the audit + # set from the clear list alone could not see it, and it cost one red run in + # a 22-run soak before anyone looked. + subjects = [(helper, None) for helper in helpers] + subjects.append(("gc/tests/support.rs (the guards themselves)", (SUPPORT, support_text))) + + out.write( + "per-test global sinks (#7672): %d clear helper(s) reached from %s, " + "plus the guards' own module\n" % (len(helpers), RESET_FN) + ) + for helper, preloaded in subjects: + if preloaded is not None: + path, helper_body = preloaded + else: + path, helper_body = find_fn_body(sources, helper) + if helper_body is None: + violations.append( + "%s is called by %s but has no definition under crates/perry-runtime/src — " + "the gate cannot classify what it clears." % (helper, RESET_FN) + ) + continue + # Follow one level of same-file accessor calls. `test_clear_closure_ + # side_tables` names no static at all — it goes through + # `get_closure_props()` — so a body-only scan classified it as "no + # storage" and would not have noticed CLOSURE_PROPS reverting. + expanded = helper_body + for callee in sorted(set(re.findall(r"\b(get_[a-z0-9_]+)\s*\(", helper_body))): + callee_path, callee_body = find_fn_body({path: sources[path]}, callee) + if callee_body: + expanded += "\n" + callee_body + idents = sorted(set(re.findall(r"\b([A-Z][A-Z0-9_]{2,})\b", expanded))) + classified = [] + for ident in idents: + kind, decl = declaration_kind(sources, ident, prefer=path) + if kind is None: + continue # a constant, a type, an Ordering variant, ... + seen_idents.add(ident) + classified.append((ident, kind)) + if kind == "static" and ident not in allowlist: + hazards[ident] = (helper, decl) + out.write( + " %-40s %s\n" + % ( + helper, + ", ".join("%s=%s" % (i, k) for i, k in classified) or "(no static storage)", + ) + ) + + for ident, (helper, decl) in sorted(hazards.items()): + violations.append( + "%s is a BARE process-global `static` (%s), written by %s. One libtest " + "thread's test then reaches another's copy of it. Declare it " + "with `per_test_global!` (crates/perry-runtime/src/per_test_global.rs) " + "or, if it truly must stay process-wide, add it to ALLOWLIST in %s with the " + "issue that says why." + % ( + ident, + rel(decl), + helper, + Path(__file__).name, + ) + ) + + for ident, issue in sorted(allowlist.items()): + if ident not in seen_idents: + violations.append( + "ALLOWLIST names %r (%s), which is no longer written by any helper %s " + "calls. An entry that matches nothing hides nothing and outlives its " + "reason — delete it." % (ident, issue, RESET_FN) + ) + + out.write( + " -> %d hazard(s), %d allowlisted, %d static(s) classified\n" + % (len(hazards), len(allowlist), len(seen_idents)) + ) + # FLOOR. Every check in this file is a regex over Rust source, and the + # `per_test_global!(...)` paren form already slipped past a `{`-only pattern + # once, silently taking the three timer tables to "(no static storage)". A + # gate whose matcher rots reports zero hazards and exits 0, which is + # indistinguishable from a clean tree. Refuse to be that. + if floor is not None and len(seen_idents) < floor: + violations.append( + "only %d static(s) were classified, below the floor of %d. The source " + "matchers have stopped matching — this run proves nothing, and a zero " + "hazard count from it means nothing." % (len(seen_idents), floor) + ) + return violations + + +# --------------------------------------------------------------------------- +# Self-test: the gate must be able to fail, and must fail for the right reason. +# --------------------------------------------------------------------------- + +_FAKE_SUPPORT = """ +pub(super) fn reset_copying_nursery_runtime_test_state() { + crate::demo::test_clear_partitioned(); + crate::demo::test_clear_bare(); + crate::demo::test_clear_tls(); + crate::demo::test_clear_paren(); +} +""" + +_FAKE_SRC = """ +per_test_global! { + static PARTITIONED_TABLE: Mutex = Mutex::new(0); +} +per_test_global!(static PAREN_TABLE: Mutex = Mutex::new(0)); +static PURE_LOCK: Mutex<()> = Mutex::new(()); +static BARE_TABLE: Mutex = Mutex::new(0); +thread_local! { + static TLS_TABLE: RefCell = RefCell::new(0); +} +pub(crate) fn test_clear_partitioned() { *PARTITIONED_TABLE.lock().unwrap() = 0; } +pub(crate) fn test_clear_bare() { *BARE_TABLE.lock().unwrap() = 0; } +pub(crate) fn test_clear_tls() { TLS_TABLE.with(|t| *t.borrow_mut() = 0); } +pub(crate) fn test_clear_paren() { *PAREN_TABLE.lock().unwrap() = 0; let _g = PURE_LOCK.lock(); } +""" + + +def self_test() -> int: + import io + + failures = [] + fake = {Path("fake.rs"): _FAKE_SRC} + sink = io.StringIO() + + # 1. A bare process-global static is a violation; the partitioned and + # thread-local ones are not. + violations = audit(fake, _FAKE_SUPPORT, {}, sink) + if len(violations) != 1 or "BARE_TABLE" not in violations[0]: + failures.append("expected exactly one BARE_TABLE violation, got %r" % (violations,)) + if any("PARTITIONED_TABLE" in v or "TLS_TABLE" in v for v in violations): + failures.append("a converted or thread-local table was reported: %r" % (violations,)) + + # 2. Allowlisting it silences exactly that one. + if audit(fake, _FAKE_SUPPORT, {"BARE_TABLE": "#1"}, sink): + failures.append("an allowlisted hazard still failed") + + # 3. An allowlist entry that matches nothing must fail — otherwise the list + # only grows and a fix never has to delete its line. + stale = audit(fake, _FAKE_SUPPORT, {"BARE_TABLE": "#1", "GONE_TABLE": "#2"}, sink) + if not any("GONE_TABLE" in v for v in stale): + failures.append("a stale allowlist entry was accepted: %r" % (stale,)) + + # 3b. THE PAREN FORM. `per_test_global!(...)` on one line is how `timer.rs` + # stays under the 2000-line cap, and a `{`-only matcher classified its + # three tables as "(no static storage)" — a silent loss of coverage that + # reads exactly like a clean tree. + if any("PAREN_TABLE" in v for v in audit(fake, _FAKE_SUPPORT, {"BARE_TABLE": "#1"}, sink)): + failures.append("a per_test_global!(...) paren-form declaration was reported as a hazard") + kind, _ = declaration_kind(fake, "PAREN_TABLE") + if kind != "per_test": + failures.append("paren-form PAREN_TABLE classified as %r" % (kind,)) + + # 3c. A LOCK IS NOT DATA: `static X: Mutex<()>` must never be a hazard, or + # the gate would demand that the serializers themselves be per-thread, + # which would turn every one of them into a no-op. + kind, _ = declaration_kind(fake, "PURE_LOCK") + if kind != "lock": + failures.append("PURE_LOCK classified as %r, not 'lock'" % (kind,)) + if any("PURE_LOCK" in v for v in audit(fake, _FAKE_SUPPORT, {"BARE_TABLE": "#1"}, sink)): + failures.append("a Mutex<()> serializer was reported as a hazard") + + # 3d. THE FLOOR: a matcher that stops matching must not read as clean. + floored = audit(fake, _FAKE_SUPPORT, {"BARE_TABLE": "#1"}, sink, floor=99) + if not any("below the floor" in v for v in floored): + failures.append("the classified-statics floor did not fire: %r" % (floored,)) + + # 4. A renamed/absent reset function must be an error, not an empty pass. + for text, why in [ + ("fn something_else() { }", "missing reset fn"), + ("pub(super) fn reset_copying_nursery_runtime_test_state() {\n}\n", "empty reset body"), + ]: + try: + audit(fake, text, {}, sink) + failures.append("%s was accepted instead of raising" % why) + except Violation: + pass + + # 5. The parsers must survive the REAL tree, or the gate is vacuous. + try: + real_support = SUPPORT.read_text() + helpers = clear_helpers(reset_body(real_support)) + if len(helpers) < 10: + failures.append("only %d clear helpers parsed from the real %s" % (len(helpers), SUPPORT.name)) + if "test_clear_closure_side_tables" not in helpers: + failures.append("the real clear list is missing a helper it certainly calls: %r" % (helpers,)) + sources = rust_sources() + kind, _ = declaration_kind(sources, "CLOSURE_PROPS") + if kind != "per_test": + failures.append("CLOSURE_PROPS classified as %r on the real tree" % (kind,)) + kind, _ = declaration_kind(sources, "ARGUMENTS_OBJECTS") + if kind != "thread_local": + failures.append("ARGUMENTS_OBJECTS classified as %r on the real tree" % (kind,)) + except Violation as exc: + failures.append("parsing the real tree failed: %s" % exc) + + for failure in failures: + print("SELF-TEST FAIL: %s" % failure, file=sys.stderr) + print("self-test: 15 checks, %d failures" % len(failures)) + return 1 if failures else 0 + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("--self-test", action="store_true") + args = parser.parse_args() + + if args.self_test: + return self_test() + + try: + violations = audit(rust_sources(), SUPPORT.read_text(), ALLOWLIST, floor=CLASSIFIED_FLOOR) + except Violation as exc: + print("ERROR: %s" % exc, file=sys.stderr) + return 1 + + for violation in violations: + print("GLOBAL SINK: %s" % violation, file=sys.stderr) + if violations: + print( + "\n%d process-global side table(s) cleared by the GC test guards are reachable " + "from another test's assertions. See #7672: this class is diagnosed by luck, " + "in a PR that did not cause it." % len(violations), + file=sys.stderr, + ) + return 1 + return 0 + + +if __name__ == "__main__": + sys.exit(main())