[Arm64/Linux] Fix GenericPInvokeCalliHelper #17659
Conversation
src/vm/arm64/pinvokestubs.S
Outdated
| // | ||
| lsl \HiddenArg, \HiddenArg, #1 | ||
| orr \HiddenArg, \HiddenArg, #1 | ||
| .endif |
There was a problem hiding this comment.
This piece is needed for Windows as well. It also looks like it is needed for arm32. @adityamandaleeka has offered to take care of Windows for me.
There was a problem hiding this comment.
Since it is difficult for me to compile for Windows, I much appreciate the offer to do this. The *.asm syntax is different and it would take lots of tries. Thanks @adityamandaleeka
There was a problem hiding this comment.
I took a look at the code in dllimport where the corresponding right-shifting is being done. It's under !ARM and !x86. Maybe we can just make it explicitly for AMD64 only and avoid the shifting back and forth on ARM64 too?
There was a problem hiding this comment.
The comment in the amd64 pinvoke code is obsolete (refers to a removed feature). It may not needed there either.
There was a problem hiding this comment.
Nice 😄. After I sent the last comment, I was trying to find out who exactly is relying on this shifting/setting stuff.
There was a problem hiding this comment.
I can't find the right shift code in dllimport.
I do see this.
// The MethodDesc pointer may in fact be the unmanaged target, see PInvokeStubs.asm.
if (pMD == NULL || (UINT_PTR)pMD & 0x1)
There was a problem hiding this comment.
This is what I was referring to: https://github.com/dotnet/coreclr/blob/master/src/vm/dllimport.cpp#L2272
src/vm/arm64/pinvokestubs.S
Outdated
| @@ -98,7 +107,7 @@ PINVOKE_STUB VarargPInvokeStub, VarargPInvokeGenILStub, VarargPInvokeStubWorker, | |||
| // x15 = VASigCookie* | |||
| // x14 = Unmanaged target | |||
src/vm/arm64/PInvokeStubs.asm
Outdated
| @@ -126,7 +126,7 @@ __PInvokeGenStubFuncName SETS "$__PInvokeGenStubFuncName":CC:"_RetBuffArg" | |||
| ; x15 = VASigCookie* | |||
| ; x14 = Unmanaged target | |||
sdmaclea
force-pushed
the
PR-ARM64-Linux-Fix-GenericPInvokeCalli
branch
2 times, most recently
from
72001b8 to
091720a
Compare
| ; | ||
| shl PINVOKE_CALLI_TARGET_REGISTER, 1 | ||
| or PINVOKE_CALLI_TARGET_REGISTER, 1 | ||
|
|
There was a problem hiding this comment.
This should have been removed when FEATURE_INCLUDE_ALL_INTERFACES was removed in 1476776
| // | ||
| shl PINVOKE_CALLI_TARGET_REGISTER, 1 | ||
| or PINVOKE_CALLI_TARGET_REGISTER, 1 | ||
|
|
There was a problem hiding this comment.
This should have been removed when FEATURE_INCLUDE_ALL_INTERFACES was removed in 1476776
| } | ||
|
|
||
| #endif // _TARGET_X86_ | ||
|
|
There was a problem hiding this comment.
This should have been removed when FEATURE_INCLUDE_ALL_INTERFACES was removed in 1476776
|
test Ubuntu arm64 Cross Debug Innerloop Build |
src/vm/dllimport.cpp
Outdated
| @@ -2255,28 +2255,9 @@ void NDirectStubLinker::DoNDirect(ILCodeStream *pcsEmit, DWORD dwStubFlags, Meth | |||
|
|
|||
| #ifndef CROSSGEN_COMPILE | |||
There was a problem hiding this comment.
Nit: #ifndef CROSSGEN_COMPILE can be removed as well, I think.
sdmaclea
force-pushed
the
PR-ARM64-Linux-Fix-GenericPInvokeCalli
branch
from
091720a to
7ec08b6
Compare
When _WIN64 is defined vm relies on the secret arg being shifted left and orred with #1. Revert part of changes from dotnet#17659 to fix dotnet/corefx#29266 Fix arm64 to match amd64 Simplify dllimport.cpp
When _WIN64 is defined vm relies on the secret arg being shifted left and orred with #1. Revert part of changes from dotnet#17659 to fix dotnet/corefx#29266 Fix arm64 to match amd64 Simplify dllimport.cpp
When _WIN64 is defined vm relies on the secret arg being shifted left and orred with #1. Revert part of changes from dotnet#17659 to fix dotnet/corefx#29266 Fix arm64 to match amd64 Simplify dllimport.cpp
Fixes #17320
@jkotas @adityamandaleeka @janvorli PTAL