gc: native roots beyond aarch64-macOS — x86-64, iOS, iPadOS, tvOS - #7349
Merged
Conversation
PERRY_RS4GC=1 was refused off aarch64 (#7324). The refusal was right — the collector segfaulted rather than reporting anything — but the cause was one unsupported call, not anything architectural. On x86-64 every root is Indirect [RSP + off], DWARF register 7, and the unwinder path resolved bases with _Unwind_GetGR(context, reg). _Unwind_GetGR is not a supported query for the stack-pointer column; it returned garbage the collector wrote through. _Unwind_GetCFA is the supported way. SP-relative roots now derive from the CFA: by the SysV/AAPCS definition it is the caller's stack pointer immediately before the call, so the body stack pointer sits one return-address slot plus this function's frame below it, and stack_size is exactly that frame, already in the map. The architecture's SP register number is a runtime-local constant, deliberately separate from the format's base tags — those stay aarch64-literal so the compiler's idea of the target and the runtime's target_arch cannot disagree. Measured on real x86-64 Linux: 10/10 probes byte-match the pinned oracle under forced evacuation with verification, .perry_gcmap present in every binary. The walker ran rather than passing vacuously — telemetry reports walks=1, frames_visited=10, records_matched=1, locations_visited=2 with fp_walks=0, and evacuation moved objects (retained_forwarded_stub_objects=5). aarch64 telemetry has the same shape (7/0/1/1 vs 10/0/1/1), which makes this an equivalence result. Those counts are low on BOTH platforms because the probes end in a manual gc() from a shallow stack — a pre-existing gate weakness worth naming, not something this introduces.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR enables pointer-width-aware compact GC maps and CFA-based x86-64 stack-pointer root resolution. It expands native GC-root support to 64-bit Apple targets and Linux, updates CI coverage, and documents validation results. ChangesNative GC-root support
Estimated code review effort: 3 (Moderate) | ~25 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 `@crates/perry-runtime/src/gc/roots/stack_maps.rs`:
- Around line 878-889: The SP-relative base calculation in the stack-map
handling must use an architecture-specific return-address adjustment: update the
base logic around _Unwind_GetCFA so x86-64 subtracts one pointer-sized
return-address slot while AArch64 subtracts zero before applying
record.stack_size. Update changelog.d/7350-statepoints-x86-64.md lines 13-20 to
document the separate CFA-to-SP calculations for both architectures, then
validate with cargo check -p perry --profile perry-dev.
🪄 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: 1e0ab59f-316d-4095-82e1-721225512af9
📒 Files selected for processing (3)
changelog.d/7350-statepoints-x86-64.mdcrates/perry-codegen/src/gc_map.rscrates/perry-runtime/src/gc/roots/stack_maps.rs
statepoints-refuse-x86 asserted that native roots REFUSE on x86-64. They work now, so that job would fail on its own success message — which said exactly what to do: 'add the x86-64 host to native-roots-rs4gc-aarch64 (rename it) and delete this job'. native-roots-rs4gc is now a two-host matrix: macos-14 for aarch64 + Mach-O, and ubuntu-latest for x86-64 + ELF. The toolchain step picks brew LLVM or the system/apt pair per host, and the liveness assertions read otool or readelf per object format. The Mach-O-only in-process step is gated to the macOS arm. ELF matters more than the arch here: every object-format bug in this design was ELF-only and invisible on Mach-O — SHF_GNU_RETAIN or --gc-sections drops the section, SHF_WRITE or the relocated addresses force a DT_TEXTREL, and eh_walker's asm used the Mach-O underscore convention. ARM64 Linux would cover the fourth corner, but those runners queue for hours here and its two components are each covered now.
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 @.github/workflows/gc-native-roots.yml:
- Around line 53-57: Complete the explanatory comment around the x86-64 matrix
entry by adding the missing action before “binary that crashes under
collection,” so the sentence is grammatically complete and accurately describes
what the old note claimed the run would do.
🪄 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: 551f1935-0a06-4e7d-b6b2-7b07a3ddbd01
📒 Files selected for processing (1)
.github/workflows/gc-native-roots.yml
Comment on lines
+53
to
+57
| # x86-64 was refused outright until #7349 taught the runtime to derive an | ||
| # SP-relative base from the CFA; it is a first-class arm of the matrix now. The | ||
| # old note said (#7324) that a run there would | ||
| # binary that crashes under collection, so an x86-64 run of this matrix would | ||
| # test nothing but the refusal — which is what `statepoints-refuse-x86` is for. | ||
| # test nothing but the refusal — no longer true, and that job is gone. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the incomplete documentation sentence.
Line 55 omits the action before “binary that crashes under collection.”
Proposed fix
-# old note said (`#7324`) that a run there would
+# old note said (`#7324`) that a run there would produce a📝 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
| # x86-64 was refused outright until #7349 taught the runtime to derive an | |
| # SP-relative base from the CFA; it is a first-class arm of the matrix now. The | |
| # old note said (#7324) that a run there would | |
| # binary that crashes under collection, so an x86-64 run of this matrix would | |
| # test nothing but the refusal — which is what `statepoints-refuse-x86` is for. | |
| # test nothing but the refusal — no longer true, and that job is gone. | |
| # x86-64 was refused outright until `#7349` taught the runtime to derive an | |
| # SP-relative base from the CFA; it is a first-class arm of the matrix now. The | |
| # old note said (`#7324`) that a run there would produce a | |
| # binary that crashes under collection, so an x86-64 run of this matrix would | |
| # test nothing but the refusal — no longer true, and that job is gone. |
🤖 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 53 - 57, Complete the
explanatory comment around the x86-64 matrix entry by adding the missing action
before “binary that crashes under collection,” so the sentence is grammatically
complete and accurately describes what the old note claimed the run would do.
iOS and iPadOS are aarch64 + Mach-O, the same shape as macOS. They did not work because the Mach-O loader, the unwinder module, the fp-chain walker and stack_top were each cfg(target_os = "macos"). On any other Apple platform that selected the no-section stub: loaded_stack_map_section() returned None, the index was empty, and the collector ran with NO native roots — silently, on the platforms hardest to debug. The compiler emitted the map; nothing read it. All four gates are now the same predicate: 64-bit Apple, or Linux. pthread_get_stackaddr_np is Apple-wide, and the mach2 dependency was widened to match the code using it — declaring it for fewer targets than the loader compiles on is how this stayed hidden. watchOS is refused deliberately: arm64_32 has 32-bit pointers while the map stores u64 addresses and the runtime does usize arithmetic on them. The check is ordered before the arm64 prefix test so it actually fires. Verified by building perry-runtime for aarch64-apple-ios, -ios-sim and -tvos. That is what found the hole: stack_top did not exist on iOS, so the build failed outright instead of quietly picking the stub. visionOS still fails in the third-party psm build script, unrelated. A device/simulator run is the verification this does not yet have.
proggeramlug
changed the title
…tchOS Two changes, one of them a bug CodeRabbit caught in the CFA derivation I added for x86-64. The return-address adjustment is NOT architecture-independent. x86-64 `call` pushes the return address, so the body stack pointer is CFA - 8 - stack_size. aarch64 `bl` writes it to x30 and pushes nothing, so it is CFA - stack_size. Subtracting the slot unconditionally shifted every SP-relative root by a word on aarch64 — and it would have stayed latent there, because chain_walkable is true on aarch64 so the fast x29 walker runs and this path is only the fallback. The probes passed 10/10 without ever exercising it. watchOS is no longer refused. The blocker was the map's function-address field being a fixed u64 while arm64_32 is ILP32; it now follows the target's pointer width, and the header's previously-reserved flags field records which width was used. The decoder asserts that against its own usize and refuses a mismatch, so a map built for the other width fails loudly instead of misreading every function address. Entries are 16 bytes on LP64, 12 on ILP32. Tests cover both widths: the emitter must produce .long/.quad and the matching flag, and the decoder must reject a blob whose recorded width disagrees. Local compile-verification for arm64_32 is still blocked by the third-party psm build script, which does not cross-compile in this environment — that is unrelated to this code, and the same failure blocks visionOS. Also fixes the incomplete sentence CodeRabbit flagged in the workflow header.
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)
crates/perry-runtime/src/gc/roots/stack_maps.rs (1)
896-913: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftDo not silently drop roots from unresolved CFA arithmetic.
checked_sub(...).and_then(...)rejects invalid SP base calculation withcontinue;walk_framecan then report successful statistics for the rest of native stack-map roots. A malformed map, ABI mismatch, or invalid frame can remove a live root from the scan. Fail the native-stack-walk path instead of continuing with a partial root set.🤖 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-runtime/src/gc/roots/stack_maps.rs` around lines 896 - 913, Update the SP-relative base calculation in walk_frame so failed CFA arithmetic propagates a native stack-walk failure instead of using continue. Preserve the checked subtraction, but return or propagate the function’s existing error result when it yields None, ensuring malformed frame data cannot produce successful statistics with a partial root set.Source: Coding guidelines
🧹 Nitpick comments (1)
crates/perry-runtime/src/gc/roots/stack_maps.rs (1)
848-851: 🩺 Stability & Availability | 🔵 TrivialVerify
_Unwind_GetCFAon every enabled Apple target.The new declaration is compiled for the expanded 64-bit Apple set. Build success does not prove that the symbol links and returns the expected CFA on iOS, tvOS, or visionOS. The supplied objectives also state that device and simulator execution remains unverified. Add a forced-evacuation probe that confirms nonzero unwinder walks and successful root relocation on each enabled target. The published
mach20.6.0 platform table lists macOS and iOS coverage, but not tvOS, visionOS, or watchOS. (docs.rs)As per coding guidelines, validate the Rust change with
cargo check -p perry --profile perry-dev.🤖 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-runtime/src/gc/roots/stack_maps.rs` around lines 848 - 851, Add a forced-evacuation GC probe covering every enabled Apple target, including device and simulator variants, that verifies _Unwind_GetCFA produces nonzero unwinder walks and that roots are relocated successfully; ensure the probe executes on iOS, tvOS, and visionOS rather than relying only on compilation. Then validate the Rust changes with cargo check -p perry --profile perry-dev.Sources: Coding guidelines, MCP tools
🤖 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 `@crates/perry-runtime/src/gc/roots/stack_maps.rs`:
- Around line 896-913: Update the SP-relative base calculation in walk_frame so
failed CFA arithmetic propagates a native stack-walk failure instead of using
continue. Preserve the checked subtraction, but return or propagate the
function’s existing error result when it yields None, ensuring malformed frame
data cannot produce successful statistics with a partial root set.
---
Nitpick comments:
In `@crates/perry-runtime/src/gc/roots/stack_maps.rs`:
- Around line 848-851: Add a forced-evacuation GC probe covering every enabled
Apple target, including device and simulator variants, that verifies
_Unwind_GetCFA produces nonzero unwinder walks and that roots are relocated
successfully; ensure the probe executes on iOS, tvOS, and visionOS rather than
relying only on compilation. Then validate the Rust changes with cargo check -p
perry --profile perry-dev.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 363ccef5-36c5-404b-8c75-072815f029e6
📒 Files selected for processing (4)
changelog.d/7349-apple-platforms.mdcrates/perry-codegen/src/gc_map.rscrates/perry-runtime/Cargo.tomlcrates/perry-runtime/src/gc/roots/stack_maps.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/perry-codegen/src/gc_map.rs
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
#7360) * test(gc): collect at stack depth, and gate walker liveness on every arm The native-root walker had no probe that made it work. Every probe in the suite calls `gc()` at the end, from a shallow stack, so on macOS and Linux `04_dead_after_deep_stack` reported 7 frames visited and **zero** root locations. Both arms would have passed unchanged with a walker that visited nothing at all -- other root sources covered the probes. Windows only walked a deep stack (5,626 frames) by accident of heap sizing, which is why the `--require-locations` gate could be applied there and nowhere else. `11_collect_at_depth` makes that coverage deliberate. `descend` holds a heap value live ACROSS its recursive call and collects at the deepest point, so at collection time there is one live root per frame, all of them mid-frame rather than in the leaf. Every slot is read after the collection returns, so a walker that stops early -- or a map with a wrong base register -- produces a wrong checksum, not merely a slower run. Under `PERRY_GC_FORCE_EVACUATE=1` every survivor moves, so a stale pointer cannot be accidentally right. Measured, byte-matching the pinned Node oracle: macOS aarch64 228 frames, 222 records, 221 locations (was 7 / 0) x86-64 Linux 231 frames, 221 locations (was 7 / 0) With both Unix arms now walking a real stack, `--require-locations` moves from the Windows-only branch to the shared path and gates all three. Full ratchet suite: 11/11 byte-identical to the oracle under `PERRY_RS4GC=1 PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1`. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF * docs: changelog fragment for #7359 Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF * ci(gc): add the aarch64-Linux arm, the one uncovered corner of the map The matrix covered aarch64+Mach-O, x86-64+ELF and x86-64+PE, and the note explaining the gap said ARM64 Linux was skipped because "its two components are each covered above." That is the exact compositional fallacy `word_width_for` in `gc_map.rs` exists to warn about. `.word` is not a fixed size -- GNU `as` defines it as the target's natural machine word -- so LLVM's AArch64 ELF backend spells every 32-bit stack-map field `.word`, while both covered arms spell it `.long`: Mach-O uses `.long` on aarch64, and on x86 `.word` means *two* bytes so LLVM will not use it for a 32-bit field. The directive width is a property of the intersection, not of either component. What this arm does and does not add, stated precisely, because overclaiming in this file is how #7321's wrong explanation survived into an issue and a job name: the `.word` spelling is already unit-tested on every arm, against a hand-written sample. What no arm has ever exercised is the end-to-end chain on this target -- real LLVM asm output, real ELF linking, real runtime walking -- where the failure mode is not a parse error but a wrong answer. `ubuntu-24.04-arm` is already in use in release-packages.yml and the repo is public, so the runners are available; the "queue for hours" half of the old rationale is stale too. Also repairs a garbled sentence in the header comment, left by an edit that spliced two clauses about the pre-#7349 x86-64 refusal. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF * docs(ci): the knob ledger said PERRY_STATEPOINT_REPORT was deleted; it is internal plumbing The env *spelling* was deleted under the kill policy (#7314) and the flag is the only entry point -- but the variable itself is still how the driver hands the format to the rayon module workers, and run_pipeline.rs remove_var's it when the flag is absent so an inherited value cannot switch reporting on. That block is a knob ledger. An entry reading "deleted" for a name still greppable in the tree makes the whole list look stale to the next auditor. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
…tform 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>
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.
PERRY_RS4GC=1was refused on anything but aarch64 (#7324). That refusal was correct — the collector segfaulted rather than reporting anything — but the cause turned out to be one unsupported call, not anything architectural.The bug
On x86-64 every root is
Indirect [RSP + off]— DWARF register 7. The unwinder path resolved bases with_Unwind_GetGR(context, reg), and_Unwind_GetGRis not a supported query for the stack-pointer column. It returned garbage, and the collector wrote through it._Unwind_GetCFAis the supported way to reach a frame's stack pointer.SP-relative roots now derive their base from the CFA: by the SysV/AAPCS definition the CFA is the caller's stack pointer immediately before the call, so this frame's body stack pointer sits one return-address slot plus the function's own frame below it — and
stack_sizeis exactly that frame, already recorded per function in the map.What this deliberately does not change
The format's base tags stay aarch64-literal. My first attempt made them architecture-relative;
gc_map.rsalready documents why that's wrong:That reasoning holds, and x86-64 roots already round-trip correctly through the explicit-register tag. So the architecture's SP number is a runtime-local constant used only to pick a base-resolution strategy — no format change, no agreement hazard.
Measured on real x86-64 Linux hardware
All ten gc-ratchet probes byte-match the pinned Node oracle under
PERRY_RS4GC=1 PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1, with.perry_gcmappresent in every binary.The walker demonstrably ran rather than passing vacuously:
fp_walks=0is correct — the fp-chain walker is aarch64-only, so the unwinder is the right path here. Evacuation genuinely moved objects (retained_forwarded_stub_objects=5).The same telemetry on aarch64 has the same shape —
7/0/1/1vs10/0/1/1on the same probes, and2locations on09_try_catch_rootson both. That equivalence is what makes this a result rather than a green light of unknown provenance.Caveat I want on the record
Those location counts are low on both platforms. The probes end in an explicit
gc()— a manual collection from a shallow stack — so precise native roots are lightly exercised by this suite regardless of architecture. That is a pre-existing gate weakness, not something this change introduces, and it means the honest claim is "x86-64 behaves identically to aarch64" rather than "x86-64 is heavily exercised".Strengthening that suite is worth doing separately: a probe that collects from a deep stack with many live roots, without a manual
gc(), would exercise the precise path on every platform.Update: Apple platforms (iOS, iPadOS, tvOS)
Same story as x86-64, different gate. iOS and iPadOS are aarch64 + Mach-O — the same shape as macOS, which already worked. They didn't, because the Mach-O loader, the unwinder module, the fp-chain walker and
stack_topwere each#[cfg(target_os = "macos")].On any other Apple platform that selected the no-section stub:
loaded_stack_map_section()returnedNone, the index came out empty, and the collector ran with no native roots at all — silently, on the platforms hardest to debug. The compiler emitted the map; nothing read it.All four gates are now one predicate: 64-bit Apple, or Linux.
pthread_get_stackaddr_npis Apple-wide, and themach2dependency was widened to match the code that uses it — declaring it for fewer targets than the loader compiles on is exactly how this stayed hidden.watchOS is refused, deliberately.
arm64_32has 32-bit pointers while the map stores function addresses asu64and the runtime doesusizearithmetic on them. The check is ordered before thearm64prefix test so it actually fires, and it refuses rather than emitting a map nothing can read.Verified by building
perry-runtimefor each target —aarch64-apple-ios,aarch64-apple-ios-sim,aarch64-apple-tvosall compile. That is what found the hole:stack_topdid not exist on iOS, so the build failed outright rather than quietly selecting the stub.aarch64-apple-visionosstill fails inside the third-partypsmbuild script, unrelated to this change.Honest limit: this is compile-level verification. A device or simulator run is the evidence it does not yet have, and I'd want that before anyone calls iOS supported rather than plumbed.
Summary by CodeRabbit
New Features
Bug Fixes