Fix interpreter delegate calls with large alignment of first arg #121936
Conversation
There is a problem with delegate calls when the first target arch is aligned to 16 bytes. One path to call the delegate removes the delegate obj from the argument list by moving the arguments on the interpreter stack by the size of the delegate obj slot. But in the problematic case, the stack slot of the delegate obj is followed by an unused slot that ensures the alignment of the first target argument that starts after it. Moving the arguments by the 8 bytes garbles the arguments and makes that unused slot part of the first target arg. Moreover, it also doesn't move the last 8 bytes of the last argument due to this. This change fixes it by ensuring that we move the arguments starting at the aligned location of the first target argument.
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the interpreter's delegate call handling when the first target method argument requires 16-byte alignment. The issue occurred because the code was incorrectly using INTERP_STACK_SLOT_SIZE (8 bytes) to calculate where to start moving arguments, but when 16-byte alignment is needed, there's an 8-byte padding slot between the delegate object and the first target argument. This caused argument corruption.
Key changes:
- In
compiler.cpp: Added logic to calculate and emit the correct offset of the first target argument (firstTargetArgOffset) based on its alignment requirement - In
interpexec.cpp: Updated the non-tail delegate call path to read and use this offset instead of the hardcodedINTERP_STACK_SLOT_SIZE
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/interpreter/compiler.cpp | Calculates and emits the alignment-based offset of the first target argument for INTOP_CALLDELEGATE instructions |
| src/coreclr/vm/interpexec.cpp | Reads the first target argument offset and uses it when shifting arguments to remove the delegate object from the argument list |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
There is some problem with the change, the interpreter test is failing in the CI. I need to investigate. |
|
I've forgotten to add the change in intops.def, that was the issue. Added now. |
|
/ba-g the failures are infra issue due to macos-13 brownout |
janvorli
deleted the
fix-delegate-calls-with-first-arg-large-alignment
branch
2 participants
There is a problem with delegate calls when the first target arch is aligned to 16 bytes. One path to call the delegate removes the delegate obj from the argument list by moving the arguments on the interpreter stack by the size of the delegate obj slot. But in the problematic case, the stack slot of the delegate obj is followed by an unused slot that ensures the alignment of the first target argument that starts after it. Moving the arguments by the 8 bytes garbles the arguments and makes that unused slot part of the first target arg. Moreover, it also doesn't move the last 8 bytes of the last argument due to this.
This change fixes it by ensuring that we move the arguments starting at the aligned location of the first target argument.
This fixes a large number of the libraries tests.