[AArch64][Windows] Fix swift async context slot placement - #13571
Draft
sebmarchand wants to merge 2 commits into
Draft
[AArch64][Windows] Fix swift async context slot placement#13571sebmarchand wants to merge 2 commits into
sebmarchand wants to merge 2 commits into
Conversation
On Windows, assignCalleeSavedSpillSlots allocated the swift async context object as the first callee-save frame index, before the loop that creates the register slots. Every other target creates it inside that loop, immediately below the FP slot. The Windows placement left MachineFrameInfo believing the slot sits above the frame record, while computeCalleeSaveRegisterPairs reserves an expanded 24-byte slot and makes the prologue store it below the record at FP-8. The two views disagreed by eight bytes, which left an unclaimed range in the middle of what MachineFrameInfo considered the callee-save block. PrologEpilogInserter's stack slot scavenger then handed that range to a live local, placing it on the saved caller frame pointer: the function stored through [x29], reloaded it, and the epilogue restored the corrupted value before returning. Scavenging is gated on the optimization level, so this only reproduced at -O2 and above. Create the object inside the callee-save loop so both views agree. The CSI array is only reversed on the needsWinCFI path while the async block was keyed on isTargetWindows, and those differ for a nounwind funclet; inserting the object between the two members of the FP/LR pair would violate the frame index adjacency that computeCalleeSaveRegisterPairs asserts, so restrict the in-loop creation to the reversed case. The remaining path is unaffected because there the alignment gap is created by setObjectAlignment on a callee-save object, making it padding rather than a range the scavenger can claim. The two updated tests pinned literal offsets; both still hold their stated invariants, and the frame in the slot-offset test shrinks from 64 to 48 bytes now that the hole no longer needs padding.
CHECK-NOT passes silently if the store comes back in a form the pattern does not match, such as a paired store or a different register, which is the regression this test exists to catch. Generating the assertions locks the whole frame layout instead, so any object placed at or above the frame record shows up as a diff. This matches the test in the upstream version of the fix.
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.
This is the Windows ARM64 miscompile from swiftlang/swift#90920, where an async continuation returns and the caller carries on with a garbage frame pointer.
On Windows
assignCalleeSavedSpillSlotscreates the swift async context object before the callee-save loop instead of inside it next to the FP slot. MachineFrameInfo then records it above the frame record while the prologue stores it below at FP-8, and that 8 byte disagreement leaves a hole inside what MachineFrameInfo thinks is the callee-save area. PEI's stack slot scavenger hands the hole to a live local, which ends up with the address of the saved caller x29. Only reproduces at -O2 and above because scavenging is gated on the optimization level.This creates the object inside the loop on the needsWinCFI path so the two agree there. It is narrower than what I put up upstream (llvm#212922), which drops the special case entirely, because only that path reverses the CSI array and keying on FP without the reversal would insert the object between the FP/LR pair.
The other path still has the same problem, exposing the saved x19 instead of the frame record. Covering both would need f406210 backported, and that touches AArch64PrologueEpilogue.cpp which does not exist on this branch. Given stable/* is supposed to take cherry-picks rather than original fixes, this probably should not land as its own patch and is better held until the upstream change is in and then adapted. Happy to close it if that is the preference.