fix(gc): decode lsl #12 frame adjustments in the aarch64 prologue walker - #7398
Merged
Conversation
…alker (#7394) `fp_to_sp_offset` masked bit 22 — the ADD/SUB (immediate) `sh` field — into its opcode comparison, so `sub sp, sp, #imm, lsl #12` did not match. LLVM emits that form for every frame ≥ 4 KiB, which generated functions cross routinely (80 in one gap-test binary). The dropped term was not the whole cost: a non-matching word also ends #7328's contiguous-`sub` accumulation run, so any further `sub sp` in the same prologue was dropped with it. `..._gc_call_argument_rooting_ts__run` resolved to fp-0x70 instead of fp-0x18B0, and the fast walker handed the collector slot addresses 6208 bytes off — which evacuation then wrote through. Reachable in the shipping configuration: RS4GC is the default root backend here and the x29 chain the default walker. The test printed `bad 1` under `PERRY_GC_HEAP_LIMIT=8` alone, conservative scan ON, where the `PERRY_RS4GC=0` build printed `bad 0` after evacuating 6344 objects.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe AArch64 stack-map walker now decodes ChangesAArch64 prologue decoding
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
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.
Fixes #7394.
What it is
fp_to_sp_offset(crates/perry-runtime/src/gc/roots/stack_maps.rs) decodes agenerated function's prologue to recover its body stack pointer, which the fast
x29-chain walker uses as the base for every SP-relative root slot in that frame.
Its
add x29, sp, #immandsub sp, sp, #immpatterns masked in bit 22 —the ADD/SUB (immediate)
shfield, which selectslsl #12on the immediate:So an instruction using the shifted form did not match the opcode comparison at
all. LLVM switches to
lsl #12the moment a frame needs 4 KiB or more, and agenerated function crosses that line routinely — each string-concat chain spills
its own
[32 x double]buffer. 80 functions in one gap-test binary carry ashifted frame adjustment.
The measured case
perry_fn_test_gap_gc_call_argument_rooting_ts__run, read out of the binary at+0x20:The dropped term is not the whole cost. Because the shifted
subfailed tomatch, it also terminated #7328's contiguous-
subaccumulation run, so thesub sp, sp, #0x840behind it was dropped too. The decoder reported0x70fora frame whose body SP is
0x18B0below the frame pointer, and the walker handedthe collector slot addresses 6208 bytes off.
Evacuation writes through the slots it is given, so this both missed live
roots and rewrote unrelated stack words.
It is reachable in the shipping configuration
RS4GC/statepoints is the default root backend wherever the runtime can walk
frames (
rs4gc_enabled(): "Default: on wherever the runtime can actually walkthe frames"), and the x29 chain is the default walker. This is not a
quarantine-only artifact:
PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0, conservative scan ONbad 1— a wrong answerPERRY_RS4GC=0(shadow stack)bad 0, while evacuating 6344 objectsPERRY_GC_HEAP_LIMITis a heap-size knob, not a correctness knob; it only makesthe collector run sooner. The shadow-stack arm is not vacuously clean — it ran a
real copying minor (
copied_objects=6344 copied_bytes=427648) in the same run.The fix
immediate_ofdecodesshfor both theaddand thesubforms, and bit 22comes out of both opcode masks (
0xFFC0_03FF→0xFF80_03FF).Verification
test_gap_gc_call_argument_rootingbad 1bad 0test_gap_gc_same_module_call_argument_rootingbad 0test_gap_gc_process_env_cache_rooting(RS4GC)bad 0Confirmed non-vacuous: the passing runs still report
[gc-copy-minor] ran copied_objects=6344.Four new decoder unit tests cover the shifted
sub, the shiftedadd, and themeasured two-
subprologue; the three #7328 tests are unchanged and still pass(7/7). Full gap suite run for regressions is reported in a comment below.
Deliberately not in scope
Two things the same investigation turned up that this PR does not claim to
fix, so they are not silently folded in:
PERRY_STACKMAP_WALKER=unwindstill returnsbad 1. Its SP-relative base(
_Unwind_GetCFAminus the recorded stack size) disagrees with the now-correctfast walk by exactly one frame size on both frames measured. That path is the
Fastwalker's fallback, so it matters; filed as GC (aarch64): the platform-unwinder stack-map walker lands one frame size below the correct SP #7399.test_gap_gc_process_env_cache_rootingstill faults on the shadow-stackbackend, which this fix does not touch — a separate defect.
Summary by CodeRabbit