fix(codegen): delete four report counters no code could ever write - #7362
Merged
Conversation
`--statepoint-report` declared `plain_stack_maps`, `stack_map_operands`, `statepoint_fallbacks` and `fallbacks_by_callee`, summed all four into the totals, and rendered them in both text and JSON. None had a writer. The mutator API is note_call / note_skipped / note_statepoint and none touches them; `git log -S note_fallback` finds nothing, so they were never populated -- not orphaned by the plain-map bridge deletion, dead from the start. So the report printed "0 statepoint parser fallback(s)" as reassurance that no root had been recorded in an unrecoverable location, a 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. The real fail-closed guarantee is gc_map.rs returning Err on an unparseable or uncompactable map -- a fallback fails the BUILD. Replaced with `every_rendered_counter_has_a_writer`, which drives a record through every mutator and asserts no rendered scalar is zero. It caught a LIVE field on its first run (`calls_without_live_roots`, because the fixture only made calls that had live roots), so the invariant has teeth. Same sweep, each verified dead rather than assumed: - PERRY_STATEPOINTS is never read anywhere, yet the empty-report diagnostic told users to set it -- a dead end that produces the same empty report forever. Now names PERRY_RS4GC=1 and the object cache, the two real causes. Dropped from the cache-key list: that test hashes the environment, so it passed for a name nothing reads and could not have caught the drift. - `declare void @llvm.experimental.stackmap` was emitted per module and never called; removing it collapses two adjacent identical native_stack_roots_enabled() blocks. - compact_and_assemble recomputed a `ptr64` it never read, which read like a width guard that had been defeated. - stack_maps.rs's module doc claimed a "research backend", two competing "prototypes", and a macOS-only implementation. It is the only backend, the plain-map lowering is deleted, and it covers Apple/Linux/Windows on aarch64 and x86-64. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF
📝 WalkthroughWalkthroughThe change removes unwritten statepoint report counters, adds counter-writer validation, emits complete statepoint declarations, updates runtime documentation, removes obsolete ChangesStatepoint reporting and backend cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/gc_gate_wiring_check.py`:
- Around line 81-84: Update the native-root gate contract described in the
documentation near the native-frame root arm list to match the actual CI
behavior: either remove the unsupported PERRY_GC_SAFEPOINT_ONLY and
PERRY_STACKMAP_WALKER fan-in claims from scripts/gc_gate_wiring_check.py, or
wire those paths into gc-native-roots-complete so all three arms genuinely gate
completion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eb03fa0d-7907-4cbf-8371-33b276ad5d83
📒 Files selected for processing (10)
changelog.d/7362-statepoint-report-dead-counters.mdcrates/perry-codegen/src/codegen/helpers.rscrates/perry-codegen/src/gc_map.rscrates/perry-codegen/src/linker.rscrates/perry-codegen/src/module.rscrates/perry-codegen/src/statepoint_report.rscrates/perry-runtime/src/gc/roots/stack_maps.rscrates/perry/src/commands/compile/object_cache.rscrates/perry/src/commands/compile/object_cache/object_cache_tests.rsscripts/gc_gate_wiring_check.py
💤 Files with no reviewable changes (2)
- crates/perry-codegen/src/module.rs
- crates/perry/src/commands/compile/object_cache/object_cache_tests.rs
Comment on lines
+81
to
84
| "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", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 'PERRY_STATEPOINTS|plain_stack_maps|stack_map_operands|statepoint_fallbacks|fallbacks_by_callee' \
scripts/statepoint_report_assert.py || true
rg -n -C 6 'statepoint_report_assert|gc-native-roots|statepoint-report' \
.github/workflows scripts || trueRepository: PerryTS/perry
Length of output: 13115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("scripts/statepoint_report_assert.py")
text = p.read_text()
needles = [
"PERRY_STATEPOINTS",
"plain_stack_maps == 0",
"statepoint_fallbacks == 0",
"stack_map_operands",
"statepoint_fallbacks",
"fallbacks_by_callee",
]
for needle in needles:
print(f"{needle}: {text.count(needle)} occurrence(s)")
for start, end in [(1,30), (300,480)]:
print(f"\n--- scripts/statepoint_report_assert.py lines {start}-{end} ---")
for i,line in enumerate(text.splitlines()[start-1:end], start=start):
print(f"{i:4}: {line}")
PY
echo
sed -n '260,350p' .github/workflows/gc-native-roots.ymlRepository: PerryTS/perry
Length of output: 7908
Update the native-root gate documentation to match the actual CI gate.
.github/workflows/gc-native-roots.yml completes from only the RS4GC-native-roots job, but scripts/gc_gate_wiring_check.py says gc-native-roots-complete fans in PERRY_RS4GC, PERRY_GC_SAFEPOINT_ONLY, and PERRY_STACKMAP_WALKER. Drop the unsupported native-root contract from this check or wire the missing safepoint-only and stack-map-walker paths into the gate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/gc_gate_wiring_check.py` around lines 81 - 84, Update the native-root
gate contract described in the documentation near the native-frame root arm list
to match the actual CI behavior: either remove the unsupported
PERRY_GC_SAFEPOINT_ONLY and PERRY_STACKMAP_WALKER fan-in claims from
scripts/gc_gate_wiring_check.py, or wire those paths into
gc-native-roots-complete so all three arms genuinely gate completion.
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
…7368) * fix(codegen): statepoint report counted zero safepoints since #7348 #7348 deleted the explicit bridge and with it the only callers of note_statepoint and note_skipped -- they lived in the bridge, which counted safepoints as it emitted them. The methods survived with no callers, so statepoints, relocations, max_live_roots, skipped_non_safepoints, live_roots_histogram and both by-callee maps went structurally zero in production. A real compile printed "0 statepoints emitted" while its binary carried 120. Counting at IR-emission time cannot work any more, and that is the lesson: Perry no longer decides which calls become safepoints -- RewriteStatepointsForGC does, inside LLVM. The only honest source is the compact-map rewrite, which already parses the assembly LLVM emitted and computed these exact numbers before dropping them into log::debug!. The report reads from there now: 120 safepoints across 6 function(s) in 1 module(s) 36 live roots recorded, 0.30 per safepoint An absent measurement no longer renders as a measured zero: gc_map.modules == 0 means "never reported", the text report says UNAVAILABLE rather than printing zeros, and JSON carries gc_map separately from totals so a consumer can tell them apart. schema_version -> 2. The CI gate now asserts the counts, not just the label. --only-backend rs4gc passed throughout the regression -- the label was right, the numbers were fiction. It now also requires records > 0 and roots > 0; verified against a synthetic report with the #7348 shape, where the label check still reports 9 functions green while the count checks exit 1. Second round of dead counters here (#7362 removed four that never had a writer at all). The new test documents why the first invariant missed this one: every_rendered_counter_has_a_writer called the mutators itself, so "has a writer" passed while "is written" was false. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF * gc: admit two provably-leaf helpers, and measure that it buys nothing (#7369) * fix(lint): split index_set.rs, over the 2000-line cap since #7342 (#7366) `scripts/check_file_size.sh` exits 1 on main HEAD: `crates/perry-codegen/src/expr/index_set.rs` is 2035 lines against a 2000 cap. It crossed in #7342. That script runs inside the `lint` job, which is a REQUIRED context -- so this is the second independent way `lint` was red on main today (the first was rustfmt on linker.rs, #7361). A required check that is red on main blocks nothing; it means every merge is a bypass. The split follows the recipe in the script's own failure message: extract a topical group into a sibling module. `lower_inline_dyn_typed_array_set` and its `emit_inline_ta_int_store` helper are the guarded inline typed-array store for a type-erased receiver -- one coherent unit, moved verbatim to `index_set_typed_array.rs`. index_set.rs drops to 1749 lines, leaving real headroom rather than landing one line under the cap. Mechanical move: the two functions are byte-identical, only the imports they need travelled with them and `lower_inline_dyn_typed_array_set` became `pub(super)` so its one caller can still reach it. cargo test -p perry-codegen --lib: 609 passed. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF Co-authored-by: Ralph Küpper <ralph@skelpo.com> * docs(plan): both statepoint adoption gates are closed; record the platform matrix (#7367) The plan still said statepoints were aarch64-only (#7321), that the matrix "therefore runs on macos-14", that `statepoints-refuse-x86` pinned the refusal, and it spelled the knob `PERRY_STATEPOINTS` four times. None of that is true now, and this document is what the adoption decision gets made from. What actually changed: - x86-64 is unblocked. `_Unwind_GetGR(ctx, 7)` does segfault and cannot be fixed as stated -- libgcc tracks only the columns CFI restores and RSP is derived, not tracked. #7349 stopped asking for it and derives the SP-relative base from `_Unwind_GetCFA`, with a per-arch return-address adjustment (x86-64 `call` pushes one, aarch64 `bl` does not). x86-64 Linux is a first-class arm. - Windows works via RtlVirtualUnwind (#7355), the one walker with no Itanium unwinder beneath it. - aarch64+ELF is now covered too (#7360) -- the only shape where LLVM spells 32-bit stack-map fields `.word`. - One mechanism, not two: PERRY_STATEPOINTS and the plain-map bridge are deleted, so the kill-policy line about "a mode that still exists" no longer applies to this pair. - The gate proves something now. Until today the Unix arms reported 7 frames and ZERO locations -- they would have passed with a walker that visited nothing. #7359's deep-collect probe took them to 221 locations. - watchOS/visionOS are not blocked by Perry: they build on stable without `dyn-eval`, and fail three crates away in psm's Mach-O guard. So the remaining adoption gate is `llvm-inprocess` becoming a default cargo feature, plus sequencing step 2 (root density) -- adopting today would regress binary size on root-dense code. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF Co-authored-by: Ralph Küpper <ralph@skelpo.com> * gc: admit two provably-leaf helpers, and measure that it buys nothing js_gc_register_global_root was the most frequent non-leaf callee in the probe suite (148 call sites) and is provably GC-leaf: its whole body is runtime_write_barrier_root_heap_word -- which js_write_barrier_root_heap_word, already CannotCollect, wraps in one line -- plus a TLS Vec::push. The "malloc count threshold" trigger does not apply to that push: the counter is MALLOC_STATE.objects.len(), a registry of Perry GC objects, and the #[global_allocator] is plain mimalloc/System with no GC hook. js_typed_feedback_maybe_dump_trace joins its already-admitted family siblings. Measured A/B on the same tree, and the result is a null: probe safepoints roots total bytes __text 06_string_retention 105 -> 100 27=27 0 -4 B 09_try_catch_roots 343 -> 339 259=259 0 -4 B 11_collect_at_depth 120 -> 117 36=36 0 -4 B Root counts are IDENTICAL. The 40 safepoints removed across the suite were all rootless, and a rootless safepoint costs essentially nothing -- which is what docs/engine-plan.md already says: "the axis is not 'statepoints are bigger', it is 'roots are bigger'". Recording it as evidence: the safepoint-count lever is not the binary-size lever, so sequencing step 2 must attack live-root SETS. Two tests come with it. One pins the wrapper's classification to the barrier it wraps. The other pins js_nanbox_string OUT of the allowlist: at 120 call sites it is the obvious next candidate and reads as pure bit manipulation, but its null guard calls js_string_from_bytes to allocate an empty string. Probe suite 11/11 byte-identical under forced evacuation + verification. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com> * fix(ci): report assertion pinned to a probe Windows cannot compile Three review fixes on #7368. The report assertion ran on 09_try_catch_roots, which contains four `try` blocks. RS4GC cannot rewrite WinEH funclet pads, so linker.rs's rs4gc_funclet_refusal rejects that probe on windows-msvc -- the probe loop above tolerates it by grepping the compile log for "funclet", but this step did not. A gate pinned to a probe that cannot compile on one arm fails for a reason unrelated to its subject. The portable assertion now uses 11_collect_at_depth (no `try`, compiles on all four arms); 09_try_catch_roots keeps its own non-Windows step so the try-specific coverage that justified deleting the bridge is not lost. The gc_map doc claimed records/roots would be ABSENT when unmeasured. They are plain u64 fields on a plain derive and always serialise; `modules` is the sentinel. Fixed to describe what the code actually does -- the same class of comment-vs-code drift this PR exists to clean up. The "map never reported" guard fired for any --require-*/--print, including fields that live in `totals` and are counted at IR-emission time whether or not the rewrite ran. Now scoped to map-backed fields: --require-positive textual_calls is answered from its measured value (verified exit 0) while --require-positive records still fails on an unreported map (exit 1). Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The finding
--statepoint-reportdeclared four fields onFunctionRecord:All four were summed into the totals and rendered into both the text and JSON reports. None of them had a writer. The mutator API is
note_call/note_skipped/note_statepoint, and none touches these fields.git log -S note_fallbackfinds nothing — so they were never populated at any point, not even before the plain-map bridge was deleted. Dead from the start.The consequence is not cosmetic. The report printed:
…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 quiet part out loud:
Someone noticed it could only ever be zero and wrote a comment claiming that was the point. It isn't. A counter that cannot be non-zero is CLAUDE.md's fourth failure mode with the subject removed entirely — the gate runs, its subject never did.
The real guarantee was elsewhere and is fine.
gc_map.rshas tenreturn Errsites: an unparseable or uncompactable map fails the build. That is a genuine fail-closed property and it is why deleting the bridge was justified. The counter added nothing and actively misled anyone reading the report for confidence.The replacement has teeth
every_rendered_counter_has_a_writerdrives a record through every mutator and asserts no scalar in the rendered totals is zero.It failed on its first run — on
calls_without_live_roots, which is a live field. My fixture only made calls that had live roots, so the counter legitimately stayed zero. Fixed the fixture, not the assertion. That's the evidence the invariant actually bites rather than being another green light.Same sweep — each verified dead, not assumed
PERRY_STATEPOINTSgrep 'var("PERRY_STATEPOINTS")'→ no hits anywherellvm.experimental.stackmapcallsites → noneptr64incompact_and_assembleunused_variablewarningstack_maps.rsmodule docThe
PERRY_STATEPOINTSone is a real user-facing bug. The empty-report diagnostic said:That variable is read by nothing. A user who hits an empty report, follows the first instruction, and re-runs gets the same empty report — forever. It now names
PERRY_RS4GC=1and the object cache, which are the two things that actually cause it.It was also in the object-cache key list. Note that the test guarding that list could not have caught this: it hashes the environment, so setting any
PERRY_*name changes the key. It passed for a variable nothing reads.The
ptr64one is worth a second look — it sat directly above the arch refusal with a comment justifying watchOS ILP32 support, computed and never read. It looked like a width guard that had been defeated. It hadn't; the emitter's ownptr64is the live one, and behavior is unchanged.Risk
Report/diagnostic surface only, plus dead-IR removal. No change to lowering, walking, or root discovery.
One thing reviewers should check me on: the JSON schema loses four keys.
schema_versionstays1. If anything outside this repo consumes that JSON, those keys go from always-0to absent. I could find no such consumer —statepoint_report_assert.pyis the only reader and it does not touch them — but I'd rather flag it than have it discovered downstream.cargo test -p perry-codegen --lib608 passed ·-p perry-runtime --lib stack_maps17 passed ·-p perry --bins object_cache46 passed ·gc_gate_wiring_check.pyOK.Summary by CodeRabbit
Bug Fixes
Documentation
Tests