diff --git a/changelog.d/7141-pshape-routing.md b/changelog.d/7141-pshape-routing.md new file mode 100644 index 0000000000..8597b2a6e9 --- /dev/null +++ b/changelog.d/7141-pshape-routing.md @@ -0,0 +1,59 @@ +### Fixed + +- **repsel Phase 5a: proven-`this` clones had zero call sites and were dead-stripped (#7128).** + `collectors/proven_this.rs` emitted a `{method}__pshape` clone — a full second + body of the method whose `this` carries the `Ptr` proof, so every + `this.field` inside it is a bare fixed-offset `load double` instead of a + guarded diamond — but nothing in the corpus ever *called* one. Every clone was + emitted, reachable from nothing, and dropped by the linker. + + The two checks in place could not see it. An object-hash A/B scores the phase + as working, because `suite_09_method_calls`' object genuinely does differ with + the analysis off — by two dead clone bodies. And the promotion census scores + it as working, because the clone's `this` is a real `Ptr` consumption, + recorded at every `this.field` site *inside the body nobody calls*. That is + the reconciliation of the two contradictory reports: #7117's "7 receiver + consumptions corpus-wide" and #7128's "zero call sites" are both true and + describe the same dead code. + + Root cause is arm ordering at both routing sites. `emit_guarded_direct_method_call` + (`lower_call/method_override.rs`) tried five typed-clone arms and consulted + `pshape_methods` only in the final `else`; the Phase 3b guard-free site + (`lower_call/property_get/dynamic_dispatch.rs`) had the same shape, routing to + the clone on its plain exit but calling the guard-ridden public body from the + typed-receiver arm's own generic fallback 25 lines above. A method can only + admit a proven-`this` clone if it touches a declared field of its own chain, + which is very nearly the definition of a typed-receiver-clone candidate — so + the typed arm won essentially whenever both were eligible. + + Both sites now resolve the clone once, up front, and use it for the generic + fallback as well as the plain exit. The typed clone is still preferred on the + fast path; what changed is that falling off it no longer discards a receiver + proof the enclosing block had already established. Same `(double this, args…)` + ABI, same shadow-bound tagged-at-rest receiver slot, no new proof obligation: + every rerouted block is dominated either by the class-id + keys-token guard or + by Phase 3b containment, which is exactly what the existing `else` arm already + relied on. + + New `collectors/proven_this_routing_tests.rs` ratchets **call sites**, not + symbol presence — it matches the callee position of a `call` so a + `ptrtoint ptr @…__pshape` operand can never be miscounted — and asserts the + clone still stores-then-`js_shadow_slot_bind`s its receiver, since + `GC_TYPE_OBJECT` moves in the shipped configuration (#7019). + + Two findings are deliberately left unfixed and filed instead: routing the + class-id-switch dispatch tower (`idispatch.caseN`) would be **unsound** — + `delete inst.field` compacts packed slots while preserving `class_id`, which + is precisely what the keys token catches — and whether the typed arm should + yield to the clone on its *fast* path is a cost-model question for the + `collectors/repsel_benefit.rs` gate added in #7132. + + Measured on a Raspberry Pi 5 (`perry-dev`, one `CARGO_TARGET_DIR` per arm, + identical package sets): `__pshape` call sites go 0 → 2 on + `fixture_ptr_shape.ts`, 0 → 2 on `fixture_ptr_shape_sites.ts` and 0 → 1 on + `09_method_calls.ts`. The body a routed call enters drops from 304 IR lines + with 4 `js_typed_feedback_class_field_*_guard` and 4 + `js_object_get_field_by_name*` calls to 104 lines with none of either + (`Point::norm2`, 19 → 7 opaque `js_*` calls). `cargo test -p perry-codegen + --lib` 409/409, `census --gate` OK with every floor held, and the three + `Ptr`/proven-`this` gap tests byte-identical against `main`. diff --git a/crates/perry-codegen/src/collectors/mod.rs b/crates/perry-codegen/src/collectors/mod.rs index 6ca59f5339..79183e2f4e 100644 --- a/crates/perry-codegen/src/collectors/mod.rs +++ b/crates/perry-codegen/src/collectors/mod.rs @@ -25,6 +25,8 @@ mod mutation; mod not_bigint_locals; mod pointer_locals; mod proven_this; +#[cfg(test)] +mod proven_this_routing_tests; mod ptr_numarray; mod ptr_shape; mod ptr_shape_report; diff --git a/crates/perry-codegen/src/collectors/proven_this.rs b/crates/perry-codegen/src/collectors/proven_this.rs index f817a52f88..fdcd3b3dc7 100644 --- a/crates/perry-codegen/src/collectors/proven_this.rs +++ b/crates/perry-codegen/src/collectors/proven_this.rs @@ -337,8 +337,9 @@ mod tests { // Naming + emission + the two proven call sites. `string_pool.rs` // (which emits `js_register_class_method`) is deliberately ABSENT: // the vtable must only ever hold the public symbol. - let allowed: [&str; 6] = [ + let allowed: [&str; 7] = [ "collectors/proven_this.rs", // this test + "collectors/proven_this_routing_tests.rs", // routing IR ratchet "codegen/typed_abi.rs", // name helper "codegen/method.rs", // clone emission "codegen/artifacts.rs", // emission driver diff --git a/crates/perry-codegen/src/collectors/proven_this_routing_tests.rs b/crates/perry-codegen/src/collectors/proven_this_routing_tests.rs new file mode 100644 index 0000000000..e633a79e19 --- /dev/null +++ b/crates/perry-codegen/src/collectors/proven_this_routing_tests.rs @@ -0,0 +1,565 @@ +//! Phase 5a proven-`this` **call-site** ratchets (#7128). +//! +//! `collectors/proven_this.rs` decides *whether* a `{public}__pshape` clone may +//! be emitted; `codegen/artifacts.rs` emits it. Neither of those is worth +//! anything unless a call site actually *targets* the clone — and for the whole +//! corpus measured in #7128, none did: every clone was emitted, reachable from +//! nothing, and dead-stripped by the linker. +//! +//! The failure was invisible to the two checks that were in place. An object +//! hash A/B scores the phase as working (`suite_09_method_calls`' object DOES +//! differ with the analysis off — by two dead clone bodies), and a promotion +//! counter scores it as working (the clone's `this` is a genuine `Ptr` +//! consumption, recorded at every `this.field` site *inside the dead body*). +//! Only reading the emitted IR for a `call` whose callee is the clone +//! distinguishes "routed" from "emitted and abandoned". +//! +//! So these tests assert on **call sites**, never on symbol presence alone: +//! every one of them requires `define`+`call` together, and +//! [`pshape_call_targets`] deliberately matches the callee position so a +//! `ptrtoint ptr @…__pshape` (the shape the typed-feedback guard passes a +//! function pointer in) can never be miscounted as a call. +//! +//! Both routing sites are covered: +//! +//! * `lower_call/method_override.rs` — the guarded `method_direct.fast` arm, +//! dominated by the class-id + keys-token guard. +//! * `lower_call/property_get/dynamic_dispatch.rs` — the Phase 3b guard-free +//! `Ptr` receiver arm. +//! +//! and in both, the case that regressed is the *typed-clone fallback*: when a +//! typed clone exists for the method, the typed arm ran first and its own +//! generic fallback called the guard-ridden public body, discarding a receiver +//! proof the enclosing block had already established. + +use crate::{compile_module, AppMetadata, CompileOptions}; +use perry_hir::types::Type; +use perry_hir::{BinaryOp, Class, ClassField, Expr, Function, Module, ModuleInitKind, Param, Stmt}; + +fn ir_opts(is_entry: bool) -> CompileOptions { + CompileOptions { + target: None, + is_entry_module: is_entry, + non_entry_module_prefixes: Vec::new(), + nextjs_path_init_modules: Vec::new(), + import_function_prefixes: std::collections::HashMap::new(), + import_function_ffi_aliases: std::collections::HashMap::new(), + import_function_origin_names: std::collections::HashMap::new(), + import_function_v8_specifiers: std::collections::HashMap::new(), + import_function_node_submodule: std::collections::HashMap::new(), + namespace_node_submodules: std::collections::HashMap::new(), + namespace_v8_specifiers: std::collections::HashMap::new(), + namespace_member_prefixes: std::collections::HashMap::new(), + namespace_member_origin_names: std::collections::HashMap::new(), + emit_ir_only: true, + verify_native_regions: false, + disable_buffer_fast_path: false, + namespace_imports: Vec::new(), + imported_classes: Vec::new(), + imported_enums: Vec::new(), + imported_async_funcs: std::collections::HashSet::new(), + type_aliases: std::collections::HashMap::new(), + imported_func_param_counts: std::collections::HashMap::new(), + imported_func_has_rest: std::collections::HashSet::new(), + imported_func_synthetic_arguments: std::collections::HashSet::new(), + imported_func_return_types: std::collections::HashMap::new(), + imported_vars: std::collections::HashSet::new(), + output_type: "executable".to_string(), + needs_stdlib: false, + needs_ui: false, + needs_geisterhand: false, + geisterhand_port: 7676, + enabled_features: Vec::new(), + native_module_init_names: Vec::new(), + js_module_specifiers: Vec::new(), + bundled_extensions: Vec::new(), + native_library_functions: Vec::new(), + i18n_table: None, + fast_math: false, + fp_contract_mode: crate::FpContractMode::Off, + app_metadata: AppMetadata::default(), + namespace_entries: Vec::new(), + dynamic_import_path_to_prefix: std::collections::HashMap::new(), + deferred_module_prefixes: std::collections::HashSet::new(), + module_init_deps: Vec::new(), + is_dynamic_import_target: false, + debug_locations: false, + module_source: None, + debug_source_line_offset: 0, + } +} + +fn field(name: &str, ty: Type) -> ClassField { + ClassField { + name: name.to_string(), + key_expr: None, + ty, + init: None, + is_private: false, + is_readonly: false, + decorators: Vec::new(), + } +} + +fn param(id: u32, name: &str, ty: Type) -> Param { + Param { + id, + name: name.to_string(), + ty, + default: None, + decorators: Vec::new(), + is_rest: false, + arguments_object: None, + } +} + +fn func(id: u32, name: &str, params: Vec, return_type: Type, body: Vec) -> Function { + Function { + id, + name: name.to_string(), + type_params: Vec::new(), + params, + return_type, + body, + is_async: false, + is_generator: false, + is_strict: false, + is_exported: false, + captures: Vec::new(), + decorators: Vec::new(), + was_plain_async: false, + was_unrolled: false, + } +} + +fn class(id: u32, name: &str, fields: Vec, methods: Vec) -> Class { + Class { + id, + name: name.to_string(), + type_params: Vec::new(), + extends: None, + extends_name: None, + native_extends: None, + extends_expr: None, + heritage_lexically_shadowed: false, + fields, + constructor: None, + methods, + getters: Vec::new(), + setters: Vec::new(), + static_accessor_names: Vec::new(), + static_accessor_fn_ids: Vec::new(), + computed_members: Vec::new(), + static_fields: Vec::new(), + static_methods: Vec::new(), + decorators: Vec::new(), + is_exported: false, + aliases: Vec::new(), + is_nested: false, + alloc_width_hint: 0, + } +} + +fn this_get(f: &str) -> Expr { + Expr::PropertyGet { + object: Box::new(Expr::This), + property: f.to_string(), + byte_offset: 0, + } +} + +fn call(recv: Expr, method: &str, args: Vec) -> Expr { + Expr::Call { + callee: Box::new(Expr::PropertyGet { + object: Box::new(recv), + property: method.to_string(), + byte_offset: 0, + }), + args, + type_args: Vec::new(), + byte_offset: 0, + } +} + +/// Every LLVM callee named `…__pshape`. +/// +/// Matches the **callee position** of a `call` instruction specifically. A +/// `__pshape` symbol also appears in `define` lines and could appear in a +/// `ptrtoint ptr @… to i64` operand (how the typed-feedback guard receives a +/// function pointer); counting either as a call site is exactly the vacuous +/// pass this file exists to prevent. +fn pshape_call_targets(ir: &str) -> Vec { + let mut out = Vec::new(); + for line in ir.lines() { + let Some(call_at) = line.find("call ") else { + continue; + }; + let tail = &line[call_at..]; + // `call @name(` — the callee is the `@…` immediately before + // the argument list. + let Some(at) = tail.find('@') else { continue }; + let Some(paren) = tail[at..].find('(') else { + continue; + }; + let name = &tail[at + 1..at + paren]; + if name.ends_with("__pshape") { + out.push(name.to_string()); + } + } + out +} + +fn pshape_definitions(ir: &str) -> Vec { + ir.lines() + .filter(|l| l.starts_with("define") && l.contains("__pshape")) + .filter_map(|l| { + let at = l.find('@')?; + let paren = l[at..].find('(')?; + Some(l[at + 1..at + paren].to_string()) + }) + .collect() +} + +fn emit(m: &Module, is_entry: bool) -> String { + String::from_utf8(compile_module(m, ir_opts(is_entry)).unwrap()) + .expect("LLVM IR should be UTF-8") +} + +/// `bump(): void { this.value = this.value + 1 }` — a `void` return, so NO +/// typed clone of any tier is eligible (every tier requires a `number`/`i32`/ +/// `boolean`/`string` return). This is the arm that already worked. +fn bump_method() -> Function { + func( + 90, + "bump", + Vec::new(), + Type::Void, + vec![Stmt::Expr(Expr::PropertySet { + object: Box::new(Expr::This), + property: "value".to_string(), + value: Box::new(Expr::Binary { + op: BinaryOp::Add, + left: Box::new(this_get("value")), + right: Box::new(Expr::Number(1.0)), + }), + })], + ) +} + +/// `scale(f: number): number { return this.value * f }` — a `number` return +/// and an f64 parameter, so the typed-receiver / typed-f64 clones ARE eligible +/// and their arm runs before the proven-`this` arm. Its generic fallback is +/// what regressed. +fn scale_method() -> Function { + func( + 91, + "scale", + vec![param(70, "f", Type::Number)], + Type::Number, + vec![Stmt::Return(Some(Expr::Binary { + op: BinaryOp::Mul, + left: Box::new(this_get("value")), + right: Box::new(Expr::LocalGet(70)), + }))], + ) +} + +fn counter_class() -> Class { + class( + 101, + "Counter", + vec![field("value", Type::Number)], + vec![bump_method(), scale_method()], + ) +} + +/// `class Point { x; y; constructor(x, y); norm2(): number }` — the shape of +/// `benchmarks/repsel_census/fixtures/fixture_ptr_shape.ts`, whose whole purpose +/// is to satisfy every rule in `collectors/ptr_shape.rs` so a local can actually +/// be shape-proven. `norm2` returns `number` and reads only declared fields, so +/// the typed-receiver clone is eligible and its arm runs first — the same +/// shadowing that hid the guarded site, on the other routing site. +fn point_class() -> Class { + let ctor = func( + 95, + "constructor", + vec![param(80, "x", Type::Number), param(81, "y", Type::Number)], + Type::Void, + vec![ + Stmt::Expr(Expr::PropertySet { + object: Box::new(Expr::This), + property: "x".to_string(), + value: Box::new(Expr::LocalGet(80)), + }), + Stmt::Expr(Expr::PropertySet { + object: Box::new(Expr::This), + property: "y".to_string(), + value: Box::new(Expr::LocalGet(81)), + }), + ], + ); + let norm2 = func( + 96, + "norm2", + Vec::new(), + Type::Number, + vec![Stmt::Return(Some(Expr::Binary { + op: BinaryOp::Add, + left: Box::new(Expr::Binary { + op: BinaryOp::Mul, + left: Box::new(this_get("x")), + right: Box::new(this_get("x")), + }), + right: Box::new(Expr::Binary { + op: BinaryOp::Mul, + left: Box::new(this_get("y")), + right: Box::new(this_get("y")), + }), + }))], + ); + let mut c = class( + 102, + "Point", + vec![field("x", Type::Number), field("y", Type::Number)], + vec![norm2], + ); + c.constructor = Some(ctor); + c +} + +/// A module whose `probe()` holds a Phase 3b **shape-proven local**: a single +/// `Let` initialised by `new` (provenance), only ever field-accessed and +/// method-called, never reassigned, captured, passed, returned or aliased +/// (containment). That combination is what routes through the guard-FREE site +/// in `lower_call/property_get/dynamic_dispatch.rs`, which is a different code +/// path from `guarded_site_module`'s typed parameter. +fn ptr_shape_local_module() -> Module { + let mut m = Module::new("pshape_local.ts"); + m.classes = vec![point_class()]; + m.functions = vec![func( + 1, + "probe", + Vec::new(), + Type::Number, + vec![ + Stmt::Let { + id: 3, + name: "total".to_string(), + ty: Type::Number, + mutable: true, + init: Some(Expr::Number(0.0)), + }, + Stmt::Let { + id: 2, + name: "p".to_string(), + ty: Type::Named("Point".to_string()), + mutable: false, + init: Some(Expr::New { + class_name: "Point".to_string(), + args: vec![Expr::Number(3.0), Expr::Number(4.0)], + type_args: Vec::new(), + byte_offset: 0, + cap_args_appended: 0, + }), + }, + // `p.x = p.x + 1` — a declared-field read and write, which keeps the + // local inside the proof. + Stmt::Expr(Expr::PropertySet { + object: Box::new(Expr::LocalGet(2)), + property: "x".to_string(), + value: Box::new(Expr::Binary { + op: BinaryOp::Add, + left: Box::new(Expr::PropertyGet { + object: Box::new(Expr::LocalGet(2)), + property: "x".to_string(), + byte_offset: 0, + }), + right: Box::new(Expr::Number(1.0)), + }), + }), + // The method result is accumulated into a separate local, exactly as + // `fixture_ptr_shape.ts` does. Returning `p.norm2()` directly puts a + // `LocalGet(p)` inside the `Return` expression, which the + // containment walk treats as an escape — the receiver of a call is + // not distinguished there — and the local silently stops being + // shape-proven. + Stmt::Expr(Expr::LocalSet( + 3, + Box::new(Expr::Binary { + op: BinaryOp::Add, + left: Box::new(Expr::LocalGet(3)), + right: Box::new(call(Expr::LocalGet(2), "norm2", Vec::new())), + }), + )), + Stmt::Return(Some(Expr::LocalGet(3))), + ], + )]; + m.init_kind = ModuleInitKind::Eager; + m +} + +/// A module whose `probe(c: Counter)` calls both methods on a statically-typed +/// parameter. A typed parameter is not a Phase 3b shape-proven local (no +/// provenance, no containment), so both calls go through the *guarded* site: +/// `emit_guarded_direct_method_call`, behind the class-id + keys-token guard. +fn guarded_site_module() -> Module { + let mut m = Module::new("pshape_guarded.ts"); + m.classes = vec![counter_class()]; + m.functions = vec![func( + 1, + "probe", + vec![param(2, "c", Type::Named("Counter".to_string()))], + Type::Void, + vec![ + Stmt::Expr(call(Expr::LocalGet(2), "bump", Vec::new())), + Stmt::Expr(call(Expr::LocalGet(2), "scale", vec![Expr::Number(1.5)])), + ], + )]; + m.init_kind = ModuleInitKind::Eager; + m +} + +/// Regression: a method with NO eligible typed clone routes to its clone. +/// +/// This half was already true before #7128 and is kept as the control — if it +/// ever goes red, the routing site itself (not the arm ordering) has broken. +#[test] +fn untyped_method_routes_to_proven_this_clone() { + let ir = emit(&guarded_site_module(), false); + let defs = pshape_definitions(&ir); + let calls = pshape_call_targets(&ir); + let bump = defs + .iter() + .find(|d| d.contains("__bump__pshape")) + .unwrap_or_else(|| panic!("no proven-`this` clone emitted for `bump`: {defs:?}")); + assert!( + calls.iter().any(|c| c == bump), + "`bump`'s proven-`this` clone is emitted but nothing calls it — it will \ + be dead-stripped. defined={defs:?} called={calls:?}" + ); +} + +/// Regression (#7128): a method that ALSO has a typed clone must still route +/// its generic fallback to the proven-`this` clone. +/// +/// Before the fix, `emit_guarded_direct_method_call` tried the five typed arms +/// first and only the final `else` consulted `pshape_methods`. Every method +/// that admits a proven-`this` clone must touch a declared field of its own +/// chain — which is very nearly the definition of a typed-receiver-clone +/// candidate — so the typed arm won essentially whenever both were eligible, +/// and the clone was emitted with no caller at all. +/// +/// The typed arm's fast path is deliberately left alone (that is a +/// cost-model question, tracked separately): what must be true is that the +/// fallback it branches to no longer throws the receiver proof away. +#[test] +fn typed_clone_fallback_routes_to_proven_this_clone() { + let ir = emit(&guarded_site_module(), false); + let defs = pshape_definitions(&ir); + let calls = pshape_call_targets(&ir); + let scale = defs + .iter() + .find(|d| d.contains("__scale__pshape")) + .unwrap_or_else(|| panic!("no proven-`this` clone emitted for `scale`: defined={defs:?}")); + assert!( + calls.iter().any(|c| c == scale), + "`scale` has a typed clone, so the typed arm runs first; its generic \ + fallback must still route to the proven-`this` clone instead of the \ + guard-ridden public body. defined={defs:?} called={calls:?}" + ); + // The typed arm itself must survive — this fix reroutes the FALLBACK, it + // does not displace the typed clone. + assert!( + ir.contains("__typed_f64_recv") || ir.contains("__typed_f64"), + "the typed clone must still be emitted and preferred on the fast \ + path:\n{ir}" + ); +} + +/// Regression (#7128), Phase 3b guard-free site: a shape-proven LOCAL whose +/// method also has a typed-receiver clone must still route to the proven-`this` +/// clone. +/// +/// `lower_call/property_get/dynamic_dispatch.rs` had the defect in its purest +/// form — the block routed to the clone on its plain exit, but the +/// typed-receiver arm 25 lines above called the guard-ridden public body from +/// its own generic fallback. Both exits are guard-free under the identical +/// Phase 3b proof, so a proven receiver whose ARGUMENTS happened to be +/// non-plain-double silently lost the receiver proof as well. +#[test] +fn ptr_shape_local_typed_fallback_routes_to_proven_this_clone() { + let ir = emit(&ptr_shape_local_module(), false); + let defs = pshape_definitions(&ir); + let calls = pshape_call_targets(&ir); + let norm2 = defs + .iter() + .find(|d| d.contains("__norm2__pshape")) + .unwrap_or_else(|| { + panic!( + "no proven-`this` clone emitted for `norm2` — the Phase 3b \ + fixture no longer satisfies the shape proof, so this test \ + would pass vacuously. defined={defs:?}" + ) + }); + assert!( + calls.iter().any(|c| c == norm2), + "a shape-proven local's method call must route to the proven-`this` \ + clone from the guard-free site too. defined={defs:?} called={calls:?}" + ); +} + +/// The routed call must still hand the callee a shadow-bound receiver slot. +/// +/// #6925 kept the clone's `(double this, …)` ABI and its shadow-bound, +/// tagged-at-rest receiver slot precisely because `GC_TYPE_OBJECT` is MOVABLE +/// in the shipped configuration (#7019) — the `TaPtr` no-bind shortcut does not +/// transfer. Routing more call sites to the clone is only safe while that +/// remains true, so assert it at the callee rather than trusting the comment. +#[test] +fn proven_this_clone_binds_its_receiver_slot() { + let mut ir = emit(&guarded_site_module(), false); + ir.push('\n'); + ir.push_str(&emit(&ptr_shape_local_module(), false)); + let names = pshape_definitions(&ir); + assert!( + !names.is_empty(), + "nothing to check — no proven-`this` clone was emitted at all" + ); + for name in names { + // Anchor on the DEFINITION line, not the first mention: a routed call + // site names the same symbol and appears earlier in the module, so + // `find("@{name}(")` alone would slice the caller's body and then + // "pass" by finding some other function's receiver bind. + let def_line = ir + .lines() + .position(|l| l.starts_with("define") && l.contains(&format!("@{name}("))) + .unwrap_or_else(|| panic!("clone {name} has no definition line")); + let body: String = ir + .lines() + .skip(def_line) + .take_while(|l| *l != "}") + .collect::>() + .join("\n"); + let body = body.as_str(); + let store = body + .find("store double %this_arg") + .unwrap_or_else(|| panic!("{name}: receiver is never stored to a slot:\n{body}")); + let bind = body + .find("call void @js_shadow_slot_bind") + .unwrap_or_else(|| panic!("{name}: receiver slot is never shadow-bound:\n{body}")); + assert!( + store < bind, + "{name}: the receiver must be stored to its slot BEFORE the slot is \ + bound, with no safepoint between:\n{body}" + ); + } +} + +// NOTE on the `PERRY_PTR_SHAPE_LOCALS=0` direction: it is deliberately NOT a +// test in this file. `ptr_shape_locals_enabled` memoises in a `OnceLock`, so a +// test that sets the variable in-process observes whatever the first reader in +// the binary cached — it would pass by skipping itself far more often than it +// checked anything, which is precisely the vacuous gate this file exists to +// avoid. The knob direction is exercised out-of-process instead, by the census +// (`benchmarks/repsel_census/README.md`), which re-runs the whole corpus under +// the knob on every CI job and asserts `ptr-shape-consumed` drops to zero. diff --git a/crates/perry-codegen/src/linker.rs b/crates/perry-codegen/src/linker.rs index 4dad530820..38172041b4 100644 --- a/crates/perry-codegen/src/linker.rs +++ b/crates/perry-codegen/src/linker.rs @@ -96,9 +96,8 @@ fn write_ll_atomically(ll_path: &Path, ll_text: &str, counter: u64) -> Result<() return Ok(()); } } - fs::write(ll_path, ll_text.as_bytes()).with_context(|| { - format!("Failed to write temp .ll file at {}", ll_path.display()) - }) + fs::write(ll_path, ll_text.as_bytes()) + .with_context(|| format!("Failed to write temp .ll file at {}", ll_path.display())) } } } diff --git a/crates/perry-codegen/src/lower_call/method_override.rs b/crates/perry-codegen/src/lower_call/method_override.rs index 25dba1a303..39f93ba0d8 100644 --- a/crates/perry-codegen/src/lower_call/method_override.rs +++ b/crates/perry-codegen/src/lower_call/method_override.rs @@ -180,6 +180,35 @@ pub(super) fn emit_guarded_direct_method_call( let expected_class_id = *ctx.class_ids.get(receiver_class_name)?; let keys_global_name = ctx.class_keys_globals.get(receiver_class_name)?.clone(); + // Representation-selection Phase 5a: the proven-`this` clone for this + // (class, method), when the emission loop produced one. + // + // Computed ONCE here rather than per-arm because the justification is the + // same for every block this helper emits below: they are all dominated by + // the `js_method_direct_shape_guard` / + // `js_typed_feedback_method_direct_call_guard` branch, which matched the + // exact class id AND the keys token. A `pshape_methods` hit additionally + // proves `receiver_class_name` DECLARES `property` (the map holds own + // declarations of module-local classes only), so the clone's `this` is + // exactly the class it was compiled for and can never be a subclass + // instance. + // + // The `perry_static_` exclusion is carried forward from the guard-free + // site (the #1787 static-receiver bug): those targets need + // `js_class_static_method_call`, not a plain `call double`, and no + // proven-`this` clone is ever emitted for them. + let pshape_fn: Option = (!direct_fn.starts_with("perry_static_") + && ctx + .pshape_methods + .contains_key(&(receiver_class_name.to_string(), property.to_string()))) + .then(|| crate::collectors::pshape_method_name(direct_fn)); + + // The body a failed typed guard falls back to. Arm-invariant (both inputs + // are), so it is resolved once here rather than five times below. + let generic_body_fn: String = pshape_fn + .clone() + .unwrap_or_else(|| crate::codegen::generic_method_body_name(direct_fn)); + let expected_class_id_str = expected_class_id.to_string(); let expected_keys_slot = ctx.func.entry_init_load_global(&keys_global_name, I64); let expected_keys = ctx.block().load(I64, &expected_keys_slot); @@ -246,7 +275,6 @@ pub(super) fn emit_guarded_direct_method_call( ctx.current_block = fast_idx; let fast_value = { if let Some((typed_fn, typed_formal_count, receiver_info)) = typed_f64_receiver_direct_fn { - let generic_body_fn = crate::codegen::generic_method_body_name(direct_fn); let formal_args: Vec<&str> = direct_arg_slices .iter() .skip(1) @@ -374,7 +402,6 @@ pub(super) fn emit_guarded_direct_method_call( ); result } else if let Some((typed_fn, typed_param_reps)) = typed_direct_fn { - let generic_body_fn = crate::codegen::generic_method_body_name(direct_fn); let formal_args: Vec<&str> = direct_arg_slices .iter() .skip(1) @@ -459,7 +486,6 @@ pub(super) fn emit_guarded_direct_method_call( ); result } else if let Some((typed_fn, typed_param_reps)) = typed_i32_direct_fn { - let generic_body_fn = crate::codegen::generic_method_body_name(direct_fn); let formal_args: Vec<&str> = direct_arg_slices .iter() .skip(1) @@ -546,7 +572,6 @@ pub(super) fn emit_guarded_direct_method_call( ); result } else if let Some((typed_fn, typed_param_reps)) = typed_i1_direct_fn { - let generic_body_fn = crate::codegen::generic_method_body_name(direct_fn); let formal_args: Vec<&str> = direct_arg_slices .iter() .skip(1) @@ -646,7 +671,6 @@ pub(super) fn emit_guarded_direct_method_call( ); result } else if let Some((typed_fn, typed_param_reps)) = typed_string_direct_fn { - let generic_body_fn = crate::codegen::generic_method_body_name(direct_fn); let formal_args: Vec<&str> = direct_arg_slices .iter() .skip(1) @@ -758,18 +782,12 @@ pub(super) fn emit_guarded_direct_method_call( // needs no such guard because it never claims `JsNumber` — its // bare loads carry generic `JsValue` semantics (see // `collectors/proven_this.rs`). - // The `perry_static_` exclusion is carried forward from the - // guard-free site (the #1787 static-receiver bug): those targets - // need `js_class_static_method_call`, not a plain `call double`, - // and no proven-`this` clone is ever emitted for them. Belt and - // braces — a static's registry key is distinct from an instance - // method's, so the two maps cannot currently disagree. - let pshape_target = (!direct_fn.starts_with("perry_static_") - && ctx - .pshape_methods - .contains_key(&(receiver_class_name.to_string(), property.to_string()))) - .then(|| crate::collectors::pshape_method_name(direct_fn)); - let target = pshape_target.as_deref().unwrap_or(direct_fn); + // + // `pshape_fn` (computed once at the top of this function, where the + // `perry_static_` exclusion and the declaring-class argument are + // written out) is the same clone the typed arms above now route + // their generic fallbacks to. + let target = pshape_fn.as_deref().unwrap_or(direct_fn); ctx.block().call(DOUBLE, target, direct_arg_slices) } }; diff --git a/crates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs b/crates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs index 9865a3b7c7..bb01846a85 100644 --- a/crates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs +++ b/crates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs @@ -870,6 +870,25 @@ pub(crate) fn try_lower_instance_method_call( .unwrap_or(false); if ptr_shape_receiver && !fallback_fn.starts_with("perry_static_") { ctx.note_ptr_shape_consumed(object, "ptr_shape_method"); + // Representation-selection Phase 5a: the proven-`this` + // clone, when one was emitted. Hoisted above the + // typed-receiver branch because BOTH exits of this block + // are guard-free under the same Phase 3b proof — the + // typed-receiver arm's own generic fallback used to call + // the guard-ridden public body while the plain exit 25 + // lines below routed to the clone, so a proven receiver + // whose ARGUMENTS happened to be non-plain-double silently + // lost the receiver proof too. + // + // A `pshape_methods` hit additionally proves `class_name` + // DECLARES `property` — so the clone's `this` is exactly + // the class it was compiled for and cannot be a subclass + // instance. + let pshape_target = ctx + .pshape_methods + .contains_key(&(class_name.clone(), property.to_string())) + .then(|| crate::collectors::pshape_method_name(&fallback_fn)); + let generic_target = pshape_target.as_deref().unwrap_or(fallback_fn.as_str()); // Prefer the typed-receiver clone (bare gep+load field // access inside the body) when one exists: the receiver // is proven, so only the ARGUMENT value classes need @@ -920,7 +939,7 @@ pub(crate) fn try_lower_instance_method_call( let typed_end = ctx.block().label.clone(); ctx.block().br(&merge_label); ctx.current_block = generic_idx; - let v_generic = ctx.block().call(DOUBLE, &fallback_fn, &arg_slices); + let v_generic = ctx.block().call(DOUBLE, generic_target, &arg_slices); let generic_end = ctx.block().label.clone(); ctx.block().br(&merge_label); ctx.current_block = merge_idx; @@ -934,19 +953,12 @@ pub(crate) fn try_lower_instance_method_call( return Ok(Some(merged)); } // Representation-selection Phase 5a: route to the - // proven-`this` clone when one exists. The receiver is - // already proven here (no guard is emitted on this path at - // all), and a `pshape_methods` hit additionally proves - // `class_name` DECLARES `property` — so the clone's `this` - // is exactly the class it was compiled for and cannot be a - // subclass instance. Same ABI, so the call is unchanged - // apart from the callee name. - let pshape_target = ctx - .pshape_methods - .contains_key(&(class_name.clone(), property.to_string())) - .then(|| crate::collectors::pshape_method_name(&fallback_fn)); - let target = pshape_target.as_deref().unwrap_or(fallback_fn.as_str()); - let direct = ctx.block().call(DOUBLE, target, &arg_slices); + // proven-`this` clone when one exists (`generic_target`, + // resolved at the top of this block). The receiver is + // already proven here — no guard is emitted on this path at + // all. Same ABI, so the call is unchanged apart from the + // callee name. + let direct = ctx.block().call(DOUBLE, generic_target, &arg_slices); return Ok(Some(direct)); } if let Some(guarded) = emit_guarded_direct_method_call(