Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable non-blittable struct returns on UnmanagedCallersOnly #45625

Merged

Conversation

jkoritzinsky
Copy link
Member

Now that #39294 is merged, we no longer need to require a stub when we return a non-primitive value type. This PR removes that restriction, which enabled using blittable value type returns with UnmanagedCallersOnly. This PR also enables passing the calling convention from UnmanagedCallersOnly to the JIT at both runtime and during crossgen with crossgen2. This enables the UnmanagedCallersOnly entrypoint to have the correct calling convention even though the entrypoint is technically managed.

This PR blocks crossgenning UnmangedCallersOnly methods in crossgen1 because the custom attribute parsing code is not currently included in crossgen1 and I didn't want to fight to get that hooked up since we're looking at deprecating/removing crossgen1 in the .NET 6 time frame anyway.

Fixes #35928.

@jkoritzinsky jkoritzinsky force-pushed the unmanaged-callers-only-valuetype-return branch from 07215bd to 1b3ed1a Compare December 5, 2020 00:50
src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs Outdated Show resolved Hide resolved
src/coreclr/src/vm/jitinterface.cpp Outdated Show resolved Hide resolved
src/coreclr/src/vm/jitinterface.cpp Outdated Show resolved Hide resolved
@ViktorHofer
Copy link
Member

// Auto-generated message

69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts.

To update your commits you can use this bash script: https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others.

@jkoritzinsky
Copy link
Member Author

This is ready for review.

…ndle all calling convention resolution and support extensible calling conventions.
@ghost
Copy link

ghost commented Dec 14, 2020

Hello @jkoritzinsky!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@jkotas
Copy link
Member

jkotas commented Dec 14, 2020

Could you please resolve the conflict?

@jkoritzinsky
Copy link
Member Author

Will do!

@sebastienros
Copy link
Member

I have been investigating a failing merge in aspnetcore, and isolated a repro by switching from runtimes

35fbaef
6.0.0-alpha.1.20614.9

and

c64861b
6.0.0-alpha.1.20614.10

The only commit in this range is from this PR.

Here is an example of error messages we get while running some functional tests with this runtime. All tests pass with the runtime build preceding this commit. There are other very similar ones, most related to SslStream, like "bad frame". This only happens on Linux and MacOS agents.

[xUnit.net 00:00:01.07]     DATA_Sent_TooSlowlyDueToOutputFlowControlOnMultipleStreams_AbortsConnectionAfterAdditiveRateTimeout [SKIP]
  Skipped DATA_Sent_TooSlowlyDueToOutputFlowControlOnMultipleStreams_AbortsConnectionAfterAdditiveRateTimeout [1 ms]
The active test run was aborted. Reason: Test host process crashed : Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
   at System.Net.Security.SslStream.<ThrowIfExceptional>g__ThrowExceptional|137_0(System.Runtime.ExceptionServices.ExceptionDispatchInfo)
   at System.Net.Security.SslStream+<ReadAsyncInternal>d__180`1[[System.Net.Security.AsyncReadWriteAdapter, System.Net.Security, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].MoveNext()
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[[System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Net.Security.SslStream+<ReadAsyncInternal>d__180`1[[System.Net.Security.AsyncReadWriteAdapter, System.Net.Security, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]], System.Net.Security, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].ExecutionContextCallback(System.Object)
   at System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[[System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Net.Security.SslStream+<ReadAsyncInternal>d__180`1[[System.Net.Security.AsyncReadWriteAdapter, System.Net.Security, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]], System.Net.Security, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].MoveNext(System.Threading.Thread)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[[System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Net.Security.SslStream+<ReadAsyncInternal>d__180`1[[System.Net.Security.AsyncReadWriteAdapter, System.Net.Security, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]], System.Net.Security, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].MoveNext()
   at System.Runtime.CompilerServices.TaskAwaiter+<>c.<OutputWaitEtwEvents>b__12_0(System.Action, System.Threading.Tasks.Task)
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore+ContinuationWrapper.Invoke()
   at System.Threading.Tasks.AwaitTaskContinuation.RunOrScheduleAction(System.Action, Boolean)
   at System.Threading.Tasks.Task.RunContinuations(System.Object)
   at System.Threading.Tasks.Task.FinishContinuations()
   at System.Threading.Tasks.Task`1[[System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].TrySetResult(Int32)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1[[System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].SetExistingTaskResult(System.Threading.Tasks.Task`1<Int32>, Int32)
   at System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder`1[[System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].SetResult(Int32)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.DuplexPipeStream+<ReadAsyncInternal>d__27.MoveNext()
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[[System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].ExecutionContextCallback(System.Object)
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(System.Threading.Thread, System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[[System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].MoveNext(System.Threading.Thread)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[[System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].ExecuteFromThreadPool(System.Threading.Thread)
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart()
   at System.Threading.ThreadHelper.ThreadStart_Context(System.Object)
   at System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object)
   at System.Threading.ThreadHelper.ThreadStart()

Do you think there could be a reason for this change to be responsible?

To reproduce, I cloned aspnetcore, updated the Versions.props file with specific runtime versions, and ran:

/src/Servers/Kestrel$ > ./build.sh; ../../../.dotnet/dotnet vstest ../../../artifacts/bin/InMemory.FunctionalTests/Debug/net6.0/InMemory.FunctionalTests.dll

@jkoritzinsky
Copy link
Member Author

Yes there was a bug in this change that we fixed last night. The next Maestro update should have the fix.

@sebastienros
Copy link
Member

Awesome, thanks for the quick feedback

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UnmanagedCallersOnlyAttribute returning non-primitive value types do not work
5 participants