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

Mono: correctly handle new 0x09 ("unmanaged") calling convention in metadata #38480

Closed
7 tasks done
lambdageek opened this issue Jun 26, 2020 · 11 comments
Closed
7 tasks done

Mono: correctly handle new 0x09 ("unmanaged") calling convention in metadata #38480

lambdageek opened this issue Jun 26, 2020 · 11 comments
Assignees
@vargaz @lambdageek @BrzVlad
Labels
area-VM-meta-mono tracking This issue is tracking the completion of other related issues.
Milestone
6.0.0

Comments

@lambdageek
Copy link
Member

lambdageek commented Jun 26, 2020 •
edited
Loading

In #38357, a new calling convention value is added to function signatures. Mono should not crash when we see this value in assemblies.

The runtime should basically treat it the same as cdecl on non-Windows platforms (and on Windows for .net 5, too).

When the execution engine sees the calli fsig IL instruction, we will need to look for unmanaged in fsig and do a GC transition around the call.

Next, a modopt on the return type of the signature will encode details of the calling convention #34805

Finally, if the unmanaged calling convention modopts have CallConvSuppressGCTransition as one of the options, we can drop the GC transition around the indirect call.

Tasks:
@lambdageek lambdageek added this to the 5.0.0 milestone Jun 26, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jun 26, 2020
@lambdageek lambdageek mentioned this issue Jun 26, 2020
Closed
@lambdageek lambdageek removed the untriaged New issue has not been triaged by the area owner label Jun 26, 2020
@lambdageek lambdageek self-assigned this Jun 26, 2020
lambdageek added a commit to lambdageek/mono that referenced this issue Jul 8, 2020
@lambdageek
[metadata] Initial support for MONO_CALL_UNMANAGED
8684def 
A MethodRefSig or a StandaloneSig can have the calling convention "unmanaged"
which is the default platform calling convention with optional modifiers
encoded in the modopts of the return type.

See dotnet/roslyn#39865 (comment)
and dotnet/runtime#34805

Contributes to dotnet/runtime#38480
@lambdageek
Copy link
Member Author

An initial attempt is at lambdageek/mono@8684def
need to chase down all the places that look at the calling convention and add a case for this one.

@lambdageek
Copy link
Member Author

Related CoreCLR PR #39030

Need to handle, in particular the case where unmanaged+modopt is different from an existing unmanaged xyz cconv https://github.com/dotnet/runtime/pull/39030/files#r452569548

@vargaz
Copy link
Contributor

vargaz commented Jul 15, 2020

ikvm changes:
mono/mono#20119

@vargaz
Copy link
Contributor

vargaz commented Jul 15, 2020

monodis prints:
calli signature-0x11000006
for all calli signatures.

@marek-safar
Copy link
Contributor

@lambdageek is there anything else to be done?

@CoffeeFlux
Copy link
Contributor

CoffeeFlux commented Aug 10, 2020 •
edited
Loading

I guess the initial work was covered by #38728? So if we're updated in ikdasm and monodis, I guess we're good?

@CoffeeFlux
Copy link
Contributor

CoffeeFlux commented Aug 10, 2020 •
edited
Loading

I'm closing this unless anyone knows of remaining work. The issue's topics seem to be covered. cc: @vargaz

@lambdageek
Copy link
Member Author

@CoffeeFlux I dont' think this is done - we still need to land lambdageek/mono@8684def or something like it in order to handle the 0x09 "unmanaged" value. This wasn't in #38728.

@lambdageek lambdageek reopened this Sep 8, 2020
@CoffeeFlux CoffeeFlux modified the milestones: 5.0.0, 6.0.0 Sep 8, 2020
@lambdageek lambdageek mentioned this issue Dec 29, 2020
Closed
@lambdageek
Copy link
Member Author

One thing I missed initially is that calli <fsig> instruction needs to look at <fsig> and emit a GC transition if its calling convention is unmanaged

@BrzVlad BrzVlad mentioned this issue Dec 30, 2020
Merged
@lambdageek lambdageek mentioned this issue Dec 31, 2020
Merged
@monojenkins monojenkins mentioned this issue Dec 31, 2020
Merged
@lambdageek lambdageek added the tracking This issue is tracking the completion of other related issues. label Jan 4, 2021
monojenkins pushed a commit to monojenkins/mono that referenced this issue Jan 5, 2021
@lambdageek
[Mono][jit] Emit GC transitions for "calli unmanaged" instructions
044aed7 
- 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 dotnet/runtime#38480
lambdageek added a commit to mono/mono that referenced this issue Jan 6, 2021
@monojenkins @lambdageek
[Mono][jit] Emit GC transitions for "calli unmanaged" instructions (#…
5bf7d8b 
…20713)

- 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 dotnet/runtime#38480

Co-authored-by: lambdageek <[email protected]>
This was referenced Jan 14, 2021
Merged
Merged
@SamMonoRT
Copy link
Member

@BrzVlad - is the last tracking item above " interp - ditto - emit GC transitions around calli instructions with the pinvoke bit set." necessary for 6.0 ?

@SamMonoRT
Copy link
Member

Assigning to @BrzVlad for the last item

@BrzVlad BrzVlad mentioned this issue Aug 10, 2021
Merged
@BrzVlad BrzVlad closed this as completed Aug 11, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@vargaz vargaz

@lambdageek lambdageek

@BrzVlad BrzVlad

Labels
area-VM-meta-mono tracking This issue is tracking the completion of other related issues.
Projects
None yet
Milestone
6.0.0
Development

No branches or pull requests
7 participants
@vargaz @marek-safar @lambdageek @CoffeeFlux @BrzVlad @Dotnet-GitSync-Bot @SamMonoRT