Fix interpreter delegate args alignment #122163
Merged
Conversation
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
The recent fix to handle alignment of the first argument of a delegate call when we were removing the delegate obj from the stack was not complete. It only handled the case when the first argument required 16 byte alignment. But the same issue exists if any of the args requires this alignment. This change fixes it by finding the first argument that requires 16 byte alignment and then moving the args before that as usual and moving that arg and all following ones to 16 byte aligned location. This preserves the alignment for the rest of the stuff too.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the interpreter's delegate argument alignment handling when removing the delegate object from the argument list. The previous fix only handled the case where the first argument required 16-byte alignment, but the issue also occurred when any argument required this alignment.
Key Changes:
- Modified the compiler to calculate the size of all arguments up to (but not including) the first 16-byte aligned argument
- Updated the runtime execution to properly shift arguments in two phases: non-aligned arguments first, then aligned arguments to their correct 16-byte boundaries
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/coreclr/interpreter/compiler.cpp | Changed from calculating offset of first target argument to calculating cumulative size of all target arguments before the first 16-byte aligned argument |
| src/coreclr/vm/interpexec.cpp | Modified argument shifting logic to use two separate memmove operations: one for non-aligned arguments and one for aligned arguments, preserving 16-byte alignment where required |
Contributor
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
davidwrighton
approved these changes
Dec 4, 2025
Member
davidwrighton
left a comment
davidwrighton
left a comment
There was a problem hiding this comment.
Copilot looks like it might have a point, but otherwise things look good.
Co-authored-by: Copilot <[email protected]>
am11
reviewed
Dec 4, 2025
BrzVlad
approved these changes
Dec 4, 2025
Member
Author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
The recent fix to handle alignment of the first argument of a delegate call when we were removing the delegate obj from the stack was not complete. It only handled the case when the first argument required 16 byte alignment. But the same issue exists if any of the args requires this alignment.
This change fixes it by finding the first argument that requires 16 byte alignment and then moving the args before that as usual and moving that arg and all following ones to 16 byte aligned location. This preserves the alignment for the rest of the stuff too.