Fix #23576: Fiber switch hides callee-saved FP registers from the GC - #23577
Open
ljmf00-wekaio wants to merge 3 commits into
Open
Fix #23576: Fiber switch hides callee-saved FP registers from the GC #23577ljmf00-wekaio wants to merge 3 commits into
ljmf00-wekaio wants to merge 3 commits into
Conversation
The ARM and AArch64 sections of switch_context_asm.S wrap their symbol names in CSYM(...) to handle the platform-dependent C symbol prefix (an underscore on Mach-O), but the macro was never defined in this file, so the file failed to assemble as soon as those sections were reached. It went unnoticed because no build so far compiled the file for ARM targets; druntime's Makefile assembles it with plain $(CC) -c and passes no extra defines, and this is a standalone .S with no includes. Add the __USER_LABEL_PREFIX__-based prelude these sections were written against - the same one ldc's threadasm.S (where they were originally ported from) has always carried - right after the GNU-stack note, so every architecture section below can rely on it. This is a prerequisite for building and testing the fix for issue 23576 on AArch64. Co-Authored-By: Claude <noreply@anthropic.com>
…he GC On AArch64 and LoongArch64, fiber_switchContext saves the callee-saved FP registers (d8-d15 / fs0-fs7) and the return address on the fiber's stack, but reported sp + 9*8 as the saved stack top (tstack), keeping those slots below the range the GC scans. The FP registers are callee saved, so the compiler may keep a live GC pointer in one of them across the suspension point (e.g. fmov d8, x0). With its only copy hidden from the scan, the object is collected while the fiber is suspended; resuming then works with freed memory. Report the true sp as tstack instead, so the whole save area is scanned. tstack is used symmetrically as the save slot on switch-out and the resume pointer on switch-in, so the restore side drops its sp adjustment in lockstep. initStack must follow the same convention: reserve the FP register slots below the trampoline return address and zero them, both to give the first switch-in a valid frame and because the slots are now GC-scanned and stale words from a previous fiber on a reused stack must not look like pointers. Scanning the saved return address as well is harmless: it is a code address, which a conservative scan ignores, and excluding it would need a save-layout reshuffle for no benefit. On LoongArch64 this also drops the dead slack word the old initStack reserved (11 GPR slots for the asm's 10) and, because the 19-word save area is odd-sized, moves the sp at fiber entry from bstack - 8 to bstack, making it 16-byte aligned as the psABI requires. Note that old-convention fiber_switchContext objects and new-convention initStack (or vice versa) crash on the first context switch; the asm and the D side must be rebuilt together. Co-Authored-By: Claude <noreply@anthropic.com>
Covers issue dlang#23576 on both affected architectures. The test holds the sole reference to a canary object (with a 4 MB payload) in the first callee-saved FP register (d8 on AArch64, fs0 on LoongArch64) across a yield, XOR-obfuscating every other copy and scrubbing the stack and scratch registers, forces collections and heap churn from the main context, then resumes and asserts the canary survived with its payload intact. On a druntime without the fix the canary is collected while the fiber is suspended and the test fails its assertion. The register-holding helper is ABI-conformant assembly: like any function using a callee-saved register, it saves the caller's copy in its prologue and restores it in its epilogue, keeping the pointer in the register across the call to yield - exactly the code shape a compiler emits when it allocates a value to a callee-saved FP register across a call. It lives in a separate assembly file because no source-level construct can force a register allocator to pick a specific register (so relying on compiler codegen would make the test flaky across compiler versions and tuning flags) and inline asm would tie the test to one compiler's dialect; the file compiles to an empty object on other architectures and the test itself reduces to an empty main there. Co-Authored-By: Claude <noreply@anthropic.com>
| { | ||
| // Like others, FP registers and return address ($r1) are kept | ||
| // below the saved stack top (tstack) to hide from GC scanning. | ||
| // Unlike others, the FP registers and return address ($ra) are |
Contributor
There was a problem hiding this comment.
did you intend to update LoongArch?
Author
There was a problem hiding this comment.
As commented in the same fiber switch code, it looks like that it suffers from the same issue. Indeed I don't have coverage for LoongArch. I mention that on the comments I added below.
Author
There was a problem hiding this comment.
I can drop the LoongArch changes and let someone that knows more about it handle it better.
ljmf00-wekaio
force-pushed
the
fix-fiber-switch-aarch64-gc-fp-regs
branch
from
August 12, 2026 05:10
5678a96 to
79563ec
Compare
ljmf00-wekaio
commented
Aug 12, 2026
Comment on lines
+1213
to
+1218
| // Only need to set return address ($ra). Everything else is fine | ||
| // zero initialized. | ||
| pstack -= size_t.sizeof * 11; // skip past space reserved for $r21-$r31 | ||
| push(cast(size_t) &fiber_entryPoint); | ||
| pstack += size_t.sizeof; // adjust sp (newp) above lr | ||
| pstack -= size_t.sizeof * 10; // skip past $s0-$s8 and $fp | ||
| push(cast(size_t) &fiber_entryPoint); // see switch_context_asm.S for docs | ||
| pstack -= size_t.sizeof * 8; // reserve $fs0-$fs7 | ||
| (cast(size_t*) pstack)[0 .. 8] = 0; // now GC-scanned; clear stale data on fiber reuse |
Author
There was a problem hiding this comment.
This part I'm not entirely sure as I'm not familiar with LoongArch64 and its ABI spec, but from judgment off of switch context code, it suffers from the same issue ARM switch code had.
| // ... | ||
| // 9: x29 (fp) <-- newp tstack | ||
| // 9: x29 (fp) | ||
| // 8: x30 (lr) [&fiber_entryPoint] |
Author
There was a problem hiding this comment.
Arguably, lr wouldn't require to be included, as GC doesn't allocate executable code, although, if someone deliberately remap with execution flags, the GC wouldn't conservatively track it. Regardless of that being legal for the perspective of a user of the GC, I would say its harmless to support this or just conservatively scan lr stored pointers.
An alternative would require reordering the layout of the save which could be less efficient.
ljmf00-wekaio
marked this pull request as ready for review
August 12, 2026 05:13
ljmf00
added
Review:Industry
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
3 participants
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.
No description provided.