Fix passing too small args for PUTARG_STK on macOS arm64 ABI #68108
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsFix #66720 I got far enough that I figured I would submit what I think is a good fix. cc @dotnet/jit-contrib PTAL @AndyAyersMS
|
| emit->emitIns_S_R(INS_str, storeAttr, srcReg, varNumOut, argOffsetOut); | ||
| argOffsetOut += EA_SIZE_IN_BYTES(storeAttr); | ||
| } | ||
| assert(argOffsetOut <= argOffsetMax); // We can't write beyond the outgoing arg area | ||
| return; | ||
| } | ||
|
|
||
| var_types slotType = genActualType(source); | ||
| if (compMacOsArm64Abi()) |
There was a problem hiding this comment.
@jakobbotsch, just for my own education; is it specific to mac ABI? i.e. if we apply the same classification to linux and windows arm64, it will break putarg on those targets?
This TODO is probably related:
runtime/src/coreclr/jit/morph.cpp
Lines 736 to 738 in d682d68
There was a problem hiding this comment.
macOS arm64 ABI is different in that multiple small arguments can be packed into the same stack slot. It means we have different behavior in some places in the JIT, and that different behavior was wrong here.
The behavior for the other ABIs did not have this bug.
|
/backport to release/6.0 |
|
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/2177458293 |
|
@jakobbotsch backporting to release/6.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Fix passing too small args for PUTARG_STK on macOS arm64 ABI
Using index info to reconstruct a base tree...
M src/coreclr/jit/codegenarmarch.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/codegenarmarch.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/codegenarmarch.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix passing too small args for PUTARG_STK on macOS arm64 ABI
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
| emit->emitIns_S_R(INS_str, storeAttr, srcReg, varNumOut, argOffsetOut); | ||
| argOffsetOut += EA_SIZE_IN_BYTES(storeAttr); | ||
| } | ||
| assert(argOffsetOut <= argOffsetMax); // We can't write beyond the outgoing arg area | ||
| return; | ||
| } | ||
|
|
||
| var_types slotType = genActualType(source); |
There was a problem hiding this comment.
do I understand this change correctly that now we have such slot types:
1byte - TYP_BYTE
2bytes - TYP_SHORT
4bytes - TYP_LONG (was TYP_INT before the change).
8bytes - TYP_LONG
?
so if we pass two TYP_INT on the stack, in one 8byte stack slot, don't we override anything?
There was a problem hiding this comment.
Not totally sure I understand. genActualType(TYP_INT) returns TYP_INT, not TYP_LONG.
The difference before was that we used the real type of the operand to decide on how much to store on the stack. That's not right, a small-typed operand still produces a value of type genActualType(operand) and it is the PUTARG_STK node that decides how much should be put on the stack. So we had a problem for e.g. a TYP_SHORT indirection feeding into a TYP_INT parameter.
A TYP_INT indirection feeding into a 4 byte parameter will still only store 4 bytes on the stack.
ghost
locked as resolved and limited conversation to collaborators
Fix #66720
I got far enough that I figured I would submit what I think is a good fix.
cc @dotnet/jit-contrib PTAL @AndyAyersMS