[MONO] Move marshal-ilgen into a component by naricc · Pull Request #71203 · dotnet/runtime

naricc · 2022-06-23T13:13:15Z

This PR moves marshal-ilgen into a component. This will save approximately 100 KB in the runtime where the component is not needed. Further refactoring should allow more code to be moved into the component in the future.

This also makes a change to the CMake files so that some components can be statically linked in the aot compiler, since marshal_ilgen is needed at AOT time.

Contributes to: #61685

srxqds · 2022-06-23T15:19:13Z

so when should we use this component or not?

naricc · 2022-06-23T16:07:09Z

@srxqds Basically whenever you have DisableRuntimeMarshallingAttribute set in a project, and publish that project, we would not have to link the component into the published. A very quick summary is that the DisableRuntimeMarshallingAttribute says not to use the old runtime marshaling, but instead use source generator marshaling.

I expect this will be the default for new projects on wasm going forward, and probably android and iOS as well. This PR doesn't contain any changes to make it the default though.

More information here: #60639 and here: #61685

MichalStrehovsky · 2022-06-23T21:20:48Z

Basically whenever you have DisableRuntimeMarshallingAttribute set in a project, and publish that project, we would not have to link the component into the published

Nit: one would also have to make sure all of the referenced NuGets have that attribute (or no pinvoke, delegate, or unmanaged Calli), Assembly.LoadFrom and similar is not called, and nobody Reflection.Emits a pinvoke.

naricc · 2022-07-07T21:02:21Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-07-07T21:02:37Z

Azure Pipelines successfully started running 1 pipeline(s).

naricc · 2022-07-07T21:03:05Z

/azp run runtime-wasm

azure-pipelines · 2022-07-07T21:03:14Z

Azure Pipelines successfully started running 1 pipeline(s).

naricc · 2022-08-10T19:42:17Z

The wasm debugger tests are known issues, so I am going to merge this.

lambdageek · 2022-08-10T23:26:02Z

@naricc please open issues for XA and XI to add the component by default!

naricc · 2022-08-10T23:39:38Z

@lambdageek
dotnet/android#7249
dotnet/macios#15668

Context: dotnet/runtime#71203Fixes: #7249 Apps on this PR currently crash with: mono-rt : [ERROR] FATAL UNHANDLED EXCEPTION: System.InvalidProgramException: Invalid IL code in (wrapper managed-to-native) object:wrapper_native_0x7725af156f10 (intptr): IL_0012: calli 0x00000003 mono-rt : at System.Runtime.InteropServices.Marshal.PtrToStructureHelper(IntPtr , Object , Boolean ) mono-rt : at System.Runtime.InteropServices.Marshal.PtrToStructure(IntPtr , Type ) mono-rt : at Java.Interop.JniRuntime.CreateInvoker(IntPtr ) mono-rt : at Java.Interop.JniRuntime..ctor(CreationOptions ) mono-rt : at Android.Runtime.AndroidRuntime..ctor(IntPtr , IntPtr , Boolean , IntPtr , IntPtr , Boolean ) mono-rt : at Android.Runtime.JNIEnv.Initialize(JnienvInitializeArgs* ) Which, I assume is due to a missing Mono component. We should add the component by default: <_MonoComponent Condition=" '$(_IncludeMarshalIlGen)' != 'false' " Include="marshal-ilgen" /> In the future, someone could experiment by setting `$(_IncludeMarshalIlGen)` to `false`.

Fixes: dotnet#7249 Context: dotnet/runtime#71203 Context: dotnet/runtime#60639 Context: dotnet/runtime#61685 Mono introduced a new `marshal-ilgen` component in dotnet/runtime@de32c446 which needs to be used when *non-blittable type* is required. `string` marshaling is a form of non-blittable marshaling. Eventually the idea is that assemblies should migrate to use the [`DllImport Generator`][0] for non-blittable marshaling purposes, then [`[assembly:DisableRuntimeMarshallingAttribute]`][1] can be applied to the assembly, and -- if no assemblies in an Android app require non-blittable marshaling -- then the `marshal-ilgen` component can be omitted from the `.apk`, reducing app size. That's a fair number of `if`s; this won't be happening soon. In the meantime, we need to start including the `marshal-ilgen` component in all .NET 7 RC1+ builds so that our existing `DllImport` declarations continue to work. Update `@(_MonoComponent)` to add `marshal-ilgen`. In order to facilitate future testing, allow the `marshal-ilgen` component to be *excluded* if the `$(_AndroidExcludeMarshalIlgenComponent)` property is True. [0]: https://github.com/dotnet/runtimelab/tree/feature/DllImportGenerator [1]: dotnet/runtime#60639

Fixes: #7249 Context: dotnet/runtime#71203 Context: dotnet/runtime#60639 Context: dotnet/runtime#61685 Context: #7276 Mono introduced a new `marshal-ilgen` component in dotnet/runtime@de32c446 which needs to be used when *non-blittable type* is required. `string` marshaling is a form of non-blittable marshaling, as is `Marshal.PtrToStructure()` (?!). Eventually the idea is that assemblies should migrate to use the [`DllImport Generator`][0] for non-blittable marshaling purposes, then [`[assembly:DisableRuntimeMarshallingAttribute]`][1] can be applied to the assembly, and -- if no assemblies in an Android app require non-blittable marshaling -- then the `marshal-ilgen` component can be omitted from the `.apk`, reducing app size. That's a fair number of `if`s; this won't be happening soon. In the meantime, we need to start including the `marshal-ilgen` component in all .NET 7 RC1+ builds so that our existing `[DllImport]` declarations continue to work. Failure to do so results in runtime errors: mono-rt : [ERROR] FATAL UNHANDLED EXCEPTION: System.InvalidProgramException: Invalid IL code in (wrapper managed-to-native) object:wrapper_native_0x7725af156f10 (intptr): IL_0012: calli 0x00000003 mono-rt : at System.Runtime.InteropServices.Marshal.PtrToStructureHelper(IntPtr , Object , Boolean ) mono-rt : at System.Runtime.InteropServices.Marshal.PtrToStructure(IntPtr , Type ) mono-rt : at Java.Interop.JniRuntime.CreateInvoker(IntPtr ) mono-rt : at Java.Interop.JniRuntime..ctor(CreationOptions ) mono-rt : at Android.Runtime.AndroidRuntime..ctor(IntPtr , IntPtr , Boolean , IntPtr , IntPtr , Boolean ) mono-rt : at Android.Runtime.JNIEnv.Initialize(JnienvInitializeArgs* ) Update `@(_MonoComponent)` to add `marshal-ilgen`. In order to facilitate future testing, allow the `marshal-ilgen` component to be *excluded* if the `$(_AndroidExcludeMarshalIlgenComponent)` property is True. [0]: https://github.com/dotnet/runtimelab/tree/feature/DllImportGenerator [1]: dotnet/runtime#60639

uweigand · 2022-08-25T17:55:46Z

When running the test suite on s390x, I just saw the following assertion:

Assertion at /home/uweigand/runtime/src/mono/mono/component/marshal-ilgen.c:63, condition `!ilgen_cb_inited' not met

with this call stack:

 #14 0x000003ffb327d0ac in mono_assertion_message (file=0x3ffb33865a0 "/home/uweigand/runtime/src/mono/mono/component/marshal-ilgen.c", line=63, condition=0x3ffb33865e0 "!ilgen_cb_inited") at /home/uweigand/runtime/src/mono/mono/eglib/goutput.c:226
#15 0x000003ffb3321a30 in mono_install_marshal_callbacks_ilgen (cb=0x3ff9a3f16d0) at /home/uweigand/runtime/src/mono/mono/component/marshal-ilgen.c:63
#16 0x000003ffb33221ce in mono_marshal_ilgen_init () at /home/uweigand/runtime/src/mono/mono/component/marshal-ilgen.c:2858
#17 0x000003ffb3322106 in get_marshal_cb () at /home/uweigand/runtime/src/mono/mono/component/marshal-ilgen.c:2753
#18 0x000003ffb3321f24 in mono_emit_marshal_ilgen (m=0x3ff9a3f1bc8, argnum=0, t=0x3ff8ad98b68, spec=0x0, conv_arg=0, conv_arg_type=0x3ff8ad99ed8, action=MARSHAL_ACTION_CONV_IN, lightweigth_cb=0x3ffb33910e8 <marshal_lightweight_cb>) at /home/uweigand/runtime/src/mono/mono/component/marshal-ilgen.c:2810
#19 0x000003ffb2e54c52 in mono_emit_marshal (m=0x3ff9a3f1bc8, argnum=0, t=0x3ff8ad98b68, spec=0x0, conv_arg=0, conv_arg_type=0x3ff8ad99ed8, action=MARSHAL_ACTION_CONV_IN) at /home/uweigand/runtime/src/mono/mono/metadata/marshal.c:3226
#20 0x000003ffb2f219aa in emit_native_wrapper_ilgen (image=0x2aa18a8b950, mb=0x3ff7c90eb50, sig=0x3ff8ad99cc8, piinfo=0x3ff8ad98af0, mspecs=0x3ff7c8f47e0, func=0x3ff82d81750 <CompressionNative_DeflateInit2_>, flags=(EMIT_NATIVE_WRAPPER_CHECK_EXCEPTIONS | EMIT_NATIVE_WRAPPER_RUNTIME_MARSHALLING_ENABLED)) at /home/uweigand/runtime/src/mono/mono/metadata/marshal-lightweight.c:937
#21 0x000003ffb2e5755a in mono_marshal_emit_native_wrapper (image=0x2aa18a8b950, mb=0x3ff7c90eb50, sig=0x3ff8ad99cc8, piinfo=0x3ff8ad98af0, mspecs=0x3ff7c8f47e0, func=0x3ff82d81750 <CompressionNative_DeflateInit2_>, flags=(EMIT_NATIVE_WRAPPER_CHECK_EXCEPTIONS | EMIT_NATIVE_WRAPPER_RUNTIME_MARSHALLING_ENABLED)) at /home/uweigand/runtime/src/mono/mono/metadata/marshal.c:6300
#22 0x000003ffb2e56b6c in mono_marshal_get_native_wrapper (method=0x3ff8ad98af0, check_exceptions=1, aot=0) at /home/uweigand/runtime/src/mono/mono/metadata/marshal.c:3672
#23 0x000003ffb30328e8 in compile_special (method=0x3ff8ad98af0, error=0x3ff9a3f2c38) at /home/uweigand/runtime/src/mono/mono/mini/mini-runtime.c:2469
#24 0x000003ffb30322b0 in mono_jit_compile_method_with_opt (method=0x3ff8ad98af0, opt=374417919, jit_only=0, error=0x3ff9a3f2c38) at /home/uweigand/runtime/src/mono/mono/mini/mini-runtime.c:2672
#25 0x000003ffb3031c04 in jit_compile_method_with_opt_cb (arg=0x3ff9a3f28e8) at /home/uweigand/runtime/src/mono/mono/mini/mini-runtime.c:2762
#26 0x000003ffb3029ea8 in jit_compile_method_with_opt (params=0x3ff9a3f28e8) at /home/uweigand/runtime/src/mono/mono/mini/mini-runtime.c:2778
#27 0x000003ffb3029c7e in mono_jit_compile_method (method=0x3ff8ad98af0, error=0x3ff9a3f2c38) at /home/uweigand/runtime/src/mono/mono/mini/mini-runtime.c:2797
#28 0x000003ffb31677ba in common_call_trampoline (regs=0x3ff9a3f2db8, code=0x3ffa806234c "\a\340\300\001", m=0x3ff8ad98af0, vt=0x0, vtable_slot=0x0, error=0x3ff9a3f2c38) at /home/uweigand/runtime/src/mono/mono/mini/mini-trampolines.c:618
#29 0x000003ffb3166b8e in mono_magic_trampoline (regs=0x3ff9a3f2db8, code=0x3ffa806234c "\a\340\300\001", arg=0x3ff8ad98af0, tramp=0x3ffb3de02c8 "\300X") at /home/uweigand/runtime/src/mono/mono/mini/mini-trampolines.c:759

This seems to be a race condition that is not easily reproducible. But looking at the code:

static MonoMarshalILgenCallbacks *
get_marshal_cb (void)
{
if (G_UNLIKELY (!ilgen_cb_inited)) {
#ifdef ENABLE_ILGEN
mono_marshal_ilgen_init ();
#else
mono_marshal_noilgen_init_heavyweight ();
#endif
}
return &ilgen_marshal_cb;
}

and

void
mono_install_marshal_callbacks_ilgen (MonoMarshalILgenCallbacks *cb)
{
g_assert (!ilgen_cb_inited);
g_assert (cb->version == MONO_MARSHAL_CALLBACKS_VERSION);
memcpy (&ilgen_marshal_cb, cb, sizeof (MonoMarshalILgenCallbacks));
ilgen_cb_inited = TRUE;
}

there does indeed appear to be a race condition with accessing the ilgen_cb_inited flag without any locks.

I'm not sure if this patch is directly responsible - there seems to have been a race condition before, but maybe with the component rework it is now somehow easier to hit? - but I never saw this assertion fail before.

This reverts commit de32c44.

ghost added the area-VM-meta-mono label Jun 23, 2022

ghost assigned naricc Jun 23, 2022

runfoappBot mentioned this pull request Jul 1, 2022
System.Security.Cryptography.X509Certificates test failures #70320
Closed

naricc force-pushed the naricc/marshal-component branch from 1b99875 to c6ad66fCompare July 7, 2022 20:37

dotnet deleted a comment from azure-pipelinesBotJul 7, 2022

This was referenced Jul 8, 2022
RunContinueWithStressTestsNoState timing out in CI #2271
Closed
system.net.security.tests work item timeout on mono #71997
Closed

naricc force-pushed the naricc/marshal-component branch 2 times, most recently from 57666a5 to 7bdf48bCompare July 15, 2022 17:03

naricc force-pushed the naricc/marshal-component branch 2 times, most recently from 3bdee74 to 4be9fcdCompare July 26, 2022 19:24

naricc marked this pull request as ready for review July 26, 2022 19:50

naricc requested review from SamMonoRT, lambdageek, lewing, marek-safar, pavelsavara, radical, thaystg and vargaz as code owners July 26, 2022 19:50

naricc changed the title ~~[DRAFT][MONO] Move marshal-ilgen into a component~~[MONO] Move marshal-ilgen into a componentJul 26, 2022

vargaz reviewed Jul 27, 2022
View reviewed changes

Comment threadsrc/mono/mono/component/CMakeLists.txt Outdated

vargaz reviewed Jul 27, 2022
View reviewed changes

Comment threadsrc/mono/mono/component/marshal_ilgen.h Outdated

Nathan Ricci added 10 commits August 10, 2022 10:18

Add assert to stub.
3de543a

Added component to runtime tests build.
0207740

Added marshal-ilgen component to more places.
7128cf5

Remvoed redundant property setting.
f0e0104

Debug prints.
fabe938

Reverted unecessary changes to tests.mobile.targets
b75ab63

Changed MARSHAL_ILGEN_LIBRARY_NAME
8f9255c

Removed debugging.
d30ef4c

White space fixes.
7639f32

Put back function table in stub.
8fe4728

naricc force-pushed the naricc/marshal-component branch from d04025f to 8fe4728Compare August 10, 2022 14:19

naricc merged commit de32c44 into dotnet:mainAug 10, 2022

naricc mentioned this pull request Aug 10, 2022
marshal-ilgen component must be included dotnet/android#7249
Closed

naricc mentioned this pull request Aug 10, 2022
marshal-ilgen component must be included dotnet/macios#15668
Closed

lambdageek mentioned this pull request Aug 11, 2022
[Mono] Add support for DisableRuntimeMarshallingAttribute #61685
Open
5 tasks

jonpryor mentioned this pull request Aug 15, 2022
[Xamarin.Android.Build.Tasks] Use marshal-ilgen component dotnet/android#7260
Merged

naricc mentioned this pull request Aug 25, 2022
Race condition on marshal-ilgen init #74603
Closed

rolfbjarne mentioned this pull request Aug 26, 2022
The mono_marshal_ilgen_init symbol is private in libmonosgen-2.0.dylib #74628
Closed

lambdageek added a commit to lambdageek/runtime that referenced this pull request Aug 26, 2022
Revert "[MONO] Move marshal-ilgen into a component (dotnet#71203)"
bc7f9d5
This reverts commit de32c44.

lambdageek added a commit to lambdageek/runtime that referenced this pull request Aug 26, 2022
Revert "[MONO] Move marshal-ilgen into a component (dotnet#71203)"
c3c7147
This reverts commit de32c44.

carlossanlop pushed a commit that referenced this pull request Aug 27, 2022
Revert "[MONO] Move marshal-ilgen into a component (#71203)" (#74675)
06aceb7
This reverts commit de32c44.

carlossanlop pushed a commit that referenced this pull request Aug 27, 2022
Revert "[MONO] Move marshal-ilgen into a component (#71203)" (#74674)
6c3c234
This reverts commit de32c44.

ghost locked as resolved and limited conversation to collaborators Sep 24, 2022

Navigation Menu

Uh oh!

[MONO] Move marshal-ilgen into a component - #71203

[MONO] Move marshal-ilgen into a component#71203
naricc merged 19 commits into
dotnet:mainfrom
naricc:naricc/marshal-component

naricc commented Jun 23, 2022•
edited
Loading

Uh oh!

srxqds commented Jun 23, 2022

Uh oh!

naricc commented Jun 23, 2022•
edited
Loading

Uh oh!

MichalStrehovsky commented Jun 23, 2022

Uh oh!

naricc commented Jul 7, 2022

Uh oh!

azure-pipelinesBot commented Jul 7, 2022

Uh oh!

naricc commented Jul 7, 2022

Uh oh!

azure-pipelinesBot commented Jul 7, 2022

Uh oh!

Uh oh!

Uh oh!

naricc commented Aug 10, 2022

Uh oh!

lambdageek commented Aug 10, 2022

Uh oh!

naricc commented Aug 10, 2022

Uh oh!

uweigand commented Aug 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Uh oh!

Conversation

naricc commented Jun 23, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srxqds commented Jun 23, 2022

Uh oh!

naricc commented Jun 23, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichalStrehovsky commented Jun 23, 2022

Uh oh!

naricc commented Jul 7, 2022

Uh oh!

azure-pipelinesBot commented Jul 7, 2022

Uh oh!

naricc commented Jul 7, 2022

Uh oh!

azure-pipelinesBot commented Jul 7, 2022

Uh oh!

Uh oh!

Uh oh!

naricc commented Aug 10, 2022

Uh oh!

lambdageek commented Aug 10, 2022

Uh oh!

naricc commented Aug 10, 2022

Uh oh!

uweigand commented Aug 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

naricc commented Jun 23, 2022•
edited
Loading

naricc commented Jun 23, 2022•
edited
Loading