diff --git a/CLAUDE.md b/CLAUDE.md index fcd497bc61..1570e98fc8 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.1384 +**Current Version:** 0.5.1385 ## TypeScript Parity Status diff --git a/Cargo.lock b/Cargo.lock index 96f48f060f..595c466a80 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5547,7 +5547,7 @@ checksum = "9b4f627cb1b25917193a259e49bdad08f671f8d9708acfd5fe0a8c1455d87220" [[package]] name = "perry" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "anyhow", "base64", @@ -5607,14 +5607,14 @@ dependencies = [ [[package]] name = "perry-api-manifest" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "serde", ] [[package]] name = "perry-audio-miniaudio" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "cc", "libc", @@ -5622,7 +5622,7 @@ dependencies = [ [[package]] name = "perry-codegen" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "anyhow", "inkwell", @@ -5639,7 +5639,7 @@ dependencies = [ [[package]] name = "perry-codegen-arkts" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "anyhow", "perry-hir", @@ -5647,7 +5647,7 @@ dependencies = [ [[package]] name = "perry-codegen-glance" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "anyhow", "perry-hir", @@ -5655,7 +5655,7 @@ dependencies = [ [[package]] name = "perry-codegen-js" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "anyhow", "perry-dispatch", @@ -5664,7 +5664,7 @@ dependencies = [ [[package]] name = "perry-codegen-swiftui" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "anyhow", "perry-hir", @@ -5672,7 +5672,7 @@ dependencies = [ [[package]] name = "perry-codegen-wasm" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "anyhow", "base64", @@ -5684,7 +5684,7 @@ dependencies = [ [[package]] name = "perry-codegen-wear-tiles" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "anyhow", "perry-hir", @@ -5692,7 +5692,7 @@ dependencies = [ [[package]] name = "perry-container-compose" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "anyhow", "async-trait", @@ -5721,14 +5721,14 @@ dependencies = [ [[package]] name = "perry-container-e2e" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "anyhow", ] [[package]] name = "perry-diagnostics" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "serde", "serde_json", @@ -5736,7 +5736,7 @@ dependencies = [ [[package]] name = "perry-dispatch" -version = "0.5.1384" +version = "0.5.1385" [[package]] name = "perry-doc-fixture-my-bindings" @@ -5747,7 +5747,7 @@ dependencies = [ [[package]] name = "perry-doc-tests" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "anyhow", "clap", @@ -5762,7 +5762,7 @@ dependencies = [ [[package]] name = "perry-ext-ads" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "block2", "objc2", @@ -5772,7 +5772,7 @@ dependencies = [ [[package]] name = "perry-ext-argon2" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "argon2", "perry-ffi", @@ -5780,7 +5780,7 @@ dependencies = [ [[package]] name = "perry-ext-axios" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-ffi", "reqwest", @@ -5789,7 +5789,7 @@ dependencies = [ [[package]] name = "perry-ext-bcrypt" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "bcrypt", "perry-ffi", @@ -5797,7 +5797,7 @@ dependencies = [ [[package]] name = "perry-ext-better-sqlite3" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-ffi", "rusqlite", @@ -5805,7 +5805,7 @@ dependencies = [ [[package]] name = "perry-ext-cheerio" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-ffi", "scraper", @@ -5813,7 +5813,7 @@ dependencies = [ [[package]] name = "perry-ext-commander" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-ffi", "perry-runtime", @@ -5821,7 +5821,7 @@ dependencies = [ [[package]] name = "perry-ext-cron" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "chrono", "cron", @@ -5831,7 +5831,7 @@ dependencies = [ [[package]] name = "perry-ext-dayjs" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "chrono", "perry-ffi", @@ -5839,7 +5839,7 @@ dependencies = [ [[package]] name = "perry-ext-decimal" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-ffi", "rust_decimal", @@ -5847,7 +5847,7 @@ dependencies = [ [[package]] name = "perry-ext-dotenv" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-ffi", "serde_json", @@ -5855,7 +5855,7 @@ dependencies = [ [[package]] name = "perry-ext-ethers" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-ffi", "rand 0.10.1", @@ -5863,7 +5863,7 @@ dependencies = [ [[package]] name = "perry-ext-events" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-ffi", "perry-runtime", @@ -5871,14 +5871,14 @@ dependencies = [ [[package]] name = "perry-ext-exponential-backoff" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-ffi", ] [[package]] name = "perry-ext-fastify" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "bytes", "http-body-util", @@ -5896,7 +5896,7 @@ dependencies = [ [[package]] name = "perry-ext-fetch" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "bytes", "lazy_static", @@ -5909,7 +5909,7 @@ dependencies = [ [[package]] name = "perry-ext-http" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "bytes", "h2", @@ -5933,7 +5933,7 @@ dependencies = [ [[package]] name = "perry-ext-ioredis" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "lazy_static", "perry-ffi", @@ -5943,7 +5943,7 @@ dependencies = [ [[package]] name = "perry-ext-jsonwebtoken" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "base64", "jsonwebtoken", @@ -5954,7 +5954,7 @@ dependencies = [ [[package]] name = "perry-ext-lru-cache" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "lru", "perry-ffi", @@ -5963,7 +5963,7 @@ dependencies = [ [[package]] name = "perry-ext-moment" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "chrono", "perry-ffi", @@ -5971,7 +5971,7 @@ dependencies = [ [[package]] name = "perry-ext-mongodb" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "bson", "futures-util", @@ -5983,7 +5983,7 @@ dependencies = [ [[package]] name = "perry-ext-mysql2" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "chrono", "perry-ffi", @@ -5993,7 +5993,7 @@ dependencies = [ [[package]] name = "perry-ext-nanoid" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "nanoid", "perry-ffi", @@ -6002,7 +6002,7 @@ dependencies = [ [[package]] name = "perry-ext-net" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "bytes", "perry-ffi", @@ -6015,7 +6015,7 @@ dependencies = [ [[package]] name = "perry-ext-node-forge" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "const-oid 0.9.6", "der 0.7.10", @@ -6034,7 +6034,7 @@ dependencies = [ [[package]] name = "perry-ext-nodemailer" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "lettre", "perry-ffi", @@ -6044,7 +6044,7 @@ dependencies = [ [[package]] name = "perry-ext-pdf" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-ffi", "printpdf", @@ -6052,7 +6052,7 @@ dependencies = [ [[package]] name = "perry-ext-pg" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-ffi", "sqlx", @@ -6061,7 +6061,7 @@ dependencies = [ [[package]] name = "perry-ext-ratelimit" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "governor", "perry-ffi", @@ -6069,7 +6069,7 @@ dependencies = [ [[package]] name = "perry-ext-sharp" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "fast_image_resize", "image", @@ -6079,14 +6079,14 @@ dependencies = [ [[package]] name = "perry-ext-slugify" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-ffi", ] [[package]] name = "perry-ext-streams" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "lazy_static", "perry-ffi", @@ -6095,7 +6095,7 @@ dependencies = [ [[package]] name = "perry-ext-undici" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-ffi", "perry-runtime", @@ -6104,7 +6104,7 @@ dependencies = [ [[package]] name = "perry-ext-uuid" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-ffi", "uuid", @@ -6112,7 +6112,7 @@ dependencies = [ [[package]] name = "perry-ext-validator" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-ffi", "regex", @@ -6122,7 +6122,7 @@ dependencies = [ [[package]] name = "perry-ext-ws" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "futures-util", "lazy_static", @@ -6135,7 +6135,7 @@ dependencies = [ [[package]] name = "perry-ext-zlib" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "brotli", "flate2", @@ -6145,7 +6145,7 @@ dependencies = [ [[package]] name = "perry-ffi" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "dashmap", "once_cell", @@ -6154,7 +6154,7 @@ dependencies = [ [[package]] name = "perry-hir" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "anyhow", "perry-api-manifest", @@ -6172,7 +6172,7 @@ dependencies = [ [[package]] name = "perry-parser" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "anyhow", "perry-diagnostics", @@ -6184,7 +6184,7 @@ dependencies = [ [[package]] name = "perry-runtime" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "anyhow", "base64", @@ -6226,14 +6226,14 @@ dependencies = [ [[package]] name = "perry-runtime-static" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-runtime", ] [[package]] name = "perry-stdlib" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "aes 0.8.4", "aes 0.9.1", @@ -6328,14 +6328,14 @@ dependencies = [ [[package]] name = "perry-stdlib-static" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-stdlib", ] [[package]] name = "perry-transform" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "anyhow", "perry-hir", @@ -6344,14 +6344,14 @@ dependencies = [ [[package]] name = "perry-ui" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-ui-model", ] [[package]] name = "perry-ui-android" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "base64", "itoa", @@ -6368,7 +6368,7 @@ dependencies = [ [[package]] name = "perry-ui-geisterhand" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "rand 0.10.1", "serde", @@ -6378,7 +6378,7 @@ dependencies = [ [[package]] name = "perry-ui-gtk4" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "base64", "cairo-rs 0.22.0", @@ -6401,7 +6401,7 @@ dependencies = [ [[package]] name = "perry-ui-ios" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "base64", "block2", @@ -6417,7 +6417,7 @@ dependencies = [ [[package]] name = "perry-ui-macos" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "base64", "block2", @@ -6432,7 +6432,7 @@ dependencies = [ [[package]] name = "perry-ui-model" -version = "0.5.1384" +version = "0.5.1385" [[package]] name = "perry-ui-test" @@ -6443,11 +6443,11 @@ dependencies = [ [[package]] name = "perry-ui-testkit" -version = "0.5.1384" +version = "0.5.1385" [[package]] name = "perry-ui-tvos" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "base64", "block2", @@ -6463,7 +6463,7 @@ dependencies = [ [[package]] name = "perry-ui-visionos" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "base64", "block2", @@ -6479,7 +6479,7 @@ dependencies = [ [[package]] name = "perry-ui-watchos" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "block2", "libc", @@ -6492,7 +6492,7 @@ dependencies = [ [[package]] name = "perry-ui-windows" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "base64", "libc", @@ -6509,14 +6509,14 @@ dependencies = [ [[package]] name = "perry-ui-windows-winui" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "perry-ui-windows", ] [[package]] name = "perry-updater" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "anyhow", "base64", @@ -6532,7 +6532,7 @@ dependencies = [ [[package]] name = "perry-wasm-host" -version = "0.5.1384" +version = "0.5.1385" dependencies = [ "wasmi", ] diff --git a/Cargo.toml b/Cargo.toml index b328dbe88a..a25b6692d0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -315,7 +315,7 @@ codegen-units = 16 codegen-units = 16 [workspace.package] -version = "0.5.1384" +version = "0.5.1385" edition = "2021" license = "MIT" repository = "https://github.com/PerryTS/perry" diff --git a/changelog.d/7675-stale-codegen-test-contract.md b/changelog.d/7675-stale-codegen-test-contract.md new file mode 100644 index 0000000000..04235615da --- /dev/null +++ b/changelog.d/7675-stale-codegen-test-contract.md @@ -0,0 +1,76 @@ +### Fixed + +**The stale codegen-test contract cluster: five suites that could no longer fail (#7505, #7504, #7503, #6988).** + +Four assertions in `crates/perry-codegen/tests/` had drifted from the contract +they name — three of them into shapes that were true of *every* program, which +is CLAUDE.md hazard 4 (the gate runs, its subject does not). Each is replaced +with a claim about a named value, and each replacement was watched go red +against a planted defect. + +* **#7505** — `assert_buffer_store_uses_dynamic_fallback` proved "no native + buffer GEP" with a MODULE-WIDE `!ir.contains("getelementptr inbounds i8")`. + The shadow-stack lowering's own inline slot addressing (#7088) emits exactly + that instruction, so sixteen tests reported a stale proof that was never + emitted the moment anyone ran the suite under `PERRY_RS4GC=0`; #7493 had + pinned them to native roots to stop the false alarm without making the + assertion right. `native_proof_support::native_buffer_element_geps` now + follows the data flow: an `inbounds` GEP is a buffer access only if its base + was loaded out of a Buffer view's DATA slot, and the helper **panics when the + function lowered no buffer view at all** rather than certifying its absence. + All sixteen pins are gone; the tests hold under both lowerings. + *Sabotage:* removing the data-slot filter — degrading the reader back into the + module-wide grep — turns 19 tests red under `PERRY_RS4GC=0`. + +* **#7504** — `scalar_replaced_slot_roots` counted `js_shadow_slot_bind` + module-wide, and since #7487 a pooled temp root emits the identical call. + Every fixture ends in `console.log(o.a, o.b)`, whose argument accumulator + contributes three binds, so `== 0` was a claim about the accumulator and + `== 1` a coincidence. New `perry_codegen::testing::root_slots` keys every bind + and every root-shading barrier by the entry alloca it names, classifies that + alloca, and **panics on one it cannot classify** so a future slot family goes + red naming itself instead of being folded into a total. + *Sabotage:* reverting #6968's `root_entry_alloca` call fails 7 positives; + disabling #6997's numeric gate fails all three `numeric_only_*` negatives — + the direction that was silently broken. + `flat_const_row_aliases_do_not_reserve_shadow_slots` is re-pointed and + renamed: its two causes separate cleanly and **the temp pool is not one of + them** (0 binds, 0 reservations there), while the property it asserted would + be a #6968 bug if codegen satisfied it — the fixture's rows are heap arrays. + Its second assertion had also gone toothless on its own, forbidding + `js_shadow_slot_set(i32 1` after #7013 moved the traffic to + `js_shadow_slot_bind`. + +* **#7503** — #7487 re-lowered temp roots onto pooled frame allocas, so + `js_gc_temp_root_push` / `_get` / `_set` / `_truncate` survive only on an FFI + fallback arm that neither shipped lowering takes. Ten assertions failed and + eight `!ir.contains(…push)` negatives held for every program in the language: + the #6951 / #6969 / #6970 / #6971 / #7114 / #7154 / #7200 contract had no + working coverage in either direction. New + `perry_codegen::testing::temp_slots` reads slot traffic in **both** spellings + (the RS4GC retype preserves register names, which is what lets one reader + serve both) and states the contract as a claim about a value: + `assert_rooted_across` proves this producer's result reached a slot and this + consuming call read its operand back out of it — strictly stronger than + proving a call existed somewhere in the module. + `temp_root_operand_temporaries.rs` is 19/19 under both lowerings, unpinned. + +* **#6988** — `tests/temp_root_argument_temporaries.rs` is deleted; its seven + tests moved into `crates/perry-codegen/src/temp_root_coverage/`, an in-crate + `#[cfg(test)]` module following #7653's `native_root_coverage` pattern, so the + temp-root emission contract now runs in the required per-PR `cargo-test` job + instead of the nightly-only integration tier (#5960). Each test runs once per + lowering. + +### Changed + +**`scripts/ci_e2e_scope.py` gains one narrow source → suite mapping (#7507).** +A `crates/perry-codegen/src/**` change now selects the three in-process +root-lowering suites that are green — under half a second of test time — closing +the gap that let #7370 take `shadow_slot_hygiene` to 0/12 with nothing red. The +general refusal to map `src/` → suites stands; this is one named exception with +a stated cost, cross-checked against `tests/` on disk so an entry that matches +nothing FAILS the scope step. `native_proof_regressions` and +`native_proof_buffer_views` are deliberately absent while they carry +pre-existing failures: a gate that is red on arrival and cannot block anything +teaches reviewers to ignore it. #7507 stays open for their addition. diff --git a/crates/perry-codegen/src/lib.rs b/crates/perry-codegen/src/lib.rs index 88db6ad4c3..0daf1f9f3e 100644 --- a/crates/perry-codegen/src/lib.rs +++ b/crates/perry-codegen/src/lib.rs @@ -51,6 +51,12 @@ pub(crate) mod stmt; pub mod strings; pub mod stubs; pub mod target_layout; +/// The #6951 temp-root emission contract, asserted in the per-PR `cargo-test` +/// gate rather than in the nightly-only integration tier (#6988), and against +/// the pooled lowering #7487 actually emits rather than the FFI spelling it +/// replaced (#7503). +#[cfg(test)] +mod temp_root_coverage; /// Test-support surface — compiled only under `cfg(test)` or the `testing` /// cargo feature (which nothing but this crate's own `[dev-dependencies]` /// enables). See the module docs for why it is a feature and not a diff --git a/crates/perry-codegen/src/temp_root_coverage/mod.rs b/crates/perry-codegen/src/temp_root_coverage/mod.rs new file mode 100644 index 0000000000..cd145a6fbc --- /dev/null +++ b/crates/perry-codegen/src/temp_root_coverage/mod.rs @@ -0,0 +1,297 @@ +//! The #6951 temp-root emission contract, in the per-PR `cargo-test` gate +//! (#6988), asserted in a form both lowerings satisfy (#7503). +//! +//! # Why it moved here +//! +//! This contract used to live only in `tests/temp_root_argument_temporaries.rs`. +//! Integration suites under `crates/*/tests/*.rs` run nightly/at-tag, not +//! per-PR (#5960), so the assertions pinning the rooting emission contract were +//! CI-invisible on exactly the PRs most likely to break them — and the failure +//! mode is silent: a lowering that goes back to threading a value through an +//! SSA register produces correct output under the default configuration, +//! because the conservative native-stack scan pins it by accident, and diverges +//! only under `PERRY_CONSERVATIVE_STACK_SCAN=off` or an evacuating minor. +//! +//! #6983 declined the move because the assertions needed ~90 lines of +//! `entry_opts()` / `module_with_init()` harness and doing that for three tests +//! was not worth it. Doing it once for the family is, and #7653's +//! `native_root_coverage` had meanwhile established the shape: an in-crate +//! `#[cfg(test)]` module that compiles hand-built HIR with `emit_ir_only`. +//! +//! # What is asserted, and why not the old spelling +//! +//! Until #7487 the contract was three runtime calls and these tests asserted +//! the calls: `ir.contains("call i32 @js_gc_temp_root_push")`. #7487 re-lowered +//! temps onto pooled frame allocas, so those calls survive only on an FFI +//! fallback arm neither shipped lowering takes. Ten assertions in this family +//! failed and eight `!ir.contains(…)` negatives became true of every program in +//! the language — the contract had no working coverage in either direction +//! (#7503). +//! +//! Every assertion here goes through [`crate::testing::temp_slots`], which +//! names the VALUE: this producer's result went into a rooted slot, and this +//! consuming call read its operand back OUT of that slot. That is strictly +//! stronger than the call-existence check it replaces, and it is +//! lowering-independent — so each test runs **unpinned under both lowerings** +//! rather than picking one. + +use crate::testing::temp_slots::{ + assert_no_temp_rooting, assert_rooted_across, first_call_result, slot_holding, slot_traffic, + SlotEvent, +}; +use crate::testing::NativeRootsPin; +use crate::{compile_module, AppMetadata, CompileOptions}; +use perry_hir::{Expr, Module, ModuleInitKind, Stmt}; + +mod operands; + +fn entry_opts() -> CompileOptions { + CompileOptions { + target: None, + is_entry_module: true, + non_entry_module_prefixes: Vec::new(), + nextjs_path_init_modules: Vec::new(), + import_function_prefixes: std::collections::HashMap::new(), + import_function_ffi_aliases: std::collections::HashMap::new(), + import_function_origin_names: std::collections::HashMap::new(), + import_function_v8_specifiers: std::collections::HashMap::new(), + import_function_node_submodule: std::collections::HashMap::new(), + namespace_node_submodules: std::collections::HashMap::new(), + namespace_v8_specifiers: std::collections::HashMap::new(), + namespace_member_prefixes: std::collections::HashMap::new(), + namespace_member_origin_names: std::collections::HashMap::new(), + emit_ir_only: true, + verify_native_regions: false, + disable_buffer_fast_path: false, + namespace_imports: Vec::new(), + imported_classes: Vec::new(), + imported_enums: Vec::new(), + imported_async_funcs: std::collections::HashSet::new(), + type_aliases: std::collections::HashMap::new(), + imported_func_param_counts: std::collections::HashMap::new(), + imported_func_has_rest: std::collections::HashSet::new(), + imported_func_synthetic_arguments: std::collections::HashSet::new(), + imported_func_return_types: std::collections::HashMap::new(), + imported_vars: std::collections::HashSet::new(), + output_type: "executable".to_string(), + needs_stdlib: false, + needs_ui: false, + needs_geisterhand: false, + geisterhand_port: 7676, + enabled_features: Vec::new(), + native_module_init_names: Vec::new(), + js_module_specifiers: Vec::new(), + bundled_extensions: Vec::new(), + native_library_functions: Vec::new(), + i18n_table: None, + fast_math: false, + fp_contract_mode: crate::FpContractMode::Off, + app_metadata: AppMetadata::default(), + namespace_entries: Vec::new(), + dynamic_import_path_to_prefix: std::collections::HashMap::new(), + deferred_module_prefixes: std::collections::HashSet::new(), + module_init_deps: Vec::new(), + is_dynamic_import_target: false, + debug_locations: false, + module_source: None, + debug_source_line_offset: 0, + } +} + +fn module_with_init(name: &str, init: Vec) -> Module { + Module { + name: name.to_string(), + imports: Vec::new(), + exports: Vec::new(), + classes: Vec::new(), + interfaces: Vec::new(), + type_aliases: Vec::new(), + enums: Vec::new(), + globals: Vec::new(), + functions: Vec::new(), + script_global_functions: Vec::new(), + references_global_this: false, + annexb_global_undefined_names: Vec::new(), + init, + exported_native_instances: Vec::new(), + exported_func_return_native_instances: Vec::new(), + exported_objects: Vec::new(), + exported_functions: Vec::new(), + widgets: Vec::new(), + uses_fetch: false, + uses_webassembly: false, + extern_funcs: Vec::new(), + init_was_unrolled: false, + has_top_level_await: false, + init_kind: ModuleInitKind::Eager, + async_step_closures: std::collections::HashSet::new(), + closure_display_names: std::collections::HashMap::new(), + class_display_names: std::collections::HashMap::new(), + closure_source_text: std::collections::HashMap::new(), + async_generator_funcs: std::collections::HashSet::new(), + gen_param_prologue_len: std::collections::HashMap::new(), + } +} + +/// The `main` body a module-init fixture compiles into. +pub(crate) fn main_ir_for(name: &str, init: Vec) -> String { + let bytes = compile_module(&module_with_init(name, init), entry_opts()) + .unwrap_or_else(|e| panic!("codegen failed for {name}: {e}")); + let ir = String::from_utf8(bytes).expect("LLVM IR should be UTF-8"); + crate::testing::root_slots::function_slice(&ir, "main").to_string() +} + +/// Run `body` once under each shipped root lowering. +/// +/// Every claim in this module is about the emission contract, not about how a +/// slot is spelled, so stating it once per lowering is the honest form. It also +/// means a `PERRY_RS4GC=0` bisection and CI assert the same thing — the +/// divergence #7493 spent a week diagnosing. +pub(crate) fn under_both_lowerings(mut body: impl FnMut(&str)) { + { + let _pin = NativeRootsPin::native(); + body("native roots"); + } + { + let _pin = NativeRootsPin::shadow(); + body("shadow stack"); + } +} + +/// `console.log(a, b, …)` — the shape in the #6951 repro. +pub(crate) fn console_log(args: Vec) -> Stmt { + Stmt::Expr(Expr::Call { + callee: Box::new(Expr::PropertyGet { + object: Box::new(Expr::GlobalGet(0)), + property: "log".to_string(), + byte_offset: 0, + }), + args, + type_args: Vec::new(), + byte_offset: 0, + }) +} + +/// An allocating argument: an object literal is a collection point, which is +/// all `expr_may_trigger_gc` needs to see. +pub(crate) fn allocating() -> Expr { + Expr::Object(Vec::new()) +} + +// --------------------------------------------------------------------------- +// The argument accumulator (#6951, #6972) +// --------------------------------------------------------------------------- + +/// The argument accumulator of a variadic call must live in a rooted slot, and +/// every use of it must be re-read from that slot. +/// +/// Pre-fix the sequence was `js_array_alloc` → N × `js_array_push_f64` with the +/// accumulator threaded through an SSA register across each argument's +/// evaluation. That register held the ONLY reference to everything pushed so +/// far, so an allocating later argument swept the half-built array and the next +/// push landed in recycled memory — `console.log("label", churn())` lost its +/// label with no crash and no diagnostic. +/// +/// Sabotage: `testing::temp_slots`'s own +/// `an_unrooted_accumulator_is_caught` / `rooted_but_not_re_read_is_caught` +/// plant both halves of the pre-fix shape and confirm this assertion rejects +/// them. Removing the store-back at the emission site fails the write-back +/// clause below in this test directly. +#[test] +fn console_argument_accumulator_is_rooted_re_read_and_written_back() { + under_both_lowerings(|lowering| { + let ir = main_ir_for( + "console_accumulator.ts", + vec![console_log(vec![ + Expr::String("label".to_string()), + allocating(), + ])], + ); + + let alloc = first_call_result(&ir, "js_array_alloc") + .unwrap_or_else(|| panic!("{lowering}: no argument array was allocated:\n{ir}")); + assert_rooted_across(&ir, &alloc, "js_array_push_f64", lowering); + + // `js_array_push_f64` may REALLOCATE, so the returned pointer has to go + // back into the slot — otherwise the root protects the old buffer and + // the consuming call reads a freed one. This is the clause the FFI + // spelling expressed as `js_array_push_f64_temp_rooted`. + let slot = slot_holding(&ir, &alloc).expect("assert_rooted_across just found it"); + let push = first_call_result(&ir, "js_array_push_f64") + .unwrap_or_else(|| panic!("{lowering}: no push:\n{ir}")); + assert!( + slot_traffic(&ir)[&slot] + .iter() + .any(|e| matches!(e, SlotEvent::Store { value, .. } if *value == push)), + "{lowering}: the reallocated array {push} must be written BACK into \ + {slot}; rooting only the pre-push pointer protects the wrong \ + allocation (#6951):\n{ir}" + ); + + // …and the consumer reads the slot, not any push's register. + assert_rooted_across(&ir, &push, "js_console_log_spread", lowering); + + // Released after the consuming call returns, not before. + let events = &slot_traffic(&ir)[&slot]; + let consume = ir + .lines() + .position(|line| line.contains("@js_console_log_spread(")) + .expect("the consumer was just asserted about"); + assert!( + events + .iter() + .any(|e| matches!(e, SlotEvent::Clear { line } if *line > consume)), + "{lowering}: the temp root must be released AFTER the consumer runs \ + (#6951):\n{ir}" + ); + }); +} + +/// The gate: rooting is emitted only when something that follows can collect. +/// +/// An array literal of plain literals has no allocation between its elements' +/// evaluation and the array's construction, so it must emit exactly the IR it +/// emitted before #6951 — no slot traffic, no cost. +/// +/// This is one of the eight negatives that #7503 found VACUOUS: it asserted +/// `!ir.contains("call i32 @js_gc_temp_root_push")`, which since #7487 holds +/// for every program in the language. Its non-vacuity is now proved by its own +/// twin below, which must go the other way. +#[test] +fn a_non_allocating_element_list_pays_for_no_temp_root() { + under_both_lowerings(|lowering| { + let ir = main_ir_for( + "array_literal_no_gc.ts", + vec![Stmt::Expr(Expr::Array(vec![ + Expr::String("a".to_string()), + Expr::String("b".to_string()), + Expr::Number(1.0), + ]))], + ); + assert_no_temp_rooting(&ir, lowering); + }); +} + +/// …and it IS emitted when an earlier element is a heap value and a later one +/// allocates: that value sits in an SSA register across the allocation, which +/// is not a root. +/// +/// Structurally identical to the test above but for one element's kind, so the +/// pair is a differential: a compiler that roots nothing fails here, and one +/// that roots everything fails there. +#[test] +fn an_array_literal_roots_elements_before_an_allocating_element() { + under_both_lowerings(|lowering| { + let ir = main_ir_for( + "array_literal_gc.ts", + // Element 0 is itself a heap value (a literal would be skipped: it + // loads from a module global that is already a registered GC root). + vec![Stmt::Expr(Expr::Array(vec![allocating(), allocating()]))], + ); + assert!( + !crate::testing::temp_slots::temp_root_slots(&ir).is_empty(), + "{lowering}: an array literal with an allocating later element must \ + root the heap elements already evaluated (#6951):\n{ir}" + ); + }); +} diff --git a/crates/perry-codegen/src/temp_root_coverage/operands.rs b/crates/perry-codegen/src/temp_root_coverage/operands.rs new file mode 100644 index 0000000000..ea2416a2e9 --- /dev/null +++ b/crates/perry-codegen/src/temp_root_coverage/operands.rs @@ -0,0 +1,160 @@ +//! #6996: the rooting gate must not root values that cannot be heap +//! references — and must still root the ones that can. +//! +//! `native_abi_packet_control`'s kernel is `(buf[i] + packet.tag + i) & 255`. +//! The left operand is a byte; the right is a property get on an `any`, so the +//! add lowers through `js_dynamic_string_or_number_add` and the operand pair is +//! rooted across the property get. Rooting the byte protected nothing and cost +//! a store, a re-read and a clear on every one of the loop's 262 144 +//! iterations — it turned the `hot_loops_no_runtime_calls` contract red. +//! +//! The two directions are stated as a matched pair so neither can pass for +//! nothing: the same fixture, differing only in the element key, must root in +//! one arm and not in the other. + +use super::{main_ir_for, under_both_lowerings}; +use crate::testing::temp_slots::{assert_no_temp_rooting, temp_root_slots}; +use perry_hir::{Expr, Stmt}; + +/// A local declared with a Buffer annotation, so the receiver of the element +/// read is a plain local get (the shape the fixture's parameter has). +fn buffer_local(id: u32) -> Stmt { + Stmt::Let { + id, + name: "buf".to_string(), + ty: perry_hir::types::Type::Named("Buffer".to_string()), + mutable: false, + init: Some(Expr::Undefined), + } +} + +/// An `any` local, so a property get on it is neither numeric-proven nor +/// allocation-free — exactly what forces the dynamic add and its rooting. +fn any_local(id: u32, name: &str) -> Stmt { + Stmt::Let { + id, + name: name.to_string(), + ty: perry_hir::types::Type::Any, + mutable: false, + init: Some(Expr::Undefined), + } +} + +fn packet_tag(id: u32) -> Expr { + Expr::PropertyGet { + object: Box::new(Expr::LocalGet(id)), + property: "tag".to_string(), + byte_offset: 0, + } +} + +/// ` + packet.tag`, bound to a local so nothing folds it away. +fn dynamic_add_with_element_read(element: Expr, packet_id: u32) -> Stmt { + Stmt::Let { + id: 90, + name: "next".to_string(), + ty: perry_hir::types::Type::Number, + mutable: false, + init: Some(Expr::Binary { + op: perry_hir::BinaryOp::Add, + left: Box::new(element), + right: Box::new(packet_tag(packet_id)), + }), + } +} + +fn element_read_fixture(name: &str, element: Expr, extra: Vec) -> String { + let mut init = vec![buffer_local(0), any_local(1, "packet")]; + init.extend(extra); + init.push(dynamic_add_with_element_read(element, 1)); + main_ir_for(name, init) +} + +/// A proven-index typed-array element read is a byte (or `undefined` out of +/// range) by construction, so it must not be rooted. +#[test] +fn a_proven_index_typed_array_read_is_not_temp_rooted() { + under_both_lowerings(|lowering| { + let ir = element_read_fixture( + "typed_element_operand.ts", + Expr::Uint8ArrayGet { + array: Box::new(Expr::LocalGet(0)), + index: Box::new(Expr::Integer(0)), + }, + Vec::new(), + ); + assert!( + ir.contains("call double @js_dynamic_string_or_number_add"), + "{lowering}: the fixture must actually reach the rooted \ + operand-pair lowering, or this proves nothing:\n{ir}" + ); + assert_no_temp_rooting(&ir, lowering); + }); +} + +/// Same for a `Buffer`-node element read, whose every lowering coerces the key +/// to i32 and reads a byte. +#[test] +fn a_buffer_index_read_is_not_temp_rooted() { + under_both_lowerings(|lowering| { + let ir = element_read_fixture( + "buffer_index_operand.ts", + Expr::BufferIndexGet { + buffer: Box::new(Expr::LocalGet(0)), + index: Box::new(Expr::Integer(0)), + }, + Vec::new(), + ); + assert_no_temp_rooting(&ir, lowering); + }); +} + +/// The soundness boundary, held from the other side: a symbol key does NOT +/// read an element. It resolves through `js_object_get_symbol_property`, which +/// hands back a `%TypedArray%.prototype` accessor — a heap value an allocating +/// sibling can sweep. It must still be rooted. +/// +/// This is the differential control for the two negatives above: same fixture, +/// same shape, one changed key. +#[test] +fn a_symbol_keyed_typed_array_read_is_still_temp_rooted() { + under_both_lowerings(|lowering| { + let ir = element_read_fixture( + "typed_symbol_key_operand.ts", + Expr::Uint8ArrayGet { + array: Box::new(Expr::LocalGet(0)), + index: Box::new(Expr::SymbolFor(Box::new(Expr::String( + "Symbol.iterator".to_string(), + )))), + }, + Vec::new(), + ); + assert!( + !temp_root_slots(&ir).is_empty(), + "{lowering}: a symbol-keyed read returns a prototype accessor, not \ + a byte — the #6996 skip must not reach it:\n{ir}" + ); + }); +} + +/// The other excluded lowering: without the integer-array-index proof the read +/// goes to `js_typed_array_index_get_dynamic`, which falls through to +/// string-keyed property lookup, and an expando can hold anything. +#[test] +fn an_unproven_key_typed_array_read_is_still_temp_rooted() { + under_both_lowerings(|lowering| { + let ir = element_read_fixture( + "typed_unproven_key_operand.ts", + Expr::Uint8ArrayGet { + array: Box::new(Expr::LocalGet(0)), + index: Box::new(Expr::LocalGet(2)), + }, + vec![any_local(2, "k")], + ); + assert!( + !temp_root_slots(&ir).is_empty(), + "{lowering}: an unproven key reads a property, not an element — the \ + #6996 skip must not reach it:\n{ir}" + ); + }); +} diff --git a/crates/perry-codegen/src/testing.rs b/crates/perry-codegen/src/testing.rs index b8d0f3e1e6..bff5635c49 100644 --- a/crates/perry-codegen/src/testing.rs +++ b/crates/perry-codegen/src/testing.rs @@ -64,5 +64,18 @@ //! //! The pin is thread-local and restoring, so it is safe under `cargo test`'s //! default parallelism: one test's pin cannot retarget another's compile. +//! +//! # What else lives here +//! +//! [`root_slots`] — the reader that attributes emitted root-slot traffic to the +//! alloca it names (#7504). [`temp_slots`] — the reader that states the #6951 +//! temp-root emission contract in a form BOTH lowerings satisfy (#7503). Both +//! are here rather than in a `tests/` support module for the same reason +//! `NativeRootsPin` is: the integration suites and this crate's own +//! `#[cfg(test)]` modules need them, and a second copy is a second thing to +//! keep in step. + +pub mod root_slots; +pub mod temp_slots; pub use crate::codegen::helpers::NativeRootsPin; diff --git a/crates/perry-codegen/src/testing/root_slots.rs b/crates/perry-codegen/src/testing/root_slots.rs new file mode 100644 index 0000000000..4ccb9df06c --- /dev/null +++ b/crates/perry-codegen/src/testing/root_slots.rs @@ -0,0 +1,315 @@ +//! Attributing emitted root-slot traffic to the slot it names (#7504). +//! +//! # Why counting the module stopped working +//! +//! `js_shadow_slot_bind` and `js_write_barrier_root_nanbox` used to have +//! exactly one producer worth counting: a named local's, or a scalar-replaced +//! field's, entry alloca. `scalar_replaced_slot_roots.rs` therefore measured +//! its subject with two whole-module counters — `ir.matches("call void +//! @js_shadow_slot_bind(").count()` and the same for the barrier. +//! +//! #7487 gave temporaries a claim on both. A pooled temp root reserves a frame +//! slot through `reserve_shadow_slot()` and emits `emit_shadow_slot_bind_ptr` +//! at every store — the identical call, on an alloca that belongs to no HIR +//! local. `console.log(p.x, p.y)`, the harmless tail of every fixture in that +//! file, contributes three binds naming the argument accumulator's slot. So +//! `bind_calls(&ir) == 0` became a claim about the accumulator and +//! `bind_calls(&ir) == 1` a coincidence. +//! +//! # What this module measures instead +//! +//! Every bind and every root-shading barrier, keyed by the **entry alloca** it +//! names, with that alloca classified: +//! +//! | kind | alloca | who reserves it | +//! |---|---|---| +//! | [`SlotKind::Value`] | `alloca double` | a named local, or a scalar-replaced field/element slot (#6968) | +//! | [`SlotKind::TempRoot`] | `alloca i64` null-initialised at entry | #7487's `TempRootPool` | +//! +//! [`bound_slots`] **panics on a bound alloca it cannot classify** rather than +//! defaulting it into either bucket. That is deliberate and is the property +//! that keeps this reader from decaying the way the counters did: when a third +//! slot family appears, the tests that use this go red naming it, instead of +//! silently folding it into whichever total happens to be asserted. +//! +//! Everything here reads the SHADOW-STACK spelling. The native-roots lowering +//! expresses the same root set as `ptr addrspace(1)` allocas with no bind call +//! to attribute, which is why the suites that use this pin +//! `NativeRootsPin::shadow()`; `crate::native_root_coverage` is the native +//! side's equivalent. + +use std::collections::BTreeMap; + +/// What a bound entry alloca belongs to. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +pub enum SlotKind { + /// A named local, or a scalar-replaced object field / array element slot. + /// These hold NaN-boxed `double`s. + Value, + /// One of #7487's pooled temp-root slots: `alloca i64`, null-initialised in + /// the entry block, holding an expression temporary rather than any HIR + /// local. + TempRoot, +} + +/// `%reg` -> the text right of `=` on its defining line, within one function. +fn defs(fn_ir: &str) -> BTreeMap<&str, &str> { + fn_ir + .lines() + .map(str::trim) + .filter(|line| line.starts_with('%')) + .filter_map(|line| line.split_once(" = ")) + .map(|(reg, def)| (reg.trim(), def.trim())) + .collect() +} + +/// The `ptr` operand of a `js_shadow_slot_bind` call line, if this is one. +fn bind_slot(line: &str) -> Option<&str> { + let rest = line + .trim() + .strip_prefix("call void @js_shadow_slot_bind(i32 ")?; + let (_, slot) = rest.split_once(", ptr ")?; + let slot = slot.trim_end_matches(')').trim(); + slot.starts_with('%').then_some(slot) +} + +fn classify(fn_ir: &str, defs: &BTreeMap<&str, &str>, slot: &str) -> SlotKind { + match defs.get(slot) { + Some(&"alloca double") => SlotKind::Value, + Some(&"alloca i64") if fn_ir.contains(&format!("store i64 0, ptr {slot}\n")) => { + SlotKind::TempRoot + } + other => panic!( + "root slot {slot} is bound but its alloca ({other:?}) belongs to no \ + known slot family. Adding one is fine — classify it HERE, in \ + `testing::root_slots`, so every test that measures root traffic \ + sees it. Silently folding it into an existing total is how a \ + whole-module bind count stopped measuring its subject (#7504)." + ), + } +} + +/// Every emitted `js_shadow_slot_bind`, keyed by the alloca it names. +/// +/// The `declare` line is unconditional, so only emitted CALLs are counted. +pub fn bound_slots(fn_ir: &str) -> BTreeMap { + let defs = defs(fn_ir); + let mut out: BTreeMap = BTreeMap::new(); + for line in fn_ir.lines() { + let Some(slot) = bind_slot(line) else { + continue; + }; + let kind = classify(fn_ir, &defs, slot); + out.entry(slot.to_string()).or_insert((kind, 0)).1 += 1; + } + out +} + +/// Binds naming a [`SlotKind::Value`] slot — the scalar-replaced fields and +/// named locals the #6968 / #6997 / #7013 contract is about. +pub fn value_slot_binds(fn_ir: &str) -> usize { + bound_slots(fn_ir) + .values() + .filter(|(kind, _)| *kind == SlotKind::Value) + .map(|(_, count)| *count) + .sum() +} + +/// Binds naming one of #7487's pooled temp-root slots. +/// +/// Not noise to be subtracted — a number to be *stated*. A test that says +/// "zero value-slot binds" is only interesting if this is non-zero, because +/// that is what proves the reader was looking at a module where binds exist. +pub fn temp_root_slot_binds(fn_ir: &str) -> usize { + bound_slots(fn_ir) + .values() + .filter(|(kind, _)| *kind == SlotKind::TempRoot) + .map(|(_, count)| *count) + .sum() +} + +/// Root-shading barriers (`js_write_barrier_root_nanbox`) keyed by the slot the +/// shaded value was loaded out of. +/// +/// The barrier takes the value, not the slot, so the attribution goes through +/// its operand's definition: `emit_shadow_slot_bind_ptr` shades what it just +/// re-loaded, so the operand is always a `load i64, ptr %slot` in the same +/// function. An operand that is not such a load is counted under the key +/// `""` rather than dropped — a barrier nobody can attribute is a +/// finding, not a rounding error. +pub fn barriers_by_slot(fn_ir: &str) -> BTreeMap { + let defs = defs(fn_ir); + let mut out: BTreeMap = BTreeMap::new(); + for line in fn_ir.lines().map(str::trim) { + let Some(rest) = line.strip_prefix("call void @js_write_barrier_root_nanbox(i64 ") else { + continue; + }; + let value = rest + .split(')') + .next() + .unwrap_or_default() + .split_whitespace() + .next() + .unwrap_or_default(); + let slot = defs + .get(value) + .and_then(|def| def.strip_prefix("load i64, ptr ")) + .map(|slot| slot.split(',').next().unwrap_or(slot).trim().to_string()) + .unwrap_or_else(|| "".to_string()); + *out.entry(slot).or_default() += 1; + } + out +} + +/// Root-shading barriers whose value came out of a [`SlotKind::Value`] slot. +pub fn value_slot_barriers(fn_ir: &str) -> usize { + let defs = defs(fn_ir); + barriers_by_slot(fn_ir) + .into_iter() + .filter(|(slot, _)| matches!(defs.get(slot.as_str()), Some(&"alloca double"))) + .map(|(_, count)| count) + .sum() +} + +/// The slot count baked into this function's `js_shadow_frame_enter`. +pub fn frame_slot_count(fn_ir: &str) -> u32 { + let needle = "call ptr @js_shadow_frame_enter(i32 "; + let start = fn_ir + .find(needle) + .map(|i| i + needle.len()) + .unwrap_or_else(|| panic!("expected a shadow frame enter in:\n{fn_ir}")); + let rest = &fn_ir[start..]; + let end = rest.find(')').expect("malformed frame enter"); + rest[..end] + .parse() + .expect("frame enter count is not a number") +} + +/// The whole `define … { … }` body of `@name`. +/// +/// Anchored on the `define` LINE — `ir.find("define i32 @main(")` would also +/// match a mention inside another function's body, and #7669 is what a slice +/// helper that cuts from the wrong anchor costs: every negative assertion +/// against the slice silently loses its subject. +pub fn function_slice<'a>(ir: &'a str, name: &str) -> &'a str { + let marker = format!("@{name}("); + let start = ir + .match_indices("define ") + .filter(|(idx, _)| *idx == 0 || ir.as_bytes()[idx - 1] == b'\n') + .find(|(idx, _)| { + let line_end = ir[*idx..].find('\n').map(|o| idx + o).unwrap_or(ir.len()); + ir[*idx..line_end].contains(&marker) + }) + .map(|(idx, _)| idx) + .unwrap_or_else(|| panic!("no function `{name}` in IR:\n{ir}")); + let end = ir[start..] + .find("\n}") + .map(|o| start + o + 2) + .unwrap_or(ir.len()); + &ir[start..end] +} + +/// The whole `define … { … }` body of the function containing `needle`. +pub fn enclosing_function<'a>(ir: &'a str, needle: &str) -> &'a str { + let at = ir + .find(needle) + .unwrap_or_else(|| panic!("no `{needle}` in:\n{ir}")); + let start = ir[..at] + .rfind("\ndefine ") + .map(|i| i + 1) + .unwrap_or_else(|| panic!("`{needle}` is outside any function in:\n{ir}")); + let end = ir[start..] + .find("\n}") + .map(|i| start + i + 2) + .unwrap_or(ir.len()); + &ir[start..end] +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A `main` carrying both slot families at once: `%v` is a scalar-replaced + /// field slot, `%t` is a pooled temp root. This is the exact shape #7504 + /// describes — three binds naming the `console.log` accumulator, one naming + /// the field the test means to measure. + const MIXED: &str = "\ +define i32 @main() { +entry.0: + %v = alloca double + store double 0x7FFC000000000001, ptr %v + %t = alloca i64 + store i64 0, ptr %t + %r3 = call ptr @js_shadow_frame_enter(i32 2) + call void @js_shadow_slot_bind(i32 1, ptr %v) + %r13 = load i64, ptr %v + call void @js_write_barrier_root_nanbox(i64 %r13) + call void @js_shadow_slot_bind(i32 2, ptr %t) + %r25 = load i64, ptr %t + call void @js_write_barrier_root_nanbox(i64 %r25) + call void @js_shadow_slot_bind(i32 2, ptr %t) + ret i32 0 +} +"; + + #[test] + fn binds_are_attributed_to_the_slot_that_owns_them() { + assert_eq!(value_slot_binds(MIXED), 1); + assert_eq!(temp_root_slot_binds(MIXED), 2); + // …and the whole-module count this replaces cannot tell them apart, + // which is the defect: 3 is what `bind_calls` reported for a program + // whose subject binds once. + assert_eq!(MIXED.matches("call void @js_shadow_slot_bind(").count(), 3); + } + + #[test] + fn barriers_are_attributed_through_the_value_they_shade() { + assert_eq!(value_slot_barriers(MIXED), 1); + assert_eq!( + barriers_by_slot(MIXED).get("%t").copied().unwrap_or(0), + 1, + "the temp root's own barrier must be visible, not merged away" + ); + assert!( + !barriers_by_slot(MIXED).contains_key(""), + "every barrier in this fixture is attributable" + ); + } + + /// The property that keeps this reader from rotting the way the counters + /// did: a bound slot from an unknown family is a hard failure. + #[test] + fn a_bound_slot_from_an_unknown_family_is_refused_not_bucketed() { + let unknown = "\ +define i32 @main() { +entry.0: + %x = alloca i32 + call void @js_shadow_slot_bind(i32 0, ptr %x) + ret i32 0 +} +"; + let refused = std::panic::catch_unwind(|| value_slot_binds(unknown)); + assert!( + refused.is_err(), + "an unclassifiable bound slot must fail loudly — folding it into a \ + total silently is #7504" + ); + } + + /// An `alloca i64` that is NOT null-initialised at entry is not a temp-root + /// slot; the frame base-index alloca is one such, and mistaking it for a + /// pool slot would let a real regression hide in the temp bucket. + #[test] + fn an_uninitialised_i64_alloca_is_not_mistaken_for_a_temp_root() { + let frame_base = "\ +define i32 @main() { +entry.0: + %b = alloca i64 + store i64 7, ptr %b + call void @js_shadow_slot_bind(i32 0, ptr %b) + ret i32 0 +} +"; + assert!(std::panic::catch_unwind(|| bound_slots(frame_base)).is_err()); + } +} diff --git a/crates/perry-codegen/src/testing/temp_slots.rs b/crates/perry-codegen/src/testing/temp_slots.rs new file mode 100644 index 0000000000..f0c390db54 --- /dev/null +++ b/crates/perry-codegen/src/testing/temp_slots.rs @@ -0,0 +1,616 @@ +//! Reading the temp-root emission contract out of IR, in either lowering +//! (#7503). +//! +//! # The contract, and why its old spelling is gone +//! +//! #6951's rule is that an evaluated-but-not-yet-consumed temporary must be a +//! precise GC root, not a bare SSA register: pushed into a rooted slot before +//! anything that can collect, **re-read** from that slot afterwards (the slot +//! is mutable, so an evacuating cycle rewrites it and the pushed register is +//! stale — #7114), written back if the consuming call reallocates, and released +//! after the last use. +//! +//! Until #7487 that was spelled with three runtime calls, and the suites +//! asserted the spelling: `ir.contains("call i32 @js_gc_temp_root_push")`. +//! #7487 re-lowered temps onto pooled frame allocas — `push` became a store, +//! `get` a load, `truncate` a slot clear — and those calls now survive only on +//! an FFI fallback arm that neither shipped lowering takes. So ten assertions +//! failed, and the eight `!ir.contains(…push)` NEGATIVES held for every program +//! in the language, rooted or not. The contract never changed; nothing was +//! measuring it in either direction. +//! +//! # What this reader does instead +//! +//! It names the *value*. [`slot_traffic`] returns, per entry alloca, the +//! registers stored into it, the registers loaded out of it and where it was +//! cleared — in program order — recognising **both** spellings: +//! +//! | event | shadow stack | native roots (RS4GC) | +//! |---|---|---| +//! | store | `store i64 %v, ptr %s` | `%t = inttoptr i64 %v to ptr addrspace(1)` + `store ptr addrspace(1) %t, ptr %s` | +//! | load | `%d = load i64, ptr %s` | `%d.rs4p = load ptr addrspace(1), ptr %s` (+ `ptrtoint`/`bitcast` back to `%d`) | +//! | clear | `store i64 0, ptr %s` | `store ptr addrspace(1) null, ptr %s` | +//! +//! The RS4GC retype preserves register NAMES — the value the shadow lowering +//! would have called `%r7` is still `%r7`, reached through `%r7.rs4p` — which +//! is what makes one reader serve both. That in turn is what lets the contract +//! be asserted UNPINNED: it is lowering-independent, and should be stated that +//! way. +//! +//! `ir.contains("call i32 @js_gc_temp_root_push")` only ever proved that a call +//! existed somewhere in the module. [`assert_rooted_across`] proves that THIS +//! value was in a slot, and that THIS call read it back out. + +use std::collections::BTreeMap; + +/// One event on one slot, tagged with its line index so order is comparable. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SlotEvent { + /// A value register was stored into the slot. + Store { value: String, line: usize }, + /// A register was loaded out of the slot. + Load { into: String, line: usize }, + /// The slot was cleared — #7487's spelling of `js_gc_temp_root_truncate`. + Clear { line: usize }, +} + +impl SlotEvent { + pub fn line(&self) -> usize { + match self { + SlotEvent::Store { line, .. } + | SlotEvent::Load { line, .. } + | SlotEvent::Clear { line } => *line, + } + } +} + +/// `%reg` -> the text right of `=` on its defining line. +fn defs(fn_ir: &str) -> BTreeMap<&str, &str> { + fn_ir + .lines() + .map(str::trim) + .filter(|line| line.starts_with('%')) + .filter_map(|line| line.split_once(" = ")) + .map(|(reg, def)| (reg.trim(), def.trim())) + .collect() +} + +fn operand_before(text: &str, sep: &str) -> Option { + let (value, _) = text.split_once(sep)?; + let value = value.trim(); + value.starts_with('%').then(|| value.to_string()) +} + +/// Every rooted-slot event in `fn_ir`, keyed by the alloca. +/// +/// Slots with no traffic at all are absent; a slot that is only ever +/// null-initialised at entry is not a root anybody used. +pub fn slot_traffic(fn_ir: &str) -> BTreeMap> { + let defs = defs(fn_ir); + let mut out: BTreeMap> = BTreeMap::new(); + let mut entry_seeds: BTreeMap = BTreeMap::new(); + + for (line_no, line) in fn_ir.lines().map(str::trim).enumerate() { + // ---- stores and clears ------------------------------------------- + if let Some(rest) = line.strip_prefix("store i64 ") { + if let Some((value, slot)) = rest.split_once(", ptr ") { + let slot = slot.split(',').next().unwrap_or(slot).trim().to_string(); + if slot.starts_with('%') { + if value.trim() == "0" { + // The entry seed and a release are the same instruction; + // only the first one in a slot's life is a seed. + if entry_seeds.contains_key(&slot) || out.contains_key(&slot) { + out.entry(slot) + .or_default() + .push(SlotEvent::Clear { line: line_no }); + } else { + entry_seeds.insert(slot, line_no); + } + } else if value.trim().starts_with('%') { + out.entry(slot).or_default().push(SlotEvent::Store { + value: value.trim().to_string(), + line: line_no, + }); + } + } + continue; + } + } + if let Some(rest) = line.strip_prefix("store ptr addrspace(1) ") { + if let Some((value, slot)) = rest.split_once(", ptr ") { + let slot = slot.split(',').next().unwrap_or(slot).trim().to_string(); + if !slot.starts_with('%') { + continue; + } + let value = value.trim(); + if value == "null" { + if entry_seeds.contains_key(&slot) || out.contains_key(&slot) { + out.entry(slot) + .or_default() + .push(SlotEvent::Clear { line: line_no }); + } else { + entry_seeds.insert(slot, line_no); + } + } else if let Some(source) = defs + .get(value) + .and_then(|def| def.strip_prefix("inttoptr i64 ")) + .and_then(|rest| operand_before(rest, " to ptr addrspace(1)")) + { + // The retype interposed an `inttoptr`; the VALUE is what + // fed it, which is the same register the shadow lowering + // would have stored directly. + out.entry(slot).or_default().push(SlotEvent::Store { + value: source, + line: line_no, + }); + } + continue; + } + } + + if let Some(rest) = line.strip_prefix("store double ") { + if let Some((value, slot)) = rest.split_once(", ptr ") { + let slot = slot.split(',').next().unwrap_or(slot).trim().to_string(); + if slot.starts_with('%') { + out.entry(slot).or_default().push(SlotEvent::Store { + value: value.trim().to_string(), + line: line_no, + }); + } + continue; + } + } + + // ---- loads -------------------------------------------------------- + let Some((dst, def)) = line.split_once(" = ") else { + continue; + }; + let dst = dst.trim(); + let def = def.trim(); + for prefix in ["load i64, ptr ", "load double, ptr "] { + if let Some(rest) = def.strip_prefix(prefix) { + let slot = rest.split(',').next().unwrap_or(rest).trim().to_string(); + if slot.starts_with('%') { + out.entry(slot).or_default().push(SlotEvent::Load { + into: dst.to_string(), + line: line_no, + }); + } + } + } + if let Some(rest) = def.strip_prefix("load ptr addrspace(1), ptr ") { + let slot = rest.split(',').next().unwrap_or(rest).trim().to_string(); + // RS4GC renames the load `%r7.rs4p` and rebuilds `%r7` from it, so + // the register the rest of the function uses is the stem. + let into = dst.strip_suffix(".rs4p").unwrap_or(dst).to_string(); + if slot.starts_with('%') { + out.entry(slot).or_default().push(SlotEvent::Load { + into, + line: line_no, + }); + } + } + } + + out +} + +/// Does `reg` derive from `ancestor` within `depth` def-use steps? +/// +/// A raw allocation result is NaN-boxed (`or` + `bitcast`) before it reaches a +/// slot, so "the value that was rooted" and "the register the allocator +/// returned" are never the same SSA name. Following the boxing is what makes an +/// assertion about `js_object_alloc`'s result mean anything. +fn derives_from(defs: &BTreeMap<&str, &str>, reg: &str, ancestor: &str, depth: usize) -> bool { + let mut reg = reg.to_string(); + for _ in 0..depth { + if reg == ancestor { + return true; + } + let Some(def) = defs.get(reg.as_str()) else { + return false; + }; + let Some(next) = def + .split(|c: char| !(c.is_alphanumeric() || c == '%' || c == '.' || c == '_')) + .find(|w| w.starts_with('%')) + else { + return false; + }; + reg = next.to_string(); + } + reg == ancestor +} + +/// The slot `value` — or the NaN-boxed form of it — was stored into, if any. +pub fn slot_holding(fn_ir: &str, value: &str) -> Option { + let defs = defs(fn_ir); + slot_traffic(fn_ir).into_iter().find_map(|(slot, events)| { + events + .iter() + .any(|e| match e { + SlotEvent::Store { value: v, .. } => derives_from(&defs, v, value, 4), + _ => false, + }) + .then_some(slot) + }) +} + +/// The `%reg` a `call`-defining line assigns, for the first call to `callee`. +pub fn first_call_result(fn_ir: &str, callee: &str) -> Option { + let needle = format!("@{callee}("); + fn_ir + .lines() + .map(str::trim) + .filter_map(|line| line.split_once(" = ")) + .find(|(_, def)| def.starts_with("call ") && def.contains(&needle)) + .map(|(dst, _)| dst.trim().to_string()) +} + +/// The operands of the first call to `callee`, as printed registers/literals. +pub fn call_operands(fn_ir: &str, callee: &str) -> Option> { + let needle = format!("@{callee}("); + let line = fn_ir + .lines() + .map(str::trim) + .find(|line| line.contains("call ") && line.contains(&needle))?; + let start = line.find(&needle)? + needle.len(); + let rest = &line[start..]; + let end = rest.find(')')?; + Some( + rest[..end] + .split(',') + .filter_map(|operand| operand.trim().rsplit_once(' ').map(|(_, r)| r.to_string())) + .collect(), + ) +} + +/// **The #6951 assertion.** `producer_result` must be parked in a rooted slot, +/// and `consumer`'s operand must be RE-READ out of that slot rather than being +/// the producer's own register. +/// +/// This is what `ir.contains("call i32 @js_gc_temp_root_push")` was standing in +/// for, and it is strictly stronger: that only proved a call existed somewhere +/// in the module, and said nothing about which value it protected or whether +/// anybody read it back. +pub fn assert_rooted_across(fn_ir: &str, producer_result: &str, consumer: &str, what: &str) { + let traffic = slot_traffic(fn_ir); + let slot = slot_holding(fn_ir, producer_result).unwrap_or_else(|| { + panic!( + "{what}: {producer_result} is never stored into a rooted slot — it \ + lives its whole life in an SSA register, which is not a GC root \ + (#6951). Slot traffic: {traffic:#?}\n{fn_ir}" + ) + }); + let defs = defs(fn_ir); + let events = &traffic[&slot]; + let store_line = events + .iter() + .find_map(|e| match e { + SlotEvent::Store { value, line } if derives_from(&defs, value, producer_result, 4) => { + Some(*line) + } + _ => None, + }) + .expect("slot_holding just found this store"); + + let operands = call_operands(fn_ir, consumer).unwrap_or_else(|| { + panic!("{what}: no call to @{consumer} — this assertion has no subject:\n{fn_ir}") + }); + let consumer_line = fn_ir + .lines() + .position(|line| line.contains(&format!("@{consumer}("))) + .expect("call_operands just found it"); + + let reread = events.iter().any(|e| match e { + SlotEvent::Load { into, line } => { + *line > store_line + && *line < consumer_line + && operands + .iter() + .any(|operand| derives_from(&defs, operand, into, 4)) + } + _ => false, + }); + assert!( + reread, + "{what}: @{consumer} takes {operands:?}, none of which was re-read from \ + {slot} between the store at line {store_line} and the call at line \ + {consumer_line}. A root buys a rewritten LOCATION; the consuming call \ + only observes the rewrite if it reads that location again (#7114). \ + Slot traffic: {events:#?}\n{fn_ir}" + ); + assert!( + !operands.contains(&producer_result.to_string()), + "{what}: @{consumer} still takes the producer's own register \ + {producer_result} — that register is stale the moment anything between \ + them collects (#6951):\n{fn_ir}" + ); +} + +/// Does `reg` derive, within `depth` def-use steps, from a value loaded out of +/// a rooted slot? +/// +/// The consuming call rarely takes the loaded register itself — the pooled +/// lowering loads `i64` and the caller wants a NaN-boxed `double`, so a +/// `bitcast`/`ptrtoint`/`or` sits in between. This is the pooled-form +/// replacement for the hand-rolled "walk back until you hit +/// `@js_gc_temp_root_get`" loops the suites grew. +pub fn derives_from_slot_load(fn_ir: &str, reg: &str, depth: usize) -> bool { + let defs = defs(fn_ir); + let loaded: std::collections::BTreeSet = slot_traffic(fn_ir) + .values() + .flat_map(|events| events.iter()) + .filter_map(|e| match e { + SlotEvent::Load { into, .. } => Some(into.clone()), + _ => None, + }) + .collect(); + let mut reg = reg.to_string(); + for _ in 0..depth { + if loaded.contains(®) { + return true; + } + let Some(def) = defs.get(reg.as_str()) else { + return false; + }; + let Some(next) = def + .split(|c: char| !(c.is_alphanumeric() || c == '%' || c == '.' || c == '_')) + .find(|w| w.starts_with('%')) + else { + return false; + }; + reg = next.to_string(); + } + false +} + +/// Slots whose FIRST traffic is a zero/null seed — `TempRootPool`'s +/// `entry_allocas_push_store(I64, "0", …)`, which the RS4GC retype re-emits as +/// a null `addrspace(1)` store. +/// +/// Required as well as the alloca type because an `alloca i64` is also how +/// codegen spells an unrelated scratch cell — the per-class inline-keys cache +/// in `@main` is one, and without this filter it read as a temp root and made +/// `a_class_that_runs_no_user_code_emits_no_instance_root` fail for a slot that +/// holds a static keys pointer. +pub fn zero_seeded_slots(fn_ir: &str) -> std::collections::BTreeSet { + let mut touched: std::collections::BTreeSet = std::collections::BTreeSet::new(); + let mut seeded: std::collections::BTreeSet = std::collections::BTreeSet::new(); + for line in fn_ir.lines().map(str::trim) { + let seed = line + .strip_prefix("store i64 0, ptr ") + .or_else(|| line.strip_prefix("store ptr addrspace(1) null, ptr ")); + if let Some(rest) = seed { + let slot = rest.split(',').next().unwrap_or(rest).trim().to_string(); + if slot.starts_with('%') && !touched.contains(&slot) { + seeded.insert(slot.clone()); + } + touched.insert(slot); + continue; + } + for slot in line + .split(", ptr ") + .skip(1) + .filter_map(|rest| rest.split(',').next()) + .map(|slot| slot.trim_end_matches(')').trim().to_string()) + .filter(|slot| slot.starts_with('%')) + { + touched.insert(slot); + } + if let Some((_, def)) = line.split_once(" = ") { + for prefix in [ + "load i64, ptr ", + "load double, ptr ", + "load ptr addrspace(1), ptr ", + ] { + if let Some(rest) = def.trim().strip_prefix(prefix) { + let slot = rest.split(',').next().unwrap_or(rest).trim().to_string(); + if slot.starts_with('%') { + touched.insert(slot); + } + } + } + } + } + seeded +} + +/// The `undefined` seed every NAMED-local and scalar-replacement slot gets at +/// entry, as codegen prints it. +fn undefined_literal() -> String { + crate::nanbox::double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) +} + +/// The slots that belong to #7487's pooled temp roots rather than to a named +/// local, a scalar-replaced field or an array-literal element. +/// +/// Two filters, because neither is sufficient on its own: +/// +/// 1. **The alloca's type.** `TempRootPool` allocates `alloca_entry(I64)`, so a +/// pooled slot is `alloca i64` — or `alloca ptr addrspace(1)` once the +/// native retype has run. Value storage (a named local, a scalar-replaced +/// field, an array-literal element) is `alloca double`, which excludes it. +/// 2. **The `undefined` seed.** Filter 1 alone is not enough under native +/// roots: RS4GC retypes every pointer-capable alloca to +/// `ptr addrspace(1)`, so a POINTER local's slot is spelled the same as a +/// temp slot. What still separates them is that a named local's slot is +/// seeded with `undefined` — `root_entry_alloca`'s contract, since the +/// collector dereferences it from function entry and must find something +/// decodable — while a pooled temp slot is seeded with zero/null. +pub fn temp_root_slots(fn_ir: &str) -> Vec { + let defs = defs(fn_ir); + let undefined = undefined_literal(); + let seeded = zero_seeded_slots(fn_ir); + slot_traffic(fn_ir) + .into_iter() + .filter(|(slot, _)| seeded.contains(slot)) + .filter(|(slot, _)| { + matches!( + defs.get(slot.as_str()), + Some(&"alloca i64") | Some(&"alloca ptr addrspace(1)") + ) + }) + .filter(|(_, events)| events.iter().any(|e| matches!(e, SlotEvent::Store { .. }))) + .filter(|(_, events)| { + !events.iter().any(|e| match e { + SlotEvent::Store { value, .. } => { + value == &undefined + || defs.get(value.as_str()).is_some_and(|def| { + def.starts_with("bitcast double ") && def.contains(&undefined) + }) + } + _ => false, + }) + }) + .map(|(slot, _)| slot) + .collect() +} + +/// No expression temporary was rooted in `fn_ir`: the #6996 / #6997 direction, +/// where rooting a value that can never be collected is pure cost. +/// +/// Named locals' own slots are excluded by [`temp_root_slots`] — this is a +/// claim about TEMPORARIES, and a fixture that declares a `Buffer` local roots +/// that local either way. +pub fn assert_no_temp_rooting(fn_ir: &str, what: &str) { + let rooted = temp_root_slots(fn_ir); + assert!( + rooted.is_empty(), + "{what}: a temp root was emitted where none is needed — a value that \ + cannot be a heap reference costs a store, a re-read and a clear for \ + nothing (#6996/#6997). Pooled temp slots with traffic: {rooted:?}. \ + Full traffic: {:#?}\n{fn_ir}", + slot_traffic(fn_ir) + ); +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The pooled shape, shadow spelling — `console.log("label", {})`. + const SHADOW: &str = "\ +define i32 @main() { +entry.0: + %s = alloca i64 + store i64 0, ptr %s + %r1 = call i64 @js_array_alloc(i32 2) + store i64 %r1, ptr %s + %r31 = load i64, ptr %s + %r32 = call i64 @js_array_push_f64(i64 %r31, double 1.0) + store i64 %r32, ptr %s + %r40 = load i64, ptr %s + call void @js_console_log_spread(i64 %r40) + store i64 0, ptr %s + ret i32 0 +} +"; + + /// The same program, native-roots spelling. Register NAMES are preserved by + /// the RS4GC retype, which is what lets one reader serve both. + const NATIVE: &str = "\ +define i32 @main() gc \"statepoint-example\" { +entry.0: + %s = alloca ptr addrspace(1) + store ptr addrspace(1) null, ptr %s + %r1 = call i64 @js_array_alloc(i32 2) + %rs4gc.s1 = inttoptr i64 %r1 to ptr addrspace(1) + store ptr addrspace(1) %rs4gc.s1, ptr %s + %r31.rs4p = load ptr addrspace(1), ptr %s + %r31 = ptrtoint ptr addrspace(1) %r31.rs4p to i64 + %r32 = call i64 @js_array_push_f64(i64 %r31, double 1.0) + %rs4gc.s2 = inttoptr i64 %r32 to ptr addrspace(1) + store ptr addrspace(1) %rs4gc.s2, ptr %s + %r40.rs4p = load ptr addrspace(1), ptr %s + %r40 = ptrtoint ptr addrspace(1) %r40.rs4p to i64 + call void @js_console_log_spread(i64 %r40) + store ptr addrspace(1) null, ptr %s + ret i32 0 +} +"; + + #[test] + fn both_lowerings_read_as_the_same_contract() { + for (name, ir) in [("shadow", SHADOW), ("native", NATIVE)] { + assert_rooted_across(ir, "%r1", "js_array_push_f64", name); + assert_rooted_across(ir, "%r32", "js_console_log_spread", name); + let slot = slot_holding(ir, "%r1").unwrap_or_else(|| panic!("{name}: no slot")); + assert_eq!(slot, "%s", "{name}"); + assert!( + slot_traffic(ir)[&slot] + .iter() + .any(|e| matches!(e, SlotEvent::Clear { .. })), + "{name}: the slot must be released after the consuming call" + ); + } + } + + /// The pre-#6951 shape: the accumulator threaded through an SSA register. + /// This is the sabotage the positive assertion has to catch, and the + /// spelling-based `ir.contains("…temp_root_push")` catches it only by + /// accident — it would also "catch" a module that rooted something else. + #[test] + fn an_unrooted_accumulator_is_caught() { + let unrooted = "\ +define i32 @main() { +entry.0: + %r1 = call i64 @js_array_alloc(i32 2) + %r32 = call i64 @js_array_push_f64(i64 %r1, double 1.0) + call void @js_console_log_spread(i64 %r32) + ret i32 0 +} +"; + assert!( + std::panic::catch_unwind(|| assert_rooted_across( + unrooted, + "%r1", + "js_array_push_f64", + "sabotage" + )) + .is_err(), + "an accumulator that never reaches a slot must fail the assertion" + ); + assert_no_temp_rooting(unrooted, "sabotage control"); + } + + /// Rooted but not RE-READ — #7114 exactly. The value has liveness and a + /// rewritten location, and the consuming call still observes the stale + /// address. The old `contains("…temp_root_push")` passes this happily. + #[test] + fn rooted_but_not_re_read_is_caught() { + let stale = "\ +define i32 @main() { +entry.0: + %s = alloca i64 + store i64 0, ptr %s + %r1 = call i64 @js_array_alloc(i32 2) + store i64 %r1, ptr %s + %r32 = call i64 @js_array_push_f64(i64 %r1, double 1.0) + ret i32 0 +} +"; + assert!( + std::panic::catch_unwind(|| assert_rooted_across( + stale, + "%r1", + "js_array_push_f64", + "sabotage" + )) + .is_err(), + "a consuming call that reuses the pushed register must fail (#7114)" + ); + } + + #[test] + fn the_negative_direction_sees_real_rooting() { + assert!( + std::panic::catch_unwind(|| assert_no_temp_rooting(SHADOW, "sabotage")).is_err(), + "a program that DOES root must fail `assert_no_temp_rooting` — \ + otherwise the negative gates are claims about nothing" + ); + assert!( + std::panic::catch_unwind(|| assert_no_temp_rooting(NATIVE, "sabotage")).is_err(), + "…in both lowerings" + ); + } +} diff --git a/crates/perry-codegen/tests/native_proof_buffer_views.rs b/crates/perry-codegen/tests/native_proof_buffer_views.rs index 7503550ee0..d808f99cb3 100644 --- a/crates/perry-codegen/tests/native_proof_buffer_views.rs +++ b/crates/perry-codegen/tests/native_proof_buffer_views.rs @@ -22,7 +22,10 @@ use perry_hir::{ #[path = "native_proof_support/mod.rs"] mod native_proof_support; -use native_proof_support::{artifact_env_lock, artifact_for_module, NativeRepsEnv}; +use native_proof_support::{ + artifact_env_lock, artifact_for_module, assert_no_native_buffer_element_access, probe_body, + NativeRepsEnv, +}; fn empty_opts() -> CompileOptions { CompileOptions { @@ -584,16 +587,9 @@ fn for_loop(counter_id: u32, bound: Expr, body: Vec) -> Stmt { for_loop_with_start_and_update(counter_id, int(0), bound, Some(increment(counter_id)), body) } -fn assert_buffer_store_uses_dynamic_fallback(ir: &str) { - assert!( - ir.contains("call void @js_buffer_set"), - "stale-proof case should keep the checked Buffer store fallback:\n{ir}" - ); - assert!( - !ir.contains("getelementptr inbounds i8"), - "stale-proof case must not emit an inbounds native buffer GEP:\n{ir}" - ); -} +// This file's second copy of `assert_buffer_store_uses_dynamic_fallback` is +// gone rather than re-fixed: since #7505 it lives in `native_proof_support`, +// where the one reader both suites share cannot drift between them. #[test] fn artifact_records_buffer_read_u32_and_unsigned_materialization() { @@ -645,9 +641,21 @@ fn artifact_records_buffer_read_u32_and_unsigned_materialization() { ); } +/// `i < buf.length` proves a ONE-byte access; a `readUInt32BE` at the same +/// index reads four. The four-byte read must therefore stay checked. +/// +/// LOWERING (#7505): unpinned. This used to prove "no native GEP" with a +/// module-wide `!ir.contains("getelementptr inbounds i8")`, which the +/// shadow-stack lowering's own inline slot addressing satisfied for reasons +/// unrelated to any buffer — so under `PERRY_RS4GC=0` it reported a proof leak +/// that was not there, and #7493 pinned it to native roots to stop the false +/// alarm. It now asks the question it means: is there an `inbounds` GEP off +/// this function's Buffer DATA slot? Asserted under both lowerings, and the +/// reader's ability to answer "yes" is proved in +/// `native_proof_regressions::invalidation::the_native_buffer_gep_detector_fires_on_a_proven_store` +/// plus this file's own `native_proof_support` self-tests. #[test] fn loop_length_bound_does_not_prove_multibyte_buffer_read_inbounds() { - let _pin = NativeRootsPin::native(); let body = vec![ buffer_let(1, "buf", int(8)), for_loop( @@ -665,11 +673,24 @@ fn loop_length_bound_does_not_prove_multibyte_buffer_read_inbounds() { Stmt::Return(Some(int(0))), ]; - let ir = compile_ir("loop_bound_multibyte_buffer_read.ts", body.clone()); - assert!( - !ir.contains("getelementptr inbounds i8"), - "`i < buf.length` only proves one-byte Buffer access; multi-byte reads must not emit an inbounds GEP:\n{ir}" - ); + for (pin_native, name) in [ + (true, "loop_bound_multibyte_buffer_read.ts"), + (false, "loop_bound_multibyte_buffer_read_shadow.ts"), + ] { + let ir = { + let _pin = if pin_native { + NativeRootsPin::native() + } else { + NativeRootsPin::shadow() + }; + compile_ir(name, body.clone()) + }; + assert_no_native_buffer_element_access( + probe_body(&ir), + "`i < buf.length` only proves a ONE-byte Buffer access; a \ + four-byte read must not consume it", + ); + } let artifact = compile_artifact_json("artifact_loop_bound_multibyte_buffer_read.ts", body); let records = artifact["records"].as_array().unwrap(); diff --git a/crates/perry-codegen/tests/native_proof_regressions.rs b/crates/perry-codegen/tests/native_proof_regressions.rs index b651ae0ea1..48f4934056 100644 --- a/crates/perry-codegen/tests/native_proof_regressions.rs +++ b/crates/perry-codegen/tests/native_proof_regressions.rs @@ -19,7 +19,10 @@ use perry_hir::{ #[path = "native_proof_support/mod.rs"] mod native_proof_support; -use native_proof_support::{artifact_env_lock, artifact_for_module, NativeRepsEnv}; +use native_proof_support::{ + artifact_env_lock, artifact_for_module, assert_native_buffer_element_access, probe_body, + NativeRepsEnv, +}; fn empty_opts() -> CompileOptions { CompileOptions { @@ -680,16 +683,11 @@ fn for_loop(counter_id: u32, bound: Expr, body: Vec) -> Stmt { for_loop_with_start_and_update(counter_id, int(0), bound, Some(increment(counter_id)), body) } -fn assert_buffer_store_uses_dynamic_fallback(ir: &str) { - assert!( - ir.contains("call void @js_buffer_set"), - "stale-proof case should keep the checked Buffer store fallback:\n{ir}" - ); - assert!( - !ir.contains("getelementptr inbounds i8"), - "stale-proof case must not emit an inbounds native buffer GEP:\n{ir}" - ); -} +// `assert_buffer_store_uses_dynamic_fallback` lives in `native_proof_support` +// since #7505 — it used to prove "no native buffer GEP" with a MODULE-WIDE +// `!ir.contains("getelementptr inbounds i8")`, which any unrelated `inbounds +// i8` in the module satisfied. +use native_proof_support::assert_buffer_store_uses_dynamic_fallback; #[test] fn artifact_schema_v6_records_consumed_native_facts_for_buffer_region() { diff --git a/crates/perry-codegen/tests/native_proof_regressions/invalidation.rs b/crates/perry-codegen/tests/native_proof_regressions/invalidation.rs index b7fed43878..260d7d6fae 100644 --- a/crates/perry-codegen/tests/native_proof_regressions/invalidation.rs +++ b/crates/perry-codegen/tests/native_proof_regressions/invalidation.rs @@ -1,21 +1,91 @@ -//! LOWERING (#7493): fifteen tests here pin `NativeRootsPin::native()`. +//! LOWERING (#7505): nothing here is pinned any more, and that is the point. //! -//! Their subject — buffer/length fact invalidation — is lowering-independent, -//! but the *assertion* is not: `assert_buffer_store_uses_dynamic_fallback` -//! proves the absence of a native buffer GEP with a MODULE-WIDE +//! Fifteen tests used to pin `NativeRootsPin::native()` because their subject — +//! buffer/length fact invalidation — is lowering-independent but their +//! *assertion* was not: `assert_buffer_store_uses_dynamic_fallback` proved the +//! absence of a native buffer GEP with a MODULE-WIDE //! `!ir.contains("getelementptr inbounds i8")`, and the shadow-stack lowering's //! own inline slot addressing (#7088) emits exactly that instruction for -//! reasons that have nothing to do with a buffer store. Run the suite under -//! `PERRY_RS4GC=0` and these fifteen report a stale proof that was never there. +//! reasons that have nothing to do with a buffer store. Under `PERRY_RS4GC=0` +//! all fifteen reported a stale proof that was never emitted. //! -//! Native roots are the default on every target the runtime can walk, so the -//! pin does not change what CI runs — it makes the assertion mean the same -//! thing during a `PERRY_RS4GC=0` bisection, which is the sweep a GC engineer -//! actually runs. The durable fix is to scope the search to the buffer-store -//! site instead of the whole module; #7505. +//! The pin stopped the false alarm without making the assertion right: the +//! proxy still could not tell the native buffer GEP it meant to catch from an +//! unrelated proven access elsewhere in the module, or from any other +//! `inbounds i8` a future pass emits. The assertion now follows the data flow +//! instead — `native_proof_support::native_buffer_element_geps` accepts only an +//! `inbounds` GEP taken off a pointer loaded out of a Buffer view's DATA slot, +//! and refuses to certify a function that lowered no buffer view at all. So the +//! claim holds under both lowerings, unpinned, and +//! `the_native_buffer_gep_detector_fires_on_a_proven_store` (below) is what +//! proves the reader can still see the thing it is asserting the absence of. use super::*; +/// The control for every `assert_buffer_store_uses_dynamic_fallback` in this +/// file: the SAME store with its proof intact must emit the unchecked native +/// element address, under BOTH lowerings. +/// +/// Without this, the fifteen negatives below are once again a claim nobody has +/// watched go the other way. With it, a reader that stopped recognising the +/// native buffer GEP — the failure mode that made the old module-wide grep +/// vacuous — turns this test red instead of turning those fifteen green. +#[test] +fn the_native_buffer_gep_detector_fires_on_a_proven_store() { + let proven = || { + vec![ + buffer_let(1, "src", int(8)), + buffer_let(2, "dst", int(8)), + for_loop(3, length(2), vec![buffer_set(2, local(3))]), + Stmt::Return(Some(int(0))), + ] + }; + + let native = { + let _pin = NativeRootsPin::native(); + compile_ir("proven_buffer_store_native.ts", proven()) + }; + assert_native_buffer_element_access(probe_body(&native), "proven store, native roots"); + + let shadow = { + let _pin = NativeRootsPin::shadow(); + compile_ir("proven_buffer_store_shadow.ts", proven()) + }; + assert_native_buffer_element_access(probe_body(&shadow), "proven store, shadow stack"); + + // …and the invalidated twin of that same store does not, in either + // lowering. This is the differential the pins were standing in for. + let invalidated = || { + vec![ + buffer_let(1, "buf", int(8)), + for_loop( + 2, + length(1), + vec![ + number_let(3, "j", true, bit_or_zero(local(2))), + Stmt::Expr(Expr::LocalSet(3, Box::new(int(16)))), + buffer_set(1, local(3)), + ], + ), + Stmt::Return(Some(int(0))), + ] + }; + for (pin_native, name) in [ + (true, "invalidated_native.ts"), + (false, "invalidated_shadow.ts"), + ] { + let ir = { + let _pin = if pin_native { + NativeRootsPin::native() + } else { + NativeRootsPin::shadow() + }; + compile_ir(name, invalidated()) + }; + assert_buffer_store_uses_dynamic_fallback(&ir); + } +} + fn block_between<'a>(ir: &'a str, start: &str, end: &str) -> &'a str { let start_pos = ir.find(start).unwrap_or_else(|| { panic!("missing block start marker {start:?} in IR:\n{ir}"); @@ -29,7 +99,6 @@ fn block_between<'a>(ir: &'a str, start: &str, end: &str) -> &'a str { #[test] fn localset_invalidates_native_i32_alias_facts() { - let _pin = NativeRootsPin::native(); let body = vec![ buffer_let(1, "buf", int(8)), for_loop( @@ -50,7 +119,6 @@ fn localset_invalidates_native_i32_alias_facts() { #[test] fn update_invalidates_native_i32_alias_facts() { - let _pin = NativeRootsPin::native(); let body = vec![ buffer_let(1, "buf", int(8)), for_loop( @@ -71,7 +139,6 @@ fn update_invalidates_native_i32_alias_facts() { #[test] fn localset_invalidates_min_length_facts() { - let _pin = NativeRootsPin::native(); let body = vec![ buffer_let(1, "src", int(8)), buffer_let(2, "dst", int(8)), @@ -87,7 +154,6 @@ fn localset_invalidates_min_length_facts() { #[test] fn localset_invalidates_active_bounded_buffer_index_facts() { - let _pin = NativeRootsPin::native(); let body = vec![ number_let(1, "n", false, int(8)), buffer_let(2, "buf", local(1)), @@ -108,7 +174,6 @@ fn localset_invalidates_active_bounded_buffer_index_facts() { #[test] fn inner_loop_bounded_buffer_fact_is_removed_after_outer_fact_invalidation() { - let _pin = NativeRootsPin::native(); let body = vec![ number_let(1, "n", false, int(8)), buffer_let(2, "a", local(1)), @@ -134,7 +199,6 @@ fn inner_loop_bounded_buffer_fact_is_removed_after_outer_fact_invalidation() { #[test] fn localset_invalidates_buffer_view_local_length_sources() { - let _pin = NativeRootsPin::native(); let body = vec![ number_let(1, "n", true, int(8)), buffer_let(2, "buf", local(1)), @@ -149,7 +213,6 @@ fn localset_invalidates_buffer_view_local_length_sources() { #[test] fn update_invalidates_buffer_view_local_length_sources() { - let _pin = NativeRootsPin::native(); let body = vec![ number_let(1, "n", true, int(8)), buffer_let(2, "buf", local(1)), @@ -164,7 +227,6 @@ fn update_invalidates_buffer_view_local_length_sources() { #[test] fn negative_loop_counter_does_not_emit_inbounds_buffer_gep() { - let _pin = NativeRootsPin::native(); let body = vec![ buffer_let(1, "buf", int(8)), for_loop_with_start_and_update( @@ -183,7 +245,6 @@ fn negative_loop_counter_does_not_emit_inbounds_buffer_gep() { #[test] fn decrementing_loop_update_does_not_emit_inbounds_buffer_gep() { - let _pin = NativeRootsPin::native(); let body = vec![ buffer_let(1, "buf", int(8)), for_loop_with_start_and_update( @@ -202,7 +263,6 @@ fn decrementing_loop_update_does_not_emit_inbounds_buffer_gep() { #[test] fn body_counter_mutation_does_not_emit_inbounds_buffer_gep() { - let _pin = NativeRootsPin::native(); let body = vec![ buffer_let(1, "buf", int(8)), for_loop( @@ -219,7 +279,6 @@ fn body_counter_mutation_does_not_emit_inbounds_buffer_gep() { #[test] fn inclusive_length_loop_does_not_emit_inbounds_buffer_gep() { - let _pin = NativeRootsPin::native(); let body = vec![ buffer_let(1, "buf", int(8)), for_loop_with_op_start_and_update( @@ -1064,7 +1123,6 @@ fn loop_local_array_alias_push_blocks_packed_u32_loop_and_artifacts() { #[test] fn inclusive_local_length_bound_does_not_use_local_length_bound_fact() { - let _pin = NativeRootsPin::native(); let body = vec![ number_let(1, "n", false, int(8)), buffer_let(2, "buf", local(1)), @@ -1085,7 +1143,6 @@ fn inclusive_local_length_bound_does_not_use_local_length_bound_fact() { #[test] fn negative_loop_counter_does_not_use_local_length_bound_fact() { - let _pin = NativeRootsPin::native(); let body = vec![ number_let(1, "n", false, int(8)), buffer_let(2, "buf", local(1)), @@ -1105,7 +1162,6 @@ fn negative_loop_counter_does_not_use_local_length_bound_fact() { #[test] fn body_mutation_of_local_bound_does_not_use_local_length_bound_fact() { - let _pin = NativeRootsPin::native(); let body = vec![ number_let(1, "n", true, int(1)), buffer_let(2, "buf", local(1)), @@ -1126,7 +1182,6 @@ fn body_mutation_of_local_bound_does_not_use_local_length_bound_fact() { #[test] fn negative_loop_counter_does_not_use_min_length_bound_fact() { - let _pin = NativeRootsPin::native(); let body = vec![ buffer_let(1, "src", int(8)), buffer_let(2, "dst", int(8)), diff --git a/crates/perry-codegen/tests/native_proof_support/mod.rs b/crates/perry-codegen/tests/native_proof_support/mod.rs index 5145d9e865..0aa88b2adb 100644 --- a/crates/perry-codegen/tests/native_proof_support/mod.rs +++ b/crates/perry-codegen/tests/native_proof_support/mod.rs @@ -28,6 +28,16 @@ //! Each including binary gets its own `ARTIFACT_ENV_LOCK` static and its own //! copy of the two tests below, which is what we want: the lock is per-process //! and so is the property being asserted. +//! +//! # Buffer-access proof reading (#7505) +//! +//! The second half of this file is the IR reader both suites use to ask "did +//! this fixture emit an UNCHECKED native buffer element address?". It replaces +//! a module-wide `!ir.contains("getelementptr inbounds i8")`, which was true of +//! the intended instruction and of every unrelated one — the shadow-stack +//! lowering's own inline slot addressing emits that exact text, so sixteen +//! tests reported a stale proof that was never emitted the moment anyone ran +//! the suite under `PERRY_RS4GC=0`. See [`native_buffer_element_geps`]. #![allow(dead_code)] @@ -178,3 +188,339 @@ fn artifact_env_is_restored_even_when_the_compile_unwinds() { "PERRY_NATIVE_REPS leaked out of a panicking compile window" ); } + +// --------------------------------------------------------------------------- +// #7505: naming the buffer access instead of grepping the module +// --------------------------------------------------------------------------- + +/// The `define … { … }` body of the one user function every fixture in this +/// family compiles: `probe`. +/// +/// Located from its `define` LINE, never from a substring of a body. #7669 is +/// why that distinction is written down: `fast_clone_slice` cut its slice from +/// the first *substring* match of a block label, which is the `br` terminator +/// four lines above the block it meant to cut — so every negative assertion +/// against that slice had no subject, and had been unable to fail since #7612, +/// on the code that then shipped the #7660 SIGBUS. +pub fn probe_body(ir: &str) -> &str { + let start = ir + .match_indices("define ") + // A `define ` must open a LINE to be a definition; anywhere else it is + // a mention (a comment, a string, a doc line). + .filter(|(idx, _)| *idx == 0 || ir.as_bytes()[idx - 1] == b'\n') + .find(|(idx, _)| { + let line_end = ir[*idx..].find('\n').map(|o| idx + o).unwrap_or(ir.len()); + let line = &ir[*idx..line_end]; + line.contains("@perry_fn_") + && line.contains("__probe(") + && !line.contains("@__perry_wrap_") + }) + .map(|(idx, _)| idx) + .unwrap_or_else(|| panic!("no `probe` function in IR:\n{ir}")); + let end = ir[start..] + .find("\n}\n") + .map(|o| start + o + 3) + .unwrap_or(ir.len()); + &ir[start..end] +} + +/// `%reg` -> the text right of `=` on its defining line, within one function. +fn defs(fn_ir: &str) -> std::collections::HashMap<&str, &str> { + fn_ir + .lines() + .map(str::trim) + .filter(|line| line.starts_with('%')) + .filter_map(|line| line.split_once(" = ")) + .map(|(reg, def)| (reg.trim(), def.trim())) + .collect() +} + +/// The base pointer of a `getelementptr … i8, ptr %B, …`. +fn gep_i8_base(def: &str) -> Option { + let rest = def + .strip_prefix("getelementptr inbounds i8, ptr ") + .or_else(|| def.strip_prefix("getelementptr i8, ptr "))?; + let base = rest.split(',').next()?.trim(); + base.starts_with('%').then(|| base.to_string()) +} + +/// The slot of a `load ptr, ptr %S`. +fn load_ptr_slot(def: &str) -> Option { + let rest = def.strip_prefix("load ptr, ptr ")?; + let slot = rest.split(',').next()?.trim(); + slot.starts_with('%').then(|| slot.to_string()) +} + +/// The `ptr` operand a `store ptr %V, ptr %S` writes into, with its value. +fn store_ptr_operands(line: &str) -> Option<(String, String)> { + let rest = line.trim().strip_prefix("store ptr ")?; + let (value, slot) = rest.split_once(", ptr ")?; + let value = value.trim(); + let slot = slot.split(',').next()?.trim(); + (value.starts_with('%') && slot.starts_with('%')).then(|| (value.to_string(), slot.to_string())) +} + +/// The allocas holding a Buffer view's DATA pointer in `fn_ir`. +/// +/// Two shapes, either of which identifies one, because +/// `emit_buffer_access_pointer` reads the length from a dedicated slot when the +/// view has one and from the header otherwise: +/// +/// * the initialising store — the NaN-boxed Buffer value is unboxed +/// (`and i64 …, 0xFFFF_FFFF_FFFF` then `inttoptr`), advanced past its header +/// (`getelementptr i8, ptr %p, i32 `) and stored into the slot; and +/// * the header length read — `load ptr` out of the slot, `getelementptr i8` +/// by the length offset, then `load i32 …, !invariant.load`. +/// +/// This is the part that makes the search name its subject. A shadow-stack +/// frame pointer, a string's data pointer and an inline-cache slot are all +/// `ptr` values that get `getelementptr inbounds i8`'d; none of them is ever +/// stored into a slot by *this* pair of shapes. +pub fn buffer_data_slots(fn_ir: &str) -> std::collections::BTreeSet { + let defs = defs(fn_ir); + let mut slots = std::collections::BTreeSet::new(); + + for line in fn_ir.lines() { + if let Some((value, slot)) = store_ptr_operands(line) { + let is_unboxed_data_pointer = defs + .get(value.as_str()) + .and_then(|def| gep_i8_base(def)) + .and_then(|base| defs.get(base.as_str()).copied()) + .is_some_and(|base_def| base_def.starts_with("inttoptr i64 ")); + if is_unboxed_data_pointer { + slots.insert(slot); + } + } + } + + for line in fn_ir.lines().map(str::trim) { + if !line.contains("!invariant.load") { + continue; + } + let Some((_, def)) = line.split_once(" = ") else { + continue; + }; + let Some(rest) = def.trim().strip_prefix("load i32, ptr ") else { + continue; + }; + let Some(header) = rest.split(',').next().map(str::trim) else { + continue; + }; + let slot = defs + .get(header) + .and_then(|def| gep_i8_base(def)) + .and_then(|base| defs.get(base.as_str()).copied()) + .and_then(load_ptr_slot); + if let Some(slot) = slot { + slots.insert(slot); + } + } + + slots +} + +/// Every `getelementptr inbounds i8` taken off a pointer loaded out of a +/// buffer data slot — i.e. every UNCHECKED native buffer element address in +/// `fn_ir`, and nothing else. +/// +/// The predicate this replaces was `!ir.contains("getelementptr inbounds i8")` +/// over the WHOLE MODULE. It could not distinguish the native buffer GEP it +/// meant to catch from an unrelated proven access elsewhere in the module, nor +/// from any other `inbounds i8` the backend happens to emit — and the +/// shadow-stack lowering's inline slot addressing (#7088) emits exactly that +/// text, which is what actually bit us (#7505). +pub fn native_buffer_element_geps(fn_ir: &str) -> Vec { + let defs = defs(fn_ir); + let slots = buffer_data_slots(fn_ir); + fn_ir + .lines() + .map(str::trim) + .filter_map(|line| line.split_once(" = ")) + .filter(|(_, def)| def.starts_with("getelementptr inbounds i8, ptr ")) + .filter_map(|(dst, def)| { + let base = gep_i8_base(def)?; + let slot = defs.get(base.as_str()).copied().and_then(load_ptr_slot)?; + slots.contains(&slot).then(|| dst.trim().to_string()) + }) + .collect() +} + +/// No unchecked native buffer element address was emitted in `fn_ir`. +/// +/// **Panics when the function has no buffer view at all**, rather than +/// reporting a clean bill of health: "there is no native GEP" and "there is no +/// buffer here to have one" are the two answers this helper exists to tell +/// apart, and only the first is evidence (CLAUDE.md hazard 4). +pub fn assert_no_native_buffer_element_access(fn_ir: &str, what: &str) { + let slots = buffer_data_slots(fn_ir); + assert!( + !slots.is_empty(), + "{what}: no Buffer data slot in this function, so `no unchecked native \ + element address` has no subject — the fixture did not lower a buffer \ + view at all:\n{fn_ir}" + ); + let geps = native_buffer_element_geps(fn_ir); + assert!( + geps.is_empty(), + "{what}: unchecked native buffer element address(es) {geps:?} taken off \ + data slot(s) {slots:?} — the invalidated proof leaked through \ + (#7505):\n{fn_ir}" + ); +} + +/// At least one unchecked native buffer element address WAS emitted — the +/// control direction, which is what keeps the negative assertion honest. +pub fn assert_native_buffer_element_access(fn_ir: &str, what: &str) { + let slots = buffer_data_slots(fn_ir); + assert!( + !slots.is_empty(), + "{what}: no Buffer data slot in this function at all:\n{fn_ir}" + ); + let geps = native_buffer_element_geps(fn_ir); + assert!( + !geps.is_empty(), + "{what}: expected an unchecked native buffer element address off data \ + slot(s) {slots:?} and found none — if this is right the DETECTOR is \ + broken, and every `assert_no_native_buffer_element_access` in this \ + family is vacuous (#7505):\n{fn_ir}" + ); +} + +/// A Buffer element store whose native proof was invalidated must fall back to +/// the checked runtime call, and must not ALSO emit an unchecked native +/// address for the same access. +pub fn assert_buffer_store_uses_dynamic_fallback(ir: &str) { + let body = probe_body(ir); + assert!( + body.contains("call void @js_buffer_set"), + "stale-proof case should keep the checked Buffer store fallback:\n{body}" + ); + assert_no_native_buffer_element_access(body, "stale-proof Buffer store"); +} + +// --------------------------------------------------------------------------- +// Self-tests for the reader above. +// +// Hand-written IR rather than a compile, deliberately: these pin the exact +// discrimination the module-wide grep could not make, and they must keep +// failing for that reason even if the fixtures that produce the real IR move. +// The complementary proof against REAL compiled IR, under both lowerings, is +// `invalidation::the_native_buffer_gep_detector_fires_on_a_proven_store`. +// --------------------------------------------------------------------------- + +/// A `probe` body with a Buffer view whose element access WAS proven: the data +/// slot `%d`, and an `inbounds` GEP off the pointer loaded out of it. +#[cfg(test)] +const UNCHECKED_NATIVE_BUFFER_PROBE: &str = "\ +define double @perry_fn_m_ts__probe() { +entry.0: + %d = alloca ptr + %r10 = and i64 %r9, 281474976710655 + %r11 = inttoptr i64 %r10 to ptr + %r12 = getelementptr i8, ptr %r11, i32 8 + store ptr %r12, ptr %d + %r40 = load ptr, ptr %d + %r44 = getelementptr inbounds i8, ptr %r40, i32 %r39 + store i8 %r45, ptr %r44 + ret double 0.0 +} +"; + +#[test] +fn the_buffer_gep_reader_sees_an_unchecked_native_element_address() { + let body = probe_body(UNCHECKED_NATIVE_BUFFER_PROBE); + assert_eq!( + buffer_data_slots(body).into_iter().collect::>(), + vec!["%d".to_string()], + "the data slot must be found from its initialising store" + ); + assert_eq!( + native_buffer_element_geps(body), + vec!["%r44".to_string()], + "the inbounds GEP off the data pointer is the unchecked access" + ); + assert_native_buffer_element_access(body, "self-test control"); +} + +/// The regression that made the old assertion unable to fail: the shadow-stack +/// lowering's inline slot addressing emits `getelementptr inbounds i8` off a +/// pointer loaded out of a slot that is NOT a buffer view. +/// +/// Sabotage record — deleting the `slots.contains(&slot)` filter in +/// [`native_buffer_element_geps`] makes this test report `%r20`, `%r21` and +/// fail, which is the whole point: without that filter the reader degenerates +/// into the module-wide grep it replaced. +#[test] +fn the_buffer_gep_reader_ignores_the_shadow_frames_own_inline_slot_gep() { + let shadow_frame_probe = "\ +define double @perry_fn_m_ts__probe() { +entry.0: + %r3 = alloca ptr + %r4 = call ptr @js_shadow_frame_enter(i32 1) + store ptr %r4, ptr %r3 + %r9 = load ptr, ptr %r3 + %r18 = load ptr, ptr %r9 + %r19 = shl i64 %r14, 4 + %r20 = getelementptr inbounds i8, ptr %r18, i64 %r19 + %r21 = getelementptr inbounds i8, ptr %r20, i64 8 + ret double 0.0 +} +"; + let body = probe_body(shadow_frame_probe); + assert!( + buffer_data_slots(body).is_empty(), + "a shadow frame's state slot is not a Buffer data slot" + ); + assert!( + native_buffer_element_geps(body).is_empty(), + "the shadow frame's inline slot GEPs must not read as buffer accesses \ + — this is the exact text that turned sixteen `PERRY_RS4GC=0` tests \ + red for a reason unrelated to their subject (#7505)" + ); +} + +/// …and a body with no buffer view at all is REFUSED, not reported clean. +#[test] +fn a_body_with_no_buffer_view_refuses_to_certify_the_absence_of_one() { + let no_buffer_probe = "\ +define double @perry_fn_m_ts__probe() { +entry.0: + ret double 0.0 +} +"; + let refused = std::panic::catch_unwind(|| { + assert_no_native_buffer_element_access(probe_body(no_buffer_probe), "self-test"); + }); + assert!( + refused.is_err(), + "a function with no Buffer view must not satisfy `no unchecked native \ + buffer access` — that is the vacuous pass this reader exists to stop" + ); +} + +/// The slice must be the function, not the first line that mentions it. +#[test] +fn probe_body_slices_the_definition_and_not_its_wrapper() { + let ir = "\ +define double @__perry_wrap_perry_fn_m_ts__probe(i64 %this_closure) { +entry.0: + %w1 = getelementptr inbounds i8, ptr %w0, i32 0 + ret double 0.0 +} + +define double @perry_fn_m_ts__probe() { +entry.0: + %r1 = add i32 0, 0 + ret double 0.0 +} +"; + let body = probe_body(ir); + assert!( + body.starts_with("define double @perry_fn_m_ts__probe()"), + "the wrapper must not be mistaken for the function under test: {body}" + ); + assert!( + !body.contains("%w1"), + "the slice must stop at the function it opened: {body}" + ); +} diff --git a/crates/perry-codegen/tests/scalar_replaced_slot_roots.rs b/crates/perry-codegen/tests/scalar_replaced_slot_roots.rs index 1730449f31..ff043dcdad 100644 --- a/crates/perry-codegen/tests/scalar_replaced_slot_roots.rs +++ b/crates/perry-codegen/tests/scalar_replaced_slot_roots.rs @@ -26,19 +26,33 @@ //! relocates; there is no bind call to count. //! //! That distinction was not cosmetic for the two `numeric_only_*_emits_no_rooting` -//! tests. They assert `bind_calls(&ir) == 0`, and under the post-#7370 default -//! that count is zero for EVERY program, rooted or not — so they were passing -//! vacuously, which is CLAUDE.md hazard 4 ("the gate runs but its subject never -//! did"). Pinning them makes them assert their subject again; they now fail for -//! a real reason, tracked in #7504 (since #7487 a pooled temp root also emits -//! `js_shadow_slot_bind`, so a whole-module bind count no longer isolates the -//! scalar-replaced slots it means to measure). A red test that is measuring -//! something beats a green one that is not. +//! tests. They assert no rooting is emitted, and under the post-#7370 default +//! the bind count is zero for EVERY program, rooted or not — so they were +//! passing vacuously, which is CLAUDE.md hazard 4 ("the gate runs but its +//! subject never did"). Pinning them made them assert their subject again. +//! +//! ATTRIBUTION (#7504): pinning then exposed a second defect one layer down. +//! The measurement was a WHOLE-MODULE counter — `ir.matches("call void +//! @js_shadow_slot_bind(").count()` — and since #7487 a pooled temp root emits +//! the identical call. Every fixture in this file ends in +//! `console.log(o.a, o.b)`, whose argument accumulator contributes three binds +//! naming an alloca that has nothing to do with any scalar-replaced field. So +//! `== 0` was a claim about the accumulator and `== 1` a coincidence. +//! +//! Every count here now goes through `perry_codegen::testing::root_slots`, +//! which keys each bind and each root-shading barrier by the entry alloca it +//! names and refuses to classify an unfamiliar one. Where a fixture's temp-root +//! traffic is non-zero the tests SAY so, because that is what proves the reader +//! was looking at a module in which binds exist at all. //! //! The native-roots side of this contract — that a scalar-replaced field //! holding a heap value becomes a relocatable `addrspace(1)` slot, and a -//! numeric one does not — has NO equivalent assertion anywhere today. #7502. +//! numeric one does not — is covered by +//! `perry_codegen::native_root_coverage::mechanics` since #7653. +use perry_codegen::testing::root_slots::{ + frame_slot_count, temp_root_slot_binds, value_slot_barriers, value_slot_binds, +}; use perry_codegen::testing::NativeRootsPin; use perry_codegen::{compile_module, AppMetadata, CompileOptions}; use perry_hir::types::Type; @@ -175,59 +189,13 @@ fn console_log(args: Vec) -> Stmt { }) } -/// Count of emitted `js_shadow_slot_bind` CALL sites. The `declare` line is -/// unconditional, so only calls count. -fn bind_calls(ir: &str) -> usize { - ir.matches("call void @js_shadow_slot_bind(").count() -} - -/// Count of emitted incremental-mark root-shading barriers. This is the -/// per-store remainder left behind once the bind is hoisted to entry. -fn root_barriers(ir: &str) -> usize { - ir.matches("call void @js_write_barrier_root_nanbox(") - .count() -} - -/// The whole `define … { … }` body of the function containing `needle`. -/// -/// The tiny modules these tests build put everything in module init, but -/// scoping the assertions to one function keeps ordering claims meaningful if -/// that ever stops being true. -fn enclosing_function<'a>(ir: &'a str, needle: &str) -> &'a str { - let at = ir - .find(needle) - .unwrap_or_else(|| panic!("no `{needle}` in:\n{ir}")); - let start = ir[..at] - .rfind("\ndefine ") - .map(|i| i + 1) - .unwrap_or_else(|| panic!("`{needle}` is outside any function in:\n{ir}")); - let end = ir[start..] - .find("\n}") - .map(|i| start + i + 2) - .unwrap_or(ir.len()); - &ir[start..end] -} - -/// The slot count baked into this module-init function's frame push. +/// The `main` these entry-module fixtures compile into. /// -/// #7088 replaced the handle-returning `js_shadow_frame_push` with -/// `js_shadow_frame_enter`, which returns the `ShadowStackState` pointer the -/// inline slot stores address (the pop handle is derived from `frame_top`). -/// The slot-count operand is unchanged, so only the callee name and its -/// return type move. -fn frame_slot_count(ir: &str) -> u32 { - let needle = "call ptr @js_shadow_frame_enter(i32 "; - let start = ir - .find(needle) - .map(|i| i + needle.len()) - .unwrap_or_else(|| { - panic!("expected a shadow frame enter in:\n{ir}"); - }); - let rest = &ir[start..]; - let end = rest.find(')').expect("malformed frame push"); - rest[..end] - .parse() - .expect("frame push count is not a number") +/// Every assertion below is scoped to one function rather than to the module, +/// so a bind emitted in an unrelated helper cannot satisfy or break a claim +/// about this one. +fn main_ir(ir: &str) -> &str { + perry_codegen::testing::root_slots::function_slice(ir, "main") } /// A scalar-replaced object literal whose field holds a heap value must bind @@ -257,7 +225,7 @@ fn scalar_replaced_object_field_holding_a_heap_value_is_bound() { ); assert!( - bind_calls(&ir) > 0, + value_slot_binds(main_ir(&ir)) > 0, "the scalar-replaced field alloca holding a heap value must be bound \ as a precise root (#6968):\n{ir}" ); @@ -284,14 +252,14 @@ fn scalar_replaced_object_field_holding_a_heap_value_is_bound() { ], ); assert!( - frame_slot_count(&ir) > frame_slot_count(&control), + frame_slot_count(main_ir(&ir)) > frame_slot_count(main_ir(&control)), "binding a scalar-replacement alloca must grow the shadow frame beyond \ what the pre-lowering pointer analysis reserved: heap-field literal \ has {} slots, the numeric-only control has {} — the pre-lowering pass \ cannot see these allocas, so the extra slot can only come from \ `reserve_shadow_slot` (#6968):\n{ir}", - frame_slot_count(&ir), - frame_slot_count(&control), + frame_slot_count(main_ir(&ir)), + frame_slot_count(main_ir(&control)), ); } @@ -321,11 +289,24 @@ fn numeric_only_scalar_replaced_object_emits_no_rooting() { ], ); + let main = main_ir(&ir); assert_eq!( - bind_calls(&ir), + value_slot_binds(main), 0, "a scalar-replaced literal with only numeric fields must not pay for \ - GC rooting:\n{ir}" + GC rooting:\n{main}" + ); + // Non-vacuity, in the same test: this module DOES bind — three times, for + // the `console.log` argument accumulator. Before #7504 those three were + // counted as the literal's, which is what made `== 0` fail; before #7493 + // the count was zero for every program under the native default, which is + // what made it pass for nothing. Assert the witness so neither can recur + // silently. + assert!( + temp_root_slot_binds(main) > 0, + "this fixture's `console.log` must still bind its pooled argument \ + accumulator — with no bind anywhere in the module, `zero value-slot \ + binds` is a claim about an empty set (#7504):\n{main}" ); } @@ -352,7 +333,7 @@ fn scalar_replaced_array_element_holding_a_heap_value_is_bound() { ); assert!( - bind_calls(&ir) > 0, + value_slot_binds(main_ir(&ir)) > 0, "the scalar-replaced array element alloca holding a heap value must be \ bound as a precise root (#6968):\n{ir}" ); @@ -377,10 +358,16 @@ fn numeric_only_scalar_replaced_array_emits_no_rooting() { ], ); + let main = main_ir(&ir); assert_eq!( - bind_calls(&ir), + value_slot_binds(main), 0, - "a scalar-replaced numeric array literal must not pay for GC rooting:\n{ir}" + "a scalar-replaced numeric array literal must not pay for GC rooting:\n{main}" + ); + assert!( + temp_root_slot_binds(main) > 0, + "the `console.log` accumulator must still bind, so `zero value-slot \ + binds` is a claim about a module in which binds exist (#7504):\n{main}" ); } @@ -435,11 +422,12 @@ fn scalar_replaced_split_parts_are_bound() { ); assert!( - bind_calls(&ir) > bind_calls(&control), + value_slot_binds(main_ir(&ir)) > value_slot_binds(main_ir(&control)), "the scalar-replaced split part slots must be bound as precise roots: \ - split IR has {} binds, the split-free control has {} (#6968):\n{ir}", - bind_calls(&ir), - bind_calls(&control), + split IR has {} value-slot binds, the split-free control has {} \ + (#6968):\n{ir}", + value_slot_binds(main_ir(&ir)), + value_slot_binds(main_ir(&control)), ); } @@ -466,7 +454,7 @@ fn later_store_into_a_scalar_replaced_field_is_bound() { ); assert!( - bind_calls(&ir) > 0, + value_slot_binds(main_ir(&ir)) > 0, "a heap value assigned into a scalar-replaced field after construction \ must be rooted as well (#6968):\n{ir}" ); @@ -488,6 +476,11 @@ fn later_store_into_a_scalar_replaced_field_is_bound() { /// Teeth: pre-hoist this IR carried one bind per store site (2), so the /// equality fails on the old compiler. It also fails if a future change drops /// the bind altogether (0), which would un-root the alloca and reopen #6968. +/// +/// The count is per-SLOT since #7504. The whole-module version of this +/// assertion read 4 on today's compiler — one for the field, three for the +/// `console.log` accumulator's pooled temp root — so `== 1` had stopped being a +/// statement about the field at all. #[test] fn repeated_stores_into_one_scalar_slot_bind_once() { let _pin = NativeRootsPin::shadow(); @@ -514,11 +507,12 @@ fn repeated_stores_into_one_scalar_slot_bind_once() { ); assert_eq!( - bind_calls(&ir), + value_slot_binds(main_ir(&ir)), 1, "two heap stores into one scalar-replaced field must share a single \ entry-hoisted bind — the alloca address is loop-invariant, so \ - re-binding is pure per-store cost (#7013):\n{ir}" + re-binding is pure per-store cost (#7013):\n{}", + main_ir(&ir) ); } @@ -557,11 +551,13 @@ fn every_store_into_a_hoisted_scalar_slot_shades_its_value() { ); assert_eq!( - root_barriers(&ir), + value_slot_barriers(main_ir(&ir)), 2, "each of the two heap stores must shade the value it wrote; the \ hoisted bind only shades what the alloca held at function entry \ - (#7013):\n{ir}" + (#7013). Barriers by slot: {:?}\n{}", + perry_codegen::testing::root_slots::barriers_by_slot(main_ir(&ir)), + main_ir(&ir) ); // The barrier must be the guarded form, not an unconditional call: the @@ -615,21 +611,31 @@ fn bind_is_hoisted_into_the_entry_block_ahead_of_the_storing_loop() { ], ); + let body = main_ir(&ir); assert_eq!( - bind_calls(&ir), + value_slot_binds(body), 1, "a scalar-replaced field stored once per iteration must be bound once, \ - not once per iteration (#7013):\n{ir}" + not once per iteration (#7013):\n{body}" ); - let body = enclosing_function(&ir, "call void @js_shadow_slot_bind("); + // The ordering claim is about THAT bind, not about whichever bind happens + // to come first in the function — the pooled temp roots of the in-loop + // `console.log` bind too, inside the loop body, and a `body.find("call void + // @js_shadow_slot_bind(")` would in general land on one of them. Name the + // slot. + let value_slot = perry_codegen::testing::root_slots::bound_slots(body) + .into_iter() + .find(|(_, (kind, _))| *kind == perry_codegen::testing::root_slots::SlotKind::Value) + .map(|(slot, _)| slot) + .expect("the value-slot bind was just counted"); // #7088: the push is `js_shadow_frame_enter` (returns the state pointer). let push = body .find("call ptr @js_shadow_frame_enter(") .unwrap_or_else(|| panic!("no frame enter in the binding function:\n{body}")); let bind = body - .find("call void @js_shadow_slot_bind(") - .expect("bind was located by enclosing_function"); + .find(&format!(", ptr {value_slot})")) + .unwrap_or_else(|| panic!("no bind naming {value_slot} in:\n{body}")); let first_branch = body .find("\n br label %") .unwrap_or_else(|| panic!("no entry-block terminator in:\n{body}")); @@ -678,10 +684,15 @@ fn scalar_replaced_array_element_slots_are_initialized_before_the_bind() { ], ); - let body = enclosing_function(&ir, "call void @js_shadow_slot_bind("); + let body = main_ir(&ir); + let value_slot = perry_codegen::testing::root_slots::bound_slots(body) + .into_iter() + .find(|(_, (kind, _))| *kind == perry_codegen::testing::root_slots::SlotKind::Value) + .map(|(slot, _)| slot) + .unwrap_or_else(|| panic!("no element slot was bound at all in:\n{body}")); let bind = body - .find("call void @js_shadow_slot_bind(") - .expect("bind was located by enclosing_function"); + .find(&format!(", ptr {value_slot})")) + .unwrap_or_else(|| panic!("no bind naming {value_slot} in:\n{body}")); // TAG_UNDEFINED as the double literal codegen emits for it. let undef = @@ -722,16 +733,23 @@ fn numeric_only_scalar_replaced_literal_emits_no_entry_rooting() { ], ); + let main = main_ir(&ir); assert_eq!( - bind_calls(&ir), + value_slot_binds(main), 0, "a proven-numeric literal must not acquire an entry-hoisted bind \ - (#7013):\n{ir}" + (#7013):\n{main}" ); assert_eq!( - root_barriers(&ir), + value_slot_barriers(main), 0, "a proven-numeric literal must not emit a store-site shading barrier \ - (#7013):\n{ir}" + (#7013). Barriers by slot: {:?}\n{main}", + perry_codegen::testing::root_slots::barriers_by_slot(main), + ); + assert!( + temp_root_slot_binds(main) > 0, + "the `console.log` accumulator must still bind, so both zeroes above \ + are claims about a module in which rooting exists (#7504):\n{main}" ); } diff --git a/crates/perry-codegen/tests/shadow_slot_hygiene.rs b/crates/perry-codegen/tests/shadow_slot_hygiene.rs index 5b99c38bae..885e026eb6 100644 --- a/crates/perry-codegen/tests/shadow_slot_hygiene.rs +++ b/crates/perry-codegen/tests/shadow_slot_hygiene.rs @@ -26,6 +26,7 @@ //! in #7502; where a mechanic has no native-side counterpart today, that issue //! names it. +use perry_codegen::testing::root_slots; use perry_codegen::testing::NativeRootsPin; use perry_codegen::{compile_module, AppMetadata, CompileOptions}; use perry_hir::types::Type; @@ -889,8 +890,42 @@ fn immutable_index_alias_binds_once_but_keeps_incremental_root_barrier() { ); } +/// A flat-const nested array literal reserves one shadow slot per pointer- +/// capable local, every reserved slot is either bound or cleared, and NONE of +/// them comes from #7487's temp-root pool. +/// +/// # What this test used to say, and why it was wrong twice over (#7504) +/// +/// It was `flat_const_row_aliases_do_not_reserve_shadow_slots`, and it asserted +/// `js_shadow_frame_enter(i32 1)` — "only the flat-const table root should +/// reserve a shadow slot". Two separate defects: +/// +/// 1. **The count is a module-level total, and #7487 gave temporaries a claim +/// on it.** That is #7504's subject, and it is the half this test can settle: +/// measured here, the temp pool contributes **zero** binds and zero +/// reservations, so the three reserved slots are entirely the locals'. The +/// two causes the issue asked to separate are separated, and only one of them +/// is present. +/// 2. **The property itself is not the contract, and satisfying it would be a +/// GC bug.** `kernel` is lowered as a real heap array (`js_inline_arena_*`), +/// so `krow = kernel[0]` holds a heap pointer and `k = krow[1]` is an `Any` +/// codegen cannot prove numeric. Leaving either unrooted is #6968 exactly. +/// The assertion presumed a flat-const lowering that emits the rows as +/// static data; this fixture does not receive one. That gap is real and +/// worth its own issue, but it is an OPTIMIZATION gap, not a hygiene +/// regression, and asserting it here made a rooting suite red for a +/// performance reason. +/// +/// The second assertion had also gone quietly toothless: it forbade +/// `js_shadow_slot_set(i32 1`, while #7013 moved the per-slot traffic to +/// `js_shadow_slot_bind`. The row aliases were touching slot 1 the whole time, +/// through a spelling the negative did not name. +/// +/// What is asserted now is the hygiene property this suite exists for: no slot +/// is reserved and then left untouched. A reserved-but-never-bound slot is the +/// #7184 shape — the collector scans a frame entry that no store ever reached. #[test] -fn flat_const_row_aliases_do_not_reserve_shadow_slots() { +fn flat_const_locals_reserve_and_use_every_slot_they_claim() { let _pin = NativeRootsPin::shadow(); let ir = String::from_utf8( compile_module(&flat_const_row_alias_shadow_module(), entry_opts()).unwrap(), @@ -898,19 +933,43 @@ fn flat_const_row_aliases_do_not_reserve_shadow_slots() { .expect("LLVM IR should be UTF-8"); let main_ir = function_slice(&ir, "main"); - // PRE-EXISTING RED, not caused by #7088: verified by running this suite - // against pristine `origin/main` sources, where the same assertion fails - // with three reserved slots instead of one. Two row aliases now take a - // persistent shadow slot each. This suite runs nightly/at-tag rather than - // per-PR, which is how it went red unnoticed. Kept asserting the intended - // property, with the string updated for `js_shadow_frame_enter`. + let reserved = root_slots::frame_slot_count(main_ir); assert!( - main_ir.contains("call ptr @js_shadow_frame_enter(i32 1)"), - "only the flat-const table root should reserve a shadow slot" + reserved > 0, + "with an empty frame the per-slot loop below iterates zero times and \ + certifies nothing — this assertion has no subject:\n{main_ir}" + ); + assert_eq!( + root_slots::temp_root_slot_binds(main_ir), + 0, + "#7504's separation: this fixture has no allocating call between \ + operands, so the pooled temp roots contribute nothing here and all {} \ + reserved slots belong to locals. If that changes, the numbers below \ + must be re-derived rather than adjusted:\n{main_ir}", + reserved, + ); + + let touched: std::collections::BTreeSet = (0..reserved) + .filter(|idx| { + main_ir.contains(&format!("call void @js_shadow_slot_bind(i32 {idx}, ptr %")) + || main_ir.contains(&format!("call void @js_shadow_slot_set(i32 {idx}")) + }) + .collect(); + assert_eq!( + touched.len() as u32, + reserved, + "every reserved shadow slot must be bound or cleared; slot(s) {:?} of \ + {reserved} were reserved and never touched, which is the #7184 shape — \ + the collector scans a frame entry no store reached:\n{main_ir}", + (0..reserved) + .filter(|idx| !touched.contains(idx)) + .collect::>(), ); + assert!( - !main_ir.contains("call void @js_shadow_slot_set(i32 1"), - "row aliases of flat-const tables must not touch shadow slots" + root_slots::value_slot_binds(main_ir) > 0, + "the row aliases hold heap arrays read out of `kernel`; leaving them \ + unrooted is #6968:\n{main_ir}" ); } diff --git a/crates/perry-codegen/tests/temp_root_argument_temporaries.rs b/crates/perry-codegen/tests/temp_root_argument_temporaries.rs deleted file mode 100644 index cf52348a4a..0000000000 --- a/crates/perry-codegen/tests/temp_root_argument_temporaries.rs +++ /dev/null @@ -1,418 +0,0 @@ -//! #6951: evaluated-but-not-yet-consumed argument temporaries must be precise -//! GC roots, not bare LLVM SSA registers. -//! -//! The end-to-end proof lives in the GC × representation matrix -//! (`scripts/gc_repsel_matrix.sh`, `cons_scan_off` arm), which runs the corpus -//! with `PERRY_CONSERVATIVE_STACK_SCAN=off` — the only configuration where the -//! bug is observable, because every automatic collection otherwise forces a -//! conservative native-stack scan that pins the temporary by accident. These -//! tests pin the *codegen contract* that arm depends on, in-process and in -//! `cargo-test`, so a lowering path that quietly goes back to threading an -//! accumulator through an SSA register fails here rather than three weeks -//! later under a narrowed forced scan. -//! -//! LOWERING (#7493): nothing in this file is pinned, because nothing in it is -//! lowering-dependent — `PERRY_RS4GC=0` moves it not at all (3/7 either way). -//! Its failures are #7487's, not #7370's, and are tracked in #7503: temp roots -//! were re-lowered onto pooled frame allocas, so `js_gc_temp_root_push` / -//! `_get` / `_truncate` now appear only on the FFI fallback arm that neither -//! lowering takes. The four positive assertions therefore fail, and all three -//! `!ir.contains("call i32 @js_gc_temp_root_push")` negatives — the entire -//! passing half of this suite — are VACUOUS: they hold for every program, -//! rooted or not. The #6951 contract is still emitted (store into an entry -//! alloca, root-bind, re-load after the allocating call); only the spelling -//! these assertions search for is gone, so the suite currently proves nothing -//! in either direction. - -use perry_codegen::{compile_module, AppMetadata, CompileOptions}; -use perry_hir::{Expr, Module, ModuleInitKind, Stmt}; - -fn entry_opts() -> CompileOptions { - CompileOptions { - target: None, - is_entry_module: true, - non_entry_module_prefixes: Vec::new(), - nextjs_path_init_modules: Vec::new(), - import_function_prefixes: std::collections::HashMap::new(), - import_function_ffi_aliases: std::collections::HashMap::new(), - import_function_origin_names: std::collections::HashMap::new(), - import_function_v8_specifiers: std::collections::HashMap::new(), - import_function_node_submodule: std::collections::HashMap::new(), - namespace_node_submodules: std::collections::HashMap::new(), - namespace_v8_specifiers: std::collections::HashMap::new(), - namespace_member_prefixes: std::collections::HashMap::new(), - namespace_member_origin_names: std::collections::HashMap::new(), - emit_ir_only: true, - verify_native_regions: false, - disable_buffer_fast_path: false, - namespace_imports: Vec::new(), - imported_classes: Vec::new(), - imported_enums: Vec::new(), - imported_async_funcs: std::collections::HashSet::new(), - type_aliases: std::collections::HashMap::new(), - imported_func_param_counts: std::collections::HashMap::new(), - imported_func_has_rest: std::collections::HashSet::new(), - imported_func_synthetic_arguments: std::collections::HashSet::new(), - imported_func_return_types: std::collections::HashMap::new(), - imported_vars: std::collections::HashSet::new(), - output_type: "executable".to_string(), - needs_stdlib: false, - needs_ui: false, - needs_geisterhand: false, - geisterhand_port: 7676, - enabled_features: Vec::new(), - native_module_init_names: Vec::new(), - js_module_specifiers: Vec::new(), - bundled_extensions: Vec::new(), - native_library_functions: Vec::new(), - i18n_table: None, - fast_math: false, - fp_contract_mode: perry_codegen::FpContractMode::Off, - app_metadata: AppMetadata::default(), - namespace_entries: Vec::new(), - dynamic_import_path_to_prefix: std::collections::HashMap::new(), - deferred_module_prefixes: std::collections::HashSet::new(), - module_init_deps: Vec::new(), - is_dynamic_import_target: false, - debug_locations: false, - module_source: None, - debug_source_line_offset: 0, - } -} - -fn module_with_init(name: &str, init: Vec) -> Module { - Module { - name: name.to_string(), - imports: Vec::new(), - exports: Vec::new(), - classes: Vec::new(), - interfaces: Vec::new(), - type_aliases: Vec::new(), - enums: Vec::new(), - globals: Vec::new(), - functions: Vec::new(), - script_global_functions: Vec::new(), - references_global_this: false, - annexb_global_undefined_names: Vec::new(), - init, - exported_native_instances: Vec::new(), - exported_func_return_native_instances: Vec::new(), - exported_objects: Vec::new(), - exported_functions: Vec::new(), - widgets: Vec::new(), - uses_fetch: false, - uses_webassembly: false, - extern_funcs: Vec::new(), - init_was_unrolled: false, - has_top_level_await: false, - init_kind: ModuleInitKind::Eager, - async_step_closures: std::collections::HashSet::new(), - closure_display_names: std::collections::HashMap::new(), - class_display_names: std::collections::HashMap::new(), - closure_source_text: std::collections::HashMap::new(), - async_generator_funcs: std::collections::HashSet::new(), - gen_param_prologue_len: std::collections::HashMap::new(), - } -} - -fn ir_for(name: &str, init: Vec) -> String { - String::from_utf8(compile_module(&module_with_init(name, init), entry_opts()).unwrap()) - .expect("LLVM IR should be UTF-8") -} - -/// `console.log(a, b, …)` — the shape in the #6951 repro. -fn console_log(args: Vec) -> Stmt { - Stmt::Expr(Expr::Call { - callee: Box::new(Expr::PropertyGet { - object: Box::new(Expr::GlobalGet(0)), - property: "log".to_string(), - byte_offset: 0, - }), - args, - type_args: Vec::new(), - byte_offset: 0, - }) -} - -/// An allocating argument: an object literal is a collection point, which is -/// all `expr_may_trigger_gc` needs to see. -fn allocating() -> Expr { - Expr::Object(Vec::new()) -} - -/// The argument accumulator of a variadic call must live in a temp root, and -/// every use of it must be re-read from that root. -/// -/// Pre-fix the sequence was `js_array_alloc` → N × `js_array_push_f64` with the -/// accumulator threaded through an SSA register across each argument's -/// evaluation. That register held the ONLY reference to everything pushed so -/// far, so an allocating later argument swept the half-built array and the -/// next push landed in recycled memory — `console.log("label", churn())` lost -/// its label with no crash and no diagnostic. -#[test] -fn console_argument_accumulator_is_temp_rooted() { - let ir = ir_for( - "console_accumulator.ts", - vec![console_log(vec![ - Expr::String("label".to_string()), - allocating(), - ])], - ); - - assert!( - ir.contains("call i32 @js_gc_temp_root_push"), - "the accumulator must be pushed onto the temp-root stack:\n{ir}" - ); - assert!( - ir.contains("call void @js_array_push_f64_temp_rooted"), - "arguments must be appended through the rooted push, which reads the \ - accumulator out of its slot and writes the reallocated pointer back:\n{ir}" - ); - assert!( - ir.contains("call i64 @js_gc_temp_root_get"), - "the accumulator must be RE-READ before the consuming call — an \ - evacuating cycle rewrites the slot, so the pushed register is stale:\n{ir}" - ); - assert!( - ir.contains("call void @js_gc_temp_root_truncate"), - "the temp root must be released after the consuming call:\n{ir}" - ); - - // The accumulator must not survive as a threaded SSA register: the raw - // two-operand push is what made it unrooted in the first place. - assert!( - !ir.contains("call i64 @js_array_push_f64(i64"), - "the console argument list must not thread the accumulator through an \ - SSA register any more (#6951):\n{ir}" - ); - - // Ordering: read, consume, then release. - let get = ir.find("call i64 @js_gc_temp_root_get").unwrap(); - let consume = ir.find("call void @js_console_log_spread").unwrap(); - let truncate = ir.find("call void @js_gc_temp_root_truncate").unwrap(); - assert!( - get < consume && consume < truncate, - "the accumulator must be read before the consumer runs and released \ - only after it returns:\n{ir}" - ); -} - -/// The gate: rooting is emitted only when something that follows can collect. -/// -/// An array literal of plain string literals has no allocation between its -/// elements' evaluation and the array's construction, so it must emit exactly -/// the IR it emitted before #6951 — no runtime calls, no cost. -#[test] -fn non_allocating_element_list_emits_no_rooting_calls() { - let ir = ir_for( - "array_literal_no_gc.ts", - vec![Stmt::Expr(Expr::Array(vec![ - Expr::String("a".to_string()), - Expr::String("b".to_string()), - Expr::Number(1.0), - ]))], - ); - - assert!( - !ir.contains("call i32 @js_gc_temp_root_push"), - "an all-literal array literal must not pay for temp rooting (the \ - `declare` line is unconditional; only an emitted CALL counts):\n{ir}" - ); -} - -/// …and it IS emitted when an earlier element is a heap value and a later one -/// allocates: that value sits in an SSA register across the allocation, which -/// is not a root. -#[test] -fn array_literal_roots_elements_before_an_allocating_element() { - let ir = ir_for( - "array_literal_gc.ts", - // Element 0 is itself a heap value (a literal would be skipped: it - // loads from a module global that is already a registered GC root). - vec![Stmt::Expr(Expr::Array(vec![allocating(), allocating()]))], - ); - - assert!( - ir.contains("call i32 @js_gc_temp_root_push"), - "an array literal with an allocating later element must root the \ - heap elements already evaluated (#6951):\n{ir}" - ); -} - -// --------------------------------------------------------------------------- -// #6996: the gate must not root values that cannot be heap references. -// -// `native_abi_packet_control`'s kernel is `(buf[i] + packet.tag + i) & 255`. -// The left operand is a byte; the right is a property get on an `any`, so the -// add lowers through `js_dynamic_string_or_number_add` and the operand pair is -// rooted across the property get. Rooting the byte protected nothing and cost -// a push / re-read / truncate on every one of the loop's 262 144 iterations — -// it turned the `hot_loops_no_runtime_calls` contract red. - -/// A local declared with a Buffer annotation, so the receiver of the element -/// read is a plain local get (the shape the fixture's parameter has). -fn buffer_local(id: u32) -> Stmt { - Stmt::Let { - id, - name: "buf".to_string(), - ty: perry_hir::types::Type::Named("Buffer".to_string()), - mutable: false, - init: Some(Expr::Undefined), - } -} - -/// An `any` local, so a property get on it is neither numeric-proven nor -/// allocation-free — exactly what forces the dynamic add and its rooting. -fn any_local(id: u32) -> Stmt { - Stmt::Let { - id, - name: "packet".to_string(), - ty: perry_hir::types::Type::Any, - mutable: false, - init: Some(Expr::Undefined), - } -} - -fn packet_tag(id: u32) -> Expr { - Expr::PropertyGet { - object: Box::new(Expr::LocalGet(id)), - property: "tag".to_string(), - byte_offset: 0, - } -} - -/// ` + packet.tag`, bound to a local so nothing folds it away. -fn dynamic_add_with_element_read(element: Expr, packet_id: u32) -> Stmt { - Stmt::Let { - id: 90, - name: "next".to_string(), - ty: perry_hir::types::Type::Number, - mutable: false, - init: Some(Expr::Binary { - op: perry_hir::BinaryOp::Add, - left: Box::new(element), - right: Box::new(packet_tag(packet_id)), - }), - } -} - -/// A proven-index typed-array element read is a byte (or `undefined` out of -/// range) by construction, so it must not be rooted. -#[test] -fn buffer_element_operand_is_not_temp_rooted() { - let ir = ir_for( - "typed_element_operand.ts", - vec![ - buffer_local(0), - any_local(1), - dynamic_add_with_element_read( - Expr::Uint8ArrayGet { - array: Box::new(Expr::LocalGet(0)), - index: Box::new(Expr::Integer(0)), - }, - 1, - ), - ], - ); - - assert!( - ir.contains("call double @js_dynamic_string_or_number_add"), - "the test must actually reach the rooted operand-pair lowering:\n{ir}" - ); - assert!( - !ir.contains("call i32 @js_gc_temp_root_push"), - "a proven-index typed-array element read is number-or-undefined by \ - construction — rooting it protects nothing and costs three runtime \ - calls per iteration (#6996):\n{ir}" - ); -} - -/// Same for a `Buffer`-node element read, whose every lowering coerces the key -/// to i32 and reads a byte. -#[test] -fn buffer_index_get_operand_is_not_temp_rooted() { - let ir = ir_for( - "buffer_index_operand.ts", - vec![ - buffer_local(0), - any_local(1), - dynamic_add_with_element_read( - Expr::BufferIndexGet { - buffer: Box::new(Expr::LocalGet(0)), - index: Box::new(Expr::Integer(0)), - }, - 1, - ), - ], - ); - - assert!( - !ir.contains("call i32 @js_gc_temp_root_push"), - "a Buffer element read is a byte or `undefined` on every lowering \ - path (#6996):\n{ir}" - ); -} - -/// The soundness boundary, held from the other side: a symbol key does NOT -/// read an element. It resolves through `js_object_get_symbol_property`, which -/// hands back a `%TypedArray%.prototype` accessor — a heap value that an -/// allocating sibling can sweep. It must still be rooted. -#[test] -fn symbol_keyed_typed_array_read_is_still_temp_rooted() { - let ir = ir_for( - "typed_symbol_key_operand.ts", - vec![ - buffer_local(0), - any_local(1), - dynamic_add_with_element_read( - Expr::Uint8ArrayGet { - array: Box::new(Expr::LocalGet(0)), - index: Box::new(Expr::SymbolFor(Box::new(Expr::String( - "Symbol.iterator".to_string(), - )))), - }, - 1, - ), - ], - ); - - assert!( - ir.contains("call i32 @js_gc_temp_root_push"), - "a symbol-keyed read returns a prototype accessor, not a byte — the \ - #6996 skip must not reach it:\n{ir}" - ); -} - -/// The other excluded lowering: without the integer-array-index proof the read -/// goes to `js_typed_array_index_get_dynamic`, which falls through to -/// string-keyed property lookup, and an expando can hold anything. -#[test] -fn unproven_key_typed_array_read_is_still_temp_rooted() { - let ir = ir_for( - "typed_unproven_key_operand.ts", - vec![ - buffer_local(0), - any_local(1), - Stmt::Let { - id: 2, - name: "k".to_string(), - ty: perry_hir::types::Type::Any, - mutable: false, - init: Some(Expr::Undefined), - }, - dynamic_add_with_element_read( - Expr::Uint8ArrayGet { - array: Box::new(Expr::LocalGet(0)), - index: Box::new(Expr::LocalGet(2)), - }, - 1, - ), - ], - ); - - assert!( - ir.contains("call i32 @js_gc_temp_root_push"), - "an unproven key reads a property, not an element — the #6996 skip \ - must not reach it:\n{ir}" - ); -} diff --git a/crates/perry-codegen/tests/temp_root_operand_temporaries.rs b/crates/perry-codegen/tests/temp_root_operand_temporaries.rs index b627376b42..e9fc9804db 100644 --- a/crates/perry-codegen/tests/temp_root_operand_temporaries.rs +++ b/crates/perry-codegen/tests/temp_root_operand_temporaries.rs @@ -26,16 +26,32 @@ //! its `!contains("@js_shadow_slot_bind")` is true of every program (hazard 4: //! the gate ran, its subject did not). //! -//! The rest of this file is lowering-INDEPENDENT and deliberately unpinned — -//! but READ #7503 BEFORE TRUSTING IT. #7487 re-lowered temp roots onto pooled -//! frame allocas, so `js_gc_temp_root_push` / `_get` / `_set` / `_truncate` are -//! now emitted only on the FFI fallback arm, which neither lowering takes here. -//! Every positive assertion spelled that way fails, and — worse — every -//! `!ir.contains("call i32 @js_gc_temp_root_push")` passes vacuously. The -//! contract itself is intact (the value is stored into an entry alloca, root- -//! bound, and re-loaded after the allocating call); only the spelling these -//! assertions look for is gone. - +//! The rest of this file is lowering-INDEPENDENT and deliberately unpinned. +//! +//! SPELLING (#7503). Until #7487 the temp-root contract was three runtime calls +//! and this file asserted the calls. #7487 re-lowered temp roots onto pooled +//! frame allocas — push became a store, get a load, truncate a slot clear — so +//! `js_gc_temp_root_push` / `_get` / `_set` / `_truncate` now survive only on an +//! FFI fallback arm that neither shipped lowering takes. Six positive +//! assertions here failed, and — worse — every +//! `!ir.contains("call i32 @js_gc_temp_root_push")` NEGATIVE held for every +//! program in the language, rooted or not. The contract itself never changed. +//! +//! Every rooting claim now goes through `perry_codegen::testing::temp_slots`, +//! which names the VALUE — this producer's result went into a rooted slot, and +//! this consuming call read it back OUT of that slot — and understands both +//! lowerings' spelling of a slot. That is strictly stronger than the call- +//! existence check it replaces: `contains("…temp_root_push")` only ever proved +//! that SOME call existed SOMEWHERE in the module. +//! +//! The argument-accumulator half of this family lives in +//! `perry_codegen::temp_root_coverage` since #6988 — in `src/`, so it runs in +//! the per-PR `cargo-test` gate rather than the nightly-only integration tier. + +use perry_codegen::testing::temp_slots::{ + assert_no_temp_rooting, assert_rooted_across, derives_from_slot_load, first_call_result, + slot_holding, slot_traffic, temp_root_slots, SlotEvent, +}; use perry_codegen::testing::NativeRootsPin; use perry_codegen::{compile_module, AppMetadata, CompileOptions}; use perry_hir::{Class, Expr, Module, ModuleInitKind, Stmt}; @@ -159,25 +175,22 @@ fn map_set_key_is_rooted_across_an_allocating_value() { })], ); - assert!( - ir.contains("call i32 @js_gc_temp_root_push"), - "the receiver and key must be pushed onto the temp-root stack before \ - the value's lowering, which collects (#6970):\n{ir}" - ); - assert!( - ir.contains("call i64 @js_gc_temp_root_get"), - "they must be RE-READ after the value is lowered — the slot is a \ - mutable root and an evacuating cycle rewrites it:\n{ir}" - ); + let f = init_ir(&ir); + let key = first_call_result(f, "js_object_alloc") + .unwrap_or_else(|| panic!("the key must allocate, or this proves nothing:\n{f}")); + assert_rooted_across(f, &key, "js_map_set", "#6970 map.set key"); - let push = ir.find("call i32 @js_gc_temp_root_push").unwrap(); - let get = ir.find("call i64 @js_gc_temp_root_get").unwrap(); - let consume = ir.find("call i64 @js_map_set(").unwrap(); - let truncate = ir.find("call void @js_gc_temp_root_truncate").unwrap(); + let slot = slot_holding(f, &key).expect("assert_rooted_across just found the slot"); + let consume = f + .lines() + .position(|line| line.contains("@js_map_set(")) + .expect("assert_rooted_across just found the call"); assert!( - push < get && get < consume && consume < truncate, - "order must be push → re-read → consuming call → release; the release \ - comes last because js_map_set allocates while it reads the key:\n{ir}" + slot_traffic(f)[&slot] + .iter() + .any(|e| matches!(e, SlotEvent::Clear { line } if *line > consume)), + "the release comes AFTER the consuming call, because js_map_set \ + allocates while it reads the key:\n{f}" ); } @@ -193,10 +206,10 @@ fn map_set_with_a_non_allocating_value_emits_no_rooting_calls() { })], ); - assert!( - !ir.contains("call i32 @js_gc_temp_root_push"), - "nothing after the key can collect, so this must cost exactly what it \ - cost before (the `declare` line is unconditional; only a CALL counts):\n{ir}" + assert_no_temp_rooting( + init_ir(&ir), + "#6970 gate: nothing after the key can collect, so this must cost \ + exactly what it cost before", ); } @@ -225,21 +238,37 @@ fn concat_accumulator_is_rooted_and_written_back() { })], ); + let f = init_ir(&ir); + let slots = temp_root_slots(f); assert!( - ir.contains("call i32 @js_gc_temp_root_push"), + !slots.is_empty(), "the concat accumulator must be rooted across an allocating argument \ - (#6971):\n{ir}" - ); + (#6971):\n{f}" + ); + let traffic = slot_traffic(f); + let written_back = slots.iter().any(|slot| { + traffic[slot] + .iter() + .filter(|e| matches!(e, SlotEvent::Store { .. })) + .count() + >= 2 + }); assert!( - ir.contains("call void @js_gc_temp_root_set"), + written_back, "each js_string_concat yields a NEW address, so the accumulator must be \ - written back into its slot — otherwise the next argument's lowering \ - keeps the INPUT alive and sweeps the string under construction:\n{ir}" - ); + written BACK into its slot — otherwise the next argument's lowering \ + keeps the INPUT alive and sweeps the string under construction. Slot \ + traffic: {traffic:#?}\n{f}" + ); + let re_read = slots.iter().any(|slot| { + traffic[slot] + .iter() + .any(|e| matches!(e, SlotEvent::Load { .. })) + }); assert!( - ir.contains("call i64 @js_gc_temp_root_get"), + re_read, "the accumulator must be re-read after the argument (and its ToString \ - coercion, which also allocates):\n{ir}" + coercion, which also allocates):\n{f}" ); } @@ -265,9 +294,10 @@ fn string_method_with_non_allocating_args_emits_no_rooting_calls() { })], ); - assert!( - !ir.contains("call i32 @js_gc_temp_root_push"), - "a numeric argument cannot collect, so the receiver needs no root:\n{ir}" + assert_no_temp_rooting( + init_ir(&ir), + "#6971 gate: a numeric argument cannot collect, so the receiver needs \ + no root", ); } @@ -332,57 +362,74 @@ fn ir_for_new(name: &str, args: Vec) -> String { fn constructor_arguments_are_rooted_across_the_instance_allocation() { let ir = ir_for_new("ctor_args_rooted.ts", vec![allocating(), allocating()]); - assert!( - ir.contains("call i32 @js_gc_temp_root_push"), - "constructor arguments must be rooted (#6969):\n{ir}" - ); - assert!( - ir.contains("call i64 @js_gc_temp_root_get"), - "they must be re-read after the instance allocation:\n{ir}" - ); + let f = init_ir(&ir); + let traffic = slot_traffic(f); - // Push order: the scope marker, then one root per heap argument — and - // argument 0's push must precede argument 1's *lowering*, not merely - // precede the instance allocation. - let pushes: Vec = ir - .match_indices("call i32 @js_gc_temp_root_push") - .map(|(i, _)| i) - .collect(); - assert!( - pushes.len() >= 3, - "expected the scope marker plus a root for each of the two heap \ - arguments, got {} pushes:\n{ir}", - pushes.len() - ); // Each argument is an object literal, so each lowers to its own // `js_object_alloc`; the SECOND one is argument 1's, i.e. the collection // point argument 0 has to survive. - let arg_allocs: Vec = ir - .match_indices("call i64 @js_object_alloc(") - .map(|(i, _)| i) + let arg_allocs: Vec<(usize, String)> = f + .lines() + .enumerate() + .filter(|(_, l)| l.contains("call i64 @js_object_alloc(")) + .filter_map(|(i, l)| l.trim().split_once(" = ").map(|(r, _)| (i, r.to_string()))) .collect(); - assert!(arg_allocs.len() >= 2, "both arguments allocate:\n{ir}"); - assert!( - pushes[1] < arg_allocs[1], - "argument 0 must be rooted BEFORE argument 1 is lowered — rooting the \ - whole list after the loop publishes an already-dangling pointer (#6969):\n{ir}" - ); + assert_eq!(arg_allocs.len(), 2, "both arguments allocate:\n{f}"); - let instance_alloc = ir - .find("call i64 @js_object_alloc_class_inline_keys") - .expect("the instance allocation"); - let get = ir.find("call i64 @js_gc_temp_root_get").unwrap(); + let (arg1_line, _) = &arg_allocs[1]; + let (_, arg0) = &arg_allocs[0]; + let slot = slot_holding(f, arg0) + .unwrap_or_else(|| panic!("constructor argument 0 ({arg0}) is never rooted (#6969):\n{f}")); + let arg0_store = traffic[&slot] + .iter() + .find_map(|e| match e { + SlotEvent::Store { line, .. } => Some(*line), + _ => None, + }) + .expect("slot_holding just found it"); assert!( - pushes[pushes.len() - 1] < instance_alloc && instance_alloc < get, - "every argument must still be rooted across the instance allocation, \ - and re-read after it:\n{ir}" + arg0_store < *arg1_line, + "argument 0 must be rooted BEFORE argument 1 is lowered — rooting the \ + whole list after the loop publishes an already-dangling pointer \ + (#6969). Store at {arg0_store}, argument 1 allocates at {arg1_line}:\n{f}" ); - let truncate = ir.find("call void @js_gc_temp_root_truncate").unwrap(); + // …and every argument is still rooted across the INSTANCE allocation, then + // re-read below it. That call is the one that always collects. + let instance_alloc = f + .lines() + .position(|l| l.contains("call i64 @js_object_alloc_class_inline_keys")) + .unwrap_or_else(|| panic!("the instance allocation:\n{f}")); assert!( - get < truncate, - "the scope cut must come after the arguments are consumed:\n{ir}" - ); + traffic + .values() + .flat_map(|events| events.iter()) + .any(|e| matches!(e, SlotEvent::Load { line, .. } if *line > instance_alloc)), + "the arguments must be re-read from their slots AFTER the instance \ + allocation — a register held across it names from-space (#6969):\n{f}" + ); + // The scope cut comes after the arguments are consumed — per SLOT. A + // module-wide "last load before any clear" would be answered by an + // unrelated named local's slot, which is the class of mistake this whole + // issue is about. + for slot in temp_root_slots(f) { + let events = &traffic[&slot]; + let last_load = events + .iter() + .filter_map(|e| match e { + SlotEvent::Load { line, .. } => Some(*line), + _ => None, + }) + .max() + .unwrap_or_else(|| panic!("{slot} was rooted and never read:\n{f}")); + assert!( + events + .iter() + .any(|e| matches!(e, SlotEvent::Clear { line } if *line > last_load)), + "{slot}'s scope cut must come after its last read (line {last_load}); \ + releasing early un-roots a value still in use:\n{f}" + ); + } } /// The gate: `new Pair(a, b)` on immediates must emit no rooting. @@ -399,10 +446,11 @@ fn constructor_arguments_on_plain_locals_emit_no_rooting_calls() { vec![Expr::Number(1.0), Expr::String("s".to_string())], ); - assert!( - !ir.contains("call i32 @js_gc_temp_root_push"), - "a number and a string literal need no root — the literal is a load \ - from a module global already registered with js_gc_register_global_root:\n{ir}" + assert_no_temp_rooting( + init_ir(&ir), + "#6969 gate: a number and a string literal need no root — the literal \ + is a load from a module global already registered with \ + js_gc_register_global_root", ); } @@ -441,13 +489,18 @@ fn registered_root_operands_are_reloaded_rather_than_rooted() { ); // And it must be a re-LOAD, not a temp root: a registered root needs no - // second liveness mechanism, so this must cost zero runtime calls for it. - // (The allocating operand still gets a real root — hence >= 1 push.) - let pushes = ir.matches("call i32 @js_gc_temp_root_push").count(); - assert!( - pushes <= 2, - "expected at most the scope marker plus the allocating operand's root; \ - the literal must not get a slot of its own, got {pushes}:\n{ir}" + // second liveness mechanism, so this must cost zero slot traffic for it. + // The allocating operand still gets a real root — so the count is exactly + // one, and "at most one" would let a regression that roots the literal but + // stops rooting the object pass. + let slots = temp_root_slots(init_ir(&ir)); + assert_eq!( + slots.len(), + 1, + "expected exactly the allocating operand's root; the literal must not \ + get a slot of its own, and the object must not lose one. Slots: \ + {slots:?}\n{}", + init_ir(&ir) ); } @@ -611,12 +664,11 @@ fn string_literal_concat_operand_is_re_derived_below_the_allocating_sibling() { address:\n{f}" ); - assert_eq!( - f.matches("call i32 @js_gc_temp_root_push").count(), - 0, - "and it must cost no runtime call. A registered root already has \ - liveness; all it was missing is the re-derivation, which is the load \ - that was going to be emitted anyway:\n{ir}" + assert_no_temp_rooting( + f, + "#7114: a registered root already has liveness; all it was missing is \ + the re-derivation, which is the load that was going to be emitted \ + anyway — so it must cost no slot", ); } @@ -649,10 +701,7 @@ fn string_literal_concat_operand_is_not_re_derived_when_nothing_collects() { "a comparison over two immediates runs no user code and allocates \ nothing, so the literal must be loaded exactly once:\n{f}" ); - assert!( - !f.contains("call i32 @js_gc_temp_root_push"), - "and no rooting at all:\n{f}" - ); + assert_no_temp_rooting(f, "#7114 gate: nothing collects, so no rooting at all"); } /// The same helper, reached from its other caller: an array literal's element @@ -675,19 +724,33 @@ fn string_literal_array_element_is_re_derived_below_an_allocating_element() { 1, "exactly one allocating element in @main:\n{f}" ); - assert_eq!( - f.matches(handle_pat).count(), - 2, - "element 0's handle must be loaded twice in @main: the original \ - lowering and the re-derivation below element 1's allocation:\n{f}" - ); - - let loads: Vec = f.match_indices(handle_pat).map(|(i, _)| i).collect(); - let element_alloc = f.find("call i64 @js_object_alloc(").unwrap(); + let element_alloc = f + .lines() + .position(|l| l.contains("call i64 @js_object_alloc(")) + .expect("just counted it"); + + // #7114's invariant admits TWO discharges, and which one a given operand + // gets is a lowering decision, not part of the contract: re-derive the + // value from immutable storage below the collection point, or park it in a + // rooted slot and re-read it from there. Asserting only the first is what + // made this test fail on a compiler that had switched to the second. What + // must never happen is the third thing: reusing the pre-collection + // register. + let re_derived = f + .lines() + .enumerate() + .any(|(i, l)| i > element_alloc && l.contains(handle_pat)); + let re_read = slot_traffic(f) + .values() + .flat_map(|events| events.iter()) + .any(|e| matches!(e, SlotEvent::Load { line, .. } if *line > element_alloc)); assert!( - loads[0] < element_alloc && loads[1] > element_alloc, - "#7114: one load above element 1's allocation and one below it, so the \ - value stored into the array is the post-relocation address:\n{f}" + re_derived || re_read, + "#7114: element 0 must reach the array store either re-derived from its \ + handle global BELOW element 1's allocation, or re-read from a rooted \ + slot below it. Neither happened, so the value stored is the \ + pre-relocation address. Slot traffic: {:#?}\n{f}", + slot_traffic(f) ); } @@ -719,18 +782,14 @@ fn wtf8_literal_operand_is_rooted_not_merely_reused() { ); let f = init_ir(&ir); + let slots = temp_root_slots(f); assert_eq!( - f.matches("call i32 @js_gc_temp_root_push").count(), + slots.len(), 1, "exactly one temp root in @main — the WTF-8 operand's. Zero means it \ was suppressed without a compensating re-derivation (#7114 for \ lone-surrogate literals); more than one means this assertion is no \ - longer about the operand it names:\n{f}" - ); - assert_eq!( - f.matches("call i64 @js_gc_temp_root_get").count(), - 1, - "and exactly one re-read of it:\n{f}" + longer about the operand it names. Slots: {slots:?}\n{f}" ); assert_eq!( f.matches("call i64 @js_object_alloc(").count(), @@ -738,14 +797,25 @@ fn wtf8_literal_operand_is_rooted_not_merely_reused() { "exactly one allocating sibling in @main:\n{f}" ); - let push = f.find("call i32 @js_gc_temp_root_push").unwrap(); - let alloc = f.find("call i64 @js_object_alloc(").unwrap(); - let get = f.find("call i64 @js_gc_temp_root_get").unwrap(); + let alloc = f + .lines() + .position(|l| l.contains("call i64 @js_object_alloc(")) + .expect("just counted it"); + let events = &slot_traffic(f)[&slots[0]]; + assert!( + events + .iter() + .any(|e| matches!(e, SlotEvent::Store { line, .. } if *line < alloc)), + "the WTF-8 literal must reach its slot BEFORE the allocating sibling \ + runs:\n{f}" + ); assert!( - push < alloc && alloc < get, - "order must be push -> allocating sibling -> re-read. Anything else \ - means the WTF-8 literal is being carried across the collection point \ - in a register, which is #7114 for lone-surrogate literals:\n{f}" + events + .iter() + .any(|e| matches!(e, SlotEvent::Load { line, .. } if *line > alloc)), + "…and be re-read AFTER it. Anything else means the literal is being \ + carried across the collection point in a register, which is #7114 for \ + lone-surrogate literals:\n{f}" ); } @@ -798,67 +868,40 @@ fn the_new_instance_is_rooted_across_the_constructor_body() { // Assertions are on the DEF-USE chain, not on textual order: the override // is emitted into the `ctor.return.after` block, which the writer appends // below the block that re-reads the root. - let def_of = |reg: &str| -> String { - let needle = format!(" %{reg} = "); - f.lines() - .find(|l| l.starts_with(&needle)) - .unwrap_or_else(|| panic!("no definition of %{reg} in:\n{f}")) - .to_string() - }; - let first_operand_reg = |line: &str| -> String { - line.split("%") - .nth(1) - .and_then(|s| { - s.split(|c: char| !(c.is_alphanumeric() || c == '_' || c == '.')) - .next() - }) - .unwrap_or_else(|| panic!("no register operand in `{line}`")) - .to_string() - }; - // 1. The allocation's result is pushed as a temp root immediately. - let alloc_line = f + // 1. The allocation's result reaches a rooted slot immediately. + let inst_reg = f .lines() .find(|l| l.contains("call i64 @js_object_alloc_class")) + .and_then(|l| l.trim().split_once(" = ").map(|(r, _)| r.to_string())) .unwrap_or_else(|| panic!("the instance allocation:\n{f}")); - let inst_reg = alloc_line - .trim_start() - .trim_start_matches('%') - .split(' ') - .next() - .unwrap() - .to_string(); assert!( - f.contains(&format!("call i32 @js_gc_temp_root_push(i64 %{inst_reg})")), - "the instance %{inst_reg} must be rooted as soon as it is allocated, \ - before the constructor body runs (#7154):\n{f}" + slot_holding(f, &inst_reg).is_some(), + "the instance {inst_reg} must be rooted as soon as it is allocated, \ + before the constructor body runs (#7154). Slot traffic: {:#?}\n{f}", + slot_traffic(f) ); - // 2. The value `js_ctor_return_override` publishes is re-derived from that + // 2. The value `js_ctor_return_override` publishes is re-read from that // root, not carried across the constructor in the original register. let override_line = f .lines() .find(|l| l.contains("call double @js_ctor_return_override")) .unwrap_or_else(|| panic!("the return-override:\n{f}")); - let mut reg = first_operand_reg( - override_line - .split_once("js_ctor_return_override") - .expect("split") - .1, - ); - let mut chain = vec![reg.clone()]; - for _ in 0..8 { - let d = def_of(®); - if d.contains("@js_gc_temp_root_get") { - return; - } - reg = first_operand_reg(d.split_once(" = ").expect("assignment").1); - chain.push(reg.clone()); - } - panic!( - "the instance handed to js_ctor_return_override must be re-read from \ - its temp root after the constructor body — walked {chain:?} without \ - reaching a js_gc_temp_root_get (#7154):\n{f}" + let published = override_line + .split_once("js_ctor_return_override(") + .expect("argument list") + .1 + .split(',') + .next() + .and_then(|a| a.trim().rsplit_once(' ').map(|(_, r)| r.to_string())) + .unwrap_or_else(|| panic!("no register operand in `{override_line}`")); + assert!( + derives_from_slot_load(f, &published, 8), + "the instance handed to js_ctor_return_override ({published}) must be \ + re-read from its root after the constructor body — the callee's own \ + `this` slot keeps it alive, so it MOVES, and the caller's register \ + names from-space (#7154):\n{f}" ); } @@ -868,10 +911,10 @@ fn the_new_instance_is_rooted_across_the_constructor_body() { #[test] fn a_class_that_runs_no_user_code_emits_no_instance_root() { let ir = ir_for_new("new_inst_no_ctor.ts", vec![Expr::Number(1.0)]); - assert!( - !ir.contains("call i32 @js_gc_temp_root_push"), - "nothing can collect between the allocation and the `new` value, so \ - rooting the instance would be pure TLS traffic:\n{ir}" + assert_no_temp_rooting( + init_ir(&ir), + "#7154 gate: nothing can collect between the allocation and the `new` \ + value, so rooting the instance would be pure cost", ); } @@ -1105,12 +1148,6 @@ fn the_object_assign_accumulator_is_rooted_across_each_source() { .expect("LLVM IR should be UTF-8"); let f = init_ir(&ir); - let def_of: std::collections::HashMap<&str, &str> = f - .lines() - .filter_map(|l| l.trim_start().split_once(" = ")) - .map(|(r, rhs)| (r.trim(), rhs)) - .collect(); - let calls: Vec<&str> = f .lines() .filter(|l| l.contains("@js_object_assign_one(")) @@ -1134,39 +1171,31 @@ fn the_object_assign_accumulator_is_rooted_across_each_source() { .and_then(|a| a.trim().strip_prefix("double ")) .unwrap_or_else(|| panic!("no accumulator operand in `{call}`")) .to_string(); - // Follow the bitcast back to the `js_gc_temp_root_get` that produced it. - let mut reg = acc.clone(); - let mut rooted = false; - for _ in 0..4 { - let Some(rhs) = def_of.get(reg.as_str()) else { - break; - }; - if rhs.contains("@js_gc_temp_root_get") { - rooted = true; - break; - } - let Some(next) = rhs - .split(|c: char| !(c.is_alphanumeric() || c == '%' || c == '.' || c == '_')) - .find(|w| w.starts_with('%')) - else { - break; - }; - reg = next.to_string(); - } assert!( - rooted, + derives_from_slot_load(f, &acc, 4), "the Object.assign accumulator passed to `{call}` must be re-read \ - from its temp root below the previous source's lowering, not \ - carried in a register (#7200):\n{f}" + from its root below the previous source's lowering, not carried \ + in a register (#7200). Slot traffic: {:#?}\n{f}", + slot_traffic(f) ); } - // And the result of each link must be republished, because the helper now - // returns the target's POST-collection address. + // And the result of each link must be republished, because the helper + // returns the target's POST-collection address. That is a SECOND store into + // the same slot — the pooled spelling of `js_gc_temp_root_set`. + let republished = temp_root_slots(f).into_iter().any(|slot| { + slot_traffic(f)[&slot] + .iter() + .filter(|e| matches!(e, SlotEvent::Store { .. })) + .count() + >= 2 + }); assert!( - f.contains("@js_gc_temp_root_set"), + republished, "each js_object_assign_one result must be written back into the \ - accumulator's root — the helper returns the moved target (#7200):\n{f}" + accumulator's root — the helper returns the moved target (#7200). \ + Slot traffic: {:#?}\n{f}", + slot_traffic(f) ); } @@ -1276,11 +1305,11 @@ fn a_collection_free_construction_emits_no_this_slot_root() { f.contains("@js_object_alloc"), "the fixture must actually construct something:\n{f}" ); - assert!( - !f.contains("@js_gc_temp_root_push"), - "an inert construction runs no user code, so it must emit no instance \ - temp root (#7192) — the `this`-slot bind is gated on the same \ - predicate:\n{f}" + assert_no_temp_rooting( + f, + "#7192: an inert construction runs no user code, so it must emit no \ + instance temp root — the `this`-slot bind is gated on the same \ + predicate", ); assert!( !f.contains("@js_shadow_slot_bind"), diff --git a/scripts/ci_e2e_scope.py b/scripts/ci_e2e_scope.py index 9191ac3369..e695e02afd 100755 --- a/scripts/ci_e2e_scope.py +++ b/scripts/ci_e2e_scope.py @@ -24,6 +24,8 @@ * `crates//tests//...` where no `.rs` suite exists (e.g. a `common/` helper module or a `fixtures/` data dir): every suite in that crate can be affected, so all of them are selected. + * `SOURCE_SUITE_MAP` — one hand-maintained exception to the rule below, for + the in-process root-lowering suites. See its docstring. * Everything else selects nothing. In particular a plain `src/` change does NOT map to suites: there is no coverage data to map it with, and a crate-level map (`perry-codegen` -> all 163 `perry` suites) is exactly the @@ -50,6 +52,45 @@ DEFAULT_CAP = 12 +# The ONE source -> suite mapping (#7507). Hand-maintained and narrow by design. +# +# The general refusal above stands: there is no coverage data, and a crate-level +# map is the full run this scoping exists to avoid. This is a named exception +# with a stated cost. +# +# Why these suites are not like the 163 `perry` ones the rule protects against: +# they are in-process compiles of hand-built HIR with `emit_ir_only: true` — no +# `perry compile` subprocess, no link, no runtime. Measured on arm64 macOS, +# debug, `--test-threads=1`: shadow_slot_hygiene 12 tests / 0.10 s, +# scalar_replaced_slot_roots 11 / 0.12 s, temp_root_operand_temporaries 19 / +# 0.16 s. Under half a second of test time; the cost is the build, which +# `e2e-scoped` already pays whenever any perry-codegen suite is in scope. +# +# Why it exists at all: #7370 changed `crates/perry-codegen/src/` and named no +# suite. `shadow_slot_hygiene` went 0/12 and nothing was red until someone ran +# the nightly tier by hand — the per-PR `cargo-test` gate is `--lib --bins`, so +# it saw none of it. +# +# NOT YET LISTED, deliberately: `native_proof_regressions` and +# `native_proof_buffer_views`. Both carry pre-existing failures that are not +# this map's to fix (2 each: typed-array artifact records and integer-modulo / +# typed-f64 clone lowering). A new gate has never been green, and wiring in a +# suite that is red on arrival makes `e2e-scoped` red on most perry-codegen PRs +# — CLAUDE.md hazard 2 with extra steps, since that job is not in branch +# protection and so cannot block anything, and reviewers learn to ignore it. +# Add them here in the same commit that turns them green. +# +# Every entry is cross-checked against `tests/` on disk (see `select`): an entry +# naming a suite that does not exist FAILS the scope step rather than silently +# dropping out, the same rule `scripts/gc_root_dominance_allowlist.json` uses. +SOURCE_SUITE_MAP = { + "crates/perry-codegen/src/": [ + ("perry-codegen", "shadow_slot_hygiene"), + ("perry-codegen", "scalar_replaced_slot_roots"), + ("perry-codegen", "temp_root_operand_temporaries"), + ], +} + _TESTS_PATH = re.compile(r"^crates/([^/]+)/tests/(.+)$") _PKG_NAME = re.compile(r'^\s*name\s*=\s*"([^"]+)"', re.M) @@ -89,11 +130,53 @@ def _suites_of(root: str, crate_dir: str): ) +def _crate_dir_of(root: str, pkg: str): + """The `crates/` whose Cargo.toml declares `pkg`, or None.""" + try: + entries = os.listdir(os.path.join(root, "crates")) + except OSError: + return None + for entry in sorted(entries): + if _package_name(root, entry) == pkg: + return entry + return None + + +def _source_map_selection(path: str, root: str): + """Suites `SOURCE_SUITE_MAP` names for one changed source path. + + Raises `SystemExit` when an entry names a suite that is not on disk: an + entry that matches nothing must FAIL, not quietly select less. A suite + renamed out from under this map is exactly how the map would rot back into + the gap it was added to close. + """ + hits = set() + for prefix, pairs in SOURCE_SUITE_MAP.items(): + if not path.startswith(prefix): + continue + for pkg, suite in pairs: + crate_dir = _crate_dir_of(root, pkg) + on_disk = crate_dir is not None and os.path.isfile( + os.path.join(root, "crates", crate_dir, "tests", suite + ".rs") + ) + if not on_disk: + raise SystemExit( + f"ci_e2e_scope: SOURCE_SUITE_MAP names {pkg}::{suite}, which " + f"has no crates/*/tests/{suite}.rs. Update the map in the " + f"same commit that renames or deletes the suite (#7507)." + ) + if pkg not in EXCLUDED: + hits.add((pkg, suite)) + return hits + + def select(changed, root: str): """-> sorted list of (package, suite) named by the changed paths.""" selected = set() for path in changed: - m = _TESTS_PATH.match(path.strip()) + path = path.strip() + selected |= _source_map_selection(path, root) + m = _TESTS_PATH.match(path) if not m: continue crate_dir, rest = m.group(1), m.group(2) @@ -137,6 +220,8 @@ def touch(rel, body=""): touch("crates/perry-codegen/Cargo.toml", '[package]\nname = "perry-codegen"\n') touch("crates/perry-codegen/tests/native_proof_regressions.rs") touch("crates/perry-codegen/tests/native_proof_regressions/invalidation.rs") + for suite in {s for _, s in SOURCE_SUITE_MAP["crates/perry-codegen/src/"]}: + touch(f"crates/perry-codegen/tests/{suite}.rs") touch("crates/perry-cc/Cargo.toml", '[package]\nname = "perry-cc"\n') touch("crates/perry-cc/tests/alpha.rs") touch("crates/perry-cc/tests/beta.rs") @@ -174,12 +259,61 @@ def touch(rel, body=""): ), # unknown crate dir (["crates/nope/tests/x.rs"], []), + # #7507: a perry-codegen SOURCE change selects exactly the mapped + # root-lowering suites — the case that made #7370 land silently. + ( + ["crates/perry-codegen/src/codegen/helpers.rs"], + sorted(SOURCE_SUITE_MAP["crates/perry-codegen/src/"]), + ), + # …and no other crate's `src/` maps to anything. The general + # refusal is still the rule; this is one named exception. + (["crates/perry-hir/src/lower.rs", "crates/perry/src/main.rs"], []), + # a perry-codegen source change plus one of its own suites is the + # union, deduplicated. + ( + [ + "crates/perry-codegen/src/gc_map.rs", + "crates/perry-codegen/tests/native_proof_regressions.rs", + ], + sorted( + set(SOURCE_SUITE_MAP["crates/perry-codegen/src/"]) + | {("perry-codegen", "native_proof_regressions")} + ), + ), ] for changed, expected in cases: got = select(changed, root) if got != expected: print(f"self-test FAILED for {changed}: {got} != {expected}", file=sys.stderr) return 1 + + # An entry that matches nothing must FAIL, not select less. Sabotage the + # map with a suite that is not on disk and require the raise. + saved = dict(SOURCE_SUITE_MAP) + SOURCE_SUITE_MAP["crates/perry-codegen/src/"] = [ + ("perry-codegen", "renamed_away_suite") + ] + try: + select(["crates/perry-codegen/src/anything.rs"], root) + except SystemExit: + pass + else: + print( + "self-test FAILED: SOURCE_SUITE_MAP naming a nonexistent suite " + "must fail the scope step (#7507)", + file=sys.stderr, + ) + return 1 + finally: + SOURCE_SUITE_MAP.clear() + SOURCE_SUITE_MAP.update(saved) + + # The real map, against the real repo: every entry must be a suite that + # exists here and now, not only in the self-test's fixture tree. + real_root = _repo_root() + for prefix in SOURCE_SUITE_MAP: + select([prefix + "probe.rs"], real_root) + print("ci_e2e_scope self-test: ok") return 0