Preliminary Interpreter Support by tonerdo · Pull Request #6182 · dotnet/corert

tonerdo · 2018-08-05T23:25:20Z

This PR includes the preliminary work needed to integrate an interpreter into CoreRT to support runtime code generation scenarios.

To test the current interpreter status:

Create a new console project with a main method with the following signature and code

public static int Main()
{
    return 100;
}

You can return any arbitrary integer value, the project must be compiled in Release mode because the interpreter currently only supports ldc.i* and ret opcodes and Debug mode introduces ldloc and stloc opcodes. This will cease to be a requirement once the interpreter supports more opcodes.

Create a new CoreRT console project and include the following code in its Main method

static void Main(string[] args)
{
    byte[] buffer = File.ReadAllBytes(@"path\to\above\release\mode\assembly.dll");
    var assembly = Assembly.Load(buffer);
    var entryPoint = assembly.EntryPoint;
    int code = (int)entryPoint.Invoke(null, null);
    Console.WriteLine(code);
}

This code loads the assembly in (1), retrieves its EntryPoint and invokes it at runtime all within the CoreRT runtime with the help of the interpreter. Add an rd.xml file to the CoreRT project with the following contents:

<Directives>
  <Application>
    <Assembly Name="System.Runtime" Dynamic="Required All" />
  </Application>
</Directives>

Include the rd.xml in the build pipeline by adding it to the CoreRT csproj:

<ItemGroup>
  <RdXmlFile Include="rd.xml" />
</ItemGroup>

Compile the CoreRT app with the experimental interpreter enabled

dotnet build /t:LinkNative /p:ExperimentalInterpreterSupport=true

Run the resulting native executable. If all goes well, 100 will be printed to the terminal. You can update the return code of the main method in (1) for further tests. You won't need to rebuild the CoreRT app, just the assembly that is loaded.

The interpreter work is still in very early stages and I intend to put in further work to add support for all CLR opcodes. Dynamically loading a .NET assembly at runtime is the current scenario being tested, after which the Reflection.Emit API can be updated to take advantage of the interpreter functionality.

#5011

cc @jkotas @MichalStrehovsky

…#5011)

morganbr

This looks like a great start, @tonerdo! I'm looking forward to seeing where the interpreter goes

morganbr · 2018-08-06T00:28:26Z

-                        PEInfo peinfo = new PEInfo(refName, null, null);
-                        s_ecmaLoadedAssemblies.Add(peinfo);
-                    }
+                    // if (!foundMatch)


Should this code be put back?

The interpreter is gonna fail if this is still in there. Can't really say whether the code is essential for other scenarios which is why I just left it commented out for now

Someone will need to understand the code. I didn't have a chance to look into it deeply yet. Maybe @davidwrighton could help us here? This code was added as part of support for satellite assemblies, but breaks the Assembly.Load(byte[]) scenario.

I'll admit that I don't know what satellite assemblies are. What scenarios are they typically used?

https://blogs.msdn.microsoft.com/global_developer/2011/07/22/introduction-to-satellite-assemblies/ - you can put managed resourced for a specific locale in them. I'm not sure if this actually works with the CoreRT compiler, but it's a supported scenario for .NET Native for UWP apps (this code is shared with the closed source runtime/compiler).

Sadly, this is necessary for satellite assembly work. What you'll need to do is add a Boolean flag to the function to indicate whether or not the caching should happen, and then thread it through such that for byte[] loads, the failure isn't cached when we do the speculative lookup to see if the assembly has already been loaded.

morganbr · 2018-08-06T00:31:07Z

+        {
+            get
+            {
+                LocalVariableType[] localVariableTypes = new LocalVariableType[_method.Signature.Length + 1];


For instance methods, you'll likely need another slot (unless the call interceptor does that for you)

Hmmm, that's true. Didn't realize I'll have to explicitly handle the this parameter. I'll leave it up to future testing to surface the need for it

morganbr · 2018-08-06T00:35:20Z

+        private readonly MethodDesc _method;
+        private readonly MethodIL _methodIL;
+        private readonly TypeSystemContext _context;
+        private readonly LowLevelStack<StackItem> _stack;


I think @jkotas had ideas on what we'd need to do to make the evaluation stack work for byref types, so this might have to change at some point (it's fine for now).

MichalStrehovsky · 2018-08-06T12:57:08Z

            ExecutionEnvironment = executionEnvironment;
-
-#if SUPPORT_JIT
+#if SUPPORT_INTERPRETER


This means that the .Experimental version will no longer work with the JIT prototype (both SUPPORT_INTERPRETER and SUPPORT_JIT are defined there).

I think the fix should be to move the initialization of GlobalExecutionStrategy out of reflection execution initialization, into Jit/Interpreter initialization. To do that, you would add this to both the S.P.Interpreter and S.P.Jit projects (with the obvious tweak):

namespace Internal.Runtime.CompilerHelpers { public class LibraryInitializer { public static void InitializeLibrary() { Internal.Runtime.TypeLoader.MethodExecutionStrategy.GlobalExecutionStrategy = new Internal.Runtime.Interpreter.InterpreterExecutionStrategy(); } } }

The compiler will make sure this is called at startup because S.P.Interpreter is one of the AutoInitializedAssemblies.

MichalStrehovsky · 2018-08-06T12:57:45Z

      <Link>System\Collections\Generic\LowLevelDictionary.cs</Link>
    </Compile>
+    <Compile Include="..\..\Common\src\System\Collections\Generic\LowLevelStack.cs">
+      <Link>System\Collections\Generic\LowLevelStack.cs</Link>


Do we need this in CoreLib?

No we don't. I added it to be consistent with the inclusion of LowLevelList. I'll remove it since we don't need it

MichalStrehovsky · 2018-08-06T13:14:12Z

    <AssemblyName>System.Private.Reflection.Core.Experimental</AssemblyName>
    <EcmaMetadataSupport>true</EcmaMetadataSupport>
    <JitSupport>true</JitSupport>
+    <InterpreterSupport>true</InterpreterSupport>


I'm wondering if we should just call this ExperimentalSupport/DynamicCodeSupport, or something like that, and replace JitSupport with that single flag. It doesn't seem like these assemblies need to know the details of the execution strategy, besides knowing that they have to reference the .Experimental flavors of the assemblies.

Sounds good

On second look, it seems there are some parts of S.P.Typeloader that are conditionally compiled based on the SUPPORT_JIT constant being set when JitSupport is true. We ideally should not have these parts when we're just interested in having an interpreter which will be the case if we use a single tag to encompass both JIT and Interpreter support

On a third look, nevermind 😄

MichalStrehovsky · 2018-08-06T13:16:04Z

            DynamicMethodHandleInfo* methodData = (DynamicMethodHandleInfo*)runtimeMethodHandleValue.ToPointer();
            declaringTypeHandle = *(RuntimeTypeHandle*)&(methodData->DeclaringType);
-            genericMethodArgs = null;
+            genericMethodArgs = new RuntimeTypeHandle[0];


Might want to replace this with Array.Empty<RuntimeTypeHandle> to avoid the allocation. Do you remember where you hit this?

Good point. I believe I hit this issue when I called ToMethodDesc on TypeSystemExtensions in my execution strategy. genericMethodArgs is an out parameter and although it's always null we unconditionally try to access the Length property here: https://github.com/dotnet/corert/blob/master/src/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeSystemExtensions.cs#L148. I was always getting an access violation exception

)

…net#5011)

davidwrighton

Looks great, but you need to avoid breaking the semantic that Assembly.Load operations cannot behave differently once they have settled on a result.

davidwrighton · 2018-08-07T17:57:22Z

-                        PEInfo peinfo = new PEInfo(refName, null, null);
-                        s_ecmaLoadedAssemblies.Add(peinfo);
-                    }
+                    // if (!foundMatch)


Sadly, this is necessary for satellite assembly work. What you'll need to do is add a Boolean flag to the function to indicate whether or not the caching should happen, and then thread it through such that for byte[] loads, the failure isn't cached when we do the speculative lookup to see if the assembly has already been loaded.

tonerdo · 2018-08-07T22:26:05Z

@davidwrighton take a look at my latest commit: 3cea4e0. Is this what you had in mind?

MichalStrehovsky · 2018-08-09T09:09:42Z

@davidwrighton take a look at my latest commit: 3cea4e0. Is this what you had in mind?

Do we need the new parameter in method signatures that have the byte[] parameter in it? Shouldn't those assume cacheMissedLookups is false?

We also have a convention to use the named argument syntax for bool arguments that pass literal bool values (so cacheMissedLookups: false instead of just false to make it more obvious what the false means).

…#5011)

davidwrighton · 2018-08-10T22:52:17Z

@tonerdo That's generally what I had in mind, but you've passed false everywhere. The proper behavior is to cache missed lookups essentially everywhere except where it causes the byte[] scenario to fail to load entirely.

So, I believe you need to pass true in every single place you marked as passing false, except for the first call in BindEcmaByteArray, which should pass false.

wjk · 2018-08-12T19:54:57Z

I have a question about this PR. Is there any reason why the JIT support (in System.Private.Jit) is not being used/developed more? While a CIL interpreter is useful in some cases (namely, environments such as iOS that don't support runtime JIT compilation), I think that using the existing CoreCLR-derived JIT will be both faster at runtime, and far quicker to implement (because we would be building off of the existing infrastructure).

Looking at the commit history for System.Private.Jit, I don't think that JIT support has been finished, but I don't know enough about CoreRT's innards to be sure. If there was a choice, I would prefer to use the JIT, but of course I don't know enough about the design decisions made for that to be sure. OTOH, having both choices available certainly doesn't hurt! Thanks for clearing this up!

MichalStrehovsky

Looks good to me modulo the two small things.

MichalStrehovsky · 2018-08-13T08:41:50Z

+    <OutputType>Library</OutputType>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+  </PropertyGroup>
+  <ItemGroup>


Could you make this conditioned on Condition="'$(IsProjectNLibrary)' != 'true'", same as the other CSPROJ files? We need this to keep things buildable on the closed-source side.

MichalStrehovsky · 2018-08-13T08:42:52Z

    <Compile Include="Internal\Runtime\JitSupport\RyuJitExecutionStrategy.cs" />
    <Compile Include="Internal\Runtime\JitSupport\VirtualMethodSlotHelper.cs" />
  </ItemGroup>
+  <ItemGroup />


Nit: unnecessary ItemGroup

wjk · 2018-08-14T01:55:50Z

You can ignore my previous question re System.Private.Jit, because I found the answer while reading #5011. Thanks!

tonerdo · 2018-08-14T16:04:52Z

@MichalStrehovsky Anything left for me to do on this?

MichalStrehovsky · 2018-08-14T16:07:15Z

@MichalStrehovsky Anything left for me to do on this?

It looks good to me - I'll merge this when the CI is finishes.

MichalStrehovsky · 2018-08-14T16:52:14Z

@tonerdo Thanks for the patience! Now that the scaffolding is in place, the work within System.Private.Interpreter should go through reviews faster.

The change to return an empty array instead of null broke a method handle test. Seems like generic arguments being null is a contract. Restoring the old logic and adding handling of null to the place that promted the change (#6182 (comment)). [tfs-changeset: 1711212]

@tonerdo

The JIT prototype was originally brought up on the closed source ".NET Native for UWP apps" compiler/runtime, so this is mostly to adapt it for CoreRT. Surprisingly, this hasn't bit rotten much over time and some of the bitrot has been cleaned up by @tonerdo in his interpreter work (dotnet#6182). Both an interpreter and a JIT are a great addition to a fully AOT compiled runtime and they complement each other nicely. With this, I'm making the JIT prototype work on CoreRT: * Adjust the hacks that were used by JIT code manager to talk to the runtime (in CoreRT, runtime is linked into the executable and the hacks didn't work). This will need further cleanup at some point. * Rename ReaderWriterLock because it was conflicting with a type of the same name in the runtime. * Add an experimental flavor of the stack trace decoder. This needs to use the experimental type loader. * Expose the JIT component from MSBuild - compile JIT support into the executable, use the experimental flavor of the reflection stack (shared with the interpreter), add the extra native library with JIT support. This is to allow enabling JIT support as part of `dotnet publish`. You can try this out pretty much the same way as @tonerdo's interpreter: see instructions in dotnet#6182. Instead of passing `/p:ExperimentalInterpreterSupport=true`, pass `/p:ExperimentalJitSupport=true`. You'll need to manually copy clrjitilc.dll from the compiler to the publish directory (we use the same codegen that is used to compile the AOT parts of the application to also JIT compile at runtime).

@tonerdo

The JIT prototype was originally brought up on the closed source ".NET Native for UWP apps" compiler/runtime, so this is mostly to adapt it for CoreRT. Surprisingly, this hasn't bit rotten much over time and some of the bitrot has been cleaned up by @tonerdo in his interpreter work (#6182). Both an interpreter and a JIT are a great addition to a fully AOT compiled runtime and they complement each other nicely. With this, I'm making the JIT prototype work on CoreRT: * Adjust the hacks that were used by JIT code manager to talk to the runtime (in CoreRT, runtime is linked into the executable and the hacks didn't work). This will need further cleanup at some point. * Rename ReaderWriterLock because it was conflicting with a type of the same name in the runtime. * Add an experimental flavor of the stack trace decoder. This needs to use the experimental type loader. * Expose the JIT component from MSBuild - compile JIT support into the executable, use the experimental flavor of the reflection stack (shared with the interpreter), add the extra native library with JIT support. This is to allow enabling JIT support as part of `dotnet publish`. You can try this out pretty much the same way as @tonerdo's interpreter: see instructions in #6182. Instead of passing `/p:ExperimentalInterpreterSupport=true`, pass `/p:ExperimentalJitSupport=true`. You'll need to manually copy clrjitilc.dll from the compiler to the publish directory (we use the same codegen that is used to compile the AOT parts of the application to also JIT compile at runtime).

drearyrainDeng · 2020-03-05T01:14:05Z

publish the console like that "dotnet publish -r win-x64", it works normal. so is it default to use "/p:ExperimentalInterpreterSupport=true" for now?

drearyrainDeng · 2020-04-01T08:13:35Z

i found the reason is that i added rd.xml (<Assembly Name="System.Runtime" Dynamic="Required All" />). is it normal ? @tonerdo

tonerdo added 16 commits July 21, 2018 16:39

add preliminary scaffolding (dotnet#5011)

ce92c8f

rename MethodInterpreter to MethodCallInterceptor (dotnet#5011)

b300b52

reference interpreter project from ReflectionExcution project (dotnet…

8cc9a18

…#5011)

rename MethodCallInterceptor to InterpreterCallInterceptor (dotnet#5011)

7743030

add preliminary InterpreterExecutionStrategy class (dotnet#5011)

332c0ff

fix interpreter project build reference errors (dotnet#5011)

dfee6b1

add implementations for InterpreterCallInterceptor properties (dotnet…

bcc5663

…#5011)

return interceptor thunk address in execution strategy (dotnet#5011)

0bc3101

add preliminary ILInterpreter class (dotnet#5011)

ae66b8e

add ILInstruction class (dotnet#5011)

08eac4b

add LowLevelStack class (dotnet#5011)

5bb44d7

add Evaluator class with support for ldc and ret opcodes (dotnet#5011)

65e3252

rename Evaluator class to ILEvaluator (dotnet#5011)

fe8330b

rename ILEvaluator to ILEvaluation (dotnet#5011)

59a2df4

integrate interpreter with reflection execution pipeline (dotnet#5011)

6814d3d

complete preliminary implementation of interpreter (dotnet#5011)

7c33c38

tonerdo mentioned this pull request Aug 5, 2018

Support for IL interpreter in CoreRT #5011

Open

morganbr approved these changes Aug 6, 2018

View reviewed changes

morganbr requested review from MichalStrehovsky and jkotas August 6, 2018 00:37

clean up native build script (dotnet#5011)

976ca8b

MichalStrehovsky reviewed Aug 6, 2018

View reviewed changes

tonerdo added 2 commits August 6, 2018 18:02

switch to LibraryInitializers to setup execution strategies (dotnet#5011

cb72a01

)

use dynamic code to represent both interpreter and JIT scenarios (dot…

6a14ae1

…net#5011)

davidwrighton suggested changes Aug 7, 2018

View reviewed changes

introduce cacheMissedLookups flag

3cea4e0

tonerdo added 2 commits August 10, 2018 11:31

remove cacheMissedLookups parameter from byte[] methods (dotnet#5011)

4ff9142

use named argument syntax to pass cacheMissedLookups paramter (dotnet…

6491231

…#5011)

default to chacing missed lookups (dotnet#5011)

3c3ec24

MichalStrehovsky approved these changes Aug 13, 2018

View reviewed changes

ensure compatibility with .NET Native (dotnet#5011)

38298b9

tonerdo closed this Aug 14, 2018

tonerdo reopened this Aug 14, 2018

MichalStrehovsky merged commit c214863 into dotnet:master Aug 14, 2018

MichalStrehovsky mentioned this pull request Sep 13, 2018

Restore JIT prototype #6327

Merged

MichalStrehovsky mentioned this pull request Jan 17, 2019

Enable detection of HW intrinsics #6836

Merged

MichalStrehovsky mentioned this pull request Apr 15, 2020

[Question] Assembly loading #6949

Open

Conversation

tonerdo commented Aug 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morganbr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwrighton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonerdo commented Aug 7, 2018

Uh oh!

MichalStrehovsky commented Aug 9, 2018

Uh oh!

davidwrighton commented Aug 10, 2018

Uh oh!

wjk commented Aug 12, 2018

Uh oh!

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjk commented Aug 14, 2018

Uh oh!

tonerdo commented Aug 14, 2018

Uh oh!

MichalStrehovsky commented Aug 14, 2018

Uh oh!

MichalStrehovsky commented Aug 14, 2018

Uh oh!

drearyrainDeng commented Mar 5, 2020

Uh oh!

drearyrainDeng commented Apr 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

tonerdo commented Aug 5, 2018 •

edited

Loading