fix(gc): split the precise-root analysis from its lowering - #7340
Open
proggeramlug wants to merge 4 commits into
Open
fix(gc): split the precise-root analysis from its lowering#7340proggeramlug wants to merge 4 commits into
proggeramlug wants to merge 4 commits into
Conversation
added 2 commits
August 3, 2026 23:01
One knob answered two questions. "Which locals hold GC pointers, and where must each stay live" is the analysis and is backend-independent. "Is that answer represented as a heap-backed shadow frame or a native stack map" is the lowering, and LlFunction already chose it independently -- enable_shadow_frame_inner and reserve_shadow_slot both take the native path first. But the eight sites that build the slot map all gated on shadow_stack_enabled(), so PERRY_SHADOW_STACK=0 switched the ANALYSIS off and left the statepoint lowering with nothing to lower. The result was a binary with no precise frame roots at all: no __perry_gcmap section, same size as a plain shadow-off build, correct output. Nothing distinguished it from a good build until a collection freed a live object. #7332 made the pair a hard error as a stopgap. Route those eight sites through precise_root_analysis_enabled() instead and the pair becomes expressible, which is what the stopgap was standing in for. Measured on 01_nursery_churn: PERRY_STATEPOINTS=1 with and without PERRY_SHADOW_STACK=0 now emit an identical 885-byte root map and an identical __text. The knob keeps its own meaning on its own -- no gcmap, and still observable against the default build. A mode nobody can select is a mode nobody can measure, so this is the prerequisite for the shadow-stack lowering ever being removed rather than merely being switched off in one configuration nobody tests.
📝 WalkthroughWalkthroughThis change decouples precise root analysis from shadow-stack lowering. Native stack-root builds keep root analysis enabled when ChangesNative root analysis
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant Codegen
participant RootMapEmitter
CI->>Codegen: Build with statepoints and shadow stack enabled
CI->>Codegen: Build with statepoints and PERRY_SHADOW_STACK=0
Codegen->>RootMapEmitter: Analyze precise roots and emit root maps
RootMapEmitter-->>CI: Return root maps and generated text
CI->>CI: Compare outputs and verify non-vacuity
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
🧹 Nitpick comments (1)
.github/workflows/gc-native-roots.yml (1)
157-212: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd shadow-stack-off coverage to the RS4GC job.
native_stack_roots_enabled() = statepoints_enabled() || rs4gc_enabled(), and the changelog notes this silent-root-loss combination affectsPERRY_RS4GC=1as well. The current RS4GC matrix only runsPERRY_RS4GC=1with forced evacuation; add thePERRY_RS4GC=1 PERRY_SHADOW_STACK=0differential path that mirrors the statepoint step.🤖 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 157 - 212, Extend the RS4GC job matrix with a differential run using PERRY_RS4GC=1 and PERRY_SHADOW_STACK=0, mirroring the existing statepoint shadow-stack-off coverage. Compare its generated root-map and emitted-text artifacts against the corresponding RS4GC-enabled run, while preserving the existing forced-evacuation coverage and assertions.
🤖 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 @.github/workflows/gc-native-roots.yml:
- Around line 157-212: Extend the RS4GC job matrix with a differential run using
PERRY_RS4GC=1 and PERRY_SHADOW_STACK=0, mirroring the existing statepoint
shadow-stack-off coverage. Compare its generated root-map and emitted-text
artifacts against the corresponding RS4GC-enabled run, while preserving the
existing forced-evacuation coverage and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 863159e0-ca25-4d89-80ad-1bff09a7a0b7
📒 Files selected for processing (6)
.github/workflows/gc-native-roots.ymlchangelog.d/7340-decouple-root-analysis.mdcrates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/codegen/helpers.rscrates/perry-codegen/src/codegen/method.rs
…echanism
Two corrections and one measurement.
The gap suite re-run against RS4GC in-process, two arms per test
(shadow-stack control + RS4GC), 479/479: 447 pass->pass, 19 pre-existing
diffs unchanged, 13 node_fail, ZERO new regressions, ZERO refusals, ZERO
compile failures. Zero refusals is the load-bearing number -- 128 of the
479 tests contain `try {}` and the bridge cannot compile any of them.
The earlier soak's "13 regressions, do not flip" was measured against
the bridge, before #7329/#7330, on a backend that structurally cannot
compile a quarter of the suite. It should not be carried forward.
And the x86-64 mechanism was wrong. The workflow comment claimed
_Unwind_GetGR(ctx, 7) "does not reliably return the stack pointer".
Measured on x86-64 Linux (glibc 2.39, gcc 13.3.0): it SEGFAULTS. RBX,
RBP and RIP return correctly; RAX and RSP both SIGSEGV, because libgcc
tracks only the columns CFI restores and RSP is derived from the CFA
rather than tracked. The fault is in the call itself, so no address
validation after it can help -- the previous wording pointed at the
wrong fix. Details and a reproducer in #7333.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/engine-plan.md`:
- Around line 206-213: Update the “On aarch64” section in the RS4GC
default-readiness discussion to classify the x86-64 `#7333` blocker as a
correctness and stability issue, explicitly distinguishing the absence of new
aarch64 regressions from the unresolved x86-64 SIGSEGV and native-root recovery
gap. Preserve the existing llvm-inprocess feature prerequisite separately.
- Around line 160-170: Update the probe-results paragraph describing the
in-process RS4GC path to state that building llvm-inprocess requires LLVM 22,
available through LLVM_SYS_221_PREFIX or llvm-config on PATH, while
PERRY_LLVM_INPROCESS=1 remains the runtime gate. Keep the existing claim about
no PERRY_LLVM_* runtime pinning, but clarify that it does not remove this build
prerequisite.
🪄 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: f48ff130-c853-4ade-9bae-297de44849e3
📒 Files selected for processing (2)
.github/workflows/gc-native-roots.ymldocs/engine-plan.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/gc-native-roots.yml
Comment on lines
+160
to
+170
| **1. There was no working statepoint path for `try` on a default toolchain | ||
| (#7339).** The explicit bridge cannot root an `invoke`, and since #7305 every | ||
| call inside a `try` *is* an invoke — so the bridge refuses those functions | ||
| outright (#7330). RS4GC handles them, but it ran as an external `opt` subprocess | ||
| whose output an older `clang` could not parse (`error: unterminated attribute | ||
| group`), making it reachable only on a hand-pinned LLVM 22. **128 of 479 gap | ||
| tests (26%) contain `try {}`**, so a quarter of the suite had no statepoint path | ||
| at all. Routing RS4GC through layer 0's in-process pipeline removes the external | ||
| boundary entirely: all nine probes now compile with no `PERRY_LLVM_*` pinning, | ||
| byte-identical to the shadow-stack control, copying 5,946–90,271 objects, with | ||
| `backend rs4gc` on every function record. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs/engine-plan.md lines 145-180 =="
sed -n '145,180p' docs/engine-plan.md
echo
echo "== locate Cargo.toml and search LLVM inprocess/preconditions =="
fd -a 'Cargo\.toml$' . | sed 's#^\./##' | head -20
echo
rg -n "llvm-inprocess|PERRY_LLVM|LLVM|llvm" -S crates docs --glob '!target/**' | head -200Repository: PerryTS/perry
Length of output: 3726
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== crates/perry/Cargo.toml relevant sections =="
rg -n '(\[.*\]|name\s*=\s*"perry"|llvm|inprocess|LLVM)' crates/perry/Cargo.toml crates --glob 'Cargo.toml' -S | head -250
echo
echo "== docs or Cargo files mentioning llvm-inprocess PERRY_LLVM LLVM 22 =="
rg -n "llvm-inprocess|llvm-inprocess|PERRY_LLVM|LLVM 22|LLVMVersion|llvm-version|LLVMVersionReq|versions_require" -S Cargo.toml crates docs README.md README** --glob '!target/**' | head -250
echo
echo "== crates/perry Cargo snippets around features/opt dependencies =="
sed -n '1,220p' crates/perry/Cargo.tomlRepository: PerryTS/perry
Length of output: 45213
Add the LLVM 22 build prerequisite to the probe results.
llvm-inprocess is non-default and requires LLVM 22 (LLVM_SYS_221_PREFIX or llvm-config on PATH) even though PERRY_LLVM_INPROCESS=1 is the runtime gate. The “no PERRY_LLVM_* pinning” wording narrows the runtime path but leaves out the build requirement.
Proposed wording
- byte-identical to the shadow-stack control, copying 5,946–90,271 objects,
+ byte-identical to the shadow-stack control when the LLVM 22 build
+ prerequisite is available, copying 5,946–90,271 objects,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **1. There was no working statepoint path for `try` on a default toolchain | |
| (#7339).** The explicit bridge cannot root an `invoke`, and since #7305 every | |
| call inside a `try` *is* an invoke — so the bridge refuses those functions | |
| outright (#7330). RS4GC handles them, but it ran as an external `opt` subprocess | |
| whose output an older `clang` could not parse (`error: unterminated attribute | |
| group`), making it reachable only on a hand-pinned LLVM 22. **128 of 479 gap | |
| tests (26%) contain `try {}`**, so a quarter of the suite had no statepoint path | |
| at all. Routing RS4GC through layer 0's in-process pipeline removes the external | |
| boundary entirely: all nine probes now compile with no `PERRY_LLVM_*` pinning, | |
| byte-identical to the shadow-stack control, copying 5,946–90,271 objects, with | |
| `backend rs4gc` on every function record. | |
| **1. There was no working statepoint path for `try` on a default toolchain | |
| (`#7339`).** The explicit bridge cannot root an `invoke`, and since `#7305` every | |
| call inside a `try` *is* an invoke — so the bridge refuses those functions | |
| outright (`#7330`). RS4GC handles them, but it ran as an external `opt` subprocess | |
| whose output an older `clang` could not parse (`error: unterminated attribute | |
| group`), making it reachable only on a hand-pinned LLVM 22. **128 of 479 gap | |
| tests (26%) contain `try {}`**, so a quarter of the suite had no statepoint path | |
| at all. Routing RS4GC through layer 0's in-process pipeline removes the external | |
| boundary entirely: all nine probes now compile with no `PERRY_LLVM_*` pinning, | |
| byte-identical to the shadow-stack control when the LLVM 22 build | |
| prerequisite is available, copying 5,946–90,271 objects, with | |
| `backend rs4gc` on every function record. |
🤖 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/engine-plan.md` around lines 160 - 170, Update the probe-results
paragraph describing the in-process RS4GC path to state that building
llvm-inprocess requires LLVM 22, available through LLVM_SYS_221_PREFIX or
llvm-config on PATH, while PERRY_LLVM_INPROCESS=1 remains the runtime gate. Keep
the existing claim about no PERRY_LLVM_* runtime pinning, but clarify that it
does not remove this build prerequisite.
Comment on lines
+206
to
+213
| **⇒ On aarch64, RS4GC-in-process is now a viable default.** Two things still gate | ||
| flipping it globally, and neither is correctness: | ||
|
|
||
| 1. **`llvm-inprocess` is a non-default cargo feature.** RS4GC-as-default requires | ||
| layer 0's feature becoming default first (#7301's scope, not this work's). | ||
| 2. **x86-64 remains blocked on #7333** — see the measured `_Unwind_GetGR(ctx, 7)` | ||
| segfault above. A default that only works on one architecture is not a | ||
| default. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Classify the x86-64 blocker as a correctness and stability blocker.
The next item documents a SIGSEGV in _Unwind_GetGR(ctx, 7) and missing native-root recovery. That is a runtime-safety blocker, not only a scope or process issue. Distinguish the absence of new aarch64 regressions from the unresolved x86-64 correctness gap.
Proposed wording
-Two things still gate flipping it globally, and neither is correctness:
+Two things still gate flipping it globally. Aarch64 has no new correctness
+regressions, but x86-64 still has a runtime-safety blocker:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **⇒ On aarch64, RS4GC-in-process is now a viable default.** Two things still gate | |
| flipping it globally, and neither is correctness: | |
| 1. **`llvm-inprocess` is a non-default cargo feature.** RS4GC-as-default requires | |
| layer 0's feature becoming default first (#7301's scope, not this work's). | |
| 2. **x86-64 remains blocked on #7333** — see the measured `_Unwind_GetGR(ctx, 7)` | |
| segfault above. A default that only works on one architecture is not a | |
| default. | |
| **⇒ On aarch64, RS4GC-in-process is now a viable default.** Two things still gate | |
| flipping it globally. Aarch64 has no new correctness | |
| regressions, but x86-64 still has a runtime-safety blocker: | |
| 1. **`llvm-inprocess` is a non-default cargo feature.** RS4GC-as-default requires | |
| layer 0's feature becoming default first (`#7301`'s scope, not this work's). | |
| 2. **x86-64 remains blocked on `#7333`** — see the measured `_Unwind_GetGR(ctx, 7)` | |
| segfault above. A default that only works on one architecture is not a | |
| default. |
🤖 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/engine-plan.md` around lines 206 - 213, Update the “On aarch64” section
in the RS4GC default-readiness discussion to classify the x86-64 `#7333` blocker
as a correctness and stability issue, explicitly distinguishing the absence of
new aarch64 regressions from the unresolved x86-64 SIGSEGV and native-root
recovery gap. Preserve the existing llvm-inprocess feature prerequisite
separately.
…y, not metadata The plan asserted 'closing that axis needs fewer roots, not a tighter encoding' on the strength of one app measurement. Measured directly with two 2000-function programs: root-free functions +0 bytes (no map emitted, text identical) root-dense functions +4,330,592 B (97% __text, 21% gcmap) So statepoints carry NO fixed cost -- a function with nothing live across a safepoint pays nothing -- and the growth is the per-root relocation sequence, not the map. #7314's compact map fully answered the metadata objection, but metadata was never the dominant term at scale. Runtime on the same probes, quiet host, median of 5: statepoints 1-2% faster, every probe neutral or faster.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/engine-plan.md`:
- Line 218: Update the measurement date in the “Binary size, measured
2026-08-04” heading to the actual run date; if the measurement has not occurred,
mark the measurement as planned instead of presenting a future date.
- Around line 228-235: Clarify the root-free benchmark row and surrounding
conclusion by identifying whether the +12 B in __text is shared binary overhead
rather than per-function statepoint cost. Update the table note or explanatory
text so it explicitly reconciles this value with the statement that root-free
functions emit no statepoint-specific text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| So the honest state is: *aarch64-viable, globally blocked on two pieces of scope | ||
| that are both already identified.* | ||
|
|
||
| ### ★ Binary size, measured 2026-08-04 — it is a ROOT-DENSITY problem, not a metadata one |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the actual measurement date.
Line [218] labels the results as measured on August 4, 2026, but the current review date is August 3, 2026. Replace it with the actual run date, or mark the measurement as planned until it exists.
🤖 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/engine-plan.md` at line 218, Update the measurement date in the “Binary
size, measured 2026-08-04” heading to the actual run date; if the measurement
has not occurred, mark the measurement as planned instead of presenting a future
date.
Comment on lines
+228
to
+235
| | 2000 **root-free** functions (scalar only) | **+0 B** | +12 B | not emitted | | ||
| | 2000 **root-dense** functions (3 heap values live across an alloc) | **+4,330,592 B (+18.95%)** | +4,203,608 B | 902,124 B | | ||
|
|
||
| Two things follow, and both matter for planning: | ||
|
|
||
| 1. **Statepoints have no fixed cost.** A function with nothing live across a | ||
| safepoint pays nothing at all — no map entry, no text. So the axis is not | ||
| "statepoints are bigger", it is "roots are bigger", and a program's exposure |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if [ -f docs/engine-plan.md ]; then
nl -ba docs/engine-plan.md | sed -n '200,255p'
else
echo "docs/engine-plan.md not found"
git ls-files | rg '(^|/)engine-plan\.md$|(^|/)docs/' | head -100
fiRepository: PerryTS/perry
Length of output: 191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if [ -f docs/engine-plan.md ]; then
awk '{printf "%6d\t%s\n", NR, $0}' docs/engine-plan.md | sed -n '200,255p'
else
echo "docs/engine-plan.md not found"
git ls-files | grep -E '(^|/)engine-plan\.md$|^docs/' | head -100 || true
fiRepository: PerryTS/perry
Length of output: 3340
Clarify the shared binary overhead in the root-free case.
The root-free row shows +12 B in __text, but the text below says functions with nothing live pay “no text” at all. State whether the 12 B is shared binary overhead, or otherwise reconcile the table and conclusion.
🤖 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/engine-plan.md` around lines 228 - 235, Clarify the root-free benchmark
row and surrounding conclusion by identifying whether the +12 B in __text is
shared binary overhead rather than per-function statepoint cost. Update the
table note or explanatory text so it explicitly reconciles this value with the
statement that root-free functions emit no statepoint-specific text.
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.
Closes #7326. Supersedes the #7332 stopgap.
The conflation
Two separable questions, one knob:
analysis. It is backend-independent.
shadow frame, or a native stack map? That is the lowering.
LlFunctionalready chose the lowering independently:enable_shadow_frame_innerand
reserve_shadow_slotboth take the native path first undernative_stack_roots_enabled(). But all eight sites that build the slot mapgated on
shadow_stack_enabled().So
PERRY_SHADOW_STACK=0 + PERRY_STATEPOINTS=1switched the analysis off andleft the statepoint lowering with nothing to lower. The output was a binary with
no precise frame roots at all — no
__perry_gcmapsection, same size as a plainshadow-off build, correct answers. Nothing about the run distinguished it from a
good build until a collection freed a live object. #7332 made the combination a
hard error as a stopgap, which was right at the time but leaves the underlying
coupling in place.
The change
Introduce
precise_root_analysis_enabled()and route those eight sites throughit. Nothing else moves — the lowering selection was already where it belonged.
Why this matters beyond the one broken combination
A mode nobody can select is a mode nobody can measure. Deleting the
shadow-stack lowering is the direction this project has committed to, and the
first step is being able to express "roots on, shadow lowering off" so it can
be run, gated, and compared. Until now that spelling produced a rootless binary,
so nobody could have run it meaningfully — which is a large part of why the
question stayed open.
This PR does not delete anything or change any default. It makes the
configuration selectable and proves the split is real.
Measurement
On
01_nursery_churn,PERRY_STATEPOINTS=1with and withoutPERRY_SHADOW_STACK=0:__perry_gcmap: 885 bytes, identical__text: 2,234,533 lines, identicalAnd the knob keeps its own meaning: alone it still emits no root map, and it is
still observable against the default build (2,234,817 vs 2,234,450 lines of
__text— the lowering is the inline #7088 path, which is why nojs_shadow_frame_pushcall symbol appears in either).Gating
New step in
native-roots-aarch64, asserting all four rows above.It compares the root map and
__text, not the binary. Two runs of the sameconfiguration already differ byte-for-byte, because the build embeds a
PID-and-nonce scratch path — an end-to-end hash here would be a test that can
only fail. I hit exactly that while writing this and it is worth stating rather
than leaving for the next person.
The step also carries a vacuity check: if the default build and
PERRY_SHADOW_STACK=0ever emit identical code, the probe roots nothing and thecomparison above proves nothing, so it fails rather than passing quietly. That is
CLAUDE.md's fourth hazard applied to this gate's own subject.
cargo test -p perry-codegen --lib: 601 passed, 0 failed.Summary by CodeRabbit
New Features
Bug Fixes
Documentation