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 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