Fix dotnet/corefx#29266 #17734
Fix dotnet/corefx#29266 #17734
Conversation
|
@dotnet-bot test Ubuntu x64 corefx_baseline |
|
Nit: We try to have more descriptive commit titles than just "Fix #XXXXX". https://github.com/dotnet/coreclr/blob/master/Documentation/project-docs/contributing.md#commit-messages |
|
(This should fix the x64 problem, but there is probably still problem on ARM64.) |
Why arm64 only and not arm64, x86, and arm32? They are all treated the same now. Also note Reflection.Emit.ILGeneration.Tests was already passing on Arm64 w/o this change |
|
https://github.com/dotnet/coreclr/blob/master/src/vm/frames.h#L2960 This is under I was not able to reproduce the MacOS failure locally. It is intermittent failure. I suspect that some of these paths are executed rarely, e.g. only when the GC runs while the PInvoke is executing. |
Descriptive names requires understanding. I thought this was a simple temporary revert and could not determine a better name. Now it looks like this is required for other reasons more or less permanently, I can come up with a better description. |
src/vm/dllimport.cpp
Outdated
| @@ -2256,6 +2256,11 @@ void NDirectStubLinker::DoNDirect(ILCodeStream *pcsEmit, DWORD dwStubFlags, Meth | |||
| // for managed-to-unmanaged CALLI that requires marshaling, the target is passed | |||
| // as the secret argument to the stub by GenericPInvokeCalliHelper (asmhelpers.asm) | |||
| EmitLoadStubContext(pcsEmit, dwStubFlags); | |||
| #if defined(_TARGET_AMD64_) || defined(_TARGET_ARM64_) | |||
There was a problem hiding this comment.
Other places use _WIN64 around this, it may be nice to use it here as well.
src/vm/dllimport.cpp
Outdated
| @@ -2256,6 +2256,11 @@ void NDirectStubLinker::DoNDirect(ILCodeStream *pcsEmit, DWORD dwStubFlags, Meth | |||
| // for managed-to-unmanaged CALLI that requires marshaling, the target is passed | |||
| // as the secret argument to the stub by GenericPInvokeCalliHelper (asmhelpers.asm) | |||
| EmitLoadStubContext(pcsEmit, dwStubFlags); | |||
| #if defined(_TARGET_AMD64_) || defined(_TARGET_ARM64_) | |||
| // the secret arg has been shifted to left and ORed with 1 (see code:GenericPInvokeCalliHelper) | |||
There was a problem hiding this comment.
Tag the other places we have found that depend on this with see code:GenericPInvokeCalliHelper so that they can be discovered later if somebody wants to come back to this?
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
|
Nits are fixed. Tests are passing. |
|
Thanks! |
@jkotas @adityamandaleeka Given I do not have a OSX platform to test. This is my proposed fix.
Fixes dotnet/corefx#29266