gc: delete the explicit statepoint bridge — one native-root backend, not two - #7348
Conversation
Perry carried two statepoint backends. The explicit bridge rewrote Perry's own IR text into gc.statepoint calls with hand-emitted relocations; RS4GC retypes root allocas and lets LLVM's RewriteStatepointsForGC insert every statepoint and relocation itself. They were never peers. RS4GC does strictly more: the bridge cannot root an invoke, so since #7330 it refused try-carrying functions and CI skipped 09_try_catch_roots on that arm. A mode that cannot compile what its sibling compiles, kept beside it with its own emitter, parser and knob, is the permanent hybrid this project keeps paying for. The bridge was also RS4GC's fallback — a bail in the recognizer silently downgraded the function to it. Measured first: 1,574 functions across test-drizzle-pg (1,543) and the probes (31) all lowered as rs4gc, none fell back. A fallback nothing takes is an untested configuration, so a bail is now a hard failure naming the function rather than a silent downgrade. Deleted with it, because only the bridge used them: the CFG root-liveness analysis (RS4GC gets liveness from LLVM's SSA form), the direct-call parser and statepoint emitter, PreciseRootBackend, and the PERRY_STATEPOINTS knob. PERRY_RS4GC=1 is the single switch; native_stack_roots_enabled() is now just rs4gc_enabled(). One fewer GC knob is one less kill-policy debt. Net -1,216 lines. The default shadow-stack path is untouched. Verified: 10/10 probes byte-match the oracle on the sole backend under forced evacuation with the verifying walker, 10/10 on the default arm, drizzle still builds, 593 codegen unit tests pass.
📝 WalkthroughWalkthroughThe PR removes the explicit statepoint bridge and ChangesRS4GC-only native-root pipeline
Estimated code review effort: 4 (Complex) | ~45 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/gc-native-roots.yml (1)
293-296: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winThe fan-in loop still reads the deleted
native-roots-aarch64job, so the gate fails on every run.Line 284 removed
native-roots-aarch64fromneeds, and the job itself is deleted. Line 294 still expands${{ needs.native-roots-aarch64.result }}. GitHub Actions resolves an unknownneedsentry to an empty context, so that expression renders as an empty string and the loop entry becomesnative-roots-aarch64=. That string does not match*=success), sofailed=1and the job exits 1.
gc-native-roots-completeis the single fan-in context for branch protection. It will now report failure on every run, including onmain, and block merges regardless of whether the real arms passed. Remove the stale entry.🐛 Proposed fix
for arm in \ - "native-roots-aarch64=${{ needs.native-roots-aarch64.result }}" \ "native-roots-rs4gc-aarch64=${{ needs.native-roots-rs4gc-aarch64.result }}" \ "statepoints-refuse-x86=${{ needs.statepoints-refuse-x86.result }}"; doAs per coding guidelines for
.github/workflows/*.{yml,yaml}: a CI gate "must be included in required branch-protection contexts, must avoid unconditional cancellation ofmainruns, and must assert that the behavior it measures actually executed." A permanently failing fan-in context defeats that 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 @.github/workflows/gc-native-roots.yml around lines 293 - 296, Remove the stale "native-roots-aarch64=${{ needs.native-roots-aarch64.result }}" entry from the fan-in loop in gc-native-roots-complete, leaving only currently defined needs contexts such as native-roots-rs4gc-aarch64 and statepoints-refuse-x86 so the gate evaluates their actual results.Source: Coding guidelines
🧹 Nitpick comments (4)
docs/src/cli/flags.md (1)
106-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the
--statepoint-reportcontract matching its current backend.
docs/src/cli/flags.mdmakes--statepoint-reportaPERRY_RS4GC=1requirement, but the CLI help incrates/perry/src/commands/compile/types.rsonly says it is useful with that variable. Also keepplain-stack-map fallbacksterminology consistent: the report supports bothplain stack mapsandstatepoint parser fallbacks, not only user-selectableplain stack-map mode.🤖 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 `@docs/src/cli/flags.md` at line 106, Update the --statepoint-report entry in docs/src/cli/flags.md to match the current CLI contract in Compile command types: describe PERRY_RS4GC=1 as recommended/useful rather than required, and revise the fallback wording to distinguish plain stack maps from statepoint parser fallbacks without implying a selectable plain stack-map mode.crates/perry-codegen/src/function.rs (1)
749-749: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
rs4gc_enabled()conjunct is now redundant.
native_stack_roots_enabled()delegates tors4gc_enabled().gc_strategyat Line 686 is already non-empty only whennative_stack_roots_enabled()is true. The second conjunct can therefore never change the result. The comment above already states this condition is "the same fact asgc_strategyabove", so drop the extra call to keep one source of truth.♻️ Proposed simplification
- let ir = if !gc_strategy.is_empty() && crate::codegen::helpers::rs4gc_enabled() { + let ir = if !gc_strategy.is_empty() { retype_landing_pads_for_statepoints(&ir) } else { ir };🤖 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 `@crates/perry-codegen/src/function.rs` at line 749, Remove the redundant crate::codegen::helpers::rs4gc_enabled() conjunct from the IR selection condition, leaving gc_strategy.is_empty() as the sole check. Keep the surrounding logic and the existing gc_strategy source of truth unchanged.crates/perry-codegen/src/module.rs (1)
651-653: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
native_stack_roots_enabled()condition in both IR renderers. RemovingPERRY_STATEPOINTScollapsed the second condition into the first, so each renderer now has two consecutiveifblocks testing the same predicate. Merge each pair so the two paths stay easy to keep in lockstep.
crates/perry-codegen/src/module.rs#L651-L653: fold thepush_statepoint_declarations(&mut ir)call into theifblock that starts at Line 648 into_ir.crates/perry-codegen/src/module.rs#L923-L925: fold thepush_statepoint_declarations(&mut pre)call into theifblock that starts at Line 920 incodegen_unit_parts.🤖 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 `@crates/perry-codegen/src/module.rs` around lines 651 - 653, Merge the consecutive native_stack_roots_enabled() checks in to_ir at crates/perry-codegen/src/module.rs:648-653 by moving push_statepoint_declarations(&mut ir) into the first block. Apply the same consolidation in codegen_unit_parts at crates/perry-codegen/src/module.rs:920-925, placing push_statepoint_declarations(&mut pre) in the existing predicate block.crates/perry-codegen/src/function/precise_roots.rs (1)
240-247: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftReturn a compile error instead of
panic!for the recognizer bail.Failing hard is correct here. Emitting a plain stack map would leave GC-managed values in ordinary allocas with no relocation, which the collector cannot follow. The concern is only the failure mechanism.
panic!insideperry-codegensurfaces to the user as a Rust panic with a backtrace, not as a compiler diagnostic. It also unwinds throughto_ir(), which runs per function and may run on worker threads during codegen-unit rendering; a panic there aborts or poisons that work rather than reporting one named function cleanly. Prefer the crate's existing compile-error path so the message reaches the user as a diagnostic. Ifto_ir()cannot return aResultwithout a wide signature change, an expliciteprintln!of this message followed bystd::process::exit(1)is still a more predictable failure than an unwind.Keep the message text as written. It names the function and the slot count, which is what a bug report needs.
As per coding guidelines for
crates/perry-codegen/**/*.rs: "Ensure generated GC root stores dominate every later site that may collect, and do not leave GC-managed values only in ordinary allocas." — the hard failure is the right policy; only the panic mechanism needs changing.🤖 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 `@crates/perry-codegen/src/function/precise_roots.rs` around lines 240 - 247, Replace the panic in the None branch of the native-root recognizer with the crate’s existing compile-error diagnostic path, preserving the exact message text and function_name/root_ptrs.len() details. If to_ir() cannot propagate a Result without a broad signature change, emit the same message with eprintln! and terminate explicitly instead of unwinding through codegen.Source: Coding guidelines
🤖 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.
Outside diff comments:
In @.github/workflows/gc-native-roots.yml:
- Around line 293-296: Remove the stale "native-roots-aarch64=${{
needs.native-roots-aarch64.result }}" entry from the fan-in loop in
gc-native-roots-complete, leaving only currently defined needs contexts such as
native-roots-rs4gc-aarch64 and statepoints-refuse-x86 so the gate evaluates
their actual results.
---
Nitpick comments:
In `@crates/perry-codegen/src/function.rs`:
- Line 749: Remove the redundant crate::codegen::helpers::rs4gc_enabled()
conjunct from the IR selection condition, leaving gc_strategy.is_empty() as the
sole check. Keep the surrounding logic and the existing gc_strategy source of
truth unchanged.
In `@crates/perry-codegen/src/function/precise_roots.rs`:
- Around line 240-247: Replace the panic in the None branch of the native-root
recognizer with the crate’s existing compile-error diagnostic path, preserving
the exact message text and function_name/root_ptrs.len() details. If to_ir()
cannot propagate a Result without a broad signature change, emit the same
message with eprintln! and terminate explicitly instead of unwinding through
codegen.
In `@crates/perry-codegen/src/module.rs`:
- Around line 651-653: Merge the consecutive native_stack_roots_enabled() checks
in to_ir at crates/perry-codegen/src/module.rs:648-653 by moving
push_statepoint_declarations(&mut ir) into the first block. Apply the same
consolidation in codegen_unit_parts at
crates/perry-codegen/src/module.rs:920-925, placing
push_statepoint_declarations(&mut pre) in the existing predicate block.
In `@docs/src/cli/flags.md`:
- Line 106: Update the --statepoint-report entry in docs/src/cli/flags.md to
match the current CLI contract in Compile command types: describe PERRY_RS4GC=1
as recommended/useful rather than required, and revise the fallback wording to
distinguish plain stack maps from statepoint parser fallbacks without implying a
selectable plain stack-map mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f5e6cf0-b970-40e6-8e17-4828330b2d40
📒 Files selected for processing (11)
.github/workflows/gc-native-roots.ymlchangelog.d/7345-delete-statepoint-bridge.mdcrates/perry-codegen/src/codegen/helpers.rscrates/perry-codegen/src/function.rscrates/perry-codegen/src/function/precise_roots.rscrates/perry-codegen/src/linker.rscrates/perry-codegen/src/module.rscrates/perry/src/commands/compile/build_cache.rscrates/perry/src/commands/compile/object_cache.rscrates/perry/src/commands/compile/types.rsdocs/src/cli/flags.md
💤 Files with no reviewable changes (2)
- crates/perry/src/commands/compile/object_cache.rs
- crates/perry/src/commands/compile/build_cache.rs
…nt-bridge # Conflicts: # crates/perry-codegen/src/function/precise_roots.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-codegen/src/function/precise_roots.rs (1)
146-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for call-effect classification.
Line [146] makes
CannotCollectthe only unconditionalgc-leaf-functioncase. Add tests forCannotCollect,AllocNoReentrywith both contract states, andUnknown. This protects the RS4GC safepoint boundary from future changes.🤖 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 `@crates/perry-codegen/src/function/precise_roots.rs` around lines 146 - 150, Add regression tests around the call-effect classification logic in precise-roots code generation, covering CannotCollect, AllocNoReentry when gc_safepoint_only_contract_enabled is both enabled and disabled, and Unknown. Assert that only CannotCollect is unconditionally treated as a gc-leaf-function case and that AllocNoReentry follows the contract state, preserving the RS4GC safepoint boundary.
🤖 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.
Nitpick comments:
In `@crates/perry-codegen/src/function/precise_roots.rs`:
- Around line 146-150: Add regression tests around the call-effect
classification logic in precise-roots code generation, covering CannotCollect,
AllocNoReentry when gc_safepoint_only_contract_enabled is both enabled and
disabled, and Unknown. Assert that only CannotCollect is unconditionally treated
as a gc-leaf-function case and that AllocNoReentry follows the contract state,
preserving the RS4GC safepoint boundary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dd0f5da7-0822-413c-b521-145209d42646
📒 Files selected for processing (1)
crates/perry-codegen/src/function/precise_roots.rs
…ly linked (#7353) Perry now links LLVM 22 statically and ships self-contained. We own the assumption rather than pushing it onto the user, and there is no "install a compatible clang" step left to get wrong. It is load-bearing, not a preference. The explicit statepoint bridge is gone (#7348), so RS4GC is the only native-root backend, and RS4GC cannot round-trip its IR through an external `opt` plus a different clang (#7339). Keeping this opt-in meant the only working statepoint path was behind a flag nobody sets. Two defaults flip together, because either alone is half a feature: * `llvm-inprocess` becomes a default cargo feature. * `inprocess_requested()` defaults to ON -- but only iff the backend is actually compiled in. Defaulting to `true` unconditionally would route every compile in a `--no-default-features` build into the not-built-in stub and fail it outright. Verified both ways. `PERRY_LLVM_INPROCESS=0` reverts to the clang subprocess for bisection, and `--no-default-features` still builds the text path. CI: a new `.github/actions/setup-llvm22` composite action, referenced from all 44 toolchain steps across 18 workflows. One definition rather than 44 inline recipes, because the three platforms need three different sources and only one is obvious -- Ubuntu 24.04's own llvm-dev is 18, and chocolatey's `llvm` is the clang toolchain with no llvm-config.exe and none of the static libs. Every arm asserts the major version. Size: 98.9 MB, not the 185.9 MB this would have cost before #7350 -- `initialize_all()` was linking ~18 backends nothing can reach. Also fixed, surfaced by the flip: PERRY_LLVM_KEEP_IR promises the whole scratch dir including the .o. The clang path got that free because the object is a file; in-process returns bytes and silently dropped it, degrading a debugging aid exactly when someone is debugging. Verified on the 81-module zod corpus with no env set: compiles, output byte-identical to the clang path, and PERRY_RS4GC=1 now compiles a try-carrying probe with no further flags. 605 codegen tests pass. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Perry carried two statepoint backends. This deletes one, leaving a single native-root path.
gc.statepointcalls with hand-emitted relocations.ptr addrspace(1), tags the functiongc "statepoint-example", and lets LLVM'sRewriteStatepointsForGCinsert every statepoint, relocation and downstream rewrite.They were never peers
RS4GC does strictly more. The bridge cannot root an
invoke, so since #7330 it refused try-carrying functions outright, and CI had to skip09_try_catch_rootson that arm. Keeping a mode that cannot compile what its sibling compiles — along with its textual emitter, its call parser, and its knob — is the permanent hybrid this project keeps paying for.Measured before removing, because it was also the fallback
The bridge wasn't only a peer: a bail in the RS4GC recognizer silently downgraded the whole function to it. Deleting it without checking would have converted a silent downgrade into a hard failure on real code.
So I measured the fallback rate using the per-function backend the report already records:
rs4gctest-drizzle-pg1,574 functions, zero fallbacks. A fallback nothing takes is an untested configuration — precisely what the GC knob kill-policy exists to prevent. A bail is now a hard failure naming the function, not a silent downgrade.
This is the same standard that made deleting the plain stack map safe (23,301 safepoints, 0 plain maps).
What went with it
Only the bridge used these:
DirectCall, and the statepoint emitterPreciseRootBackendenum — there is one backend, so there is no enumPERRY_STATEPOINTSknob;PERRY_RS4GC=1is the single switch andnative_stack_roots_enabled()is now justrs4gc_enabled()One fewer GC knob is one less kill-policy debt. Removed from both cache keys (
PERRY_RS4GCremains keyed in each, so no vacuous-A/B hazard), CI, and docs.Net −1,216 lines. The default shadow-stack path is untouched.
Verification
PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1 PERRY_STACKMAP_WALKER=verifytest-drizzle-pgstill builds under the sole backendNote on #7344
#7344 fixes the bridge job's toolchain and adds an ELF arm. Its bridge half is moot now — that job is deleted here. The ELF arm is still wanted, repointed at RS4GC; I'd suggest closing #7344 and letting me re-land the ELF arm on top of this, rather than merging a fix for a job that no longer exists.
Summary by CodeRabbit
Breaking Changes
PERRY_STATEPOINTSconfiguration.--statepoint-reportusage to requirePERRY_RS4GC=1.Documentation