[mono] Add support for UnmanagedCallersOnlyAttribute #38728

lambdageek · 2020-07-02T21:41:07Z

When we see a method tagged with UnmanagedCallersOnlyAttribute in an ldtn instruction, load a pointer to a native-to-managed wrapper instead of a pointer to the managed method.

See https://github.com/dotnet/csharplang/blob/master/proposals/functionpointers.md#systemruntimeinteropservicesunmanagedcallersonlyattribute

TODO:

Throw exceptions for all the unsupported scenarios:
- Attempting to call by creating a delegate
- Invoking via reflection
- Using unsupported argument or return types
~~Figure out why the CoreCLR tests are either not running or somehow passing.~~
The tests are in https://github.com/dotnet/runtime/tree/master/src/coreclr/tests/src/Interop/UnmanagedCallersOnly which doesn't seem to be in https://github.com/dotnet/runtime/blob/master/src/coreclr/tests/issues.targets but in that case Mono should be failing a number of the tests in there on CI.
It's because we only run P0 tests on CI for Mono. I temporarily set it to P0 for this PR just to watch the tests fail.
Revert tests back to P1

Dotnet-GitSync-Bot · 2020-07-02T21:41:11Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

src/mono/mono/mini/aot-runtime.c

vargaz · 2020-07-02T22:09:40Z

Looks ok.

src/mono/mono/metadata/marshal.c

src/mono/mono/mini/jit-icalls.c

alexischr

LGTM, documenting that this will increase AOT binaries sizes by 1 byte per native-to-managed wrapper AFAICT

src/mono/mono/metadata/marshal.c

lambdageek · 2020-07-06T21:50:20Z

this will increase AOT binaries sizes by 2 bytes per native-to-managed wrapper AFAICT

1 byte, I think - AOT uses the same value encoding as CIL images - small values (<127) encode to one byte.

lambdageek · 2020-07-06T22:04:45Z

Should pass the positive tests now. Negative tests will still fail.

lambdageek · 2020-07-06T22:16:14Z

~~I think preventing calls via calli will be difficult using Mono's current design - the native to managed wrapper can't really tell it's being called from a managed method.~~

I misunderstood the test - it's in fact just crashing. So there's nothing to detect. Mono's behavior of not crashing is ok.

src/mono/mono/mini/method-to-ir.c

lambdageek · 2020-07-07T20:30:47Z

Some progress on the negative tests. The InvalidProgramException ones should be in good shape.

lambdageek · 2020-07-07T21:37:42Z

The TestUnmanagedCallersOnlyValid test is failing with the interpreter due to #38897

…elegate class In that case, create a wrapper based on the signature of the method itself. TODO: - check that the method is static - check that the method does not have any marshal info (currently it's assumed to not be present and ignored)

…thout a delegate class Bump the AOT file format

…ethods

…llersOnly methods

…wrapper The wrapper might be called from a thread that's not attached to the runtime, and the jump trampoline will look at TLS vars that are not initialized

…agedCallersOnly method

…hods

…UnmanagedCallersOnly methods Instead of throwing while JITing (or transforming), throw when the LDFTN is executed.

lambdageek · 2020-07-08T17:30:05Z

JIT is passing all the tests except TestPInvokeMarkedWithUnmanagedCallersOnly. Working on that one now.

throw NotSupportedException

lambdageek · 2020-07-08T18:52:19Z

Once I see the JIT pass all the tests, I'll bump the test back to Pri1 and then the PR is basically complete.

lambdageek · 2020-07-08T20:08:37Z

From https://helix.dot.net/api/2019-06-17/jobs/51ac422f-058f-4fa3-aa34-f41bb363a4fb/workitems/Interop/console the "runtime (Mono Pri0 Runtime Tests Run OSX x64 release)" JIT job:

Interop.UnmanagedCallersOnly.XUnitWrapper           Total:  1, Errors: 0, Failed: 0, Skipped: 0, Time: 0.252s

As previously mentioned, the interpreter job is failing due to #38897, so this is the best we'll get for now.

CoffeeFlux

Lots of questions but looks fine to me. Would be really good if Zoltan gets a chance to look at this before it's merged since I'm not terribly confident in my review of the code generation parts. Thanks!

CoffeeFlux · 2020-07-08T19:28:20Z

src/mono/mono/metadata/marshal.c

+		csig = mono_metadata_signature_dup_full (get_method_image (method), sig);
+		csig->pinvoke = 0;
+		res = mono_mb_create_and_cache_full (cache, method, mb, csig,
+											 csig->param_count + 16, info, NULL);


Where is this 16 coming from? It looks like it's used elsewhere, but I'm not familiar enough with this code to understand why.

I don't know. I'm cargo-culting the other mono_mb_create_and_cache_full calls in this function.

Ugh, that's what I feared. @vargaz any idea? For my own learning if nothing else :)

No idea, it probably comes from the fact that these are wrappers so the deepest stack will be calling the 'wrapped' method, so param_count + is good for max stack.

src/mono/mono/metadata/marshal.c

CoffeeFlux · 2020-07-08T19:37:20Z

src/mono/mono/metadata/marshal.c

+			return NULL;
+		}
+		if (!method_signature_is_blittable (invoke_sig)) {
+			mono_error_set_invalid_program (error, "method %s with UnmanagedCallersOnlyAttribute has non-blittable parameters", mono_method_full_name (method, TRUE));


It could also have a non-blittable return type. I'd change this error slightly to reflect that.

CoffeeFlux · 2020-07-08T19:40:11Z

src/mono/mono/metadata/marshal.c

+{
+	ERROR_DECL (attr_error);
+	MonoClass *attr_klass = NULL;
+#ifdef ENABLE_NETCORE


I guess this accomplishes the same thing, but wouldn't it be clearer to immediately return false on non-netcore?

src/mono/mono/mini/jit-icalls.c

src/mono/mono/metadata/marshal.c

lambdageek · 2020-07-08T20:32:29Z

src/mono/mono/metadata/marshal.c

+	case MONO_TYPE_R8:
+	case MONO_TYPE_I:
+	case MONO_TYPE_U:
+	case MONO_TYPE_CHAR:


I think char shouldn't be here. Doesn't look like it's supposed to be blittable. (mono_class_setup_mono_type doesn't set the blittable bit on it)

Ah yeah, good catch. Yes, it's definitely not for the same reason as string—ambiguity over converting to ANSI, UTF-8, or UTF-16.

Co-authored-by: Ryan Lucia <ryan@luciaonline.net>

vargaz · 2020-07-09T05:09:27Z

src/mono/mono/metadata/marshal.c

@@ -3940,10 +3958,54 @@ emit_managed_wrapper_noilgen (MonoMethodBuilder *mb, MonoMethodSignature *invoke
 }
 #endif

+static gboolean
+type_is_blittable (MonoType *type)


There is already a class->blittable flag.

We discussed this, but it's not the same because of byref types.

lambdageek added area-Interop-mono runtime-mono specific to the Mono runtime labels Jul 2, 2020

monojenkins mentioned this pull request Jul 2, 2020

[netcore][mono] Add support for UnmanagedCallersOnlyAttribute mono/mono#20060

Merged

lambdageek force-pushed the hack-unmanaged-callers-only branch from e05143d to fe9f96c Compare July 2, 2020 21:56

vargaz reviewed Jul 2, 2020

View reviewed changes

src/mono/mono/mini/aot-runtime.c Outdated Show resolved Hide resolved

lambdageek commented Jul 2, 2020

View reviewed changes

src/mono/mono/metadata/marshal.c Outdated Show resolved Hide resolved

lambdageek commented Jul 3, 2020

View reviewed changes

src/mono/mono/mini/jit-icalls.c Outdated Show resolved Hide resolved

alexischr reviewed Jul 6, 2020

View reviewed changes

src/mono/mono/metadata/marshal.c Outdated Show resolved Hide resolved

lambdageek marked this pull request as ready for review July 6, 2020 22:04

lambdageek requested review from BrzVlad, CoffeeFlux, SamMonoRT and thaystg as code owners July 6, 2020 22:04

lambdageek commented Jul 7, 2020

View reviewed changes

src/mono/mono/mini/method-to-ir.c Outdated Show resolved Hide resolved

lambdageek and others added 9 commits July 8, 2020 09:42

[marshal] Add mono_method_has_unmanaged_callers_only_attribute

e6009c9

[aot] Allow decode_method_ref to decode NATIVE_TO_MANAGED wrappers wi…

a8d4d57

…thout a delegate class Bump the AOT file format

[mini] return ptr to wrapper in mono_ldftn for UnmanagedCallersOnly m…

40662c4

…ethods

[interp] ldftn will return a native-to-managed wrapper to UnmanagedCa…

92ef04c

…llersOnly methods

[tests] Temporarily set UnmanagedCallersOnly tests to Pri0

36ebfaa

fixup build

376db97

fixup null

ff12195

aot: emit byte when we don't expect a class

04b6da5

lambdageek and others added 10 commits July 8, 2020 09:42

fix typo

1f6d260

don't get callconv from Invoke if there's no invoke

2579230

jit: don't create a jump trampoline for ldftn of a native-to-managed …

00ed38d

…wrapper The wrapper might be called from a thread that's not attached to the runtime, and the jump trampoline will look at TLS vars that are not initialized

interp: transform LDFTN into LDC of a create_method_pointer for Unman…

b100d40

…agedCallersOnly method

remove obsolete comment

7b00b07

marshal: throw invalid program exception for instance and generic met…

6340c36

…hods

Emit IPE throw instead of aborting JIT or interp compilation for bad …

a7199ad

…UnmanagedCallersOnly methods Instead of throwing while JITing (or transforming), throw when the LDFTN is executed.

Use domain mempool for IPE error message

d6b9c56

disallow delegate constructor calls on UnmanagedCallersOnly methods

1b5422f

throw IPE if UnmanagedCallersOnly method has non-blittable args

6e85742

lambdageek force-pushed the hack-unmanaged-callers-only branch from 9317eb0 to 6e85742 Compare July 8, 2020 13:50

disallow DllImport and UnmanagedCallersOnly together

9d502c5

throw NotSupportedException

lambdageek requested a review from vargaz July 8, 2020 18:52

Change UnamangedCallersOnlyTest.csproj back to Pri1

70075a9

CoffeeFlux approved these changes Jul 8, 2020

View reviewed changes

lambdageek commented Jul 8, 2020

View reviewed changes

lambdageek and others added 5 commits July 8, 2020 16:38

Apply suggestions from code review

025dd73

Co-authored-by: Ryan Lucia <ryan@luciaonline.net>

System.Char isn't blittable

0e6f067

fixup non-blittable exception message

a2f0d94

non-netcore doesn't have UnmanagedCallersOnlyAttribute

89c8c28

remove dead code

ee50f3e

safern mentioned this pull request Jul 8, 2020

Builds timing out on CI #38964

Closed

vargaz reviewed Jul 9, 2020

View reviewed changes

lambdageek merged commit 1e3c959 into dotnet:master Jul 10, 2020

lambdageek mentioned this pull request Jul 10, 2020

[mono] Implement support for UnmanagedCallersOnlyAttribute #38209

Closed

CoffeeFlux mentioned this pull request Aug 10, 2020

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

Closed

7 tasks

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mono] Add support for UnmanagedCallersOnlyAttribute #38728

[mono] Add support for UnmanagedCallersOnlyAttribute #38728

lambdageek commented Jul 2, 2020 •

edited

Loading

Dotnet-GitSync-Bot commented Jul 2, 2020

vargaz commented Jul 2, 2020

alexischr left a comment •

edited

Loading

lambdageek commented Jul 6, 2020

lambdageek commented Jul 6, 2020

lambdageek commented Jul 6, 2020 •

edited

Loading

lambdageek commented Jul 7, 2020

lambdageek commented Jul 7, 2020

lambdageek commented Jul 8, 2020

lambdageek commented Jul 8, 2020

lambdageek commented Jul 8, 2020

CoffeeFlux left a comment

CoffeeFlux Jul 8, 2020

lambdageek Jul 8, 2020

CoffeeFlux Jul 8, 2020

vargaz Jul 9, 2020

CoffeeFlux Jul 8, 2020

CoffeeFlux Jul 8, 2020

lambdageek Jul 8, 2020

CoffeeFlux Jul 8, 2020 •

edited

Loading

vargaz Jul 9, 2020

CoffeeFlux Jul 9, 2020

[mono] Add support for UnmanagedCallersOnlyAttribute #38728

[mono] Add support for UnmanagedCallersOnlyAttribute #38728

Conversation

lambdageek commented Jul 2, 2020 • edited Loading

Dotnet-GitSync-Bot commented Jul 2, 2020

vargaz commented Jul 2, 2020

alexischr left a comment • edited Loading

Choose a reason for hiding this comment

lambdageek commented Jul 6, 2020

lambdageek commented Jul 6, 2020

lambdageek commented Jul 6, 2020 • edited Loading

lambdageek commented Jul 7, 2020

lambdageek commented Jul 7, 2020

lambdageek commented Jul 8, 2020

lambdageek commented Jul 8, 2020

lambdageek commented Jul 8, 2020

CoffeeFlux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoffeeFlux Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdageek commented Jul 2, 2020 •

edited

Loading

alexischr left a comment •

edited

Loading

lambdageek commented Jul 6, 2020 •

edited

Loading

CoffeeFlux Jul 8, 2020 •

edited

Loading