fix(gc): walk legal 8-mod-16 frame records, and stop moving unwinder roots a frame - #7400
Merged
Merged
Conversation
…roots a frame Two independent defects in the native-root walkers, both aarch64, both invisible until the ELF arm could finally run a probe (#7392). 1. The x29 chain walk required a 16-byte-aligned frame record. AAPCS64 does not promise that — it fixes the record's contents and leaves its placement in the frame unspecified — and LLVM's AArch64 ELF frame lowering puts the x29,x30 pair below the other callee-saved GPRs, so an odd number of those lands it 8 mod 16. Measured in a real runtime frame (x19..x23 + v8 saved, CFA = x29+56). Darwin pins the record to the top of the frame, which is why this never fired on macOS. The check treated the legal record as corruption and abandoned the walk mid-stack. 2. The unwinder fallback it abandoned into resolved SP-relative roots against `CFA - stack_size`. Inside an `_Unwind_Backtrace` callback the CFA already IS the stack pointer of the frame whose return address `_Unwind_GetIP` reports, so the subtraction placed every such root one whole frame too low. Verified directly on 02_survivor_promotion: at the CFA the slot holds a NaN-boxed pointer (0x7ffd...), one frame below it holds a stack address. Together: on aarch64-Linux the chain walk bailed, the fallback then rewrote the wrong words, the frame's real roots were never updated after an evacuation, and the mutator dereferenced a stale from-space pointer — the SIGSEGV in #7392. The CFA semantics are pinned by a new test that records each frame's real stack pointer and matches it against a live walk; it passes on aarch64 Linux (libgcc), aarch64 macOS (Apple libunwind) and x86-64 Linux, so `CFA_RETURN_ADDRESS_BYTES` had no case left to serve and is deleted. Neither walker was exercised anywhere: nothing in the tree ever set PERRY_STACKMAP_WALKER, though the workflow's knob ledger claimed an arm for it. gc-native-roots now runs every probe under `unwind`, and under `verify` on aarch64 where the fp-chain walk exists, each byte-diffed against the pinned Node oracle. Measured on aarch64-Linux: 2 of 11 probes passed all three walkers before this change, 11 of 11 after.
proggeramlug
pushed a commit
that referenced
this pull request
Aug 4, 2026
|
Warning Review limit reached
Next review available in: 34 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe runtime corrects SP-relative unwind root resolution and frame alignment validation. It adds unwind-contract tests and extends the native-root workflow to verify non-default walkers against pinned probe outputs under forced evacuation. ChangesNative GC root walkers
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant ProbeBinaries
participant NativeWalker
participant PinnedOracles
Workflow->>ProbeBinaries: select walker and run probes
ProbeBinaries->>NativeWalker: execute with forced evacuation
NativeWalker-->>Workflow: return probe output
Workflow->>PinnedOracles: compare output
PinnedOracles-->>Workflow: report match or mismatch
Possibly related issues
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 |
proggeramlug
force-pushed
the
fix/7392-rs4gc-survivor-promotion
branch
from
August 4, 2026 20:35
04580cd to
2bd184a
Compare
… the mask off aarch64 `stack_pointer()` as a function is inlined only once the optimiser is on, so at the `opt-level=0` `cargo test` uses it reported its own frame — every recorded value off by one frame, and the contract test failed for a reason unrelated to the walker. A macro expands at the call site, which removes the question. `FRAME_RECORD_ALIGN_MASK` is read only by the fp-chain walker, which exists on aarch64 Unix alone; carry that exact cfg so an x86-64 or Windows build does not warn on a constant it has no walker for.
![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 @.github/workflows/gc-native-roots.yml:
- Around line 439-454: Strengthen the non-default walker loop around the probe
execution and oracle diff so each selected mode is proven to run: execute a
traced deep-stack probe for every `$mode`, then validate telemetry identifies
`$mode`, reports nonzero frames, records, and locations, and includes an
evacuation. Preserve the existing all-probe oracle comparisons and failure
handling, and keep the final `$checked` assertion.
🪄 Autofix
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: 0d080ee3-2e9d-4bcf-b8a7-ca2e3c1d7f9d
📒 Files selected for processing (4)
.github/workflows/gc-native-roots.ymlchangelog.d/7400-native-root-walkers-aarch64.mdcrates/perry-runtime/src/gc/roots/stack_maps.rscrates/perry-runtime/src/gc/roots/stack_maps_unwind_contract.rs
Oracle-diffing every probe proves a process exited zero and printed the right bytes. It does not prove PERRY_STACKMAP_WALKER selected the walker under test, that the walker reached a mapped frame, or that anything moved — and all three modes are meant to produce identical output, so program output cannot tell them apart. That is the shape of #7392 itself. Per mode, off one traced run of 11_collect_at_depth: fp_walks == 0 proves 'unwind' took effect and > 0 proves 'verify' cross-checked something, --require-locations proves frames were stepped and roots enumerated, and the evacuation assert proves a copying minor moved objects. Measured on aarch64-Linux: unwind fp_walks=0, verify fp_walks=5, both with 663 locations and 6015 objects copied; and the negative control — judging a default-walker trace as if it were 'unwind' — fails, so the assert can.
This was referenced Aug 4, 2026
GC (aarch64): the platform-unwinder stack-map walker lands one frame size below the correct SP
#7399
Closed
proggeramlug
added a commit
that referenced
this pull request
Aug 5, 2026
* ci(gc): the macOS in-process RS4GC arm could never pass `RS4GC works on a stock toolchain via the in-process backend` asserts that a copying minor actually moved objects, by counting `[gc-copy-minor] ran copied_objects=` lines in the probe's stderr. Both of those prints are gated on PERRY_GC_DIAG (gc/copying.rs:993 and :1246), and this step never set it -- the sibling walker step does. So the trace held nothing but the probe's own #gcmetric lines, the assert read 0 copying minors / 0 objects copied off an effectively empty file, and the step failed regardless of how the collector behaved. The inverse of the usual hazard: not a gate that cannot fail, but one that cannot PASS. It shipped with the step in #7339 and had never executed, because three of the four arms in this matrix were permanently queued until #7393 added a concurrency group. Also makes the assert say what actually happened. A trace with no [gc-*] diagnostics at all is indistinguishable, by counts alone, from a collector that moved nothing, and the old message asserted the latter. That misdiagnosis is what made this cost a build to identify. Reproduced on macOS aarch64 against current main (cd29706, which contains #7398 and #7400, so it was not already fixed): the step as written reproduces the CI error byte-for-byte with a 3-line stderr; the same binary with PERRY_GC_DIAG=1 reports 2 copying minors / 10892 objects copied and the full step exits 0, stdout unchanged so the control diff still holds. The new branch is capable of failing: it fires on the pre-fix trace, passes on the post-fix one, and two negative controls (diagnostics present but zero copies; a synthetic manual_collect trace) still fail with the original message. This does not make the workflow green -- the other three arms fail earlier in "Probe matrix" for unrelated reasons. * docs: changelog fragment for #7414 --------- 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.
Closes #7392.
What the segfault was
Reproduced on a real aarch64-Linux host (Ubuntu 24.04 arm64, LLVM 22.1.8, Node 26.5.1 — the CI arm's toolchain), then localised by mode:
PERRY_RS4GC=1 PERRY_GC_FORCE_EVACUATE=1(default walker)PERRY_STACKMAP_WALKER=unwindPERRY_STACKMAP_WALKER=verifyPERRY_RS4GC=1without forced evacuationSo it was not statepoint lowering: it was the walkers. Two separate defects, each of which needs the other to become a crash.
1. A legal frame record rejected as corruption
The x29 chain walk required
fp & 0xF == 0. AAPCS64 §6.4.6 fixes what a frame record contains and leaves where it sits in the frame unspecified; only SP has to be 16-byte aligned. LLVM's AArch64 ELF frame lowering puts thex29,x30pair below the other callee-saved GPRs, so an odd number of those lands the record 8 mod 16. From the.eh_frameof the runtime frame that actually tripped it:x29 = CFA - 56, and CFA is 16-aligned, sox29 ≡ 8 (mod 16). Darwin pins the record to the top of the frame, so x29 is always 16-aligned there and this check could never fire on macOS.The walk treated it as a corrupt chain and bailed to the platform unwinder — which is where the second defect lives.
2. The unwinder fallback put every SP-relative root one frame too low
It computed
CFA - stack_size. That follows DWARF's definition of a CFA (the caller's SP), and it is wrong for what_Unwind_GetCFAhands back: inside an_Unwind_Backtracecallback the CFA already is the stack pointer of the frame whose return address_Unwind_GetIPjust reported.Measured in-process on
02_survivor_promotion, same collection, same record, same frame — the word at each candidate address:0x7ffd…is POINTER_TAG. The chain walk's address holds the root; the unwinder's holds a stack address and a code address, 240 bytes (this frame'sstack_size) away.The CFA semantics are not assumed here — a standalone probe recorded each frame's real SP and matched it against a live walk on three platforms:
_Unwind_GetCFA== frame's own SPSame answer regardless of whether the return address is pushed, so
CFA_RETURN_ADDRESS_BYTEShad no case left to serve and is deleted. That probe is nowunwind_cfa_is_the_frames_stack_pointer, incargo-teston every host.Why it crashed only here
02_survivor_promotionkeeps ~40k survivors live across forced evacuation. The chain walk bailed at the 8-mod-16 record, the fallback rewrote the wrong words, the frame's real roots kept their from-space pointers, and the mutator dereferenced one.01_nursery_churnhas a zero live set atgc(), which is why it survived and the arm's failure looked like "02 specifically".The gate that should have caught it
PERRY_STACKMAP_WALKERhad no arm anywhere in the tree — nothing ever set it — while the knob ledger at the top ofgc-native-roots.ymlclaimed one. Both non-default walkers were broken the whole time, andverifyis the only check that can catch a wrong base at all, since nothing downstream knows what a root slot should contain.Added: every probe under
unwind, plusverifyon the aarch64 arms (it needs the fp-chain walk to exist), each byte-diffed against the pinned Node oracle.Measured, aarch64-Linux, 11 probes × 3 walkers, oracle-diffed
main@ d5c0c65Before, beyond the reported crash:
09_try_catch_rootsalso diverged from the oracle under the default walker,02diverged underunwind, and 9 of 11 aborted underverify.cargo test -p perry-runtime gc::roots::stack_maps— 19 passed on aarch64 Linux and on aarch64 macOS.Notes for the reviewer
RS4GC forced-evacuation matrix: 11/11onmacos-14. The question RS4GC: 02_survivor_promotion segfaults under forced evacuation on aarch64-ELF #7392 opens with is answered — the crash was ELF-only in effect, but defect 2 is platform-independent and was simply unreachable on Darwin.PERRY_GC_SAFEPOINT_ONLYhas no arm either (the ledger comment now says so instead of claiming one), andscripts/check_file_size.shis red onmainatcrates/perry-runtime/src/object/field_set_by_name.rs(2048 lines), unrelated to this change..perry_gcmap-missing compile of01_nursery_churnappeared in one of four full local matrix runs and did not reproduce (two direct retries, two further full runs, all clean). It is a compiler/object-cache question, not a runtime one; flagging it rather than leaving it unsaid.Summary by CodeRabbit
Bug Fixes
Validation