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.

Sign up for GitHub

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

Jump to bottom

Reverse P/Invokes should notify profiler when transitioning between unmanaged and managed #46177

Closed
jkoritzinsky opened this issue Dec 17, 2020 · 7 comments
Closed

Reverse P/Invokes should notify profiler when transitioning between unmanaged and managed #46177

jkoritzinsky opened this issue Dec 17, 2020 · 7 comments
Labels
area-Interop-coreclr
Milestone
6.0.0

Comments

@jkoritzinsky
Copy link
Member

Today, the runtime notifies the profiler when an unmanaged<->managed transition happens in the following scenarios:

  • P/Invokes on all platforms
  • COM->CLR calls on Windows
  • CLR->COM calls on Windows
  • Reverse P/Invokes on Windows x86 only

We should either notify the profiler about transitions for all Reverse P/Invokes or none (probably all).

cc: @noahfalk @AaronRobinsonMSFT
@jkoritzinsky jkoritzinsky added this to the 6.0.0 milestone Dec 17, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Dec 17, 2020
@AaronRobinsonMSFT
Copy link
Member

/cc @tommcdon @davmason

@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Dec 17, 2020
@davmason
Copy link
Member

Thanks for reporting this, if you have any other context could we capture that? Do you know why we only notify on x86 for reverse pinvokes? Is it due to IL stubs not being on x86?

@jkoritzinsky
Copy link
Member Author

On x86 we use a dynamically emitted assembly stub that wraps the IL stub. That assembly stub is what emits the profiler notifications. On other platforms we just use the IL stubs, so nothing is calling the profiler.

@jkotas
Copy link
Member

jkotas commented Dec 17, 2020

I assume that you mean UnmanagedCallersOnly methods. Is that right?

Marshaled delegates that are sometimes also called reverse PInvoke should be fine.

@jkoritzinsky
Copy link
Member Author

From what I could tell both delegate reverse pinvoke and UnmanagedCallersOnly reverse pinvoke are the same for this case.

@jkotas
Copy link
Member

jkotas commented Dec 17, 2020

FWIW, DirectStubLinker::EmitProfilerBeginTransitionCallback is supposed to handle the delegate reverse pinvoke case.

jkoritzinsky added a commit to jkoritzinsky/runtime that referenced this issue Dec 18, 2020
@jkoritzinsky
Add SEH hookup and profiler/debugger hooks to Reverse P/Invoke entry …
d54420b 
…helper to match custom x86 thunk.

Fixes dotnet#46177
@jkoritzinsky jkoritzinsky mentioned this issue Dec 19, 2020
Closed
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 19, 2020
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 20, 2021
MatthewSteeples pushed a commit to MatthewSteeples/runtime that referenced this issue Jan 20, 2021
@jkoritzinsky
Remove extra UnmanagedCallersOnly overhead on x86 (dotnet#46238)
5ec0c7a 
* Implement emitting an unmanaged calling convention entry point with the correct argument order and register usage on x86.

* Move Unix x86 to the UnmanagedCallersOnly plan now that we don't need to do argument shuffling.

* Add SEH hookup and profiler/debugger hooks to Reverse P/Invoke entry helper to match custom x86 thunk.

Fixes dotnet#46177

* Remove Windows x86 assembly stub for individual reverse p/invokes. Move Windows x86 unmanaged callers only to not have extra overhead and put reverse P/Invoke stubs for Windows x86 on the UnmanagedCallersOnly plan.

* Further cleanup

* Remove extraneous UnmanagedCallersOnly block now that x86 UnmanagedCallersOnly has been simplified.

* Undo ArgOrder size specifier since it isn't needed and it doesn't work.

* Fix copy constructor reverse marshalling. Now that we don't have the emitted unmanaged thunk stub, we need to handle the x86 differences for copy-constructed parameters in the IL stub.

* Fix version guid syntax.

* Remove FastNExportHandler.

* Revert "Remove FastNExportHandler."

This reverts commit 423f70e.

* Fix setting up entry frame for new thread.

* Allow the NExportSEH record to live below ESP so we don't need to create a new stack frame.

* Fix formatting.

* Assign an offset for the return buffer on x86 since it might come in on the stack.

* Make sure we use the TC block we just put in on x86 as well.

* Shrink the ReversePInvokeFrame on non-x86 back to master's size.

* Fix arch-specific R2R constant.

* Pass the return address of the ReversePInvokeEnter helper to TraceCall instead of the entry point and call TraceCall from all JIT_ReversePInvokeEnter* helpers.

* Fix ILVerification and ILVerify

* fix R2R constants for crossgen1

* Don't assert ReversePInvokeFrame size for cross-bitness scenarios.
@jkoritzinsky
Copy link
Member Author

Fixed by #46238

@ghost ghost locked as resolved and limited conversation to collaborators May 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
area-Interop-coreclr
Projects
None yet
Milestone
6.0.0
Development

Successfully merging a pull request may close this issue.

Remove extra UnmanagedCallersOnly overhead on x86 jkoritzinsky/runtime
5 participants
@jkoritzinsky @jkotas @davmason @AaronRobinsonMSFT @Dotnet-GitSync-Bot