Add ability to disable reflection emit for testing

[davidfowl]

When trying to target platforms without ref emit support, it would be nice to run CoreCLR in a mode with it disabled so it would be possible to write unit tests to see if your library breaks or not. This mode should also cause RuntimeFeature.IsDynamicCodeCompiled to return false.

cc @jkotas

[jkotas]

@eerhardt @vitek-karas Would it make sense to make this linker and feature switch bundle, similar to what we are doing elsewhere?

[eerhardt]

I’m not sure using the linker would result in the end-user experience being asked for here. Someone would need to run the linker on the tests - it only runs during publish - and the app/test would need to be self contained.

This mode should also cause RuntimeFeature.IsDynamicCodeCompiled to return false.

You could do this today with the linker. You would just need to pass a substitution file to the linker that had the following:

runtime/src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.wasm.xml

Lines 9 to 10 in 3a4298c

	<type fullname="System.Runtime.CompilerServices.RuntimeFeature">
	<method signature="System.Boolean get_IsDynamicCodeCompiled()" body="stub" value="false" />

[davidfowl]

The end to end here it so be able to have confidence to run on a platform like unity. That's why I think it should be a runtime mode. Something I can turn on in tests with an environment variable or something.

Though, maybe It'll break the test runner as well 😄

[jkotas]

I’m not sure using the linker would result in the end-user experience being asked for here.

I agree that exposing it only via linker would not help. That's why I have said linked and feature switch bundle, for example like the binary serialization switch. You can turn the feature off via AppContext, you can tell linker to strip the feature code, and some platforms have the feature turned off unconditionally.

[eerhardt]

The end to end here it so be able to have confidence to run on a platform like unity.

Probably a naive question, but why not test it directly on unity? That’s probably the only way you’d have confidence it works on that platform. Ref emit is just one of many things that will be different.

[GrabYourPitchforks]

I agree with @eerhardt that running on Unity is the only way to be 100% certain. But there is something very nice about being able to use our existing dev environment for rapid prototyping. Assuming our normal dev environment would detect a significant percentage (50%? 90%?) of these problems I'd consider that a win.

[davidfowl]

The same reason I don't want to test on Xamarin, it requires a new toolchain, setup, sometimes devices. The idea here is to get closer to what the actual target device/environment is without needing one. Sure you can say "if I don't test on the platform then I can't know it'll work there" but given my experience 90% of the code just works and there are known missing capabilities of the underlying platform that we workaround (the code even works in WASM and we don't explicit test there!).

I also think running tests after linking is super important as part of this overall story

[vitek-karas]

The feature switch solution as @jkotas suggested should work nicely. Whether it's bound to linker or not is secondary (but if we do it, we should do it for linker as well, just for completeness). It should just mean defining a new runtime property and wiring it into the RuntimeFeatures.IsDynamicCodeCompiled. This will obviously not guarantee that the runtime will not allow usage of Ref.Emit - this will only be as good as the code which checks that property. So there might be some followup work to have more pieces of the framework check that property and fail/change behavior as necessary.

[steveharter]

I agree that exposing it only via linker would not help. That's why I have said linked and feature switch bundle, for example like the binary serialization switch. You can turn the feature off via AppContext, you can tell linker to strip the feature code, and some platforms have the feature turned off unconditionally.

Yes even if RuntimeFeatures.IsDynamicCodeCompiled returns false, the Emit code will still there for CoreCLR and will get executed if called. So the source would need to be stripped out, or have the source check for that variable (if a more dynamic approach is wanted).

[steveharter]

Moving to future; this week is the last week before ask mode.

[steveharter]

Evaluated - keeping in future; considered lower priority.

The likely approach is:

• Add a new feature switch
• Have RuntimeFeatures.IsDynamicCodeCompiled and IsDynamicCodeSupported return false if the switch is on.
• Disable Emit by having the root classes detect the new feature switch (DynamicMethod and AssemblyBuilder?)

[eerhardt]

I will look to implement this in the next couple weeks. We have identified a couple areas where this functionality will be helpful for ASP.NET.

Do we have an idea for a name of the feature switch? Names I could imagine are:

• System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported
• System.Runtime.CompilerServices.RuntimeFeature.DynamicCode.IsSupported

The XXX.IsSupported matches existing feature switch name format: https://github.com/dotnet/runtime/blob/main/docs/workflow/trimming/feature-switches.md. But the IsXXXSupported matches the property name.

[jkotas]

I think it should be System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported.

Feature switch names have been trying to mirror managed API names. There are several cases where we even have both managed API and feature switch of the same name (e.g. MetadataUpdater.IsSupported). A lot of existing feature switches control the whole type that gives you the <typename>.IsSupported name. It is not the case here. Dynamic code is spread over many different types and namespaces.

[davidfowl]

I agree with @jkotas

[eerhardt]

@jkotas @marek-safar @lambdageek @vargaz - Should the Mono runtime also support this feature switch? My assumption is "yes", but I wanted to check first before doing the work.

There will be scenarios where this feature switch won't work. For example:

When in "AOT" (either NativeAOT or Mono AOT), and setting the switch to System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported=true, the switch won't be honored. You can't make dynamic code be supported when it isn't.

Scenarios where the switch will work:

• CoreCLR JIT
• Mono JIT
• Mono Interpreter

In these scenarios, IsDynamicCodeSupported is hard-coded to true. Setting the feature switch to false will turn it off. It will also set IsDynamicCodeCompiled to false as well, since it doesn't make sense to have IsDynamicCodeCompiled=true when IsDynamicCodeSupported=false.

[eerhardt]

One more "requirements" question - Should this switch actually make new DynamicMethod(...) and AssemblyBuilder.DefineDynamicAssembly(...) throw exceptions when invoked and IsDynamicCodeSupported == false?

[vargaz]

These switches already exist, i.e.:

runtime/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeFeature.Mono.cs

Line 7 in 517698d

public partial class RuntimeFeature

[eerhardt]

These switches already exist

The ask is to be able to configure the values of these switches. For example, run using CoreCLR's (or Mono's) JIT runtime, but with RuntimeFeature.IsDynamicCodeSupported set to false.

[vargaz]

It's doable in mono with not much work i think.

[marek-safar]

Should the Mono runtime also support this feature switch?

Yes

[lambdageek]

We (Eric, Zoltan and I) discussed support for this in Mono on Discord.

The approach we will try is:

• Add code in monovm_initialize (in parse_properites) to set a flag in Mono if IsDynamicCodeSupported property is changed from the default.
• In code that tries to produce an intrinsic for get_IsDynamicCodeSupported: if the "non-default" flag is unset, then only treat get_IsDynamicCodeSupported as an intrinsic if we're in JIT compilation mode, or if we're in full AOT compilation mode without interpreter fallback.
• Otherwise just use the C# implementation of get_IsDynamicCodeSupported - (shared with CoreCLR).

The complication is scenarios like profiled AOT. In profiled AOT normally get_IsDynamicCodeSupported would return true (dynamic code might be supported by the JIT or interpreter execution engines). In that case code that is checking the IsDynamicCodeSupported might or might not be in the profile. if it is in the profile, it would be wrong for get_IsDynamicCodeSupported to be an intrinsic since at execution time someone could set the feature flag to false.
The only situation where we could intrinsify the flag is if we're in the fallback execution engine (JIT or interp) or if we're in AOTed code and we know there's no fallback execution engine (this is the "full AOT without interp" scenario).

[jkotas]

One more "requirements" question - Should this switch actually make new DynamicMethod(...) and AssemblyBuilder.DefineDynamicAssembly(...) throw exceptions when invoked and IsDynamicCodeSupported == false?

I see good arguments for both doing this (allow certain mistakes to be caught earlier) and not doing this (this instrumentation is not 100% reliable - we won't be able to instrument APIs like Type.MakeGenericType in similar way).

What would you propose?

[eerhardt]

What would you propose?

My proposal is that it throws PlatformNotSupportedException when you try to create a DynamicMethod or AssemblyBuilder.DefineDynamicAssembly when IsDynamicCodeSupported ==false. If you are setting this feature switch (and PublishAot=true in your .csproj will default this switch to false), you are saying you want to run your app like it would when publishing NativeAOT.