Skip to content

fix(gc): fold the post-fp-setup stack allocation into the walker's frame base (#7328) - #7329

Merged
proggeramlug merged 1 commit into
mainfrom
fix/7328-fp-offset-trailing-sub
Aug 3, 2026
Merged

fix(gc): fold the post-fp-setup stack allocation into the walker's frame base (#7328)#7329
proggeramlug merged 1 commit into
mainfrom
fix/7328-fp-offset-trailing-sub

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Fixes #7328.

Root cause

The fast x29-chain stack-map walker derived a frame's base from add x29, sp, #imm alone, on the premise — stated in the function's own doc comment — that establishing the frame pointer is a function's last stack adjustment.

It is not. LLVM emits a further allocation after the add when a function has a large or separately laid-out local area:

stp  x29, x30, [sp, #0x90]
add  x29, sp, #0x90      ; fp established — decoder stopped here
sub  sp, sp, #0x170      ; body SP drops a further 368 bytes

So every slot in such a frame was read 368 bytes high.

Reproduced, then fixed

PERRY_STACKMAP_WALKER=verify on notry_control with collections forced:

left  (fast):     [...776, ...7376, ...7384, ...7392, ...7400, ...7408]
right (unwinder): [...776, ...7008, ...7016, ...7024, ...7032, ...7040]

The first slot agrees — that frame has no trailing sub — and the other five are each exactly +368, matching sub sp, sp, #0x170 found in the disassembly.

After the fix, the same command is clean, with movement asserted: eligible=true, copied_objects=28 per cycle. That distinction matters here — my first attempt reported 5/5 clean while PERRY_GC_DIAG showed eligible=false fallback=conservative_stack, i.e. no copying minor ran at all. PERRY_CONSERVATIVE_STACK_SCAN=off was needed to make copying eligible (#7255).

Why this is worse than a crash

It is a silent wrong answer. verify is not the default, so an ordinary run has nothing to disagree with the fast walker — it enumerates wrong addresses, the collector treats them as the root set, and live objects are missed. Forcing PERRY_STACKMAP_WALKER=unwind on the same probe raised objects copied from 23 to 110.

It affects both statepoint backends, since they share the walker.

The fix

Accumulate the contiguous run of sub sp, sp, #imm immediately following the add. A sub sp separated from that run is a body operation — a dynamic alloca, a call-argument area — whose effect the stack map's own slot offsets already carry, and is deliberately not folded in.

Verification

Four unit tests: the trailing-sub case, the unchanged common shape, a sub after the prologue run (must not be counted), and a leaf with no fp setup (must still fail closed so the caller falls back to the unwinder).

Sabotage-checked — neutralising the accumulation fails a_sub_after_the_fp_setup_is_included with "the trailing sub sp, sp, #0x170 must be added to the fp offset".

cargo fmt clean; file-size, GC store-site and addr-class gates green. (ci_public_baseline_check is expected-red — the artifact is deliberately stale during the current optimization work.)

Not verified

aarch64 only — the decoder is #[cfg(target_arch = "aarch64")] and x86-64 returns None (falls back to the unwinder), so there is no x86-64 path to regress. Whether this clears any of the 13 gap regressions the statepoint soak found is not measured here; #7327 (the bridge emitting no statepoint on invoke) is the more likely cause of the exception-shaped ones.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed AArch64 stack scanning for functions with contiguous stack-pointer adjustments after frame-pointer setup.
    • Improved detection of live objects during garbage collection by calculating stack locations correctly.
    • Preserved correct behavior for separated stack adjustments and functions without frame-pointer setup.
  • Tests

    • Added coverage for trailing adjustments, unchanged prologues, separated adjustments, and leaf functions.

@proggeramlug
proggeramlug merged commit b06b7c7 into main Aug 3, 2026
26 of 44 checks passed
@proggeramlug
proggeramlug deleted the fix/7328-fp-offset-trailing-sub branch August 3, 2026 18:19
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3ad23871-6c2b-4f85-8151-a3c58f5c5505

📥 Commits

Reviewing files that changed from the base of the PR and between 721180f and 6ff881e.

📒 Files selected for processing (2)
  • changelog.d/7329-fp-offset-trailing-sub.md
  • crates/perry-runtime/src/gc/roots/stack_maps.rs

📝 Walkthrough

Walkthrough

The AArch64 stack-map walker now includes contiguous sub sp adjustments after add x29, sp when calculating offsets. Tests cover valid prologues, separated adjustments, and missing frame-pointer setup. A changelog documents the fix.

Changes

AArch64 stack-map offset fix

Layer / File(s) Summary
Decode contiguous trailing stack adjustments
crates/perry-runtime/src/gc/roots/stack_maps.rs, changelog.d/7329-fp-offset-trailing-sub.md
The decoder accumulates contiguous post-frame-pointer sub sp, sp, #imm`` instructions and stops at the first unrelated instruction. The changelog records the address-calculation correction.
Validate offset-decoding boundaries
crates/perry-runtime/src/gc/roots/stack_maps.rs
AArch64 tests cover trailing stack allocation, unchanged prologues, separated body adjustments, and functions without frame-pointer setup.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

  • PerryTS/perry#7328: The issue describes incorrect slot addresses from the fast x29-chain stack-map walker, which this offset correction addresses.

Possibly related PRs

  • PerryTS/perry#7314: Both changes modify AArch64 frame-pointer-based native stack-map walking in crates/perry-runtime/src/gc/roots/stack_maps.rs.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/7328-fp-offset-trailing-sub

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

proggeramlug pushed a commit that referenced this pull request Aug 3, 2026
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fast x29-chain stack-map walker computes wrong slot addresses (5 of 6 slots off by 368 bytes)

2 participants