diff --git a/changelog.d/7362-statepoint-report-dead-counters.md b/changelog.d/7362-statepoint-report-dead-counters.md new file mode 100644 index 0000000000..02a26e459f --- /dev/null +++ b/changelog.d/7362-statepoint-report-dead-counters.md @@ -0,0 +1,43 @@ +### Fixed + +**`--statepoint-report`: four counters that no code could ever write, and a test asserting one of them was zero.** + +`FunctionRecord` declared `plain_stack_maps`, `stack_map_operands`, +`statepoint_fallbacks` and `fallbacks_by_callee`. All four were summed into the +totals and rendered into both the text and JSON reports. **None had a writer.** +The mutator API is `note_call` / `note_skipped` / `note_statepoint`, and none of +them touches those fields; `git log -S note_fallback` finds nothing, so they +were never populated — not orphaned when the plain-map bridge was deleted, but +dead from the start. + +The report therefore printed `0 statepoint parser fallback(s)` as reassurance +that no root had been recorded in an unrecoverable location, a unit test +asserted that zero, and the comment above the assert said the structural zero +"is the point". A counter that cannot be non-zero is not evidence — it is +CLAUDE.md's fourth failure mode with the subject removed outright. The real +fail-closed guarantee is in `gc_map.rs`, which returns `Err` on an unparseable +or uncompactable map, so a fallback fails the *build*. + +Replaced by `every_rendered_counter_has_a_writer`, which drives a record through +every mutator and asserts no scalar in the rendered totals is zero. It caught a +live field (`calls_without_live_roots`) on its first run, because the fixture +only made calls that had live roots — so the invariant has teeth. + +Also cleaned up in the same sweep, all verified dead rather than assumed: + +- `PERRY_STATEPOINTS` is **never read anywhere**. The empty-report diagnostic + told users to set it (`Enable PERRY_STATEPOINTS=1`), which is a dead end: + following the instruction produces the same empty report forever. It now names + `PERRY_RS4GC=1` and the cache as the two real causes. Removed from the + object-cache key list too — that test hashes the environment, so it passed for + a name nothing reads and could never have flagged the drift. +- `declare void @llvm.experimental.stackmap` was emitted into every module and + never called; removing it collapses two adjacent identical + `native_stack_roots_enabled()` blocks into one. +- `compact_and_assemble` recomputed a `ptr64` predicate it never read, which + read like a width guard that had been defeated. The emitter's own `ptr64` is + the live one. +- `stack_maps.rs`'s module doc described a "research backend", two competing + "prototypes" and a macOS-only implementation. It is the only backend, the + plain-map lowering is deleted, and it supports Apple/Linux/Windows across + aarch64 and x86-64. diff --git a/crates/perry-codegen/src/codegen/helpers.rs b/crates/perry-codegen/src/codegen/helpers.rs index a4b93f0c69..480c9cc048 100644 --- a/crates/perry-codegen/src/codegen/helpers.rs +++ b/crates/perry-codegen/src/codegen/helpers.rs @@ -88,7 +88,8 @@ pub(super) fn shadow_stack_enabled() -> bool { /// and it is chosen inside `LlFunction` (`enable_shadow_frame_inner` and /// `reserve_shadow_slot` both return the native path first). /// -/// Conflating them made `PERRY_SHADOW_STACK=0 + PERRY_STATEPOINTS=1` produce a +/// Conflating them made `PERRY_SHADOW_STACK=0` plus native-root lowering (then +/// spelled `PERRY_STATEPOINTS=1`, since deleted; `PERRY_RS4GC=1` today) produce a /// binary with **no precise frame roots at all** — the analysis was switched /// off, so the statepoint lowering had nothing to lower. No `__perry_gcmap` /// section, same size as a plain shadow-off build, correct output. Nothing diff --git a/crates/perry-codegen/src/gc_map.rs b/crates/perry-codegen/src/gc_map.rs index 8fb1959b04..83dc73ab1d 100644 --- a/crates/perry-codegen/src/gc_map.rs +++ b/crates/perry-codegen/src/gc_map.rs @@ -897,10 +897,12 @@ pub fn compact_and_assemble( let arch_supported = target.starts_with("aarch64") || target.starts_with("arm64") || target.starts_with("x86_64"); - // watchOS is ILP32. The map's function-address field follows the target's - // pointer width rather than assuming 8 bytes, so `arm64_32` is a supported - // width here, not an excluded target. - let ptr64 = !target.starts_with("arm64_32"); + // No pointer-width refusal here on purpose. watchOS `arm64_32` is ILP32, + // and the emitter handles that by following the target's width for the + // function-address field (see `ptr64` in `compact_stack_map_asm`) rather + // than assuming 8 bytes — so a narrow pointer is a supported width, not an + // excluded target. This spot used to recompute that predicate and never + // read it, which read like a guard that had been defeated. if !arch_supported { return Err(anyhow!( "perry: native GC roots (PERRY_RS4GC) are not supported for target \ diff --git a/crates/perry-codegen/src/linker.rs b/crates/perry-codegen/src/linker.rs index 072a864d22..070d36c01e 100644 --- a/crates/perry-codegen/src/linker.rs +++ b/crates/perry-codegen/src/linker.rs @@ -511,19 +511,23 @@ fn build_clang_compile_plan( /// Matched on the ` within ` instruction syntax rather than the bare opcode /// names so a user string literal containing "catchpad" cannot trip it. pub(crate) fn rs4gc_funclet_refusal(ll_text: &str) -> Option { - ["catchswitch within ", "catchpad within ", "cleanuppad within "] - .iter() - .any(|needle| ll_text.contains(needle)) - .then(|| { - "PERRY_RS4GC: this module contains a `try`/`catch` that lowered to \ + [ + "catchswitch within ", + "catchpad within ", + "cleanuppad within ", + ] + .iter() + .any(|needle| ll_text.contains(needle)) + .then(|| { + "PERRY_RS4GC: this module contains a `try`/`catch` that lowered to \ WinEH funclet pads (catchswitch/catchpad — the windows-msvc EH \ shape), and LLVM's rewrite-statepoints-for-gc pass does not \ support funclet EH: it crashes with an access violation rather \ than reporting anything. Refusing before the pass runs. \ Compile without PERRY_RS4GC, or keep `try` out of RS4GC-compiled \ modules on Windows. Tracked in #7354." - .to_string() - }) + .to_string() + }) } fn maybe_rs4gc_preprocess(ll_text: &str) -> Result> { @@ -584,10 +588,7 @@ fn maybe_rs4gc_preprocess(ll_text: &str) -> Result> { // left only a symbol-less stack dump. Write the exact input next to // the other failure artifacts and name it, so the crash is // reproducible with one command. - let ir_path = env::temp_dir().join(format!( - "perry_rs4gc_failed_{}.ll", - std::process::id() - )); + let ir_path = env::temp_dir().join(format!("perry_rs4gc_failed_{}.ll", std::process::id())); let ir_note = match fs::write(&ir_path, ll_text) { Ok(()) => format!("input IR left at: {}", ir_path.display()), Err(error) => format!("(could not write input IR: {error})"), diff --git a/crates/perry-codegen/src/module.rs b/crates/perry-codegen/src/module.rs index faba2ba28f..346d2b1bb9 100644 --- a/crates/perry-codegen/src/module.rs +++ b/crates/perry-codegen/src/module.rs @@ -645,9 +645,6 @@ impl LlModule { ir.push_str(decl); ir.push('\n'); } - if crate::codegen::helpers::native_stack_roots_enabled() { - ir.push_str("declare void @llvm.experimental.stackmap(i64, i32, ...)\n"); - } if crate::codegen::helpers::native_stack_roots_enabled() { push_statepoint_declarations(&mut ir); } @@ -917,9 +914,6 @@ impl LlModule { pre.push_str(decl); pre.push('\n'); } - if crate::codegen::helpers::native_stack_roots_enabled() { - pre.push_str("declare void @llvm.experimental.stackmap(i64, i32, ...)\n"); - } if crate::codegen::helpers::native_stack_roots_enabled() { push_statepoint_declarations(&mut pre); } diff --git a/crates/perry-codegen/src/statepoint_report.rs b/crates/perry-codegen/src/statepoint_report.rs index d6e74e70ff..f5fd1c83d1 100644 --- a/crates/perry-codegen/src/statepoint_report.rs +++ b/crates/perry-codegen/src/statepoint_report.rs @@ -27,14 +27,10 @@ pub struct FunctionRecord { skipped_non_safepoints: u64, statepoints: u64, relocations: u64, - plain_stack_maps: u64, - stack_map_operands: u64, - statepoint_fallbacks: u64, max_live_roots: usize, live_roots_histogram: BTreeMap, statepoints_by_callee: BTreeMap, skipped_by_callee: BTreeMap, - fallbacks_by_callee: BTreeMap, } impl FunctionRecord { @@ -137,14 +133,10 @@ struct Totals { skipped_non_safepoints: u64, statepoints: u64, relocations: u64, - plain_stack_maps: u64, - stack_map_operands: u64, - statepoint_fallbacks: u64, max_live_roots: usize, live_roots_histogram: BTreeMap, statepoints_by_callee: BTreeMap, skipped_by_callee: BTreeMap, - fallbacks_by_callee: BTreeMap, } fn totals(records: &[FunctionRecord]) -> Totals { @@ -161,9 +153,6 @@ fn totals(records: &[FunctionRecord]) -> Totals { out.skipped_non_safepoints += record.skipped_non_safepoints; out.statepoints += record.statepoints; out.relocations += record.relocations; - out.plain_stack_maps += record.plain_stack_maps; - out.stack_map_operands += record.stack_map_operands; - out.statepoint_fallbacks += record.statepoint_fallbacks; out.max_live_roots = out.max_live_roots.max(record.max_live_roots); for (width, count) in &record.live_roots_histogram { *out.live_roots_histogram.entry(*width).or_default() += count; @@ -174,9 +163,6 @@ fn totals(records: &[FunctionRecord]) -> Totals { for (callee, count) in &record.skipped_by_callee { *out.skipped_by_callee.entry(callee.clone()).or_default() += count; } - for (callee, count) in &record.fallbacks_by_callee { - *out.fallbacks_by_callee.entry(callee.clone()).or_default() += count; - } } out } @@ -204,13 +190,13 @@ pub fn render_text(records: &[FunctionRecord]) -> String { ); if records.is_empty() { out.push_str( - "No native-stack lowering records were emitted. Enable PERRY_STATEPOINTS=1\n\ - or PERRY_RS4GC=1 and ensure codegen is not served from cache.\n", + "No native-stack lowering records were emitted. Set PERRY_RS4GC=1 and\n\ + ensure codegen is not served from cache (PERRY_NO_AUTO_OPTIMIZE=1, or\n\ + clear the object cache) — a cached .o emits no records.\n", ); return out; } - let emitted = totals.statepoints + totals.plain_stack_maps; let _ = writeln!( out, "{} function(s), {} bound native root slots ({} logical slots reserved)", @@ -223,18 +209,13 @@ pub fn render_text(records: &[FunctionRecord]) -> String { ); let _ = writeln!( out, - "{} safepoints emitted: {} statepoints, {} plain stack maps", - emitted, totals.statepoints, totals.plain_stack_maps + "{} statepoints emitted; {} non-collecting calls skipped", + totals.statepoints, totals.skipped_non_safepoints ); let _ = writeln!( out, - "{} non-collecting calls skipped; {} statepoint parser fallback(s)", - totals.skipped_non_safepoints, totals.statepoint_fallbacks - ); - let _ = writeln!( - out, - "{} relocations, {} plain-map operands; maximum {} live roots at one safepoint\n", - totals.relocations, totals.stack_map_operands, totals.max_live_roots + "{} relocations; maximum {} live roots at one safepoint\n", + totals.relocations, totals.max_live_roots ); if !totals.live_roots_histogram.is_empty() { @@ -254,11 +235,6 @@ pub fn render_text(records: &[FunctionRecord]) -> String { "Calls omitted by the GC-effect audit", &totals.skipped_by_callee, ); - render_ranked_map( - &mut out, - "Plain-map fallbacks in statepoint mode", - &totals.fallbacks_by_callee, - ); out } @@ -294,16 +270,58 @@ mod tests { let text = render_text(std::slice::from_ref(&record)); assert!(text.contains("2 bound native root slots")); assert!(text.contains("1 non-collecting calls skipped")); - // The plain-map fallback is gone, so this can only ever report zero — - // which is the point: it is the report's evidence that no root was - // recorded in an unrecoverable location. - assert!(text.contains("0 statepoint parser fallback(s)")); assert!(text.contains("@js_gc_temp_root_get")); let json = render_json(&[record]); let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); assert_eq!(parsed["schema_version"], 1); assert_eq!(parsed["totals"]["relocations"], 2); - assert_eq!(parsed["totals"]["statepoint_fallbacks"], 0); + } + + /// Every counter this report prints must have a writer. + /// + /// It did not. `plain_stack_maps`, `stack_map_operands`, + /// `statepoint_fallbacks` and `fallbacks_by_callee` were declared, summed + /// and rendered, and **no mutator ever wrote them** — `git log -S + /// note_fallback` finds nothing, so they were never populated, not even + /// before the plain-map bridge was deleted. The report printed + /// "0 statepoint parser fallback(s)" as reassurance, a test asserted that + /// zero, and the comment above that assert said the structural zero "is + /// the point". A counter that cannot be non-zero is not evidence; it is + /// CLAUDE.md's fourth failure mode with the subject removed entirely. + /// + /// The real fail-closed guarantee is in `gc_map.rs`, which returns `Err` + /// on an unparseable or uncompactable map, so a fallback fails the BUILD + /// rather than incrementing a number nobody reads. + /// + /// This test pins the invariant that let the dead fields hide: a totals + /// field that is always zero for a record with real activity is either + /// unwritten or misrendered. + #[test] + fn every_rendered_counter_has_a_writer() { + let mut record = FunctionRecord::new("f", "rs4gc", 2, 2); + // Both call shapes: `calls_without_live_roots` only moves for a call + // with an empty live set, so a fixture of all-live calls would accuse + // a perfectly live field of having no writer. + record.note_call(1); + record.note_call(0); + record.note_statepoint("@js_alloc", 1); + record.note_skipped("@js_gc_temp_root_get"); + + let json = render_json(&[record]); + let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); + let totals = parsed["totals"].as_object().expect("totals is an object"); + + for (name, value) in totals { + // Maps and histograms carry their own emptiness; scalars are the + // ones that silently read as "checked, and fine". + let Some(n) = value.as_u64() else { continue }; + assert_ne!( + n, 0, + "totals.{name} is zero for a record with a call, a statepoint \ + and a skip — it has no writer, or nothing reaches it. Give it \ + one or delete the field; do not print a number that cannot move." + ); + } } } diff --git a/crates/perry-runtime/src/gc/roots/stack_maps.rs b/crates/perry-runtime/src/gc/roots/stack_maps.rs index b11269f3d1..11d7c97bdb 100644 --- a/crates/perry-runtime/src/gc/roots/stack_maps.rs +++ b/crates/perry-runtime/src/gc/roots/stack_maps.rs @@ -1,17 +1,25 @@ -//! Research precise-root backend for LLVM stack maps. +//! Precise GC roots read from native frames, via LLVM statepoints. //! -//! The plain-map prototype places `llvm.experimental.stackmap` immediately -//! before mapped calls and records the address of each native root alloca. -//! The statepoint prototype instead records LLVM-owned spill slots for -//! `gc.relocate` values. Both are writable frame-register-relative locations -//! in the emitted stack-map section. +//! Under `PERRY_RS4GC=1` the compiler runs `RewriteStatepointsForGC`, which +//! records each live root as an LLVM-owned spill slot for a `gc.relocate` +//! value: a writable, frame-register-relative location in the emitted +//! stack-map section. This module finds that section in the running image, +//! walks the native frames, and hands each live slot to the collector as a +//! `MutableRootSlot` — mutable because evacuation rewrites through it. //! -//! This first implementation deliberately targets macOS, where the experiment -//! is being measured. It discovers the concatenated `__PERRY_GCMAP` section -//! in the main Mach-O image and uses the platform unwinder to recover the -//! frame-register value for each active generated frame. Unsupported targets -//! return no roots; neither native-stack experiment may be used for correctness -//! there. +//! A second lowering used to exist, placing `llvm.experimental.stackmap` +//! before mapped calls and recording alloca addresses directly. It was +//! unsound and is deleted; only the statepoint path remains, so there is no +//! backend selection here and no fallback between them. +//! +//! Platform support is per-shape, not one target: Apple (macOS/iOS/iPadOS/ +//! tvOS/watchOS) reads a concatenated `__PERRY_GCMAP` Mach-O section, Linux +//! reads `.perry_gcmap` from ELF, and Windows reads `.pgcmap` from the PE +//! image. Frame walking is per-platform too — an x29 chain walk where the +//! map proves every frame is chain-walkable, the Itanium unwinder elsewhere +//! on Unix, and `RtlVirtualUnwind` on Windows. Targets outside that set +//! return no roots, and the compiler refuses to emit a map for them rather +//! than producing a binary whose collector would silently free live objects. use super::{MutableRootSlot, MutableRootSlotKind}; use crate::gc::telemetry::RootSourcesTraceStats; diff --git a/crates/perry/src/commands/compile/object_cache.rs b/crates/perry/src/commands/compile/object_cache.rs index 9428ccaa67..d3def842b2 100644 --- a/crates/perry/src/commands/compile/object_cache.rs +++ b/crates/perry/src/commands/compile/object_cache.rs @@ -759,10 +759,11 @@ fn compute_object_cache_key_with_env( // calls at heap-store sites (codegen.rs / expr.rs). // - PERRY_SHADOW_STACK=0/off/false suppresses generated frame/slot // roots at function entry and pointer local stores. - // - (historical) PERRY_STACK_MAPS lowered precise roots to plain LLVM stackmap records; deleted, statepoint fallback keeps the lowering internal. PERRY_STATEPOINTS=1 lowers roots to native-frame - // stack maps instead of the runtime shadow stack. - // - PERRY_STATEPOINTS=1 replaces supported calls with LLVM statepoint - // relocation sequences and uses native stack maps for the remainder. + // - (historical) PERRY_STACK_MAPS lowered precise roots to plain LLVM + // stackmap records, and PERRY_STATEPOINTS selected native-frame roots. + // Both are deleted; PERRY_RS4GC is the only spelling now, and it is + // listed above. Left here because a cache key that silently stops + // covering a knob is indistinguishable from one that never did. // - PERRY_DISABLE_BUFFER_FAST_PATH=1 overrides CompileOptions and // changes Buffer/Uint8Array lowering. // - PERRY_VERIFY_NATIVE_REGIONS=1 overrides CompileOptions and must diff --git a/crates/perry/src/commands/compile/object_cache/object_cache_tests.rs b/crates/perry/src/commands/compile/object_cache/object_cache_tests.rs index 721c506ae8..d2dec5c4f9 100644 --- a/crates/perry/src/commands/compile/object_cache/object_cache_tests.rs +++ b/crates/perry/src/commands/compile/object_cache/object_cache_tests.rs @@ -584,7 +584,6 @@ fn key_changes_with_codegen_env_vars() { "PERRY_LLVM_INPROCESS", "PERRY_WRITE_BARRIERS", "PERRY_SHADOW_STACK", - "PERRY_STATEPOINTS", "PERRY_RS4GC", "PERRY_GC_SAFEPOINT_ONLY", "PERRY_DISABLE_BUFFER_FAST_PATH", diff --git a/scripts/gc_gate_wiring_check.py b/scripts/gc_gate_wiring_check.py index 1a6fa17d78..b680278cba 100644 --- a/scripts/gc_gate_wiring_check.py +++ b/scripts/gc_gate_wiring_check.py @@ -78,9 +78,9 @@ ( ".github/workflows/gc-native-roots.yml", "gc-native-roots-complete", - "the native-frame root arms (PERRY_STATEPOINTS / PERRY_RS4GC / " + "the native-frame root arms (PERRY_RS4GC / " "PERRY_GC_SAFEPOINT_ONLY / PERRY_STACKMAP_WALKER) — the fan-in that " - "makes one context speak for all four, so adding an arm later never " + "makes one context speak for all three, so adding an arm later never " "needs a branch-protection edit", ), ]