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

[Mono][jit] Emit GC transitions for "calli unmanaged" instructions #46491

Merged
merged 11 commits into from
Jan 6, 2021

Conversation

lambdageek
Copy link
Member

@lambdageek lambdageek commented Dec 31, 2020

  • Add mono_marshal_get_native_func_wrapper_indirect
    This will be used to add a wrapper around calli <sig> indirect calls where sig has an unmanaged calling convention.

    We need to do a GC transition before calling an unmanaged function from managed. So this wrapper does it.

    This is reusing much of the code implemented for mono_marshal_get_native_func_wrapper_aot which is used for delegates.

  • Treat CallConv bit 0x09 as MONO_CALL_UNMANAGED_MD
    This is the C#9 function pointers "unmanaged"+ext calling convention.
    Additional calling convention details are encoded in the modopts of the return type.

    This PR doesn't handle the modopts yet

  • Rewrite mono_marshal_emit_native_wrapper to take a new MonoNativeWrapperFlags argument instead of a collection of bools. Add a new EMIT_NATIVE_WRAPPER_FUNC_PARAM_UNBOXED value. Add support to emit a wrapper that uses an unboxed function pointer as the indirect arg.

Related to #38480

@ghost
Copy link

ghost commented Dec 31, 2020

Tagging subscribers to this area: @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Add mono_marshal_get_native_func_wrapper_indirect
    This will be used to add a wrapper around calli <sig> indirect calls where sig has an unmanaged calling convention.

    We need to do a GC transition before calling an unmanaged function from managed. So this wrapper does it.

    This is reusing much of the code implemented for mono_marshal_get_native_func_wrapper_aot which is used for delegates. Unfortunately that means that the function pointer is (pointlessly) boxed when it is passed as the first argument. In theory we should be able to just pass it directly and get much simpler code. But that will require changing the code in emit_native_wrapper_ilgen to get an unboxed function pointer arg

  • Treat CallConv bit 0x09 as MONO_CALL_UNMANAGED_MD
    This is the C#9 function pointers "unmanaged"+ext calling convention.
    Additional calling convention details are encoded in the modopts of the return type.

    This PR doesn't handle the modopts yet

Related to #38480

Author: lambdageek
Assignees: -
Labels:

area-VM-meta-mono

Milestone: -

@lambdageek
Copy link
Member Author

@vargaz I'm not sure if I'm doing the stuff in method-to-ir in the best way. Does this look ok?

This is the C#9 function pointers "unmanaged"+ext calling convention.
Additional calling convention details are encoded in the modopts of the return
type.

This PR doesn't handle the modopts yet
This will be used to add a wrapper around "calli sig" indirect calls where "sig" has
an unmanaged calling convention.

We need to do a GC transition before calling an unmanaged function from
managed. So this wrapper does it.

This is reusing much of the code implemented for
mono_marshal_get_native_func_wrapper_aot which is used for delegates.
Unfortunately that means that the function pointer is (pointlessly) boxed when
it is passed as the first argument.
If there's a calli with an unmanaged signature, invoke a wrapper that
does a transition to GC Safe mode and then do the call.

The wrapper is always inlined into the callee.

Because we're reusing much of the code of
mono_marshal_get_native_func_wrapper_aot, the function pointer first has to be
boxed before it's passed to the wrapper.  In theory we should be able to just
pass it directly and get much simpler code.  But that will require changing the
code in emit_native_wrapper_ilgen to get an unboxed function pointer arg
@lambdageek lambdageek force-pushed the hack-unmanaged-cconv branch from 97d6f71 to 64abb62 Compare January 1, 2021 00:06
@lambdageek lambdageek force-pushed the hack-unmanaged-cconv branch from 64abb62 to be21ade Compare January 1, 2021 01:34
@lambdageek lambdageek force-pushed the hack-unmanaged-cconv branch from be21ade to af21cca Compare January 1, 2021 02:23
assume that if the callee is a wrapper, it doesn't need the transition, and
only add it to normal managed methods.
@lambdageek lambdageek force-pushed the hack-unmanaged-cconv branch from ed79ef5 to 82a6dec Compare January 4, 2021 18:30
@lambdageek
Copy link
Member Author

Looks like it's passing all the tests if we only insert the GC transition for calli when it's not in a wrapper (ie it's a normal managed function).

There are a lot of different wrapper types, but I didn't go through all of them to see if it makes sense to omit the GC transition wrapper from all of them. That means we're potentially going to be missing GC transitions in some wrappers. But going the other way (only ignoring the native-to-managed wrapper and adding transitions in every other kind) caused a ton of crashes in mono/mono - things like cross-domain invokes, etc.

It would be nice to add a test that can verify that the pinvoked method is in GC Safe mode (using mono_threads_assert_gc_safe_region but maybe not only in checked build modes), but I haven't done it yet.

I'm going to rebase one more time to get rid of some debugging cruft but otherwise I think this is ready.

Replace the 4 boolean args by a flags arg.

Also add a new EMIT_NATIVE_WRAPPER_FUNC_PARAM_UNBOXED and use it in
mono_marshal_get_native_func_wrapper_indirect.  The new flag has no effect yet.
@lambdageek lambdageek force-pushed the hack-unmanaged-cconv branch from 9316137 to dda4b7d Compare January 5, 2021 18:00
@lambdageek
Copy link
Member Author

lambdageek commented Jan 5, 2021

Addressed all the PR feedback, and added code to use an unboxed fnptr arg. Tested on a HelloWorld sample locally that it emits the correct code.

@vargaz please review again

Add support for the EMIT_NATIVE_WRAPPER_FUNC_PARAM_UNBOXED flag to mono_marshal_emit_native_wrapper
@lambdageek lambdageek force-pushed the hack-unmanaged-cconv branch from dda4b7d to 25b94ac Compare January 5, 2021 18:48
@lambdageek lambdageek merged commit 106098f into dotnet:master Jan 6, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 5, 2021
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.

3 participants