Use precompiled PInvoke stubs from R2R image without the shared IL stub #101

fadimounir · 2019-11-18T16:59:57Z

Taking @MichalStrehovsky's work from MichalStrehovsky/coreclr@ffc8fff and completing the remaining portions.

Changes include some refactoring work around the signature parsing to compute marshalling requirements. We had two separate implementations after pulling the PInvoke work from the single-exe branch. Consolidated into one implementation.

cc @sergiy-k @dotnet/crossgen-contrib

fadimounir · 2019-11-18T17:03:19Z

src/coreclr/src/inc/readytorun.h

+    READYTORUN_FLAG_PLATFORM_NEUTRAL_SOURCE     = 0x00000001,   // Set if the original IL assembly was platform-neutral
+    READYTORUN_FLAG_SKIP_TYPE_VALIDATION        = 0x00000002,   // Set of methods with native code was determined using profile data
+    READYTORUN_FLAG_PARTIAL                     = 0x00000004,
+    READYTORUN_FLAG_CROSSGEN2_IMAGE             = 0x00000008,   // Set if image was compiled using crossgen2


The name of this flag will change of course once we settle on the final crossgen2 name

I don't like having a name that is tied to crossgen2 at all. These flags are about the format. Something like AOT_DIRECT_PINVOKE_STUBS seems better to me.

I have suggested this flag to Fadi. My thinking was that the flag will disappear once we get rid of the old crossgen.

I completely agree this flag currently looks pretty ugly (i've spent some time thinking about this too as I was writing it), but I do see it as a temporary flag.
We might end up adding some more cases where we do something different with crossgen2 compared to the old crossgen, and it would be better to not add a new flag every time we do that. For this reason, I think we should keep the flag the way it is. @davidwrighton thoughts?

I don't think we'll be rid of this flag any time soon.
I feel that

Crossgen won't disappear within a year

Having the flag be specific to some characteristic of the r2r file is straightforward, and allows us to adjust crossgen if we want to. (For example if we get rid of shared IL stubs in the runtime, we'll probably also want to delete them from crossgen even if we aren't doing active feature development there, and having the crossgen2 flag set by crossgen would be very confusing.)

If we change major version of r2r after we establish crossgen2, we can get rid of the flag then.

Going with READYTORUN_NON_SHARED_PINVOKE_STUBS

fadimounir · 2019-11-18T17:05:51Z

...s/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunSingleAssemblyCompilationModuleGroup.cs

-            return _versionBubbleModuleSet.Contains(((EcmaMethod)method).Module)
-                && _versionBubbleModuleSet.Contains(method.Context.SystemModule);
+
+            if (((EcmaMethod)method).Module.Equals(method.Context.SystemModule))


I've tested a small scenario where we enable these pinvokes that require marshalling when corelib is part of the version bubble, and it works correctly (and gets inlined). I didn't enable this scenario though in this PR based on what @davidwrighton and I were discussing, but it should be totally doable.

src/coreclr/src/tools/crossgen2/Common/TypeSystem/Interop/IL/Marshaller.cs

davidwrighton

Please fix the naming, otherwise, this looks good.

src/coreclr/src/tools/crossgen2/Common/TypeSystem/Interop/IL/Marshaller.cs

davidwrighton · 2019-11-19T00:27:11Z

src/coreclr/src/inc/readytorun.h

+    READYTORUN_FLAG_PLATFORM_NEUTRAL_SOURCE     = 0x00000001,   // Set if the original IL assembly was platform-neutral
+    READYTORUN_FLAG_SKIP_TYPE_VALIDATION        = 0x00000002,   // Set of methods with native code was determined using profile data
+    READYTORUN_FLAG_PARTIAL                     = 0x00000004,
+    READYTORUN_FLAG_CROSSGEN2_IMAGE             = 0x00000008,   // Set if image was compiled using crossgen2


I don't like having a name that is tied to crossgen2 at all. These flags are about the format. Something like AOT_DIRECT_PINVOKE_STUBS seems better to me.

src/coreclr/src/jit/importer.cpp

src/coreclr/src/tools/crossgen2/Common/Internal/Runtime/ModuleHeaders.cs

src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs

jkotas · 2019-11-19T02:42:58Z

src/coreclr/src/tools/crossgen2/Common/TypeSystem/Interop/IL/Marshaller.cs

@@ -70,6 +70,8 @@ public enum MarshallerType
    // and also argument specific marshalling informaiton
    abstract partial class Marshaller
    {
+        static MethodMarshallersCache s_marshallersCache = new MethodMarshallersCache();


changing IL stream addresses has a side effect on the inlining behavior of the JIT

Could you please elaborate on this?

This static cache should not be needed. If it is needed, it is working around about more fundamental problem somewhere else.

I removed the static cache here already. What we had discovered through testing is that having multiple IL streams for the same method has a side effect of having multiple pinning addresses given back to the jit, which could affect inlining behavior.

I removed the static cache here already.

I'm only on my phone but s_marshallersCache I still see here.

:( Sorry for the confusion... I had added 2 caches, one for marshallers and one for the emitted IL. I removed the latter, but thought I removed the one in marshallers too. I'll fix that.

This is only for images compiled with crossgen2. Changes include some refactoring work around the signature parsing to compute marshalling requirements. We had two separate implementations after pulling the PInvoke work from the single-exe branch. Consolidated into one implementation.

MichalStrehovsky · 2019-11-20T20:18:05Z

src/coreclr/src/tools/crossgen2/Common/TypeSystem/Interop/IL/Marshaller.cs

@@ -70,6 +70,8 @@ public enum MarshallerType
    // and also argument specific marshalling informaiton
    abstract partial class Marshaller
    {
+        static MethodMarshallersCache s_marshallersCache = new MethodMarshallersCache();


I removed the static cache here already.

I'm only on my phone but s_marshallersCache I still see here.

MichalStrehovsky · 2019-11-20T20:22:24Z

src/coreclr/src/tools/crossgen2/Common/TypeSystem/Interop/IL/Marshaller.cs

+        {
+            Debug.Assert(targetMethod.IsPInvoke);
+
+            if (targetMethod.GetPInvokeMethodMetadata().Flags.SetLastError)


This should also check for the PreserveSig bit that is getting deleted.

I've added that flag to the type system in #27286

@MichalStrehovsky, I'm noticing a lot of code from dotnet/coreclr#27286 were never included in your dotnet/coreclr#27585 PR to move the PInvoke stuff out of the single-exe branch. Was that intentional?

The PreserveSig is part of the things that were left out.

If that wasn't intentional, and the changes in dotnet/coreclr#27286 need to be in the master branch as well, I'll port them from single-exe in a separate PR.

Sigh. I don't know how that happened. Yeah, those are all good bugfixes that should be moved over. Everything I touched in the single-exe branch should move.

Can you please check if thats the only thing? I would hate if a bugfix I already did in single-exe ends up having to be reinvestigated because I forgot it in the branch

Yep, I’ll look at the history and port the missing stuff over in new PRs

Found the following commits that will need to be ported:
dotnet/coreclr@4684d8f
dotnet/coreclr@7528827
dotnet/coreclr@52714c6
dotnet/coreclr@1d2ef20
dotnet/coreclr@24cdf66

I'll bring them to master. What tests were you running made you find and fix these bugs?

The pinvoke interop tests in the coreclr test tree.

Sorry about this. I must have used a stale version of the single-exe branch. Doing this work in the branch caused nothing but trouble.

Latest Roslyn builds started optimizing (is it really an optimization?) generic virtual method calls into more specific methods. ```diff .method public hidebysig virtual instance !!T Alloc<.ctor T>() cil managed { // Code size 11 (0xb) .maxstack 8 IL_0000: newobj instance void My/AllocViaSecondLevelDerived::.ctor() - IL_0005: callvirt instance !!0 My/AllocViaGVMSecondLevelBase::ActuallyAlloc<!!0>() + IL_0005: callvirt instance !!0 My/AllocViaSecondLevelDerived::ActuallyAlloc<!!0>() IL_000a: ret } // end of method AllocViaGVMDerived::Alloc ``` This breaks generic virtual method resolution because the resolution assumes we're looking for the slot defining method. The fix is to normalize to the handle of the slot defining method during codegen. I hit this in the DynamicGenerics test, but I'm adding more coverage to the Generics test because the newly added logic that walks base hierarchies wouldn't be covered by the existing test. This also required a RyuJIT fix, but that one turned out to be a nice simplification.

Dotnet-GitSync-Bot added the area-ReadyToRun-coreclr label Nov 18, 2019

fadimounir requested a review from davidwrighton November 18, 2019 17:00

fadimounir commented Nov 18, 2019

View reviewed changes

MichalStrehovsky reviewed Nov 18, 2019

View reviewed changes

src/coreclr/src/tools/crossgen2/Common/TypeSystem/Interop/IL/Marshaller.cs Outdated Show resolved Hide resolved

src/coreclr/src/tools/crossgen2/Common/TypeSystem/Interop/IL/Marshaller.cs Outdated Show resolved Hide resolved

davidwrighton requested changes Nov 19, 2019

View reviewed changes

jkotas reviewed Nov 19, 2019

View reviewed changes

src/coreclr/src/jit/importer.cpp Outdated Show resolved Hide resolved

jkotas reviewed Nov 19, 2019

View reviewed changes

src/coreclr/src/tools/crossgen2/Common/Internal/Runtime/ModuleHeaders.cs Outdated Show resolved Hide resolved

jkotas reviewed Nov 19, 2019

View reviewed changes

src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs Outdated Show resolved Hide resolved

jkotas reviewed Nov 19, 2019

View reviewed changes

jkotas approved these changes Nov 20, 2019

View reviewed changes

Fadi Hanna added 2 commits November 20, 2019 10:19

CR feedback

69bcb8b

MichalStrehovsky reviewed Nov 20, 2019

View reviewed changes

Remove MarshallersCache.cs

b4210a1

fadimounir merged commit 6c93838 into dotnet:master Nov 21, 2019

fadimounir deleted the NonShareablePInvokeStubs branch November 27, 2019 02:04

karelz added this to the 5.0.0 milestone Aug 18, 2020

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

Use precompiled PInvoke stubs from R2R image without the shared IL stub #101

Use precompiled PInvoke stubs from R2R image without the shared IL stub #101

Uh oh!

Conversation

fadimounir commented Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

fadimounir Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

davidwrighton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

fadimounir Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fadimounir commented Nov 18, 2019 •

edited

Loading

fadimounir Nov 19, 2019 •

edited

Loading

fadimounir Nov 20, 2019 •

edited

Loading