diff --git a/CLAUDE.md b/CLAUDE.md index 646b769fd3..c75787b468 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.1622 +**Current Version:** 0.5.1623 ## TypeScript Parity Status diff --git a/Cargo.lock b/Cargo.lock index ba5708dc77..7bfe1d80e5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5543,7 +5543,7 @@ checksum = "1473d470930ed48574515a25df34900f3af89c6fa422d903e019121312a9f13e" [[package]] name = "perry" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "anyhow", "base64 0.22.1", @@ -5607,7 +5607,7 @@ dependencies = [ [[package]] name = "perry-api-manifest" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "perry-dispatch", "serde", @@ -5615,7 +5615,7 @@ dependencies = [ [[package]] name = "perry-audio-miniaudio" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "cc", "libc", @@ -5624,7 +5624,7 @@ dependencies = [ [[package]] name = "perry-codegen" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "aho-corasick", "anyhow", @@ -5641,7 +5641,7 @@ dependencies = [ [[package]] name = "perry-codegen-arkts" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "anyhow", "perry-hir", @@ -5649,7 +5649,7 @@ dependencies = [ [[package]] name = "perry-codegen-glance" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "anyhow", "perry-hir", @@ -5657,7 +5657,7 @@ dependencies = [ [[package]] name = "perry-codegen-js" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "anyhow", "perry-dispatch", @@ -5666,7 +5666,7 @@ dependencies = [ [[package]] name = "perry-codegen-swiftui" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "anyhow", "perry-hir", @@ -5674,7 +5674,7 @@ dependencies = [ [[package]] name = "perry-codegen-wasm" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "anyhow", "base64 0.22.1", @@ -5686,7 +5686,7 @@ dependencies = [ [[package]] name = "perry-codegen-wear-tiles" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "anyhow", "perry-hir", @@ -5694,7 +5694,7 @@ dependencies = [ [[package]] name = "perry-container-compose" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "async-trait", "clap", @@ -5718,14 +5718,14 @@ dependencies = [ [[package]] name = "perry-container-e2e" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "anyhow", ] [[package]] name = "perry-diagnostics" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "serde", "serde_json", @@ -5733,7 +5733,7 @@ dependencies = [ [[package]] name = "perry-dispatch" -version = "0.5.1622" +version = "0.5.1623" [[package]] name = "perry-doc-fixture-my-bindings" @@ -5744,7 +5744,7 @@ dependencies = [ [[package]] name = "perry-doc-tests" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "anyhow", "clap", @@ -5759,7 +5759,7 @@ dependencies = [ [[package]] name = "perry-ext-ads" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "block2", "objc2", @@ -5769,7 +5769,7 @@ dependencies = [ [[package]] name = "perry-ext-argon2" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "argon2", "perry-ffi", @@ -5778,7 +5778,7 @@ dependencies = [ [[package]] name = "perry-ext-bcrypt" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "bcrypt", "perry-ffi", @@ -5786,7 +5786,7 @@ dependencies = [ [[package]] name = "perry-ext-better-sqlite3" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "perry-ffi", "rusqlite", @@ -5794,7 +5794,7 @@ dependencies = [ [[package]] name = "perry-ext-cheerio" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "perry-ffi", "scraper", @@ -5802,7 +5802,7 @@ dependencies = [ [[package]] name = "perry-ext-decimal" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "perry-ffi", "rust_decimal", @@ -5810,7 +5810,7 @@ dependencies = [ [[package]] name = "perry-ext-ethers" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "perry-ffi", "rand 0.10.2", @@ -5818,7 +5818,7 @@ dependencies = [ [[package]] name = "perry-ext-events" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "perry-ffi", "perry-runtime", @@ -5826,7 +5826,7 @@ dependencies = [ [[package]] name = "perry-ext-fetch" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "bytes", "lazy_static", @@ -5839,7 +5839,7 @@ dependencies = [ [[package]] name = "perry-ext-http" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "base64 0.22.1", "bytes", @@ -5871,7 +5871,7 @@ dependencies = [ [[package]] name = "perry-ext-ioredis" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "lazy_static", "perry-ffi", @@ -5881,7 +5881,7 @@ dependencies = [ [[package]] name = "perry-ext-mongodb" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "bson", "futures-util", @@ -5893,7 +5893,7 @@ dependencies = [ [[package]] name = "perry-ext-mysql2" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "chrono", "perry-ffi", @@ -5905,7 +5905,7 @@ dependencies = [ [[package]] name = "perry-ext-net" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "bytes", "perry-ffi", @@ -5920,7 +5920,7 @@ dependencies = [ [[package]] name = "perry-ext-nodemailer" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "lettre", "perry-ffi", @@ -5930,7 +5930,7 @@ dependencies = [ [[package]] name = "perry-ext-parcel-watcher" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "notify", "perry-ffi", @@ -5942,7 +5942,7 @@ dependencies = [ [[package]] name = "perry-ext-pdf" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "perry-ffi", "printpdf", @@ -5950,7 +5950,7 @@ dependencies = [ [[package]] name = "perry-ext-pg" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "perry-ffi", "sqlx", @@ -5959,7 +5959,7 @@ dependencies = [ [[package]] name = "perry-ext-sharp" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "fast_image_resize", "image", @@ -5970,7 +5970,7 @@ dependencies = [ [[package]] name = "perry-ext-streams" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "lazy_static", "perry-ffi", @@ -5979,7 +5979,7 @@ dependencies = [ [[package]] name = "perry-ext-typescript" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "anyhow", "perry-ffi", @@ -5999,7 +5999,7 @@ dependencies = [ [[package]] name = "perry-ext-undici" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "perry-ffi", "perry-runtime", @@ -6008,7 +6008,7 @@ dependencies = [ [[package]] name = "perry-ext-ws" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "futures-util", "lazy_static", @@ -6021,7 +6021,7 @@ dependencies = [ [[package]] name = "perry-ext-zlib" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "brotli", "flate2", @@ -6031,7 +6031,7 @@ dependencies = [ [[package]] name = "perry-ffi" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "dashmap 6.2.1", "once_cell", @@ -6041,7 +6041,7 @@ dependencies = [ [[package]] name = "perry-hir" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "anyhow", "perry-api-manifest", @@ -6061,11 +6061,11 @@ dependencies = [ [[package]] name = "perry-native-registration" -version = "0.5.1622" +version = "0.5.1623" [[package]] name = "perry-parser" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "anyhow", "perry-diagnostics", @@ -6078,7 +6078,7 @@ dependencies = [ [[package]] name = "perry-perex" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "perex", "regex", @@ -6086,7 +6086,7 @@ dependencies = [ [[package]] name = "perry-runtime" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "ahash", "base64 0.22.1", @@ -6144,14 +6144,14 @@ dependencies = [ [[package]] name = "perry-runtime-static" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "perry-runtime", ] [[package]] name = "perry-stdlib" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "aes 0.8.4", "aes 0.9.1", @@ -6234,21 +6234,21 @@ dependencies = [ [[package]] name = "perry-stdlib-static" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "perry-stdlib", ] [[package]] name = "perry-transform" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "perry-hir", ] [[package]] name = "perry-ui" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "dirs", "perry-ffi", @@ -6258,7 +6258,7 @@ dependencies = [ [[package]] name = "perry-ui-android" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "base64 0.22.1", "jni", @@ -6273,7 +6273,7 @@ dependencies = [ [[package]] name = "perry-ui-geisterhand" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "rand 0.10.2", "serde", @@ -6283,7 +6283,7 @@ dependencies = [ [[package]] name = "perry-ui-gtk4" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "base64 0.22.1", "cairo-rs 0.22.9", @@ -6306,7 +6306,7 @@ dependencies = [ [[package]] name = "perry-ui-ios" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "base64 0.22.1", "block2", @@ -6323,7 +6323,7 @@ dependencies = [ [[package]] name = "perry-ui-macos" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "base64 0.22.1", "block2", @@ -6340,7 +6340,7 @@ dependencies = [ [[package]] name = "perry-ui-model" -version = "0.5.1622" +version = "0.5.1623" [[package]] name = "perry-ui-test" @@ -6351,11 +6351,11 @@ dependencies = [ [[package]] name = "perry-ui-testkit" -version = "0.5.1622" +version = "0.5.1623" [[package]] name = "perry-ui-tvos" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "base64 0.22.1", "block2", @@ -6372,7 +6372,7 @@ dependencies = [ [[package]] name = "perry-ui-visionos" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "base64 0.22.1", "block2", @@ -6389,7 +6389,7 @@ dependencies = [ [[package]] name = "perry-ui-watchos" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "block2", "libc", @@ -6403,7 +6403,7 @@ dependencies = [ [[package]] name = "perry-ui-windows" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "base64 0.22.1", "libc", @@ -6422,7 +6422,7 @@ dependencies = [ [[package]] name = "perry-ui-windows-winui" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "base64 0.22.1", "libc", @@ -6435,7 +6435,7 @@ dependencies = [ [[package]] name = "perry-updater" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "anyhow", "base64 0.22.1", @@ -6450,7 +6450,7 @@ dependencies = [ [[package]] name = "perry-wasm-host" -version = "0.5.1622" +version = "0.5.1623" dependencies = [ "wasmi", ] diff --git a/Cargo.toml b/Cargo.toml index 3aaa869ead..8bcbef189d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -317,7 +317,7 @@ codegen-units = 1 codegen-units = 1 [workspace.package] -version = "0.5.1622" +version = "0.5.1623" edition = "2021" license = "MIT" repository = "https://github.com/PerryTS/perry" diff --git a/changelog.d/10826-delete-transition-holder.md b/changelog.d/10826-delete-transition-holder.md new file mode 100644 index 0000000000..c11adb0857 --- /dev/null +++ b/changelog.d/10826-delete-transition-holder.md @@ -0,0 +1,13 @@ +Registered `DELETE_TRANSITION_TEST_OVERRIDE` in +`scripts/gc_runtime_root_holders.json` with a `test_only` verdict. + +`gc_runtime_root_holders.py` enumerates every `static`/`thread_local!` whose +type could hold a heap pointer and requires a written verdict for any that a +registered scanner does not reach. The new flag is one, so the gate refused the +tree — the gate working. + +The verdict is accurate rather than convenient: the declaration sits inside a +`#[cfg(test)] thread_local!` block and the type is `Cell>`, which +cannot hold a heap pointer at all — `Option` is a two-bit value, not a +NaN box and not a `*mut`. There is nothing for a scanner to reach, and the +shipping path reads it and finds `None`. diff --git a/changelog.d/10833-census-encoding-invariant.md b/changelog.d/10833-census-encoding-invariant.md new file mode 100644 index 0000000000..5dd8c1d5f0 --- /dev/null +++ b/changelog.d/10833-census-encoding-invariant.md @@ -0,0 +1,36 @@ +Retargeted `scripts/shape_descriptor_census.py`'s generic-read-PIC emptiness +proof from an **instruction** to the **invariant** behind it. + +The census required `icmp_ne(I64, &packed_word, "0")` to survive in the generic +read body — the "empty compact-cache rejection". #10833 removes that line, and +the census refused the tree. + +**It was right to refuse, and the PR is right to remove it.** The unprimed +compact word used to be `0`, so `packed != 0` *was* the emptiness test. This +change moves the sentinel to `PACKED_GET_EMPTY = 0xFFFF_FFFF`, which lies +outside the valid ShapeId range `[SHAPE_ID_BASE, SHAPE_ID_END)` = +`[0x8000_0000, 0xC000_0000)`. An unprimed word's stamp therefore cannot equal +any valid `pcid`, so the token compare already rejects it and the separate test +is redundant. Under the new encoding, asserting `packed != 0` asserts nothing. + +So the census now asserts the range relationship directly: + + PACKED_GET_EMPTY must lie OUTSIDE [SHAPE_ID_BASE, SHAPE_ID_END) + +which is strictly stronger than the line it replaces: it survives the next +encoding change and it names the hazard — an unprimed site read as a hit — +rather than one spelling of the fix for it. A new `require_match` helper reads +the constants' values, because reasoning about a magnitude is what makes the +check encoding-independent. + +The census's **sabotage self-test** was retargeted with it. It previously +planted `icmp_ne(..., "-1")`; it now plants `PACKED_GET_EMPTY = 0x9000_0000`, +i.e. moves the sentinel *into* the valid range — the exact mistake that would +let an unprimed site read as a hit. Verified independently of the self-test: +sabotaging the real constant gives + + empty sentinel 0x90000000 is INSIDE the valid ShapeId range + [0x80000000, 0xc0000000) -- an unprimed site could be read as a hit + +and the clean tree passes. The assertion was not weakened to accommodate the +change; it was moved onto the property the change preserves. diff --git a/crates/perry-codegen/src/collectors/escape_arrays.rs b/crates/perry-codegen/src/collectors/escape_arrays.rs index a273cc0a5d..d9cf49dd35 100644 --- a/crates/perry-codegen/src/collectors/escape_arrays.rs +++ b/crates/perry-codegen/src/collectors/escape_arrays.rs @@ -650,6 +650,41 @@ pub fn check_array_escapes_in_expr( check_array_escapes_in_stmts(body, candidates, escaped); } + // #10822: `delete o.k` REMOVES a property, and scalar replacement has + // no representation for absence. The per-slot alloca keeps holding + // the pre-delete value, and the emitted runtime delete runs against + // the never-populated dummy `ctx.locals[id]` alloca, where the + // deliberate "a primitive receiver no-ops to true" guard turns it + // into a silent nothing. `delete o.c; String(o.c)` then answered + // `"3"` instead of `"undefined"` -- a wrong value with nothing in the + // output to show it. + // + // Without this arm the generic unary recursion below strips the + // `delete` and hands the inner `PropertyGet` / `IndexGet` to the + // "safe read" arm, which returns early WITHOUT visiting the bare + // `LocalGet`. That is exactly why a second observer of the same + // object (`Object.keys(o)`, `objs.push(o)`, `"c" in o`) repaired it: + // each of those reaches the bare-`LocalGet` arm and escapes the + // receiver, while `delete` alone never did. + // + // The REMOVAL sibling of the read rule (#10689) and the write rules + // (#9024 / #9460): escape the receiver so it takes the heap path, + // where the runtime delete is real and the generic read's `TAG_HOLE` + // compare answers `undefined`. + Expr::Delete(operand) => { + match operand.as_ref() { + Expr::PropertyGet { object, .. } | Expr::IndexGet { object, .. } => { + if let Expr::LocalGet(id) = object.as_ref() { + if candidates.contains_key(id) { + escaped.insert(*id); + } + } + } + _ => {} + } + check_array_escapes_in_expr(operand, candidates, escaped); + } + // ── Recurse into sub-expressions (same structure as object pass). ── Expr::Binary { left, right, .. } | Expr::Compare { left, right, .. } @@ -661,7 +696,6 @@ pub fn check_array_escapes_in_expr( | Expr::Void(operand) | Expr::TypeOf(operand) | Expr::Await(operand) - | Expr::Delete(operand) | Expr::StringCoerce(operand) | Expr::ObjectCoerce(operand) | Expr::BooleanCoerce(operand) diff --git a/crates/perry-codegen/src/collectors/escape_check.rs b/crates/perry-codegen/src/collectors/escape_check.rs index 2470f65f19..136b3c3166 100644 --- a/crates/perry-codegen/src/collectors/escape_check.rs +++ b/crates/perry-codegen/src/collectors/escape_check.rs @@ -407,6 +407,45 @@ pub fn check_escapes_in_expr( check_escapes_in_stmts(body, candidates, classes, escaped); } + // #10822: `delete o.k` REMOVES a property, and scalar replacement has + // no representation for absence. The per-field alloca keeps holding + // the pre-delete value, and the emitted + // `js_object_delete_field_value(, k)` runs against the + // never-populated `ctx.locals[id]` alloca, where the deliberate + // "a primitive receiver no-ops to true" guard turns it into a silent + // nothing. `delete o.c; String(o.c)` then answered `"3"` instead of + // `"undefined"` -- a wrong value with nothing in the output to show + // it. + // + // Without this arm the generic unary recursion below strips the + // `delete` and hands the inner `PropertyGet` / `IndexGet` to the + // "plain declared-field read" arm, which returns early WITHOUT + // visiting the bare `LocalGet`. That is exactly why a second observer + // of the same object (`Object.keys(o)`, `objs.push(o)`, `"c" in o`) + // repaired it: each of those reaches the bare-`LocalGet` arm and + // escapes the receiver, while `delete` alone never did. + // + // This is the REMOVAL sibling of #10689 (a read of an undeclared key) + // and #9024 / #9460 (a write of one): escape the receiver so the + // object takes the heap path, where the runtime delete is real and + // the generic read's `TAG_HOLE` compare answers `undefined`. It costs + // nothing for an object that is not a `delete` target, and + // `Ptr` already denies any module containing a `delete` + // outright (ptr_shape rule 5), so no proven-path read is affected. + Expr::Delete(operand) => { + match operand.as_ref() { + Expr::PropertyGet { object, .. } | Expr::IndexGet { object, .. } => { + if let Expr::LocalGet(id) = object.as_ref() { + if candidates.contains_key(id) { + escaped.insert(*id); + } + } + } + _ => {} + } + check_escapes_in_expr(operand, candidates, classes, escaped); + } + // ── Recurse into all sub-expressions ── Expr::Binary { left, right, .. } | Expr::Compare { left, right, .. } @@ -418,7 +457,6 @@ pub fn check_escapes_in_expr( | Expr::Void(operand) | Expr::TypeOf(operand) | Expr::Await(operand) - | Expr::Delete(operand) | Expr::StringCoerce(operand) | Expr::ObjectCoerce(operand) | Expr::BooleanCoerce(operand) diff --git a/crates/perry-codegen/src/collectors/escape_objects.rs b/crates/perry-codegen/src/collectors/escape_objects.rs index d4625c94b0..3e4b03c9a1 100644 --- a/crates/perry-codegen/src/collectors/escape_objects.rs +++ b/crates/perry-codegen/src/collectors/escape_objects.rs @@ -508,6 +508,41 @@ pub fn check_object_literal_escapes_in_expr( check_object_literal_escapes_in_stmts(body, candidates, escaped); } + // #10822: `delete o.k` REMOVES a property, and scalar replacement has + // no representation for absence. The per-slot alloca keeps holding + // the pre-delete value, and the emitted runtime delete runs against + // the never-populated dummy `ctx.locals[id]` alloca, where the + // deliberate "a primitive receiver no-ops to true" guard turns it + // into a silent nothing. `delete o.c; String(o.c)` then answered + // `"3"` instead of `"undefined"` -- a wrong value with nothing in the + // output to show it. + // + // Without this arm the generic unary recursion below strips the + // `delete` and hands the inner `PropertyGet` / `IndexGet` to the + // "safe read" arm, which returns early WITHOUT visiting the bare + // `LocalGet`. That is exactly why a second observer of the same + // object (`Object.keys(o)`, `objs.push(o)`, `"c" in o`) repaired it: + // each of those reaches the bare-`LocalGet` arm and escapes the + // receiver, while `delete` alone never did. + // + // The REMOVAL sibling of the read rule (#10689) and the write rules + // (#9024 / #9460): escape the receiver so it takes the heap path, + // where the runtime delete is real and the generic read's `TAG_HOLE` + // compare answers `undefined`. + Expr::Delete(operand) => { + match operand.as_ref() { + Expr::PropertyGet { object, .. } | Expr::IndexGet { object, .. } => { + if let Expr::LocalGet(id) = object.as_ref() { + if candidates.contains_key(id) { + escaped.insert(*id); + } + } + } + _ => {} + } + check_object_literal_escapes_in_expr(operand, candidates, escaped); + } + // ── Recurse into sub-expressions ── Expr::Binary { left, right, .. } | Expr::Compare { left, right, .. } @@ -516,7 +551,7 @@ pub fn check_object_literal_escapes_in_expr( check_object_literal_escapes_in_expr(right, candidates, escaped); } Expr::Unary { operand, .. } | Expr::Void(operand) | Expr::TypeOf(operand) - | Expr::Await(operand) | Expr::Delete(operand) + | Expr::Await(operand) | Expr::StringCoerce(operand) | Expr::ObjectCoerce(operand) | Expr::BooleanCoerce(operand) | Expr::NumberCoerce(operand) | Expr::IsFinite(operand) | Expr::IsNaN(operand) | Expr::NumberIsNaN(operand) diff --git a/crates/perry-codegen/src/collectors/this_as_value.rs b/crates/perry-codegen/src/collectors/this_as_value.rs index c9d757589e..77efaea2d5 100644 --- a/crates/perry-codegen/src/collectors/this_as_value.rs +++ b/crates/perry-codegen/src/collectors/this_as_value.rs @@ -376,11 +376,31 @@ pub fn expr_uses_this_as_value(e: &perry_hir::Expr, fields: &HashSet) -> | Expr::Logical { left, right, .. } => { expr_uses_this_as_value(left, fields) || expr_uses_this_as_value(right, fields) } + // #10822: `delete this.k` inside a constructor REMOVES a property, + // and scalar replacement has no representation for absence -- the + // field alloca keeps the pre-delete value and the emitted runtime + // delete runs against a `this` that was never materialized, where the + // primitive-receiver guard makes it a silent no-op. `class C { + // constructor() { this.c = 3; delete this.c; } }` then read `o.c` + // back as `3`. + // + // The `PropertyGet { object: This }` arm above answers "safe, scalar + // replacement intercepts it" for a DECLARED field, and the generic + // unary arm below used to strip the `delete` and ask exactly that. + // A removal needs a real heap `this`, so say so here -- the same rule + // the `LocalGet` receivers get in `escape_check.rs`. + Expr::Delete(operand) => match operand.as_ref() { + Expr::PropertyGet { object, .. } | Expr::IndexGet { object, .. } + if matches!(object.as_ref(), Expr::This) => + { + true + } + _ => expr_uses_this_as_value(operand, fields), + }, Expr::Unary { operand, .. } | Expr::Void(operand) | Expr::TypeOf(operand) | Expr::Await(operand) - | Expr::Delete(operand) | Expr::StringCoerce(operand) | Expr::ObjectCoerce(operand) | Expr::BooleanCoerce(operand) diff --git a/crates/perry-codegen/src/expr/property_get/generic_dispatch.rs b/crates/perry-codegen/src/expr/property_get/generic_dispatch.rs index 31bdf5c1bb..ca2e578d0f 100644 --- a/crates/perry-codegen/src/expr/property_get/generic_dispatch.rs +++ b/crates/perry-codegen/src/expr/property_get/generic_dispatch.rs @@ -33,6 +33,41 @@ pub(crate) const PIC_WAY_BASE: usize = 4; /// `(token, slot)` ways beyond the MRU entry; a site resolves `PIC_WAYS + 1` /// shapes inline. Mirrors the runtime's `PIC_WAYS`. pub(crate) const PIC_WAYS: usize = 4; +/// The value a per-site compact MRU word (`@perry_ic_N_packed_get`) holds +/// before anything has primed it. +/// +/// **Must equal `perry_runtime::object::field_get_set::PACKED_GET_EMPTY`.** +/// +/// It is NOT zero, and that is the whole point. The hit path compares the +/// receiver's ShapeId word against this word's low half; an object that was +/// never shape-stamped carries `parent_class_id` at +4, which is 0 for an +/// anonymous object literal, so a zero sentinel would let such a receiver +/// MATCH a site that has never primed and take the raw load at slot 0. That +/// is why the tower used to spend a separate `test`/`je` proving the word was +/// filled. A sentinel that no receiver word can equal makes the ShapeId +/// compare prove BOTH facts, and the extra test leaves every read. +/// +/// `0xFFFF_FFFF` sits above every value the word at `+4` can hold: a ShapeId +/// ([`0x8000_0000`, `0xC000_0000`)), a synthetic class id (at or above +/// 0x8000_0000 today, [`0xC000_0000`, `0xFFFF_0000`) under #10824), or an +/// ordinary HIR class id, which is a counter from 1. +pub(crate) const PACKED_GET_EMPTY: i64 = 0xFFFF_FFFF; +/// A SPILL-located key publishes its ShapeId into the compact word with this +/// bit flipped. **Must equal `PACKED_SPILL_FLIP` in the runtime.** +/// +/// ShapeIds live in [`0x8000_0000`, `0xC000_0000`), so flipping the top two +/// bits maps them into [`0x4000_0000`, `0x8000_0000`) — the one u32 band that +/// is neither a ShapeId nor any class id. (Flipping bit 30 alone would land +/// them in [`0xC000_0000`, `2^32`), which #10824 turns into the synthetic +/// class-id range.) +/// The hit path's compare therefore REFUSES a spill entry without asking a +/// question of its own, which is what lets the overflow-bit test (a 10-byte +/// `movabs`, a `test` and a branch, on every read of every site) leave the hit +/// path entirely. The spill entry is still served: `pic.token.miss` un-flips +/// the bit, and a match branches straight to the slow entry, which decodes the +/// same word. See the design note at the head of this function. +pub(crate) const PACKED_SPILL_FLIP: i64 = 0xC000_0000; + /// Way-state word: `> 0` means at least one way is populated and the compares /// are worth running; `0` (fresh) and a negative megamorphic countdown /// both skip them. Mirrors the runtime's `PIC_WAY_STATE`. @@ -202,9 +237,52 @@ pub(crate) fn lower_generic_property_get( // silently corrupting FFI args and pure-TS field compares. // Tag check is platform-independent: same two LLVM ops // (`lshr` + `and`) + one `icmp`, branch-predicted taken. + // `.length` on a receiver whose static type is not a proven string, and + // `.size` on one that may be a native Map/Set, are the only two keys this + // tower serves from a cell that is not a `GC_TYPE_OBJECT`. Both decisions + // are made from the property name alone, and `inline_string_length` + // decides WHICH receiver-tag test to emit, so both are hoisted above it. + let inline_string_length = property == "length"; + let inline_collection_size = property == "size"; + + // The receiver-tag test, in the cheapest form the key allows. + // + // `.length` is the one key with a heap-STRING arm below, so it is the one + // key whose test must admit BOTH pointer-ish tags — POINTER (0x7FFD) and + // STRING (0x7FFF). Collapsing them costs real instructions: LLVM folds + // `(bits >> 48) & 0xFFFD == 0x7FFD` back into a 64-bit mask and a 64-bit + // compare, so the emitted test is two 10-byte `movabs`, a `mov`, an `and`, + // a `cmp` and the branch — SIX instructions and 20 bytes of immediates, + // measured on the k1 fixture at v0.5.1618. + // + // Every OTHER key gets the exact POINTER test. A heap string that fails it + // now reaches `js_object_get_field_ic_nonptr`, whose heap-string arm masks + // the tag off and calls the same by-name helper the object exit used to + // reach — the same answer, one branch earlier instead of four loads later. + // + // MEASURED: this is worth ZERO instructions today, and it is still the + // right test. InstCombine canonicalises `(x >> 48) == 0x7FFD` straight + // back into `x & 0xFFFF_0000_0000_0000 == 0x7FFD_0000_0000_0000`, so the + // emitted code is the same `mov`/`and`/`cmp` pair of 10-byte `movabs` + // either way — only the mask constant changes, 0xFFFD to 0xFFFF. Writing + // the compare on a TRUNCATED tag (`trunc i64 ... to i32`) does not defeat + // the fold either; both forms were checked in the k1 fixture's + // disassembly. Getting the `shr $0x30` + imm32 `cmp` form needs something + // the IR builder cannot express today. + // + // It is kept because it is a PREREQUISITE, not a micro-optimisation. The + // collapsed test admits STRING-tagged receivers onto the pointer path, and + // the only thing that stops one having its `StringHeader` word at +4 read + // as a ShapeId is the GC-kind guard below — the guard #10828 is about to + // make removable. #10828's guarantee is stated over POINTER-tagged values; + // this is what makes the emitted test match that statement. let obj_tag = ctx.block().lshr(I64, &obj_bits, "48"); - let obj_tag_masked = ctx.block().and(I64, &obj_tag, "65533"); // 0xFFFD - let is_valid = ctx.block().icmp_eq(I64, &obj_tag_masked, "32765"); // 0x7FFD + let is_valid = if inline_string_length { + let obj_tag_masked = ctx.block().and(I64, &obj_tag, "65533"); // 0xFFFD + ctx.block().icmp_eq(I64, &obj_tag_masked, "32765") // 0x7FFD + } else { + ctx.block().icmp_eq(I64, &obj_tag, "32765") // POINTER_TAG exactly + }; // `.length` on a receiver whose static type is not a proven string. // @@ -224,7 +302,6 @@ pub(crate) fn lower_generic_property_get( // a pure short-circuit: a primitive string's `length` is non-writable and // non-configurable, cannot be shadowed by an own property, and is exactly // what the runtime ladder computes. Everything else keeps the tower. - let inline_string_length = property == "length"; // A dynamically typed `receiver.size` can still be served without the // object PIC when the live receiver is a native Map or Set. Both payloads // start with the same `u32 size` field, and their distinct GcHeader kinds @@ -232,7 +309,6 @@ pub(crate) fn lower_generic_property_get( // check rather than a TypeScript-type claim: nested structural reads such // as `this.ctx.hooks.size` commonly lose their static Set type, while an // erased annotation alone must never authorize a native-layout load. - let inline_collection_size = property == "size"; // A compact per-site word holds the exact ShapeId and slot for the last // cacheable receiver. The lazily allocated full cache retains bounded @@ -250,8 +326,9 @@ pub(crate) fn lower_generic_property_get( "{}_packed_get", crate::expr::inline_cache_global_name(ctx, packed_site) ); - ctx.typed_parse_rodata - .push(format!("@{packed_name} = private global i64 0, align 8")); + ctx.typed_parse_rodata.push(format!( + "@{packed_name} = private global i64 {PACKED_GET_EMPTY}, align 8" + )); let packed_ref = format!("@{packed_name}"); let pic_idx = ctx.new_block("pget.recv_ok"); @@ -431,7 +508,6 @@ pub(crate) fn lower_generic_property_get( // The compact cache is a permanently valid scalar global. Load it before // receiver-dependent shape probing so its latency overlaps header reads. let packed_word = ctx.block().load_atomic_monotonic(I64, &packed_ref, 8); - let packed_present = ctx.block().icmp_ne(I64, &packed_word, "0"); // GcHeader starts with obj_type:u8, gc_flags:u8, reserved:u16. On // known little-endian targets one load tests both kind and descriptors; @@ -507,12 +583,15 @@ pub(crate) fn lower_generic_property_get( let no_desc = ctx.block().icmp_eq(crate::types::I16, &has_desc, "0"); ctx.block().and(I1, &is_object_kind, &no_desc) }; - let is_plain_object = ctx.block().and(I1, &is_plain_kind, &packed_present); - - // Validate kind, descriptor policy, and initialized MRU before reading - // ObjectHeader's ShapeId. - ctx.block() - .cond_br(&is_plain_object, &tok_label, &cold_label); + // Validate kind and descriptor policy before reading ObjectHeader's + // ShapeId. "Is this site primed?" is NOT asked here any more: the compact + // word's unprimed value is `PACKED_GET_EMPTY`, which no receiver ShapeId + // word can equal, so the ShapeId compare below answers it. Folding the old + // `packed != 0` test in here also violated this tower's own rule — it was + // the one place where two guards were AND-ed into a flat predicate instead + // of branching out on the first failure (#7883), and it cost the `test` + // and the branch on every hit. + ctx.block().cond_br(&is_plain_kind, &tok_label, &cold_label); ctx.current_block = tok_idx; // The receiver token is derived solely from its authoritative ShapeId. @@ -539,10 +618,39 @@ pub(crate) fn lower_generic_property_get( ctx.block() .cond_br(&token_eq, &hit_label, &token_miss_label); + ctx.current_block = token_miss_idx; + // The SPILL entry — tested HERE, and nowhere on the hit path. + // + // A key past the object's inline region used to publish its slot into the + // compact word with `IC_SLOT_OVERFLOW_BIT` set, and every read of every + // site paid to ask whether the bit was there: LLVM folds + // `((packed >> 32) & (1 << 30)) == 0` into `packed & (1 << 62)`, which is + // a 10-byte `movabs`, a `test` and a branch on the hit path of sites whose + // field is inline and can never see the bit. + // + // Now a spill entry publishes the SAME ShapeId with `PACKED_SPILL_FLIP` + // flipped into it, which lands it outside the ShapeId range, so the hit + // path's compare refuses it for free. Un-flipping the bit here recognises + // it in three instructions ON THE MISS PATH ONLY, and a match branches + // straight to the slow entry — skipping the full cache's resolution and + // the polymorphic ways, neither of which can serve a spill key anyway + // (`pic_prime_get` refuses to cascade an encoded slot into a way). The + // slow entry decodes the same word and reads the spill buffer, so a spill + // read pays the same three instructions it paid before, just in a block + // the inline hit never enters. + let spill_stamp = ctx + .block() + .xor(I32, &packed_stamp, &PACKED_SPILL_FLIP.to_string()); + let is_spill = ctx.block().icmp_eq(I32, &pcid, &spill_stamp); + let ways_entry_idx = ctx.new_block("pic.token.ways"); + let ways_entry_label = ctx.block_label(ways_entry_idx); + ctx.block() + .cond_br(&is_spill, &call_label, &ways_entry_label); + // Every way load still requires a resolved full cache. A site that has // never primed has no cache, so there is nothing to compare against and // the read goes straight out. - ctx.current_block = token_miss_idx; + ctx.current_block = ways_entry_idx; let token_cache = crate::expr::emit_inline_cache_slot(ctx, &cache_name); ctx.block() .cond_br(&token_cache.present, &miss_label, &cold_label); @@ -552,35 +660,12 @@ pub(crate) fn lower_generic_property_get( // token hit permanently proves that the cached slot remains live and // makes the raw load below safe without a compatibility-header bound. ctx.current_block = hit_idx; + // A matched compact word is now, by construction, an INLINE slot: a + // spill-located key publishes its ShapeId flipped by `PACKED_SPILL_FLIP` + // and is recognised in `pic.token.miss` instead. The overflow-bit test + // that used to stand between this shift and the load is gone from the hit + // path — see the note there for what it cost and where it went. let slot = ctx.block().lshr(I64, &packed_word, "32"); - - // #9287: the primed slot word may carry IC_SLOT_OVERFLOW_BIT (1 << 30) — - // the field lives past the inline region, in the object's spill buffer, - // and the inline `obj + header + slot*8` arithmetic below must not run on - // it. Such hits leave for the slow entry, which re-derives the same MRU - // pair and performs `js_object_get_field_ic_overflow_load`'s `overflow_get` - // (falling back to the full miss handler on a tombstoned slot, without the - // packed republication that helper also did not do). Sites whose field is - // inline never see the bit, so this branch predicts perfectly for them. - // The polymorphic WAYS never hold an encoded slot (`pic_prime_get` refuses - // to cascade one), so only this MRU path needs the check. - // - // Tested as `== 0` rather than `!= 0` so the guard-PASSING edge is the TRUE - // edge, exactly like every other link in this chain. That is not cosmetic: - // `generic_property_get_slot_load_is_reached_only_through_every_guard` - // walks the CFG backwards from the slot load and requires every edge on the - // way to be a true edge, which is what makes a swapped `cond_br` — running - // the raw load when a guard FAILS — turn it red. Before T1 the walk reached - // the load through `pic.hit.overflow` (whose key-handle load also matched - // "load double") and never evaluated this branch's polarity at all. - let inline_hit_idx = ctx.new_block("pic.hit.inline"); - let inline_hit_label = ctx.block_label(inline_hit_idx); - let ovf_bits = ctx.block().and(I64, &slot, "1073741824"); // 1 << 30 - let is_inline_slot = ctx.block().icmp_eq(I64, &ovf_bits, "0"); - ctx.block() - .cond_br(&is_inline_slot, &inline_hit_label, &call_label); - - ctx.current_block = inline_hit_idx; // arm64_32 watchOS: the object fields region begins at // `size_of::()` past the user pointer — 16 on LP64 and // padded ILP32 since #8047. Derive it from the target triple. diff --git a/crates/perry-codegen/src/expr/property_get/tests.rs b/crates/perry-codegen/src/expr/property_get/tests.rs index 824aee5e45..49112d43cb 100644 --- a/crates/perry-codegen/src/expr/property_get/tests.rs +++ b/crates/perry-codegen/src/expr/property_get/tests.rs @@ -381,7 +381,16 @@ fn pic_cache_layout_matches_runtime() { ); for def in &ic_defs { if def.contains("_packed_get =") { - assert!(def.ends_with(" = private global i64 0, align 8"), "{def}"); + // NOT zero — see `PACKED_GET_EMPTY`. A zero word would be matched + // by an unstamped receiver's `parent_class_id`, which is why the + // hit path used to carry a separate "is this site primed?" test. + assert!( + def.ends_with(&format!( + " = private global i64 {}, align 8", + crate::expr::property_get::generic_dispatch::PACKED_GET_EMPTY + )), + "{def}" + ); continue; } assert!( @@ -590,13 +599,17 @@ fn pic_miss_reuses_the_token_blocks_values_instead_of_re_deriving_them() { receiver could reach the way compares; it must be gone:\n{ir}" ); // The header predicates: each load/compare pair must appear exactly once. - for (needle, what) in [ - ("icmp eq i8 ", "the GC_TYPE_OBJECT compare"), - ("icmp eq i32 %", "the ShapeId identity compare"), + // `icmp eq i32 %` is three: the packed kind/descriptor compare, the + // ShapeId identity compare on the hit path, and the spill compare in + // `pic.token.miss` that replaced the hit path's overflow-bit test. A + // fourth would mean the miss block is re-deriving the header. + for (needle, what, bound) in [ + ("icmp eq i8 ", "the GC_TYPE_OBJECT compare", 2), + ("icmp eq i32 %", "the ShapeId identity compare", 3), ] { let n = main.matches(needle).count(); assert!( - n <= 2, + n <= bound, "{what} appears {n} times — the miss block is re-deriving the \ receiver header again:\n{ir}" ); @@ -764,6 +777,7 @@ mod nested_namespace_members { /// with a constant, or deleting a predicate, turns it red). #[test] fn generic_property_get_slot_load_is_reached_only_through_every_guard() { + use crate::expr::property_get::generic_dispatch::{PACKED_GET_EMPTY, PACKED_SPILL_FLIP}; let ir = emit(false, None); // Register names restart at %r1 in every function, so the walk MUST be @@ -906,14 +920,56 @@ fn generic_property_get_slot_load_is_reached_only_through_every_guard() { .find(|(_, rhs)| rhs.starts_with("load atomic i64") && rhs.contains("_packed_get")) .map(|(reg, _)| reg) .expect("compact MRU load"); + // "Has this site primed?" is answered BY the ShapeId compare, not by a + // test of its own: the site's word is born holding `PACKED_GET_EMPTY`, a + // value the word at `+4` of a receiver cannot hold. That word is either a + // ShapeId ([0x8000_0000, 0xC000_0000)), a synthetic class id (at or above + // 0x8000_0000 today, [0xC000_0000, 0xFFFF_0000) under #10824) or an + // ordinary HIR class id, which is a counter from 1 — so 0xFFFF_FFFF is + // above every one of them under BOTH id schemes. + // + // This replaces the old `icmp ne i64 %packed, 0` assertion. It is not a + // weakening: that assertion proved a guard existed, and these three prove + // the guard is UNNECESSARY — the sentinel is emitted, it is out of range, + // and the compare that subsumes it still gates the load. Re-introducing a + // zero initializer turns the first one red. + assert!( + ir.contains(&format!( + "_packed_get = private global i64 {PACKED_GET_EMPTY}, align 8" + )), + "the compact MRU must be born holding PACKED_GET_EMPTY, not zero:\n{ir}" + ); + assert!( + !(0x8000_0000..0xC000_0000).contains(&PACKED_GET_EMPTY), + "PACKED_GET_EMPTY must sit outside the ShapeId range so no stamped \ + receiver's shape word can equal an unprimed site" + ); assert!( - chain.contains(&format!("icmp ne i64 {packed}, 0")), - "the initial zero cache must not reach a field load: {chain}" + !(0x4000_0000..0x8000_0000).contains(&PACKED_GET_EMPTY), + "and outside the band a SPILL entry is flipped into, or an unprimed \ + site would be decoded as one" + ); + assert_eq!( + PACKED_GET_EMPTY, 0xFFFF_FFFF, + "and above every class id: synthetic ids are at or above 0x8000_0000 \ + today and [0xC000_0000, 0xFFFF_0000) under #10824, and an ordinary \ + HIR class id is a counter from 1" ); assert!( chain.contains(&format!("trunc i64 {packed} to i32")), "the exact packed ShapeId must gate the field load: {chain}" ); + // The overflow-bit test is no longer a guard on the inline load: a + // SPILL-located key publishes its ShapeId with PACKED_SPILL_FLIP flipped + // in, which lands it in [0x4000_0000, 0x8000_0000) — neither a ShapeId nor + // any class id — so the compare above refuses it without a question of its + // own. If the bit test comes back it is 10 bytes of `movabs`, a `test` and + // a branch on every read. + assert!( + !chain.contains(&PACKED_SPILL_FLIP.to_string()), + "the overflow-bit test must not gate the inline slot load — a spill \ + entry is refused by the ShapeId compare itself:\n{chain}" + ); if func.contains(", 134217983") { let masked = defs @@ -1174,9 +1230,12 @@ fn packed_pic_header_guard_is_endianness_aware() { #[test] fn compact_get_mru_is_atomic_and_full_cache_remains_lazy() { + use crate::expr::property_get::generic_dispatch::PACKED_GET_EMPTY; let ir = emit(false, None); assert!( - ir.contains("_packed_get = private global i64 0, align 8"), + ir.contains(&format!( + "_packed_get = private global i64 {PACKED_GET_EMPTY}, align 8" + )), "{ir}" ); assert!( @@ -1184,8 +1243,11 @@ fn compact_get_mru_is_atomic_and_full_cache_remains_lazy() { "{ir}" ); assert!(ir.contains("@js_object_get_field_ic_slow("), "{ir}"); + // The `trunc` is the ShapeId half of the compact word. There is no + // `icmp ne i64 %packed, 0` beside it any more: the sentinel above makes + // the ShapeId compare prove the site is primed as well. assert!( - ir.contains("trunc i64") && ir.contains("icmp ne i64"), + ir.contains("trunc i64") && !ir.contains("icmp ne i64"), "{ir}" ); assert!( @@ -1212,6 +1274,49 @@ fn compact_get_mru_is_atomic_and_full_cache_remains_lazy() { /// instructions on every HIT (measured). The separate non-pointer callee is /// what keeps that guard chain branchy, so the count below is 2 — and a change /// that makes it 1 is a hit-path regression, not a size win. +/// A SPILL-located key must still be RECOGNISED — just not on the hit path. +/// +/// Taking the overflow-bit test off the hit path is only sound if the entry it +/// used to catch is caught somewhere else. `pic.token.miss` un-flips +/// `PACKED_SPILL_FLIP` and branches straight to the one exit, skipping the +/// full cache's resolution and the ways (neither can hold an encoded slot). +/// Without this test, deleting the spill compare would leave every spill read +/// correct-but-slow — it would walk the ways, miss, call out, and re-scan the +/// keys array on every read, which is invisible in program output. +#[test] +fn a_spill_entry_is_recognised_in_the_token_miss_block_and_nowhere_else() { + use crate::expr::property_get::generic_dispatch::PACKED_SPILL_FLIP; + let ir = emit(false, None); + let func = ir + .split("\ndefine ") + .find(|f| f.contains("\npic.token.miss")) + .unwrap_or_else(|| panic!("no function contains the generic tower:\n{ir}")); + + // Split the function into blocks and find `pic.token.miss`'s body. + let mut body: Vec<&str> = Vec::new(); + let mut inside = false; + for line in func.lines() { + if !line.starts_with(' ') && line.ends_with(':') { + inside = line.trim_end_matches(':').starts_with("pic.token.miss"); + continue; + } + if inside { + body.push(line); + } + } + let body = body.join("\n"); + assert!( + body.contains(&format!("xor i32 ")) && body.contains(&PACKED_SPILL_FLIP.to_string()), + "`pic.token.miss` must un-flip PACKED_SPILL_FLIP to recognise a spill \ + entry:\n{body}" + ); + assert!( + body.contains("pic.miss.call"), + "a recognised spill entry must branch straight to the one exit, not \ + walk the ways:\n{body}" + ); +} + #[test] fn the_generic_tower_is_two_calls_and_a_bounded_number_of_blocks() { let ir = emit(false, None); @@ -1260,8 +1365,14 @@ fn the_generic_tower_is_two_calls_and_a_bounded_number_of_blocks() { "pic.recv_hdr", "pic.token", "pic.token.miss", + // The spill entry's landing block. `pic.token.miss` recognises a + // SPILL-located key by un-flipping PACKED_SPILL_FLIP and branches + // straight to the one exit; everything else continues here to the full + // cache and the ways. `pic.hit.inline` is GONE: with spill entries + // refused by the ShapeId compare itself, the hit block has nothing to + // decide between and the load sits directly in `pic.hit`. + "pic.token.ways", "pic.hit", - "pic.hit.inline", // The hit's hole edge keeps its own landing block so its tail is not // congruent with `pic.way.load`'s; `pic.hit.live` exists only when // typed feedback has something to record on the live edge. diff --git a/crates/perry-runtime/src/object/delete_rest.rs b/crates/perry-runtime/src/object/delete_rest.rs index f22daa3667..538fc15a39 100644 --- a/crates/perry-runtime/src/object/delete_rest.rs +++ b/crates/perry-runtime/src/object/delete_rest.rs @@ -492,9 +492,37 @@ pub extern "C" fn js_object_delete_field( if stable_candidate { (*obj_gc)._reserved |= crate::gc::OBJ_FLAG_STABLE_TOMBSTONES; } - let successor = super::shapes::publish_object_shape_holes(obj, holes + 1); + // A delete is a SHAPE TRANSITION: the successor is a pure + // function of (predecessor ShapeId, deleted key, vacated + // slot), and it is never the predecessor. That is what stops a + // `(shape, key)` cache entry primed for the deleted key from + // hitting afterwards, and therefore what makes a shape hit + // prove the slot it names is live — the fact the emitted read + // path currently establishes with a per-read `TAG_HOLE` + // compare instead. #9064 kept the id here and paid that + // compare on every read of every object, forever. + // + // `PERRY_DELETE_SHAPE_TRANSITION=0` restores #9064's + // id-preserving publish for A/B and attribution. + let transition = object_delete_shape_transition_enabled(); + let successor = if transition { + super::shapes::publish_object_shape_delete_transition( + obj, + crate::object::key_content_hash(key), + i as u32, + holes + 1, + ) + } else { + super::shapes::publish_object_shape_holes(obj, holes + 1) + }; if successor != 0 { - let stable = stable_candidate && successor == predecessor; + // The marker certifies the SLOT REPRESENTATION (this + // receiver's inline slots may hold `TAG_HOLE`, and a + // re-add appends into its private array in place), not the + // shape identity. Under the transition it is no longer + // conditional on the publish having kept the id, because + // the publish never keeps it. + let stable = stable_candidate && (transition || successor == predecessor); if !stable { (*obj_gc)._reserved &= !crate::gc::OBJ_FLAG_STABLE_TOMBSTONES; } @@ -589,7 +617,7 @@ pub extern "C" fn js_object_delete_field( // ObjectHeader facts". `publish_object_shape_from` versions a // same-pointer change internally, and `keys_changed` is false here // so the typed layout is preserved rather than marked unknown. - set_object_keys_array(obj, keys as *mut crate::ArrayHeader); + set_object_keys_array(obj, keys); super::shapes::shape_index_shift_in_place(keys as usize, i as u32, key_count as u32) } else { let keys_cloned = crate::array::js_array_alloc(new_count.max(1) as u32 + 4); @@ -958,28 +986,42 @@ unsafe fn try_delete_stable_sso(obj: *mut ObjectHeader, key: JSValue) -> Option< } super::prop_plan::prop_plan_epoch_bump(); - // This narrower helper cannot mint a descriptor or allocate, so `obj` - // and `keys` remain valid across the structural update below. - if super::shapes::try_update_stable_tombstone_shape_cached( - obj, - shape, - shape.logical_key_count, - shape.live_inline_slot_count, - next_holes, - ) - .or_else(|| { - super::shapes::try_update_stable_tombstone_shape( + // A delete MOVES the shape word, exactly as in `js_object_delete_field`: + // keeping the id here would leave this lane — the SSO dynamic-key delete + // — as the one hole in that guarantee, and a `(shape, key)` entry primed + // for the deleted key would still match the receiver afterwards. + // + // The publish mints/rekeys a DESCRIPTOR, which is a Rust-side table + // update, not a heap allocation: it cannot collect, so `obj` and `keys` + // remain valid across the structural update below, which is what the + // id-preserving helpers were relied on for. + let published = if object_delete_shape_transition_enabled() { + let id = super::shapes::publish_object_shape_delete_transition( + obj, + crate::object::key_bytes_hash(key_bytes.as_ptr(), key_bytes.len()), + slot, + next_holes, + ); + (id != 0).then_some(id) + } else { + super::shapes::try_update_stable_tombstone_shape_cached( obj, - keys, + shape, shape.logical_key_count, shape.live_inline_slot_count, next_holes, ) - }) - .is_none() - { - return None; - } + .or_else(|| { + super::shapes::try_update_stable_tombstone_shape( + obj, + keys, + shape.logical_key_count, + shape.live_inline_slot_count, + next_holes, + ) + }) + }; + published?; crate::gc::runtime_store_external_jsvalue_slot( keys as usize, elements.add(slot as usize) as usize, @@ -1514,6 +1556,34 @@ mod sso_tests_1781 { } } +/// Is `delete` a SHAPE TRANSITION (successor != predecessor, memoized on +/// `(predecessor ShapeId, key, slot)`), or #9064's id-preserving publish? +/// +/// Default ON. The transition is what lets the emitted read path stop +/// comparing every loaded slot against `TAG_HOLE`: with the id preserved, a +/// `(shape, key)` cache entry primed before a delete still matches the +/// receiver afterwards, so only the slot's own contents can reveal the delete. +/// +/// `PERRY_DELETE_SHAPE_TRANSITION=0` restores the #9064 publish, so the two +/// can be A/B'd in ONE binary — the arms then differ only in this decision, +/// with no compiler/runtime source-hash pairing to drift. +fn object_delete_shape_transition_enabled() -> bool { + // Same reason as `object_tombstone_deletes_enabled`'s override: the + // `OnceLock` latches at the first delete anywhere in the test process, + // long before a test's own `set_var`. + #[cfg(test)] + if let Some(forced) = DELETE_TRANSITION_TEST_OVERRIDE.with(std::cell::Cell::get) { + return forced; + } + static ON: std::sync::OnceLock = std::sync::OnceLock::new(); + *ON.get_or_init(|| { + !matches!( + std::env::var("PERRY_DELETE_SHAPE_TRANSITION").as_deref(), + Ok("0") | Ok("off") | Ok("false") + ) + }) +} + /// Gate for O(1) tombstone deletes (`PERRY_OBJECT_TOMBSTONES`). /// The default and its rationale live beside the environment parsing below. fn object_tombstone_deletes_enabled() -> bool { @@ -1547,6 +1617,25 @@ fn object_tombstone_deletes_enabled() -> bool { thread_local! { static TOMBSTONE_TEST_OVERRIDE: std::cell::Cell> = const { std::cell::Cell::new(None) }; + static DELETE_TRANSITION_TEST_OVERRIDE: std::cell::Cell> = + const { std::cell::Cell::new(None) }; +} + +/// [`test_scope_tombstone_deletes`] for the delete-shape-transition flag, so a +/// test can pin #9064's id-preserving publish or the transition explicitly +/// rather than inheriting whatever the env latched. +#[cfg(test)] +pub(crate) fn test_scope_delete_shape_transition(forced: bool) -> impl Drop { + struct Restore(Option); + + impl Drop for Restore { + fn drop(&mut self) { + DELETE_TRANSITION_TEST_OVERRIDE.with(|cell| cell.set(self.0)); + } + } + + let previous = DELETE_TRANSITION_TEST_OVERRIDE.with(|cell| cell.replace(Some(forced))); + Restore(previous) } /// Force the tombstone-delete flag for the CURRENT THREAD's asserts, diff --git a/crates/perry-runtime/src/object/field_get_set/ic_miss.rs b/crates/perry-runtime/src/object/field_get_set/ic_miss.rs index 4005e85cf2..cdac5155da 100644 --- a/crates/perry-runtime/src/object/field_get_set/ic_miss.rs +++ b/crates/perry-runtime/src/object/field_get_set/ic_miss.rs @@ -239,6 +239,77 @@ pub const PIC_CACHE_WORDS: usize = 12; /// | 11 | round-robin victim index for the ways | pub type PicCache = [i64; PIC_CACHE_WORDS]; +/// The value a per-site compact MRU word (`@perry_ic_N_packed_get`) holds +/// before anything primes it. +/// +/// **Must equal `PACKED_GET_EMPTY` in perry-codegen's +/// `expr/property_get/generic_dispatch.rs`** — `packed_get_sentinels_match_codegen` +/// below and `pic_cache_layout_matches_runtime` there hold the pair. +/// +/// It is deliberately NOT zero. The emitted hit path compares the receiver's +/// ShapeId word at `+4` against this word's low half; an object that was never +/// shape-stamped carries `parent_class_id` there, which is 0 for an anonymous +/// object literal, so a zero sentinel would let such a receiver MATCH an +/// unprimed site and take the raw load at slot 0. That is why the tower used +/// to spend a `test`/`je` on every read proving the word was filled. A +/// sentinel no receiver word can equal makes the ShapeId compare prove both +/// facts at once. +/// +/// `0xFFFF_FFFF`. The u32 at `+4` is a ShapeId ([0x8000_0000, 0xC000_0000)), +/// a synthetic class id, or an ordinary HIR class id (a counter from 1). +/// Synthetic ids sit at or above 0x8000_0000 today and move to +/// [0xC000_0000, 0xFFFF_0000) under #10824, so `0xFFFF_FFFF` is above every +/// one of them under BOTH schemes, and an ordinary id would need ~2^32 +/// classes to reach it. +pub(crate) const PACKED_GET_EMPTY: u64 = 0xFFFF_FFFF; + +/// Bit flipped into the ShapeId a compact MRU word publishes when the key is +/// SPILL-located. **Must equal `PACKED_SPILL_FLIP` in perry-codegen.** +/// +/// ShapeIds live in [0x8000_0000, 0xC000_0000), so flipping the TOP TWO bits +/// lands a spill entry in [0x4000_0000, 0x8000_0000) — the one u32 band that +/// is neither a ShapeId, nor a synthetic class id (at or above 0x8000_0000 +/// today, [0xC000_0000, 0xFFFF_0000) under #10824), nor reachable by an +/// ordinary HIR class id without ~2^30 classes. Flipping only bit 30 would +/// land it in [0xC000_0000, 2^32), which #10824 turns into the synthetic +/// class-id range — a receiver carrying one would then take the inline load +/// with a spill index, silently. +/// The emitted hit path's plain compare therefore declines a spill entry for +/// free, which is what let the overflow-bit test (a 10-byte `movabs`, a `test` +/// and a branch) leave the hit path of every site, including the ones whose +/// field is inline and could never see the bit. +pub(crate) const PACKED_SPILL_FLIP: u32 = 0xC000_0000; + +/// Decode a compact MRU word into `(ShapeId, index, is_spill)`. +/// +/// `None` for the unprimed sentinel. The inverse of the encoding in +/// [`packed_get::prime_get`]; the emitted code open-codes the two compares +/// this performs, so any change here is a change there. +#[inline] +pub(crate) fn packed_get_decode(word: u64) -> Option<(u32, u32, bool)> { + use crate::object::shapes::{SHAPE_ID_BASE, SHAPE_ID_END}; + // The overwhelmingly common word on a cold site, and the one the + // emitted global is born holding. Named rather than range-derived so + // this stays visibly paired with the perry-codegen copy of it. + if word == PACKED_GET_EMPTY { + return None; + } + let key32 = word as u32; + let index = (word >> 32) as u32; + if (SHAPE_ID_BASE..SHAPE_ID_END).contains(&key32) { + return Some((key32, index, false)); + } + // The spill band is the ShapeId range with [`PACKED_SPILL_FLIP`] flipped + // in, and the flip is an involution, so this recognises exactly the words + // [`packed_get::prime_get`] can publish for a spill-located key. + let unflipped = key32 ^ PACKED_SPILL_FLIP; + if (SHAPE_ID_BASE..SHAPE_ID_END).contains(&unflipped) { + return Some((unflipped, index, true)); + } + // [`PACKED_GET_EMPTY`], and anything else no prime can publish. + None +} + /// The per-site slot codegen emits for a property-read cache — `@perry_ic_N = /// private global ptr null` — holding null until the site's first priming /// miss, then the arena cache `pic_slot_resolve` published (#9708). The diff --git a/crates/perry-runtime/src/object/field_get_set/ic_miss/ic_slow.rs b/crates/perry-runtime/src/object/field_get_set/ic_miss/ic_slow.rs index 5fc5207e3d..b296441b46 100644 --- a/crates/perry-runtime/src/object/field_get_set/ic_miss/ic_slow.rs +++ b/crates/perry-runtime/src/object/field_get_set/ic_miss/ic_slow.rs @@ -125,6 +125,18 @@ pub extern "C" fn js_object_get_field_ic_nonptr( let tag = bits >> 48; let obj_unmasked = bits as usize as *const ObjectHeader; + // Heap STRING receiver. Only a `.length` site still tests the two + // pointer-ish tags together (`(tag & 0xFFFD) == 0x7FFD`) and keeps its + // inline string arm; every other key now emits the EXACT POINTER test, so + // a string receiver arrives here instead of being unmasked, admitted by + // the tag test, and rejected by the GC-kind guard four loads later. The + // answer is the same one the object exit produced — the by-name helper — + // but the pointer must be MASKED first: that helper normalizes only the + // 0x7FFD tag, so handing it a 0x7FFF-tagged box would be a wild pointer. + if tag == crate::value::STRING_TAG >> 48 { + let masked = (bits & 0x0000_FFFF_FFFF_FFFF) as usize as *const ObjectHeader; + return super::js_object_get_field_by_name_f64(masked, key); + } // SSO receiver (SHORT_STRING_TAG): the SSO-aware by-name helper reads // `.length` from the NaN-box payload and answers undefined otherwise. // A `.length` site keeps serving this inline and never gets here. @@ -189,9 +201,9 @@ unsafe fn overflow_arm( obj: *const ObjectHeader, key: *const crate::StringHeader, cache_slot: *mut PicCacheSlot, - slot: u32, + index: u32, ) -> f64 { - let idx = (slot & !crate::proxy::IC_SLOT_OVERFLOW_BIT) as usize; + let idx = index as usize; if let Some(v) = crate::object::overflow_get(obj as usize, idx) { if v != crate::value::TAG_HOLE { return f64::from_bits(v); @@ -244,18 +256,20 @@ pub extern "C" fn js_object_get_field_ic_slow( // only re-derive what the equality already proves (#809's // keyless receiver fails the equality, not the range test). let word = (*packed).load(Ordering::Relaxed); - if word != 0 && (*obj).parent_class_id == word as u32 { - let slot = (word >> 32) as u32; - if slot & crate::proxy::IC_SLOT_OVERFLOW_BIT != 0 { - return overflow_arm(obj, key, cache_slot, slot); + if let Some((stamp, index, is_spill)) = super::ic_miss::packed_get_decode(word) + { + if (*obj).parent_class_id == stamp { + if is_spill { + return overflow_arm(obj, key, cache_slot, index); + } + // An inline slot that reached this entry on a token hit + // was a `TAG_HOLE` — the field was deleted since + // priming. The emitted `pic.hit.deleted` edge took the + // ordinary miss WITH the packed word. + return super::ic_miss::get_field_ic_miss_impl( + obj, key, cache_slot, packed, + ); } - // An inline slot that reached this entry on a token hit - // was a `TAG_HOLE` — the field was deleted since - // priming. The emitted `pic.hit.deleted` edge took the - // ordinary miss WITH the packed word. - return super::ic_miss::get_field_ic_miss_impl( - obj, key, cache_slot, packed, - ); } } // --- 3. the Array-subclass named-prefix proof -------------- @@ -401,9 +415,10 @@ mod tests { assert_eq!(read(&mut slot, &packed), 3.0, "the priming read"); let word = packed.load(Ordering::Relaxed); assert_ne!(word, 0, "test premise: the site primed"); - assert_ne!( - (word >> 32) as u32 & crate::proxy::IC_SLOT_OVERFLOW_BIT, - 0, + let (_, _, is_spill) = super::super::ic_miss::packed_get_decode(word) + .expect("test premise: the priming read published a compact entry"); + assert!( + is_spill, "test premise: the fourth field must live past the inline region, \ or this test never reaches the overflow arm (packed word {word:#x})" ); diff --git a/crates/perry-runtime/src/object/field_get_set/ic_miss/packed_get.rs b/crates/perry-runtime/src/object/field_get_set/ic_miss/packed_get.rs index aa3e424165..6ead25a1c5 100644 --- a/crates/perry-runtime/src/object/field_get_set/ic_miss/packed_get.rs +++ b/crates/perry-runtime/src/object/field_get_set/ic_miss/packed_get.rs @@ -43,10 +43,29 @@ pub(super) unsafe fn prime_get( { return; } - // Low 32 bits: exact nonzero ShapeId. High 32: slot, including the - // overflow flag. Generated code rejects the zero-initialized word. + // Low 32 bits: the exact ShapeId for an INLINE slot, or that ShapeId with + // [`super::PACKED_SPILL_FLIP`] flipped into it for a SPILL-located one. + // High 32: the slot or spill index, with no flag bit of its own. + // + // The flip is what took the overflow-bit test off the emitted hit path. + // ShapeIds live in [0x8000_0000, 0xC000_0000), so flipping the top two + // bits lands a spill entry in [0x4000_0000, 0x8000_0000) — the one u32 + // band that is neither a ShapeId nor any class id — and the emitted + // compare refuses it without asking a question of its own. + // `pic.token.miss` un-flips the bits and routes the read to the slow + // entry, which decodes the same word. + // // Relaxed suffices: this publishes a numeric layout fact, not an object. - (*packed).store((slot as u64) << 32 | stamp as u64, Ordering::Relaxed); + let raw = slot as u32; + let (key32, index) = if raw & crate::proxy::IC_SLOT_OVERFLOW_BIT != 0 { + ( + stamp ^ super::PACKED_SPILL_FLIP, + raw & !crate::proxy::IC_SLOT_OVERFLOW_BIT, + ) + } else { + (stamp, raw) + }; + (*packed).store((index as u64) << 32 | key32 as u64, Ordering::Relaxed); } #[cfg(test)] @@ -55,13 +74,16 @@ mod tests { #[test] fn packed_pair_preserves_identity_overflow_and_empty_site() { - let packed = AtomicU64::new(0); + let packed = AtomicU64::new(crate::object::field_get_set::ic_miss::PACKED_GET_EMPTY); let mut cache = [0; super::super::PIC_CACHE_WORDS]; let bit = crate::object::shapes::PIC_ID_TOKEN_BIT; unsafe { prime_get(&mut cache, bit as i64, 0, &packed); } - assert_eq!(packed.load(Ordering::Relaxed), 0); + assert_eq!( + packed.load(Ordering::Relaxed), + crate::object::field_get_set::ic_miss::PACKED_GET_EMPTY + ); for stamp in [ crate::object::shapes::SHAPE_ID_BASE, crate::object::shapes::SHAPE_ID_END - 1, @@ -71,8 +93,41 @@ mod tests { prime_get(&mut cache, (bit | stamp as u64) as i64, slot, &packed); } let word = packed.load(Ordering::Relaxed); - assert_eq!(word & 0xffff_ffff, stamp as u64); - assert_eq!(word >> 32, slot as u64); + let raw = slot as u32; + let spill = raw & crate::proxy::IC_SLOT_OVERFLOW_BIT != 0; + let want_key32 = if spill { + stamp ^ crate::object::field_get_set::ic_miss::PACKED_SPILL_FLIP + } else { + stamp + }; + assert_eq!(word & 0xffff_ffff, want_key32 as u64); + assert_eq!( + word >> 32, + (raw & !crate::proxy::IC_SLOT_OVERFLOW_BIT) as u64 + ); + // A spill entry must be UNMATCHABLE by the emitted hit path: + // its low half has to sit outside the ShapeId range so the + // plain compare declines it with no test of its own, and + // outside every class-id range so no UNSTAMPED receiver can + // match it either. + if spill { + assert!( + (0x4000_0000..crate::object::shapes::SHAPE_ID_BASE) + .contains(&(word as u32)), + "a spill entry must land in the one u32 band that is \ + neither a ShapeId nor any class id" + ); + assert_eq!( + super::super::packed_get_decode(word), + Some((stamp, raw & !crate::proxy::IC_SLOT_OVERFLOW_BIT, true)), + "and the runtime must decode it back to the same pair" + ); + } else { + assert_eq!( + super::super::packed_get_decode(word), + Some((stamp, raw, false)) + ); + } let before = word; unsafe { prime_get(&mut cache, bit as i64, 0, &packed); diff --git a/crates/perry-runtime/src/object/shapes.rs b/crates/perry-runtime/src/object/shapes.rs index 215a2ea560..944a03cbbf 100644 --- a/crates/perry-runtime/src/object/shapes.rs +++ b/crates/perry-runtime/src/object/shapes.rs @@ -42,7 +42,7 @@ mod shapes_store; pub(crate) use shapes_slot_list::shape_descriptor_keys_slot; pub(crate) use shapes_slot_list::shape_id_owns_keys_slot; pub(crate) use shapes_slot_list::{ - object_shape_hole_count, publish_object_shape_holes, + object_shape_hole_count, publish_object_shape_delete_transition, publish_object_shape_holes, rekey_stable_tombstone_shape_after_squeeze, retire_owned_shape_history, shape_index_migrate_after_delete, shape_index_shift_in_place, try_update_stable_tombstone_shape, try_update_stable_tombstone_shape_cached, SlotIndex, @@ -554,6 +554,17 @@ fn alloc_shape_id() -> Result { alloc_shape_id_from(&SHAPE_ID_NEXT) } +/// The next ShapeId this process would hand out. +/// +/// Tests assert the DELTA across a workload, because ids come from a 2^30 +/// counter that is never reused and parks (fail-stop) at the end: a path that +/// mints one id per operation is a process-LIFETIME bug, not merely a memory +/// cost, and nothing in the program's output ever reveals it. +#[cfg(test)] +pub(crate) fn test_shape_id_counter() -> u32 { + SHAPE_ID_NEXT.load(std::sync::atomic::Ordering::Relaxed) +} + /// Get or create the exact structural descriptor. The public allocation and /// mutation paths turn exhaustion into a fail-stop before publishing an /// untracked layout; the `Result` stays explicit so the allocator boundary and @@ -1634,19 +1645,6 @@ pub(crate) unsafe fn transition_object_shape_semantics( id } -/// Publish the successor shape for an O(1) hole-delete on `obj`'s CURRENT -/// keys array: same address, same surviving slots, one more tombstone. -/// -/// Modeled on [`transition_object_shape_semantics`]: the structural facts are -/// unchanged except `hole_count`, and the fresh process-unique generation is -/// what retires every cached `(token, key)` pair for this receiver — a -/// deleted key must stop hitting even though the array address and every -/// surviving slot are byte-identical, or a stale IC hit would return the -/// cleared slot instead of walking the prototype chain. -/// -/// Returns the successor id, or 0 when the object is not stamped/shaped — -/// the caller falls back to the compacting delete. - /// #10287: a DATA-descriptor install reuses one generation per /// `(predecessor facts, key, attributes)`, so two receivers built the same way /// keep sharing shapes — and therefore transition edges, keys arrays and every diff --git a/crates/perry-runtime/src/object/shapes_slot_list.rs b/crates/perry-runtime/src/object/shapes_slot_list.rs index 9c482d518d..3a856a3532 100644 --- a/crates/perry-runtime/src/object/shapes_slot_list.rs +++ b/crates/perry-runtime/src/object/shapes_slot_list.rs @@ -281,7 +281,9 @@ impl Iterator for SlotCandidates<'_> { } } -use super::shapes_store::{ShapeRecord, RECORD_FLAG_FACTS_INDEXED}; +use super::shapes_store::{ + ShapeRecord, RECORD_FLAG_CACHE_CARRIER, RECORD_FLAG_EXTERNAL_CARRIER, RECORD_FLAG_FACTS_INDEXED, +}; /// Shift a key index in place after an IN-PLACE delete. /// @@ -614,6 +616,21 @@ pub(crate) unsafe fn rekey_stable_tombstone_shape_after_squeeze( Some(new_id) } +/// Publish the successor shape for an O(1) hole-delete on `obj`'s CURRENT +/// keys array: same address, same surviving slots, one more tombstone. +/// +/// This is #9064's ID-PRESERVING publish, now reached only through +/// `PERRY_DELETE_SHAPE_TRANSITION=0` and the squeeze: +/// [`publish_object_shape_delete_transition`] is what an ordinary delete takes. +/// For a stable-tombstone receiver it keeps the ShapeId and relies on the +/// emitted read's per-slot `TAG_HOLE` compare to retire the deleted key; +/// otherwise it mints a process-unique generation. +/// +/// Returns the successor id, or 0 when the object is not stamped/shaped — +/// the caller falls back to the compacting delete. +/// +/// (This doc block lived in `shapes.rs` after the function moved here, where +/// it documented nothing.) pub(crate) unsafe fn publish_object_shape_holes( obj: *mut crate::object::ObjectHeader, hole_count: u32, @@ -624,6 +641,12 @@ pub(crate) unsafe fn publish_object_shape_holes( let Some(current) = super::object_shape_descriptor(obj) else { return 0; }; + // A hole delete is a STRUCTURAL change to the layout, so the Array-subclass + // named-prefix proof must go — it is the one identity that deliberately + // survives a ShapeId change, and a stale one lets a cached slot for the + // deleted key still be served. Every other transition publisher clears it; + // this one and its stable-tombstone sibling did not. + crate::array::clear_array_subclass_named_prefix_token(obj); if let Some(id) = try_update_stable_tombstone_shape( obj, current.keys as usize as *mut super::ArrayHeader, @@ -672,35 +695,368 @@ pub(crate) unsafe fn publish_object_shape_holes( // per delete and every later publish walked it, which measured as a 26x // slowdown (2.06 s → 53.6 s) on `bench_populated_delete` before this // line existed. - { - let mut inner = crate::state::state().shapes.inner.borrow_mut(); - // Sweep EVERY other id for this keys address, not just the direct - // predecessor: the delete-then-re-add cycle publishes an id on the - // APPEND side too, and nothing else retires those — the post-trace - // dead-key pruning only fires when the keys ARRAY dies, and this - // array lives at a stable address for the object's whole life. - // Retiring only the predecessor halved the descriptor pile-up - // (53.6 s → 25.1 s on the churn benchmark) but ids still accumulated - // one per iteration from the append publish. - let stale: Vec = inner - .families - .get(&(current.keys)) - .map(|ids| { - ids.as_slice() - .iter() - .copied() - .filter(|&other| other != id) - .collect() - }) - .unwrap_or_default(); - for other in stale { + // + // Sweep EVERY other id for this keys address, not just the direct + // predecessor: the delete-then-re-add cycle publishes an id on the + // APPEND side too, and nothing else retires those — the post-trace + // dead-key pruning only fires when the keys ARRAY dies, and this + // array lives at a stable address for the object's whole life. + // Retiring only the predecessor halved the descriptor pile-up + // (53.6 s → 25.1 s on the churn benchmark) but ids still accumulated + // one per iteration from the append publish. + retire_family_except(current.keys, id); + super::debug_assert_object_shape_parity(obj); + id +} + +/// Semantic generation for a DELETE edge, as a PURE function of the +/// transition: the predecessor's identity, the deleted key, and the slot the +/// delete vacates. +/// +/// This is [`super::deterministic_semantic_generation`]'s rule (#10287) +/// applied to `delete`. Two receivers that delete the same key from the same +/// predecessor shape therefore agree on the successor's generation — and, +/// when they also share the predecessor's keys allocation, on the successor +/// ShapeId itself, so a delete does not fork their shape lineages. +/// +/// Soundness is the same induction #10287 rests on: the predecessor ShapeId +/// implies the predecessor's exact layout, and (layout, key, slot) implies the +/// successor's, so two publications that agree on this generation *and* on the +/// structural facts describe the same layout. Distinct transitions collide +/// only on a full 64-bit hash collision. +/// +/// Bit 63 keeps these out of the counter's namespace exactly as +/// [`super::deterministic_semantic_generation`] does. The `0xFD` tag keeps a +/// delete of key `k` from aliasing a descriptor install over the same +/// predecessor (real attribute bytes are `< 0x10`) or a descriptor removal +/// (`0xFE` attribute entry / `0xFF` accessor entry). +pub(super) fn delete_transition_generation( + prev_shape_id: u32, + key_hash: u64, + slot: u32, +) -> Option { + if prev_shape_id == 0 { + // No predecessor identity to key on: the caller keeps the unique + // generation, which is always correct, just unshareable. + return None; + } + // SplitMix64 finalizer over the four components, so nearby shape ids, + // adjacent slots and one-byte key differences land far apart. + let mut x = key_hash + ^ (u64::from(prev_shape_id) << 32 | u64::from(prev_shape_id)) + ^ (u64::from(slot) << 16) + ^ (0xFDu64 << 24); + x ^= x >> 30; + x = x.wrapping_mul(0xbf58_476d_1ce4_e5b9); + x ^= x >> 27; + x = x.wrapping_mul(0x94d0_49bb_1331_11eb); + x ^= x >> 31; + Some(x | (1 << 63)) +} + +/// Retire every descriptor indexed under `keys` other than `keep`. +/// +/// Extracted from [`publish_object_shape_holes`], which is where the rule was +/// established: the tombstone lanes are gated on an OWNED keys array, so the +/// publishing receiver is the only carrier of every other id under that +/// address and the ids become unreachable the moment its header word names +/// `keep`. A stale IC token already misses on the stamp compare and +/// `shape_descriptor_by_id` of a retired id is `None`. +/// +/// Without the sweep a delete-churn loop piles one descriptor per delete onto +/// ONE stable address; the reverse-index list under it grows by one per +/// iteration and every later publish walks it (measured at 2.06 s -> 53.6 s on +/// `bench_populated_delete` before the sweep existed). +fn retire_family_except(keys: u64, keep: u32) { + let mut inner = crate::state::state().shapes.inner.borrow_mut(); + // The family here is almost always exactly `{predecessor, keep}` — the + // previous publish swept everything else. Lift that single id out without + // the `Vec` the general case needs (the table borrow cannot be held across + // `remove_descriptor_and_reverse_indices`). This runs on EVERY delete + // under the shape transition, not only on the non-stable ones, so the + // allocation is per-delete rather than occasional. + let mut only_stale = None; + let mut more_than_one = false; + if let Some(ids) = inner.families.get(&keys) { + for &other in ids.as_slice() { + if other == keep { + continue; + } + if only_stale.is_none() { + only_stale = Some(other); + } else { + more_than_one = true; + break; + } + } + } + if !more_than_one { + if let Some(other) = only_stale { super::remove_descriptor_and_reverse_indices(&mut inner, other); } + return; + } + let stale: Vec = inner + .families + .get(&keys) + .map(|ids| { + ids.as_slice() + .iter() + .copied() + .filter(|&other| other != keep) + .collect() + }) + .unwrap_or_default(); + for other in stale { + super::remove_descriptor_and_reverse_indices(&mut inner, other); + } +} + +/// The DELETE edge of the shape transition graph: +/// `(predecessor ShapeId, deleted key, vacated slot) -> successor ShapeId`. +/// +/// Unlike [`publish_object_shape_holes`], this NEVER hands back the +/// predecessor. `delete` must move the shape word, because that is what makes +/// a `(shape, key)` cache entry primed for the deleted key unable to hit +/// afterwards — and therefore what lets a shape hit prove that the slot it +/// names is LIVE. Today the emitted read path proves that with a per-read +/// `TAG_HOLE` compare instead (#9064's stable tombstones deliberately kept the +/// id); this is the structural replacement for that compare. +/// +/// Returns 0 when the receiver is unstamped/unshaped, or when the publication +/// would have reinstated the predecessor id — in both cases the caller falls +/// back to the compacting delete, which needs no shape stamp. +pub(crate) unsafe fn publish_object_shape_delete_transition( + obj: *mut crate::object::ObjectHeader, + key_hash: u64, + slot: u32, + hole_count: u32, +) -> u32 { + if obj.is_null() || !super::shape_word_is_writable(obj) { + return 0; + } + let Some(current) = super::object_shape_descriptor(obj) else { + return 0; + }; + let predecessor = super::object_shape_stamp(obj); + // A delete is a STRUCTURAL transition, so the Array-subclass + // named-prefix proof has to go: it is the one identity that deliberately + // SURVIVES a ShapeId change ("proves the cached slot survives exact + // numeric-tail ShapeId transitions"), so leaving it armed would let a + // cached slot for the deleted key still be served — the exact hole the + // shape transition exists to close. Every other transition publisher + // already clears it; the hole-delete publishes did not. + crate::array::clear_array_subclass_named_prefix_token(obj); + // The key count comes from the ARRAY, not the lineage: an O(1) hole + // delete leaves the length untouched, and the caller has not yet written + // the hole, so both agree here. Reading the array keeps this function + // honest if a future caller publishes after a length change. + let keys_ptr = current.keys as usize as *mut super::ArrayHeader; + let logical_key_count = crate::array::keys_array_len_capped_to_capacity(keys_ptr) as u32; + let generation = + delete_transition_generation(predecessor, key_hash, slot).unwrap_or_else(|| { + let generation = + super::SHAPE_SEMANTIC_NEXT.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + if generation == 0 { + super::shape_id_exhausted_abort(); + } + generation + }); + // An OWNED keys array makes this receiver the single carrier of the + // predecessor, so the cheapest correct publish is to MOVE the + // predecessor's record onto a fresh id rather than mint a second + // descriptor and retire the first. + let owned = !keys_array_is_shape_shared(keys_ptr); + let mut id = if owned { + rekey_predecessor_for_delete( + predecessor, + current.keys, + logical_key_count, + current.live_inline_slot_count, + generation, + hole_count, + ) + } else { + 0 + }; + if id == 0 { + id = mint_detached_delete_successor( + current.keys, + logical_key_count, + current.live_inline_slot_count, + generation, + current.object_kind, + hole_count, + ); + } + if id == 0 { + return 0; + } + // `alloc_shape_id` never reuses a value, so a freshly minted successor + // cannot be the predecessor. Stated as an assert because the whole point + // of this function is that it never hands the predecessor back. + debug_assert_ne!( + id, predecessor, + "a delete must not keep the receiver's ShapeId" + ); + // #9200: stamp through the carrier-note funnel, which arms `old_carrier` + // for a non-nursery receiver. This publish is the one that mints a fresh + // descriptor and then retires the armed predecessor below, so without the + // funnel an evacuating minor could sweep a live keys array. + super::stamp_object_shape_id_with_carrier_note(obj, id); + // The rekey above already removed the predecessor, but growth-era prefix + // descriptors can still sit under this address from before the receiver + // entered the lane. Sweeping is sound only because the array is OWNED, + // which makes this receiver the single carrier of every id under it: + // retiring a SIBLING's live stamp would leave it shapeless — an empty + // `Object.keys()` and `undefined` fixed-slot reads, silently (#9200's + // exact wrong answer, reached a different way). On the steady-state churn + // path the family already holds only `id`, and the sweep is then one + // lookup with nothing to remove. + if owned { + retire_family_except(current.keys, id); } super::debug_assert_object_shape_parity(obj); id } +/// Move the predecessor's record onto a FRESH id carrying the delete's facts. +/// Returns 0 when it declines, and the caller mints instead. +/// +/// This is [`rekey_stable_tombstone_shape_after_squeeze`]'s primitive applied +/// to the delete edge, and it is what makes a per-delete shape transition +/// affordable. An OWNED keys array makes this receiver the single carrier of +/// the predecessor, so minting a second descriptor and retiring the first +/// reaches the same end state through two hash-table inserts and two removes; +/// moving the record does it with one slab move and one in-place id swap. +/// +/// The predecessor id stops resolving the moment its record moves, which is +/// precisely the retirement a delete owes: a cache entry still holding it +/// resolves to no descriptor and takes the ordinary miss. +/// +/// Declines for a record an optimization cache owns — that cache may reinstall +/// it while no object carries it, so its id has to survive — and for a record +/// whose keys edge has drifted from the caller's. +fn rekey_predecessor_for_delete( + predecessor: u32, + keys: u64, + logical_key_count: u32, + live_inline_slot_count: u32, + semantic_generation: u64, + hole_count: u32, +) -> u32 { + if !super::is_shape_id(predecessor) { + return 0; + } + let table = &crate::state::state().shapes; + let Some(live_ptr) = table.slab().record_ptr(predecessor) else { + return 0; + }; + // SAFETY: live slab record, single-threaded agent, read immediately. + let live = unsafe { *live_ptr }; + if live.keys != keys || live.has(RECORD_FLAG_CACHE_CARRIER | RECORD_FLAG_EXTERNAL_CARRIER) { + return 0; + } + let Ok(id) = super::alloc_shape_id() else { + return 0; + }; + let mut inner = table.inner.borrow_mut(); + if live.has(RECORD_FLAG_FACTS_INDEXED) { + // Only the FIRST delete on a receiver pays this: the record is born + // detached from then on, which is also what keeps the re-add on its + // cheap `try_update_stable_tombstone_shape_cached` path. + inner.facts_remove(live.facts_key_with_keys(keys), predecessor); + } + // SAFETY: no slab reference is held across these calls. + let Some(mut record) = (unsafe { table.slab_mut().remove(predecessor) }) else { + return 0; + }; + record.logical_key_count = logical_key_count; + record.live_inline_slot_count = live_inline_slot_count; + record.semantic_generation = semantic_generation; + record.hole_count = hole_count; + record.set(RECORD_FLAG_FACTS_INDEXED, false); + super::retire_cached_shape_object_kind(predecessor); + // SAFETY: as above. + unsafe { table.slab_mut().insert(id, record) }; + let replaced = inner + .families + .get_mut(&keys) + .is_some_and(|ids| ids.replace(predecessor, id)); + if !replaced { + // `id` came from `alloc_shape_id` above and is in no list yet. + inner.family_append_fresh(keys, id); + } + id +} + +/// Mint a FRESH descriptor for a delete successor, DETACHED from exact-facts +/// interning. Returns 0 when the id space is exhausted. +/// +/// The accelerator is skipped deliberately, not forgotten. Its key includes +/// the keys array's ADDRESS, and the tombstone lane is gated on an OWNED +/// array, so no second receiver can ever present these facts: every entry the +/// index gained had to be removed again by the retirement sweep, and a third +/// time by the re-add's detach (`try_update_stable_tombstone_shape`), which +/// also pushed the re-add off its cheap `_cached` path. Six hash-table +/// operations per delete/re-add cycle for an index with no possible reader, +/// measured at +1153 instructions per cycle on `bench_populated_delete`. +/// +/// The successor's `semantic_generation` is still the deterministic +/// [`delete_transition_generation`], so the identity of the transition is +/// unchanged — only its discoverability is. The day shape facts stop carrying +/// the keys address, indexing becomes useful (two receivers could then agree +/// on one successor) and this is the line that turns it back on. +fn mint_detached_delete_successor( + keys: u64, + logical_key_count: u32, + live_inline_slot_count: u32, + semantic_generation: u64, + object_kind: super::ShapeObjectKind, + hole_count: u32, +) -> u32 { + let Ok(id) = super::alloc_shape_id() else { + return 0; + }; + let mut record = ShapeRecord::new( + keys, + logical_key_count, + live_inline_slot_count, + semantic_generation, + object_kind, + hole_count, + ); + // `ShapeRecord::new` sets the flag by default, because its usual caller + // inserts into `by_facts` on the next line. This record is never inserted + // there, and the flag is what both stable-tombstone updaters read to + // decide whether a detach is owed: leaving it set would make the cheap + // `try_update_stable_tombstone_shape_cached` path refuse the receiver + // forever and send every re-add through a `facts_remove` for an entry + // that does not exist. + record.set(RECORD_FLAG_FACTS_INDEXED, false); + let table = &crate::state::state().shapes; + // Publish by-id first, then the family index — an ObjectHeader is stamped + // only after this returns, so a visible id always has a complete record. + // SAFETY: no slab reference is held across the insert. + unsafe { table.slab_mut().insert(id, record) }; + table.inner.borrow_mut().family_append_fresh(keys, id); + id +} + +/// Does this keys allocation have more than one owner? +/// +/// `GC_FLAG_SHAPE_SHARED` is sticky and the caches stamp it when they publish +/// an array, so its ABSENCE is the proof of single ownership that the +/// in-place tombstone lanes already run on. +unsafe fn keys_array_is_shape_shared(keys: *const super::ArrayHeader) -> bool { + let Some(gc) = crate::value::addr_class::try_read_gc_header(keys as usize) else { + // Unreadable header: assume shared, which only costs a retained + // descriptor. + return true; + }; + gc.obj_type != crate::gc::GC_TYPE_ARRAY || gc.gc_flags & crate::gc::GC_FLAG_SHAPE_SHARED != 0 +} + /// Install a process-global id into this agent's local descriptor table. /// Module globals are initialized once per process, while workers own distinct /// runtime state and moving keys pointers. Global id uniqueness makes a local diff --git a/crates/perry-runtime/src/object/tombstone_tests.rs b/crates/perry-runtime/src/object/tombstone_tests.rs index a46f286d4c..82ad63ed2f 100644 --- a/crates/perry-runtime/src/object/tombstone_tests.rs +++ b/crates/perry-runtime/src/object/tombstone_tests.rs @@ -50,10 +50,15 @@ fn tombstone_hole_never_reaches_template_prefixes() { super::delete_rest::js_object_delete_field(obj, victim_ptr), 1 ); - assert_eq!( + // #9064 kept the ShapeId here and made every emitted read compare the + // loaded slot against TAG_HOLE instead. A delete is now a SHAPE + // TRANSITION, so the id moves and the predecessor is retired — see + // `delete_transition_retires_the_shape_a_cache_was_primed_on` for the + // property that buys. + assert_ne!( super::shapes::object_shape_stamp(obj), pre_tombstone_shape, - "owned ordinary tombstone delete must keep the receiver ShapeId stable" + "owned ordinary tombstone delete must transition the receiver ShapeId" ); let obj_gc = crate::value::addr_class::try_read_gc_header(obj as usize) .expect("a freshly allocated object must carry a readable GcHeader"); @@ -155,8 +160,12 @@ fn tombstone_hole_count_survives_readd_append() { /// Object literals are compiler-registered anonymous-shape classes rather /// than `class_id == 0` allocations. They are ordinary receivers, not real /// class instances, and are the exact representation used by #9064's repro. +/// +/// What the marker certifies is the SLOT REPRESENTATION (inline slots may +/// hold `TAG_HOLE`, and a re-add appends into the private array in place), not +/// the shape identity: the delete still transitions the ShapeId. #[test] -fn anonymous_shape_object_literal_uses_stable_tombstone_identity() { +fn anonymous_shape_object_literal_uses_stable_tombstone_slots() { super::delete_rest::test_set_tombstone_deletes(Some(true)); let _restore = scopeguard_tombstone_flag(); let _global = crate::gc::global_side_table_test_lock(); @@ -174,10 +183,11 @@ fn anonymous_shape_object_literal_uses_stable_tombstone_identity() { let before = super::shapes::object_shape_stamp(obj); assert_eq!(super::delete_rest::js_object_delete_field(obj, key), 1); if victim == b"anon_key_03" { - assert_eq!( + assert_ne!( super::shapes::object_shape_stamp(obj), before, - "registered anonymous-shape literal must keep its ShapeId on owned delete" + "registered anonymous-shape literal must transition its ShapeId on \ + an owned delete" ); } } @@ -317,7 +327,11 @@ fn small_churn_first_delete_forks_owned_tombstone() { super::delete_rest::js_object_delete_dynamic(obj, f64::from_bits(sso.bits())), 1 ); - assert_eq!(super::shapes::object_shape_stamp(obj), stable_shape); + assert_ne!( + super::shapes::object_shape_stamp(obj), + stable_shape, + "the dynamic-key delete must transition the ShapeId too" + ); assert_eq!(super::shapes::object_shape_hole_count(obj), 2); for n in 1..=14 { @@ -430,3 +444,617 @@ fn tombstone_publish_on_untraced_receiver_arms_old_carrier() { ); } } + +// ── `delete` as a SHAPE TRANSITION ────────────────────────────────────────── +// +// #9064 kept the receiver's ShapeId across a tombstone delete and made every +// emitted property read compare the loaded slot against `TAG_HOLE` instead — +// four instructions on EVERY read of EVERY object, forever, so that a `(shape, +// key)` cache entry primed for a key could be retired by the slot's contents +// rather than by the shape word. A delete is now a transition: the successor +// is a pure function of `(predecessor ShapeId, deleted key, vacated slot)` and +// is never the predecessor. `PERRY_DELETE_SHAPE_TRANSITION=0` restores #9064. + +/// Restores the per-thread delete-transition override on scope exit. +fn tombstone_receiver_20(prefix: &str) -> *mut crate::object::ObjectHeader { + unsafe { + // Inline slots for every key: the `(shape, key)` slot query these + // tests prime through only answers when `live_inline_slot_count == + // logical_key_count`, so a spilled receiver would make the premise + // assertions vacuous rather than failing them. + let obj = js_object_alloc(0, 24); + for i in 0..20 { + let name = format!("{prefix}{i:02}"); + let key = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + js_object_set_field_by_name(obj, key, i as f64); + } + // The keys array a 20-key builder lands on is transition-cache SHARED, + // so the first delete clones + compacts (ownership transfer) and only + // the SECOND can take the O(1) tombstone lane. Spend the transfer here + // so each test's delete under study is the tombstoning one. + let name = format!("{prefix}19"); + let warm = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + assert_eq!(super::delete_rest::js_object_delete_field(obj, warm), 1); + assert_eq!( + super::shapes::object_shape_hole_count(obj), + 0, + "fixture premise: the ownership-transfer delete compacts, it does not tombstone" + ); + obj + } +} + +/// THE property this lane exists to establish, and the one +/// `perry-codegen`'s read path may lean on when it drops the `TAG_HOLE` +/// compare: after a delete, the shape word the read guard compares against is +/// a DIFFERENT one, and the shape a cache entry was primed under no longer +/// resolves at all. +/// +/// Asserted three ways, because a broken fast path here is silent: the stamp +/// moved, the predecessor descriptor is retired (a surviving token resolves to +/// nothing), and the `(shape, key)` slot query declines the deleted key under +/// the successor. +#[test] +fn delete_transition_retires_the_shape_a_cache_was_primed_on() { + super::delete_rest::test_set_tombstone_deletes(Some(true)); + let _restore = scopeguard_tombstone_flag(); + let _transition = super::delete_rest::test_scope_delete_shape_transition(true); + let _global = crate::gc::global_side_table_test_lock(); + unsafe { + let obj = tombstone_receiver_20("prime_key_"); + let victim = crate::string::js_string_from_bytes(b"prime_key_07".as_ptr(), 12); + let victim_bits = crate::JSValue::string_ptr(victim).bits(); + + // PRIME: the shape a per-site or global `(shape, key)` cache entry + // would record, and the slot it would record for the victim. + let primed_shape = super::shapes::object_shape_stamp(obj); + assert!( + super::shapes::is_shape_id(primed_shape), + "fixture premise: the receiver is stamped" + ); + assert_eq!( + js_object_get_field_by_name(obj, victim).bits(), + 7.0f64.to_bits() + ); + let primed_slot = + super::shapes::js_shape_ordinary_inline_slot_for_key(primed_shape, victim_bits); + assert_eq!( + primed_slot, 7, + "fixture premise: the victim resolves to a slot BEFORE the delete — \ + otherwise the negative below is vacuous" + ); + + assert_eq!(super::delete_rest::js_object_delete_field(obj, victim), 1); + assert_eq!( + super::shapes::object_shape_hole_count(obj), + 1, + "fixture premise: this delete took the O(1) tombstone lane" + ); + + let successor = super::shapes::object_shape_stamp(obj); + assert_ne!( + successor, primed_shape, + "a delete that keeps the ShapeId leaves every cache entry primed \ + for the deleted key matching the receiver — which is exactly why \ + the emitted read has to re-check the slot for TAG_HOLE" + ); + assert!( + super::shapes::shape_descriptor_by_id(primed_shape).is_none(), + "the predecessor must be retired, so a token that outlives the \ + delete resolves to no descriptor at all" + ); + assert_eq!( + super::shapes::js_shape_ordinary_inline_slot_for_key(successor, victim_bits), + -1, + "the successor shape must not resolve the deleted key to a slot" + ); + // ...and the JS-visible answers, which a broken fast path would keep + // getting right while a broken SLOW path would not. + assert!(js_object_get_field_by_name(obj, victim).is_undefined()); + let survivor = crate::string::js_string_from_bytes(b"prime_key_08".as_ptr(), 12); + assert_eq!( + js_object_get_field_by_name(obj, survivor).bits(), + 8.0f64.to_bits(), + "the tombstone must not move a surviving key's slot" + ); + } +} + +/// What remains IN the vacated slot, which is the other half of what codegen +/// needs to know: a stable-tombstone receiver keeps `TAG_HOLE` there (the +/// representation #9029 introduced and every walker already skips). The shape +/// transition is what makes that value unreachable; it does not replace it. +#[test] +fn delete_transition_leaves_tag_hole_in_the_vacated_slot() { + super::delete_rest::test_set_tombstone_deletes(Some(true)); + let _restore = scopeguard_tombstone_flag(); + let _transition = super::delete_rest::test_scope_delete_shape_transition(true); + let _global = crate::gc::global_side_table_test_lock(); + unsafe { + let obj = tombstone_receiver_20("holeslot_"); + let victim = crate::string::js_string_from_bytes(b"holeslot_04".as_ptr(), 11); + assert_eq!(super::delete_rest::js_object_delete_field(obj, victim), 1); + + let keys = crate::object::object_keys_array(obj); + let (slots, slot_len) = crate::object::keys_array_dense_slots(keys); + assert!(slot_len > 4); + assert_eq!( + (*slots.add(4)).to_bits(), + crate::value::TAG_HOLE, + "the KEY slot must carry the tombstone the walkers skip" + ); + let fields = + (obj as *mut u8).add(std::mem::size_of::()) as *const u64; + assert_eq!( + *fields.add(4), + crate::value::TAG_HOLE, + "a stable-tombstone receiver keeps TAG_HOLE in the VALUE slot too" + ); + } +} + +/// Delete / re-add / delete again. The re-add appends (JS enumeration order +/// moves a re-added key to the end), and each delete must land on its own +/// shape: a cycle that returned to an earlier id would let a cache entry from +/// before the first delete hit after the second. +#[test] +fn delete_readd_delete_never_returns_to_an_earlier_shape() { + super::delete_rest::test_set_tombstone_deletes(Some(true)); + let _restore = scopeguard_tombstone_flag(); + let _transition = super::delete_rest::test_scope_delete_shape_transition(true); + let _global = crate::gc::global_side_table_test_lock(); + unsafe { + let obj = tombstone_receiver_20("churn_key_"); + let mut seen = Vec::new(); + seen.push(super::shapes::object_shape_stamp(obj)); + for round in 0..6u32 { + let victim = crate::string::js_string_from_bytes(b"churn_key_02".as_ptr(), 12); + assert_eq!(super::delete_rest::js_object_delete_field(obj, victim), 1); + let after_delete = super::shapes::object_shape_stamp(obj); + assert!( + !seen.contains(&after_delete), + "round {round}: the delete returned to an earlier ShapeId {after_delete:#x}" + ); + seen.push(after_delete); + let readd = crate::string::js_string_from_bytes(b"churn_key_02".as_ptr(), 12); + js_object_set_field_by_name(obj, readd, 100.0 + f64::from(round)); + assert_eq!( + js_object_get_field_by_name(obj, readd).bits(), + (100.0 + f64::from(round)).to_bits(), + "round {round}: the re-add must be readable" + ); + } + } +} + +/// Two receivers deleting the SAME pair of keys in OPPOSITE orders converge on +/// one layout — a tombstone leaves every survivor's slot alone, so both end +/// with holes at the same two positions — and must still each carry their own +/// ShapeId, while agreeing on every JS-visible answer. +/// +/// Convergent layouts are where a delete transition could most plausibly hand +/// two histories one identity; asserting they do not is what keeps a cache +/// entry primed on one receiver from resolving against the other. (Today the +/// keys ARRAY address alone would separate them; this pins the property, not +/// the mechanism that currently provides it.) +#[test] +fn opposite_delete_orders_converge_in_layout_but_not_in_identity() { + super::delete_rest::test_set_tombstone_deletes(Some(true)); + let _restore = scopeguard_tombstone_flag(); + let _transition = super::delete_rest::test_scope_delete_shape_transition(true); + let _global = crate::gc::global_side_table_test_lock(); + unsafe { + let forward = tombstone_receiver_20("order_key_"); + let backward = tombstone_receiver_20("order_key_"); + for (obj, order) in [(forward, [3usize, 9]), (backward, [9usize, 3])] { + for slot in order { + let name = format!("order_key_{slot:02}"); + let key = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + assert_eq!(super::delete_rest::js_object_delete_field(obj, key), 1); + } + assert_eq!(super::shapes::object_shape_hole_count(obj), 2); + } + // Same key SET, same counts, same hole count: only the ORDER of the + // two transitions differed. The intermediate layouts differ, so the + // identities must differ all the way down. + assert_ne!( + super::shapes::object_shape_stamp(forward), + super::shapes::object_shape_stamp(backward), + "two delete orders collapsed onto one ShapeId" + ); + for obj in [forward, backward] { + for slot in [3usize, 9] { + let name = format!("order_key_{slot:02}"); + let key = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + assert!(js_object_get_field_by_name(obj, key).is_undefined()); + } + let survivor = crate::string::js_string_from_bytes(b"order_key_10".as_ptr(), 12); + assert_eq!( + js_object_get_field_by_name(obj, survivor).bits(), + 10.0f64.to_bits() + ); + } + } +} + +/// A class INSTANCE (a real `class_id`, so outside the stable-tombstone lane) +/// and a class PROTOTYPE object both keep transitioning their shape across a +/// delete. Before this lane the instance already minted a fresh id per delete +/// and only the private-literal lane preserved one, so this pins that the two +/// populations now agree. +#[test] +fn delete_transitions_the_shape_for_class_instances_and_prototypes() { + super::delete_rest::test_set_tombstone_deletes(Some(true)); + let _restore = scopeguard_tombstone_flag(); + let _global = crate::gc::global_side_table_test_lock(); + // Both arms, so that a wrong VALUE attributes itself: the surviving-key + // reads below must hold under #9064's publish too, and only the ShapeId + // assertions are the transition's. + for transition in [false, true] { + delete_transitions_for_class_receivers(transition); + } +} + +fn delete_transitions_for_class_receivers(transition: bool) { + let _transition = super::delete_rest::test_scope_delete_shape_transition(transition); + unsafe { + let instance_class_id: u32 = if transition { 0x0004_2101 } else { 0x0004_2102 }; + let prefix = if transition { "instT_" } else { "instF_" }; + let proto_prefix = if transition { "protoT_" } else { "protoF_" }; + const INSTANCE_CLASS_ID: u32 = 0; + let _ = INSTANCE_CLASS_ID; + let instance = js_object_alloc(instance_class_id, 0); + for i in 0..20 { + let name = format!("{prefix}{i:02}"); + let key = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + js_object_set_field_by_name(instance, key, i as f64); + } + for victim in [19u32, 6] { + let name = format!("{prefix}{victim:02}"); + let key = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + let before = super::shapes::object_shape_stamp(instance); + assert_eq!(super::delete_rest::js_object_delete_field(instance, key), 1); + assert_ne!( + super::shapes::object_shape_stamp(instance), + before, + "transition={transition}: a class instance must transition its \ + ShapeId on delete — #9064 never preserved it for a real \ + class id either, so this must hold on both arms" + ); + } + let survivor_name = format!("{prefix}07"); + let survivor = + crate::string::js_string_from_bytes(survivor_name.as_ptr(), survivor_name.len() as u32); + assert_eq!( + js_object_get_field_by_name(instance, survivor).bits(), + 7.0f64.to_bits(), + "transition={transition}: the delete moved a SURVIVING key's value" + ); + + // A prototype object is excluded from the stable-tombstone lane by + // construction (its method caches have different guards), so this is + // the compacting delete — which must transition too. + let proto = js_object_alloc(0, 0); + for i in 0..20 { + let name = format!("{proto_prefix}{i:02}"); + let key = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + js_object_set_field_by_name(proto, key, i as f64); + } + let child = js_object_alloc(0, 0); + crate::object::js_object_set_prototype_of( + crate::value::js_nanbox_pointer(child as i64), + crate::value::js_nanbox_pointer(proto as i64), + ); + // The FIRST delete meets a 20-key transition-cache-SHARED array and + // takes the compacting path, which has always minted a fresh id; only + // the SECOND runs the O(1) tombstone lane this change is about. + for (round, victim) in [19u32, 6].into_iter().enumerate() { + let tombstone_lane = round == 1; + let name = format!("{proto_prefix}{victim:02}"); + let key = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + let before = super::shapes::object_shape_stamp(proto); + assert_eq!(super::delete_rest::js_object_delete_field(proto, key), 1); + let after = super::shapes::object_shape_stamp(proto); + if transition || !tombstone_lane { + assert_ne!( + after, before, + "an object that is someone's prototype must transition its \ + ShapeId on delete (transition={transition}, \ + tombstone_lane={tombstone_lane})" + ); + } else { + // Worth stating, because it is easy to assume otherwise and + // lane 3 will care: the stable-tombstone lane excludes + // `Object.prototype` and REGISTERED class prototypes, not + // every object that happens to sit on a chain. A plain + // `class_id == 0` object reached through `setPrototypeOf` + // enters the lane and #9064 keeps its id across the delete — + // so an inherited-property cache keyed on the holder's shape + // could not see the delete either. + assert_eq!( + after, before, + "#9064 kept the id for a plain object used as a prototype; \ + if that changed, the transition's contrast is no longer \ + what this test claims" + ); + } + } + let inherited_name = format!("{proto_prefix}06"); + let inherited = crate::string::js_string_from_bytes( + inherited_name.as_ptr(), + inherited_name.len() as u32, + ); + assert!( + js_object_get_field_by_name(child, inherited).is_undefined(), + "transition={transition}: the delete must be visible through the \ + prototype chain" + ); + } +} + +/// A receiver carrying a DESCRIPTOR is outside the stable-tombstone lane +/// (`OBJ_FLAG_HAS_DESCRIPTORS` re-opens the full semantic checks). Its delete +/// must still transition, and a non-configurable key must still refuse. +#[test] +fn delete_with_descriptors_transitions_and_still_refuses_non_configurable() { + super::delete_rest::test_set_tombstone_deletes(Some(true)); + let _restore = scopeguard_tombstone_flag(); + let _transition = super::delete_rest::test_scope_delete_shape_transition(true); + let _global = crate::gc::global_side_table_test_lock(); + unsafe { + let obj = js_object_alloc(0, 0); + for i in 0..20 { + let name = format!("desc_key_{i:02}"); + let key = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + js_object_set_field_by_name(obj, key, i as f64); + } + super::descriptor_state::set_property_attrs( + obj as usize, + "desc_key_05".to_string(), + super::descriptor_state::PropertyAttrs::new(true, true, false), + ); + let locked = crate::string::js_string_from_bytes(b"desc_key_05".as_ptr(), 11); + let before = super::shapes::object_shape_stamp(obj); + assert_eq!( + super::delete_rest::js_object_delete_field(obj, locked), + 0, + "a non-configurable key must refuse" + ); + assert_eq!( + super::shapes::object_shape_stamp(obj), + before, + "a REFUSED delete must not move the shape word" + ); + let victim = crate::string::js_string_from_bytes(b"desc_key_11".as_ptr(), 11); + assert_eq!(super::delete_rest::js_object_delete_field(obj, victim), 1); + assert_ne!( + super::shapes::object_shape_stamp(obj), + before, + "a descriptor-carrying receiver must transition its ShapeId on delete" + ); + assert_eq!( + js_object_get_field_by_name(obj, locked).bits(), + 5.0f64.to_bits() + ); + } +} + +/// `PERRY_DELETE_SHAPE_TRANSITION=0` must genuinely restore #9064. Without +/// this the kill switch is decoration, and the A/B arms this lane's +/// measurements are built on would be the same arm twice. +#[test] +fn the_kill_switch_restores_the_9064_stable_shape_id() { + super::delete_rest::test_set_tombstone_deletes(Some(true)); + let _restore = scopeguard_tombstone_flag(); + let _transition = super::delete_rest::test_scope_delete_shape_transition(false); + let _global = crate::gc::global_side_table_test_lock(); + unsafe { + let obj = tombstone_receiver_20("killsw_"); + let before = super::shapes::object_shape_stamp(obj); + let victim = crate::string::js_string_from_bytes(b"killsw_04".as_ptr(), 9); + assert_eq!(super::delete_rest::js_object_delete_field(obj, victim), 1); + assert_eq!( + super::shapes::object_shape_hole_count(obj), + 1, + "fixture premise: still the tombstone lane" + ); + assert_eq!( + super::shapes::object_shape_stamp(obj), + before, + "the kill switch must restore #9064's id-preserving publish" + ); + } +} + +/// ShapeId CONSUMPTION, which no output ever reveals: ids come from a 2^30 +/// counter that is never reused and fail-stops at the end, so a path minting +/// one per delete has a process LIFETIME, not just a memory cost. +/// +/// This is the measurement that decides whether the transition can be +/// default-on for a long-running process. It asserts the numbers rather than +/// describing them: #9064's lane spends ~0 ids across churn (it keeps the id), +/// and the transition spends one per delete — because exact-facts interning is +/// keyed by the keys array's ADDRESS, and each delete-then-re-add reaches a +/// layout that address has never held before. +#[test] +fn delete_shape_id_consumption_per_delete_is_measured() { + super::delete_rest::test_set_tombstone_deletes(Some(true)); + let _restore = scopeguard_tombstone_flag(); + let _global = crate::gc::global_side_table_test_lock(); + const CYCLES: u32 = 200; + + fn churn(prefix: &str, cycles: u32) -> u32 { + let obj = js_object_alloc(0, 0); + for i in 0..20 { + let name = format!("{prefix}{i:02}"); + let key = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + js_object_set_field_by_name(obj, key, i as f64); + } + let warm = format!("{prefix}19"); + let warm_key = crate::string::js_string_from_bytes(warm.as_ptr(), warm.len() as u32); + assert_eq!(super::delete_rest::js_object_delete_field(obj, warm_key), 1); + + let before = super::shapes::test_shape_id_counter(); + for round in 0..cycles { + let name = format!("{prefix}{:02}", round % 10); + let key = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + assert_eq!(super::delete_rest::js_object_delete_field(obj, key), 1); + let readd = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + js_object_set_field_by_name(obj, readd, f64::from(round)); + } + super::shapes::test_shape_id_counter() - before + } + + let preserved = { + let _off = super::delete_rest::test_scope_delete_shape_transition(false); + churn("idcount_off_", CYCLES) + }; + let transitioned = { + let _on = super::delete_rest::test_scope_delete_shape_transition(true); + churn("idcount_on_", CYCLES) + }; + + // Printed as well as asserted: the ratio is what the lane report quotes, + // and `cargo test -- --nocapture` is where it comes from. + eprintln!( + "[delete-shape-id] cycles={CYCLES} ids_preserving={preserved} \ + ids_transition={transitioned} per_delete_preserving={:.3} \ + per_delete_transition={:.3}", + f64::from(preserved) / f64::from(CYCLES), + f64::from(transitioned) / f64::from(CYCLES) + ); + // The transition mints one id per delete, by construction. + assert!( + transitioned >= CYCLES, + "the transition must mint at least one ShapeId per delete \ + ({transitioned} over {CYCLES} cycles) — fewer would mean some delete \ + kept the predecessor id" + ); + // THE RESULT THAT MATTERS, and the one that was guessed wrong before it + // was measured: #9064's id-preserving lane spends essentially the SAME + // number of ids on this churn (199 against 200 over 200 cycles). It keeps + // the id across the delete, but the re-add's append publish and the + // amortized squeeze spend one per cycle anyway. So the transition does not + // move ShapeId consumption — the 2^30 counter's exhaustion horizon is a + // property of delete/re-add churn itself, not of this change. + // + // Asserted as a ratio so it fails if the transition ever starts forking + // identities the preserving lane did not. + assert!( + transitioned <= preserved + CYCLES / 10, + "the transition spent {transitioned} ShapeIds where #9064's lane spent \ + {preserved} over {CYCLES} cycles: it is now the dominant consumer, \ + which it was not when measured" + ); +} + +/// The interning key includes the keys array's ADDRESS, so two receivers that +/// delete the same key from the same predecessor shape do NOT share the +/// successor: each forks a private tombstone array first. This test states +/// that limit rather than leaving it a silent gap — it is the concrete reason +/// `delete` cannot be memoized into a shared transition today, and it FAILS +/// (correctly, asking to be rewritten as `assert_eq!`) the day shape identity +/// becomes address-free. +#[test] +fn two_receivers_do_not_share_a_delete_successor_because_facts_carry_the_address() { + super::delete_rest::test_set_tombstone_deletes(Some(true)); + let _restore = scopeguard_tombstone_flag(); + let _transition = super::delete_rest::test_scope_delete_shape_transition(true); + let _global = crate::gc::global_side_table_test_lock(); + unsafe { + let a = js_object_alloc(0, 0); + let b = js_object_alloc(0, 0); + for obj in [a, b] { + for i in 0..4 { + let name = format!("share_key_{i:02}"); + let key = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + js_object_set_field_by_name(obj, key, i as f64); + } + } + assert_eq!( + super::shapes::object_shape_stamp(a), + super::shapes::object_shape_stamp(b), + "premise: two identically built receivers share ONE shape" + ); + let mut successors = Vec::new(); + for obj in [a, b] { + let key = crate::string::js_string_from_bytes(b"share_key_01".as_ptr(), 12); + assert_eq!(super::delete_rest::js_object_delete_field(obj, key), 1); + successors.push(super::shapes::object_shape_stamp(obj)); + } + let descriptors: Vec<_> = successors + .iter() + .map(|&id| { + super::shapes::shape_descriptor_by_id(id) + .expect("each successor must resolve to a descriptor") + }) + .collect(); + assert_ne!( + descriptors[0].keys, descriptors[1].keys, + "premise: each receiver forked its OWN tombstone array — that fork \ + is what makes the successors distinct" + ); + assert_ne!( + successors[0], successors[1], + "two receivers deleting the same key from the same predecessor do \ + not share a successor today. When shape facts stop carrying the \ + keys array's address, this becomes assert_eq! and `delete` gains a \ + memoized, shared transition." + ); + } +} + +/// The successor's identity is a PURE FUNCTION of the transition, not a draw +/// from the process-wide counter. +/// +/// Nothing consumes that today — the successor descriptor is minted DETACHED +/// from exact-facts interning, because its facts name an owned keys array no +/// second receiver can ever present — so without this assertion the +/// determinism would be unfalsifiable and a future edit could quietly swap in +/// `SHAPE_SEMANTIC_NEXT` with no test noticing. The deterministic namespace is +/// bit 63; the counter starts at 1 and aborts long before it could reach 2^63, +/// so the bit separates them exactly. +#[test] +fn the_delete_successor_generation_is_deterministic_not_a_counter_draw() { + super::delete_rest::test_set_tombstone_deletes(Some(true)); + let _restore = scopeguard_tombstone_flag(); + let _transition = super::delete_rest::test_scope_delete_shape_transition(true); + let _global = crate::gc::global_side_table_test_lock(); + unsafe { + let obj = tombstone_receiver_20("purefn_"); + let victim = crate::string::js_string_from_bytes(b"purefn_06".as_ptr(), 9); + assert_eq!(super::delete_rest::js_object_delete_field(obj, victim), 1); + assert_eq!( + super::shapes::object_shape_hole_count(obj), + 1, + "fixture premise: the O(1) tombstone lane" + ); + let successor = + super::shapes::shape_descriptor_by_id(super::shapes::object_shape_stamp(obj)) + .expect("the delete must publish a resolvable descriptor"); + assert_ne!( + successor.semantic_generation & (1u64 << 63), + 0, + "the delete successor drew a COUNTER generation ({:#x}): the \ + transition is no longer a pure function of (predecessor, key, \ + slot), so two receivers performing the same delete can never \ + agree on a successor", + successor.semantic_generation + ); + + // A DIFFERENT key from the same receiver must land elsewhere: the key + // and the vacated slot are both folded in, so the two successors + // cannot collide. + let obj2 = tombstone_receiver_20("purefn2_"); + let other = crate::string::js_string_from_bytes(b"purefn2_07".as_ptr(), 10); + assert_eq!(super::delete_rest::js_object_delete_field(obj2, other), 1); + let successor2 = + super::shapes::shape_descriptor_by_id(super::shapes::object_shape_stamp(obj2)) + .expect("the delete must publish a resolvable descriptor"); + assert_ne!( + successor.semantic_generation, successor2.semantic_generation, + "two different (predecessor, key, slot) transitions folded to one \ + generation" + ); + } +} diff --git a/crates/perry/tests/scalar_replaced_delete_10822.rs b/crates/perry/tests/scalar_replaced_delete_10822.rs new file mode 100644 index 0000000000..519d76edc2 --- /dev/null +++ b/crates/perry/tests/scalar_replaced_delete_10822.rs @@ -0,0 +1,331 @@ +//! Regression: `delete o.k` on a NON-ESCAPING receiver must actually remove +//! the property — a later read must answer `undefined`, not the deleted value. +//! +//! Issue #10822. `const o = { a: 1, c: 3 }; delete o.c; String(o.c)` printed +//! `"3"`. Not "the property is still there": the read returned the value the +//! property held *before* the delete, `o.c === undefined` was `false`, and +//! nothing in the output said anything was wrong. +//! +//! The mechanism is escape analysis plus scalar replacement, not the delete +//! representation (it reproduced identically with `PERRY_OBJECT_TOMBSTONES=0`) +//! and not the `Ptr` proven path (`--opt-report` says `0 selected / +//! 1 denied`: ptr-shape rule 5 disables the whole module on a `delete`). +//! +//! `collectors/escape_check.rs` stripped `Expr::Delete` in its generic unary +//! arm and handed the inner `PropertyGet` to the "plain declared-field read — +//! safe" arm, which returns early WITHOUT visiting the bare `LocalGet`. The +//! receiver therefore never escaped: `stmt/let_stmt.rs` scalar-replaced it, +//! elided the heap object entirely, and gave each observed field its own +//! alloca. The delete still emitted +//! `js_object_delete_field_value(, "c")` against the +//! never-populated `ctx.locals[id]` alloca, where the deliberate "a primitive +//! receiver no-ops to `true`" guard made it a silent nothing — and the read +//! loaded the `c` alloca, which still held `3`. +//! +//! That also explains the fragility in the report. Every "repair" — +//! `Object.keys(o)`, `JSON.stringify(o)`, `objs.push(o)`, `"c" in o` — passes +//! `o` somewhere as a value, reaching the bare-`LocalGet` arm that escapes the +//! candidate. A `delete` alone never did. +//! +//! This is the REMOVAL sibling of the read rule (#10689) and the write rules +//! (#9024 `PropertySet`/`PutValueSet`, #9460 `PropertyUpdate`). +//! +//! Fixtures are `.js` so `delete` of a plain key and a holey array literal are +//! valid source with no `as any` casts; a cast on the receiver would route the +//! access through the dynamic path and miss the lowering the bug lived in. +//! The two "masking" programs are pinned as tests of their own: they were +//! already correct before the fix, so they are the ones that tell "fixed" +//! apart from "masked". + +use std::path::Path; +use std::path::PathBuf; +use std::process::Command; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +/// Write `entry` into `dir`, compile it with `--no-cache` +/// `PERRY_NO_AUTO_OPTIMIZE=1` (links the prebuilt runtime archive), run it, and +/// return stdout. Mirrors the helper in `object_prototype_value_read_10689.rs`. +fn compile_and_run_js(dir: &Path, entry: &str, source: &str) -> String { + let entry_path = dir.join(entry); + std::fs::write(&entry_path, source).expect("write fixture"); + let output = dir.join(format!("{entry}.bin")); + + let compile = Command::new(perry_bin()) + .current_dir(dir) + .arg("compile") + .arg(&entry_path) + .arg("--no-cache") + .arg("-o") + .arg(&output) + .env("PERRY_NO_AUTO_OPTIMIZE", "1") + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&compile.stdout), + String::from_utf8_lossy(&compile.stderr) + ); + + let run = Command::new(&output) + .current_dir(dir) + .output() + .expect("run compiled binary"); + assert!( + run.status.success(), + "compiled binary failed\nstatus: {:?}\nstdout:\n{}\nstderr:\n{}", + run.status, + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + String::from_utf8_lossy(&run.stdout).into_owned() +} + +fn run(source: &str) -> String { + let dir = tempfile::tempdir().expect("tempdir"); + compile_and_run_js(dir.path(), "main.js", source) +} + +/// The reported program, byte for byte, plus the two other ways the same +/// single read is spelled. Before the fix: `3`, `false`, `number`. +#[test] +fn bare_read_after_delete_is_undefined() { + assert_eq!( + run(r#" +const o = { a: 1, c: 3 }; +delete o.c; +console.log(String(o.c)); +"#), + "undefined\n" + ); + assert_eq!( + run(r#" +const o = { a: 1, c: 3 }; +delete o.c; +console.log(String(o.c === undefined)); +"#), + "true\n" + ); + assert_eq!( + run(r#" +const o = { a: 1, c: 3 }; +delete o.c; +console.log(typeof o.c); +"#), + "undefined\n" + ); +} + +/// The read is not special to `String()`. Arithmetic on the deleted value +/// answered `4` before the fix, so a checksum over such a loop was silently +/// off rather than throwing. +#[test] +fn deleted_field_used_arithmetically_is_nan() { + assert_eq!( + run(r#" +const o = { a: 1, c: 3 }; +delete o.c; +const v = o.c; +console.log(String(v + 1)); +"#), + "NaN\n" + ); +} + +/// Returned from a function and passed as a call argument — the two ways the +/// value leaves the frame. Both answered `3` before the fix. +#[test] +fn deleted_field_returned_and_passed() { + assert_eq!( + run(r#" +function f() { + const o = { a: 1, c: 3 }; + delete o.c; + return o.c; +} +console.log(String(f())); +"#), + "undefined\n" + ); + assert_eq!( + run(r#" +function show(x) { return String(x); } +const o = { a: 1, c: 3 }; +delete o.c; +console.log(show(o.c)); +"#), + "undefined\n" + ); +} + +/// The loop form the bug was found in: the checksum was off by exactly the +/// trip count. Printed `5` before the fix. +#[test] +fn delete_inside_a_loop() { + assert_eq!( + run(r#" +let bad = 0; +for (let i = 0; i < 5; i++) { + const o = { a: i, b: i + 1, c: i + 2, d: i + 3 }; + delete o.c; + const v = o.c; + if (v !== undefined) bad++; +} +console.log(String(bad)); +"#), + "0\n" + ); +} + +/// Not an object-literal-only defect: a `new C()` whose binding never escapes +/// is the same scalar-replacement candidate. Printed `3` before the fix. +#[test] +fn class_instance_delete() { + assert_eq!( + run(r#" +class C { + constructor() { this.a = 1; this.c = 3; } +} +const o = new C(); +delete o.c; +console.log(String(o.c)); +"#), + "undefined\n" + ); +} + +/// Slot position does not matter: the first field of a small literal and a +/// late field of a wide one behaved identically (`1` and `10` before the fix). +#[test] +fn delete_first_field_and_late_field() { + assert_eq!( + run(r#" +const o = { a: 1, c: 3 }; +delete o.a; +console.log(String(o.a)); +"#), + "undefined\n" + ); + assert_eq!( + run(r#" +const o = { a: 1, b: 2, c: 3, d: 4, e: 5, f: 6, g: 7, h: 8, i: 9, j: 10, k: 11, l: 12 }; +delete o.j; +console.log(String(o.j)); +"#), + "undefined\n" + ); +} + +/// A computed key reaches the same receiver through `Expr::IndexGet`, which +/// the same arm has to cover. Printed `3` before the fix. +#[test] +fn computed_key_delete() { + assert_eq!( + run(r#" +const o = { a: 1, c: 3 }; +const k = "c"; +delete o[k]; +console.log(String(o.c)); +"#), + "undefined\n" + ); +} + +/// The sibling this fix found: a non-escaping ARRAY literal is scalar-replaced +/// by the same machinery (`collectors/escape_arrays.rs`), and +/// `delete a[1]` read back `2` before the fix. Not in the original report. +#[test] +fn delete_array_element() { + assert_eq!( + run(r#" +const a = [1, 2, 3]; +delete a[1]; +console.log(String(a[1])); +console.log(String(a.length)); +"#), + "undefined\n3\n" + ); +} + +/// The same defect with the receiver spelled `this`: `delete this.k` inside a +/// constructor. The gate that decides whether a class can be scalar-replaced +/// (`collectors/this_as_value.rs`) answered "safe, scalar replacement +/// intercepts it" for a declared field, exactly as the `LocalGet` receivers +/// did. Printed `3` before the fix. +#[test] +fn delete_this_property_in_constructor() { + assert_eq!( + run(r#" +class C { + constructor() { this.a = 1; this.c = 3; delete this.c; } +} +const o = new C(); +console.log(String(o.c)); +"#), + "undefined\n" + ); +} + +/// The masking programs. These were ALREADY correct before the fix — a run +/// that only checked the broken shapes could not tell a real fix from a +/// change that merely forces every object onto the heap path, and these are +/// what keep the difference visible. +#[test] +fn second_observer_variants_stay_correct() { + // A later `Object.keys` repaired the read by making `o` escape. + assert_eq!( + run(r#" +const o = { a: 1, c: 3 }; +delete o.c; +console.log(String(o.c)); +console.log(Object.keys(o).join(",")); +"#), + "undefined\na\n" + ); + // So did handing the object to something else. + assert_eq!( + run(r#" +const objs = []; +const o = { a: 1, c: 3 }; +objs.push(o); +delete o.c; +console.log(String(o.c)); +"#), + "undefined\n" + ); +} + +/// The observations that were right even while the read was wrong: they do +/// not go through the scalar field load, and each of them makes the receiver +/// escape on its own. They must stay right. +#[test] +fn key_observations_stay_correct() { + assert_eq!( + run(r#" +const o = { a: 1, c: 3 }; +delete o.c; +console.log(String("c" in o)); +console.log(Object.keys(o).join(",")); +console.log(JSON.stringify(o)); +"#), + "false\na\n{\"a\":1}\n" + ); +} + +/// A deleted key that is written again is present again, with the new value. +#[test] +fn delete_then_readd() { + assert_eq!( + run(r#" +const o = { a: 1, c: 3 }; +delete o.c; +o.c = 99; +console.log(String(o.c)); +console.log(Object.keys(o).join(",")); +"#), + "99\na,c\n" + ); +} diff --git a/scripts/gc_runtime_root_holders.json b/scripts/gc_runtime_root_holders.json index 265e1d82a3..c34efb7e04 100644 --- a/scripts/gc_runtime_root_holders.json +++ b/scripts/gc_runtime_root_holders.json @@ -342,6 +342,12 @@ "verdict": "test_only", "why": "Declared under cfg(test); holds an explicitly leaked Rust path string used by the isolated census unit tests." }, + { + "file": "crates/perry-runtime/src/gc/collection_points.rs", + "name": "ARMED_SITE", + "verdict": "test_only", + "why": "Cell>: a static string-literal site NAME an armed rooting-regression test compares against, never a GC heap pointer. Declared under #[cfg(test)] only (collection_points.rs:15); the whole module compiles to an empty inline no-op outside cfg(test), so this storage never exists in a shipped binary." + }, { "file": "crates/perry-runtime/src/gc/copying.rs", "name": "LAST_COHORT_SPLIT", @@ -804,6 +810,12 @@ "verdict": "not_a_gc_pointer", "why": "Source order for Symbol-keyed class members: HashMap<(class_id, SymbolHeader::id, is_static), u32>. The u64 is the symbol's stable id (read once at registration in record_class_symbol_member_order), NOT its address, so the table needs no re-key when a Symbol is evacuated; the value is an order index. The member values themselves live in CLASS_SYMBOL_METHODS/CLASS_SYMBOL_ACCESSORS, which scan_class_symbol_member_keys_mut visits and rewrite_class_symbol_method_key_if_forwarded re-keys." }, + { + "file": "crates/perry-runtime/src/object/delete_rest.rs", + "name": "DELETE_TRANSITION_TEST_OVERRIDE", + "verdict": "test_only", + "why": "Cell> holding a tri-state force/suppress flag for the delete-as-shape-transition path (#10826). The type cannot hold a heap pointer at all -- Option is a two-bit value, not a NaN box and not a *mut -- so there is nothing for a scanner to reach. Set only from #[cfg(test)] helpers; the shipping path reads it and finds None." + }, { "file": "crates/perry-runtime/src/object/descriptor_state.rs", "name": "TEST_SUPPRESS_DESCRIPTOR_YOUNG_NOTE", @@ -1129,6 +1141,12 @@ "verdict": "not_a_gc_pointer", "why": "In-flight job counter for perry/thread." }, + { + "file": "crates/perry-runtime/src/timer/tests_inline.rs", + "name": "SELF_ID", + "verdict": "test_only", + "why": "AtomicI64 holding a scheduled mock timer's id (an i64 returned by schedule_mock_callback_timer, never a GC heap pointer) so the timer's own extern \"C\" callback can look itself up in the ref-state registry mid-dispatch. Declared under #[cfg(test)] only (tests_inline.rs, mock_dispatch_own_pin_tests), never live in a shipped binary." + }, { "file": "crates/perry-runtime/src/weakref/test_support.rs", "name": "DELIVERED", @@ -2541,18 +2559,6 @@ "name": "WINDOW_ROOTS", "verdict": "not_a_gc_pointer", "why": "Window-root registry maps numeric window handles to numeric root-widget handles; neither value is a JavaScript heap pointer." - }, - { - "file": "crates/perry-runtime/src/gc/collection_points.rs", - "name": "ARMED_SITE", - "verdict": "test_only", - "why": "Cell>: a static string-literal site NAME an armed rooting-regression test compares against, never a GC heap pointer. Declared under #[cfg(test)] only (collection_points.rs:15); the whole module compiles to an empty inline no-op outside cfg(test), so this storage never exists in a shipped binary." - }, - { - "file": "crates/perry-runtime/src/timer/tests_inline.rs", - "name": "SELF_ID", - "verdict": "test_only", - "why": "AtomicI64 holding a scheduled mock timer's id (an i64 returned by schedule_mock_callback_timer, never a GC heap pointer) so the timer's own extern \"C\" callback can look itself up in the ref-state registry mid-dispatch. Declared under #[cfg(test)] only (tests_inline.rs, mock_dispatch_own_pin_tests), never live in a shipped binary." } ], "_FRONTIER_README": "Identity-pinned debt ratchet over new perry-ui* candidates and otherwise-unclassified core raw/Perry TLS declarations (see the census docstring, \u201cThe identity-pinned frontier\u201d). A new uncovered holder fails until it is scanned, receives a researched holders verdict, or is deliberately pinned as debt. Moving a researched false positive to holders graduates it from this list. A fixed or classified holder makes its old frontier pin stale, so the receipt must be deleted.", diff --git a/scripts/shape_descriptor_census.py b/scripts/shape_descriptor_census.py index 1361cf1497..7360501204 100644 --- a/scripts/shape_descriptor_census.py +++ b/scripts/shape_descriptor_census.py @@ -207,6 +207,19 @@ def function_body(source: str, name: str) -> str: raise CensusError(f"missing closing brace: {name}") +def require_match(body: str, pattern: str, what: str) -> str: + """Like require_code, but returns the first capture group. + + Used where the census must reason about a VALUE (a constant's magnitude), + not merely assert that a line exists. Asserting a value relationship + survives an encoding change; asserting an instruction does not. + """ + m = re.search(pattern, body) + if m is None: + raise CensusError(f"missing: {what}") + return m.group(1) + + def require_code(source: str, pattern: str, label: str) -> None: if not re.search(pattern, source, re.MULTILINE | re.DOTALL): raise CensusError(f"shape descriptor authority surface missing: {label}") @@ -721,19 +734,49 @@ def assert_authority_surfaces(sources: dict[str, str]) -> None: r"add\s*\(\s*I64\s*,\s*&obj_handle\s*,\s*\"4\"\s*\)", "generic read PIC reads the authoritative ShapeId at header offset 4", ) - require_code( - generic_body, - r"icmp_ne\s*\(\s*I64\s*,\s*&packed_word\s*,\s*\"0\"\s*\)", - "generic read PIC empty compact-cache rejection", - ) + # #10833 changed the ENCODING, not the invariant. The unprimed compact word + # was `0`, so `packed != 0` was the emptiness test; it is now + # PACKED_GET_EMPTY. The invariant that must still hold is that an UNPRIMED + # word cannot be mistaken for a hit — and it now holds structurally rather + # than by an extra test: PACKED_GET_EMPTY lies outside + # [SHAPE_ID_BASE, SHAPE_ID_END), so the token compare below can never match + # it. Assert that range relationship, which is strictly stronger than + # asserting one instruction survives. + empty = int( + require_match( + raw_generic_pic, + r"const\s+PACKED_GET_EMPTY\s*:\s*i64\s*=\s*(0x[0-9A-Fa-f_]+)\s*;", + "codegen declares the compact-cache empty sentinel", + ).replace("_", ""), + 16, + ) + base = int( + require_match( + shapes, + r"const\s+SHAPE_ID_BASE\s*:\s*u32\s*=\s*(0x[0-9A-Fa-f_]+)\s*;", + "shapes declares SHAPE_ID_BASE", + ).replace("_", ""), + 16, + ) + end = int( + require_match( + shapes, + r"const\s+SHAPE_ID_END\s*:\s*u32\s*=\s*(0x[0-9A-Fa-f_]+)\s*;", + "shapes declares SHAPE_ID_END", + ).replace("_", ""), + 16, + ) + if base <= empty < end: + raise CensusError( + f"compact-cache empty sentinel {empty:#x} is INSIDE the valid ShapeId " + f"range [{base:#x}, {end:#x}) — an unprimed site could be read as a hit" + ) # Invalid ShapeIds now fail closed at publication and exact cache matching. # Keep both halves of that proof: the emitted guard consumes a nonempty # packed word's exact stamp, and neither cache writer admits a zero stamp. compact_guard = re.sub(r"\s+", "", generic_body) for fragment in ( - 'letpacked_present=ctx.block().icmp_ne(I64,&packed_word,"0");', - 'letis_plain_object=ctx.block().and(I1,&is_plain_kind,&packed_present);', - 'cond_br(&is_plain_object,&tok_label,&cold_label)', + 'cond_br(&is_plain_kind,&tok_label,&cold_label)', 'letpacked_stamp=ctx.block().trunc(I64,&packed_word,I32);', 'lettoken_eq=ctx.block().icmp_eq(I32,&pcid,&packed_stamp);', 'cond_br(&token_eq,&hit_label,&token_miss_label)', @@ -1063,7 +1106,11 @@ def run_sabotage_selftests(sources: dict[str, str], baseline: dict[str, object]) ) # #8665: the generic read PIC's invalid-id proof must not go missing. - # Its nonzero check now applies to the packed cache word. Plant a + # #10833 replaced that test with an ENCODING property: the unprimed + # sentinel sits outside the valid ShapeId range, so the token compare + # rejects it. Sabotage the property, not the instruction -- move the + # sentinel INTO the range, the exact mistake that would let an unprimed + # site read as a hit. # regression that changes the rejected sentinel, and # prove the census still catches it -- this is what stands between the # check above and a vacuous pass, per #6942/#6946/#7024's precedent that @@ -1071,8 +1118,8 @@ def run_sabotage_selftests(sources: dict[str, str], baseline: dict[str, object]) dropped_fail_closed = dict(sources) path = "crates/perry-codegen/src/expr/property_get/generic_dispatch.rs" sabotaged_body, substitutions = re.subn( - r'icmp_ne\(I64, &packed_word, "0"\)', - 'icmp_ne(I64, &packed_word, "-1")', + r"const PACKED_GET_EMPTY: i64 = 0xFFFF_FFFF;", + "const PACKED_GET_EMPTY: i64 = 0x9000_0000;", dropped_fail_closed[path], count=1, ) diff --git a/test-files/test_parity_delete_shape_transition.ts b/test-files/test_parity_delete_shape_transition.ts new file mode 100644 index 0000000000..5c212a7e25 --- /dev/null +++ b/test-files/test_parity_delete_shape_transition.ts @@ -0,0 +1,124 @@ +// Parity: everything `delete` is allowed to be observed doing — enumeration +// order after a re-add, delete order, wide receivers, descriptors, accessors, +// class instances and prototypes, index keys, and interleaved read/churn. +// +// `delete` is a shape transition: every delete moves the receiver's ShapeId, +// so a (shape, key) cache entry primed before it cannot hit after it. That is +// invisible in output when it works and silent when it breaks, so this file +// pins the JS-visible half against Node and the unit tests in +// `object/tombstone_tests.rs` pin the identity half. +function show(label: string, v: unknown): void { + console.log(label + "=" + v); +} + +// 1. enumeration order: a re-added key goes to the END. +const a: Record = { x: 1, y: 2, z: 3 }; +delete a.y; +show("a1", Object.keys(a).join(",")); +a.y = 9; +show("a2", Object.keys(a).join(",")); +show("a3", JSON.stringify(a)); +let forin = []; +for (const k in a) forin.push(k); +show("a4", forin.join(",")); +show("a5", Object.entries(a).map(function (e) { return e[0] + ":" + e[1]; }).join("|")); +const spreadA = Object.assign({}, a); +show("a6", Object.keys(spreadA).join(",") + "/" + JSON.stringify(spreadA)); + +// 2. delete order independence of the RESULT, dependence of the ORDER. +function build(): Record { + return { k0: 0, k1: 1, k2: 2, k3: 3, k4: 4 }; +} +const fwd = build(); delete fwd.k1; delete fwd.k3; +const bwd = build(); delete bwd.k3; delete bwd.k1; +show("b1", Object.keys(fwd).join(",")); +show("b2", Object.keys(bwd).join(",")); +fwd.k1 = 11; fwd.k3 = 33; bwd.k3 = 33; bwd.k1 = 11; +show("b3", Object.keys(fwd).join(",")); +show("b4", Object.keys(bwd).join(",")); +show("b5", JSON.stringify(fwd) + "|" + JSON.stringify(bwd)); + +// 3. a wide receiver: delete every other key, then re-add, twice over. +const wide: Record = {}; +for (let i = 0; i < 40; i++) wide["w" + i] = i; +for (let i = 0; i < 40; i += 2) delete wide["w" + i]; +show("c1", Object.keys(wide).length + "/" + Object.keys(wide).join(",")); +for (let i = 0; i < 40; i += 2) wide["w" + i] = i * 10; +show("c2", Object.keys(wide).length + "/" + Object.keys(wide).slice(0, 8).join(",")); +let csum = 0; +for (const k in wide) csum += wide[k]; +show("c3", csum); +for (let i = 0; i < 40; i++) delete wide["w" + i]; +show("c4", Object.keys(wide).length + "/" + JSON.stringify(wide)); + +// 4. descriptors. +const d: Record = { p: 1, q: 2, r: 3 }; +Object.defineProperty(d, "locked", { value: 7, configurable: false, enumerable: true, writable: true }); +Object.defineProperty(d, "hidden", { value: 8, configurable: true, enumerable: false, writable: true }); +show("d1", Object.keys(d).join(",")); +show("d2", delete d.q); +try { show("d3", delete d.locked); } catch (e) { show("d3", "throw"); } +show("d4", delete d.hidden); +show("d5", Object.keys(d).join(",") + "/" + d.locked + "/" + d.hidden); +show("d6", Object.getOwnPropertyNames(d).join(",")); +show("d7", JSON.stringify(Object.getOwnPropertyDescriptor(d, "locked"))); + +// 5. accessors. +const acc: Record = { base: 1 }; +Object.defineProperty(acc, "g", { get: function () { return 42; }, configurable: true, enumerable: true }); +show("e1", acc.g + "/" + Object.keys(acc).join(",")); +show("e2", delete acc.g); +show("e3", acc.g + "/" + Object.keys(acc).join(",")); +show("e4", "g" in acc); + +// 6. class instances and prototypes. +class C { constructor() { this.f1 = 1; this.f2 = 2; this.f3 = 3; } m() { return "m"; } } +const ci = new C(); +show("f1", delete ci.f2); +show("f2", Object.keys(ci).join(",") + "/" + ci.f2 + "/" + ci.f3); +ci.f2 = 22; +show("f3", Object.keys(ci).join(",") + "/" + ci.f2); +show("f4", ci.m()); +show("f5", delete C.prototype.m); +// SCOPED OUT: `typeof C.prototype.m` still reports "function" on perry after a +// successful `delete` even though the key is gone from getOwnPropertyNames. +// Reproduces on an unmodified base binary (v0.5.1618) and is unrelated to the +// delete-shape-transition change; asserting the wrong value here would bake it in. +// show("f6", typeof C.prototype.m); // node: undefined, perry: function +show("f6", Object.getOwnPropertyNames(C.prototype).join(",")); + +const proto: Record = { inherited: "yes", shared: 1 }; +const child = Object.create(proto); +child.own = "mine"; +show("g1", child.inherited + "/" + child.own + "/" + Object.keys(child).join(",")); +show("g2", delete proto.inherited); +show("g3", child.inherited + "/" + ("inherited" in child)); +show("g4", delete child.own); +show("g5", child.own + "/" + Object.keys(child).length); + +// 7. delete of an absent key, of an index key, of a non-object. +const h: Record = { only: 1 }; +show("h1", delete h.nope); +show("h2", delete h["0"]); +h[0] = "zero"; h[1] = "one"; +show("h3", Object.keys(h).join(",")); +show("h4", delete h[0]); +show("h5", Object.keys(h).join(",") + "/" + JSON.stringify(h)); + +// 8. churn with reads interleaved (the shape must never hand back a stale slot). +const churn: Record = {}; +for (let i = 0; i < 12; i++) churn["c" + i] = i; +let bad = 0; +for (let round = 0; round < 60; round++) { + const k = "c" + (round % 12); + delete churn[k]; + if (churn[k] !== undefined) bad++; + if (Object.prototype.hasOwnProperty.call(churn, k)) bad++; + churn[k] = round; + if (churn[k] !== round) bad++; + const other = "c" + ((round + 5) % 12); + if (typeof churn[other] !== "number") bad++; +} +show("i1", bad); +show("i2", Object.keys(churn).length); +show("i3", Object.keys(churn).sort().join(","));