runtime
runtime copied to clipboard
dotnet
[MONO] Move marshal-ilgen into a component
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: https://github.com/dotnet/runtime/issues/61685
@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: https://github.com/dotnet/runtime/issues/60639 and here: https://github.com/dotnet/runtime/issues/61685
Basically whenever you have
DisableRuntimeMarshallingAttributeset 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.
![azure-pipelines[bot] avatar](https://avatars.githubusercontent.com/in/9426?v=4 "Published by a pub.dev verified publisher"){kind=small}
@vargaz @lambdageek I think this is ready to go in, unless you have any additional feedback.
I don't really know much about the actual changes, so I can't review anything else 😬
@naricc when this is merged, please open issues for xamarin-macios and xamarin-android to make sure they include the marshal-ilgen component by default.
/cc @jonathanpeppers @rolfbjarne
We'll also need to know what MSBuild property toggles the inclusion of this component. We have these basically hardcoded right now:
https://github.com/xamarin/xamarin-android/blob/73f10df63b00ee8a21d61183056a26922c83e89d/src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.AssemblyResolution.targets#L198-L200
We'll also need to know what MSBuild property toggles the inclusion of this component. We have these basically hardcoded right now:
https://github.com/xamarin/xamarin-android/blob/73f10df63b00ee8a21d61183056a26922c83e89d/src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.AssemblyResolution.targets#L198-L200
@jonathanpeppers @naricc There are going to be two phases to this:
- The mobile SDKs should just hardcode always using the
marshal-ilgencomponent. For net7 this is the conservative backward-compatible behavior and will basically maintain the same functionality we have today. - Separately we will need to do some work to decide when to not include
marshal-ilgen. This will bring some space savings to apps that fully convert toLibraryImportGenerator(and to nugets that all use LibraryImportGenerator, and to reflection emit that only generates blittable pinvokes, etc). Probably as a first step we will try this out in the runtime and in Wasm and once we have some experience we will work on a design together with the mobile SDKs (and probably CoreCLR-based scenarios like single file, or native AOT). I doubt this is going to be in 7.
tl;dr hardcoding the component to always be used is exactly what we want to do for 7.
Ok, great. We will probably put something like:
<_MonoComponent Condition=" '$(_IncludeMarshalIlGen)' != 'false' " Include="marshal-ilgen" />
Then you could experiment with _IncludeMarshalIlGen=false. but it's always there by default.
Catalyst failure might be this, although it is a different test: https://github.com/dotnet/runtime/issues/72840
wasm issue appears to be this: https://github.com/dotnet/runtime/issues/73419
- How is this getting tested?
- Should
System.Diagnostics.Tracingtests be run for AOT? It is disabled right now. - How does a user trigger this? Is there a msbuild property? Also, if that is set then will it trigger relinking when building the project (and not publishing)?
- How is this getting tested?
The marshaling code itself is not really getting changed, just moved into a component. And we are running it throuhg all the test suites to make sure that component gets included in every scenario.
- Should
System.Diagnostics.Tracingtests be run for AOT? It is disabled right now.
I don't know. @mdh1418 Should it?
- How does a user trigger this? Is there a msbuild property? Also, if that is set then will it trigger relinking when building the project (and not publishing)?
This PR doesn't yet have any way to trigger it; the component is always included for now. The next step of this work is to have a way to drop the component when it is not needed. The plan is to have some property set in the project, along with an anlyzer that checks that DisableRuntimeMarshaling is set on all the referenced projects (as well as the project itself).
- Should
System.Diagnostics.Tracingtests be run for AOT? It is disabled right now.I don't know. @mdh1418 Should it?
Ah, I thought they were re-enabled in https://github.com/dotnet/runtime/pull/72545 from one of the earlier commits but looks like I missed that a later commit only re-enabled it for iOS (should have reenabled for tvOS too). That doesn't seem to change anything testing wise on CI since we only run System.Runtime.Tests.csproj for iOS due to many timeouts in the past from overloading iOS capacity, and we generally do all of the testing on tvOS.
I'm not sure why it is disabled for Browser wasm.
https://github.com/dotnet/runtime/pull/73581 will check if the suite passes on atleast tvOS
Looks like the wasm errors are this: https://github.com/dotnet/linker/issues/2963
@naricc please open issues for XA and XI to add the component by default!
@lambdageek https://github.com/xamarin/xamarin-android/issues/7249 https://github.com/xamarin/xamarin-macios/issues/15668
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.
