Multi-module framework assembly compilation #2126

nattress · 2016-11-02T23:18:47Z

Enable compiling all framework / SDK assemblies in multi-module library
mode

Add a work-around to compile System.Console as a library in PInvokeILEmitter.cs

System.Console has a p/invoke to GetLargestConsoleWindowSize which
returns a COORD struct containing two 16 byte ints. The struct return
type prevents the JIT from emitting an inline p/invoke in the IL stub we
generate and instead it treats the external p/invoke target as a managed
call and tries to compile it. This work-around alters the return type
marshalling to be a 32bit int and allows the JIT to generate a proper
unmanaged call.

Fix bug in TypeSystemThrowingILEmitter where a null message argument
caused the jit to fail creating a string literal

Address library module compilation failures due to unsuccessful type
loads by checking on EEType creation whether the type can be loaded
sufficiently to write out the EETypeNode. Code for this stolen from one of Michal's previous PRs :)

Eagerly create EETypeNodes in ReadyToRunHelperNode so the failure point
happens during the referencing method compilation, not later on when
emitting the graph

Check method signatures on library compilation method rooting

Methods whose signatures contain types that cannot be loaded (they are
in a separate, non-existent assembly for example) should be skipped
since we cannot even replace their IL body with a throw helper. The JIT
will fail trying to getClassAttribs on the argument types.

This check is done when rooting methods instead of in MethodCodeNode's
constructor because MethodCodeNodes are created during graph sweeping
for things like virtual method combined dependencies.

Add a test that verifies we can compile all framework assemblies to their own separate object files successfully.

jkotas · 2016-11-02T23:40:36Z

The struct return type prevents the JIT from emitting an inline p/invoke

This sounds like a JIT bug - we should get a bug on it. The JIT should be able to inline p/invoke with struct return just fine.

MichalStrehovsky · 2016-11-02T23:45:51Z

src/Common/src/TypeSystem/IL/Stubs/TypeSystemThrowingILEmitter.cs

@@ -61,7 +61,7 @@ public static MethodIL EmitIL(MethodDesc methodThatShouldThrow, TypeSystemExcept

            foreach (var arg in exception.Arguments)
            {
-                codeStream.Emit(ILOpcode.ldstr, emitter.NewToken(arg));
+                codeStream.Emit(ILOpcode.ldstr, emitter.NewToken(arg ?? ""));


How are we ending up with a null string here? That kind of sounds like a bug.

The messageArg argument to TypeLoadException defaults to null. We could change the default to empty string in the constructor to TypeLoadException instead. I don't mind either way; I think this combined with the assert you added to NewToken is fine

This means we're passing more formatting arguments than the exception wants. Are you sure this won't throw at runtime while trying to construct the exception? This might be my fault for not testing it, but this solution only moves the problem to somewhere else.

MichalStrehovsky · 2016-11-02T23:48:01Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/EETypeNode.cs

@@ -67,6 +67,9 @@ public EETypeNode(TypeDesc type)
            Debug.Assert(!type.IsRuntimeDeterminedSubtype);
            _type = type;
            _optionalFieldsNode = new EETypeOptionalFieldsNode(this);
+
+            if (!type.IsGenericDefinition)


CheckCanGenerateEEType is skipping generic definitions at the right spot - why is this necessary?

(We still need to validate e.g. the base type can get an EEType.)

We don't create an EEType for the base type of a generic definition though, so we shouldn't validate it. I think a better way would be to move the IsGenericDefinition to the top of CheckCanGenerateEEType so we don't go down that path for defs.

That makes sense.

MichalStrehovsky · 2016-11-02T23:48:38Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/EETypeNode.cs

+            {
+                // Ensure we can compute the type layout
+                defType.ComputeInstanceLayout(InstanceLayoutKind.TypeAndFields);
+                defType.ComputeStaticFieldLayout(StaticLayoutKind.StaticRegionSizesAndFields);


We don't need static field layout to build the EEType

Correction: we need the static layout if there's a deferred .cctor. Having an EEType allows people to pass it to the runtime method that runs the .cctor, which is why EEType has that dependency.

(Adding an explanatory comment in that sense might be useful for future reference.)

Will do - Now I'll have to pipe NodeFactory through EEType's constructors :(

MichalStrehovsky · 2016-11-02T23:50:56Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/EETypeNode.cs

+            }
+
+            // Don't validate __System.Canon since it is not present in the output image
+            if (type.IsCanonicalSubtype(CanonicalFormKind.Specific))


There is an assert in EETypeNode constructor that checks this never happens. We should crash and burn if someone asks for this.

This was needed when the IsGenericDefinition check came after the base type check. Removed.

MichalStrehovsky · 2016-11-02T23:54:11Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/EETypeNode.cs

+            TypeDesc baseType = type.BaseType;
+            if (baseType != null)
+            {
+                CheckCanGenerateEEType(baseType);


Instead of recursively calling these, we should probably just grab the EETypeNode from the NodeFactory for these so that the answer is essentially cached (this was one of Jan's comments when I initially implemented this.)

Yeah, will do

MichalStrehovsky · 2016-11-03T00:01:37Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/MethodCodeNode.cs

+        {
+            MethodSignature signature = method.Signature;
+
+            EETypeNode.CheckCanGenerateEEType(signature.ReturnType);


This is a pretty expensive check. I think what we actually need is somewhere along the lines of "make sure Get_CORINFO_SIG_INFO can succeed".

Maybe this method should also be part of CompilationModuleGroup or something like that. This file is RyuJIT specific. (We should really make the split of these into separate assemblies so that it gets more obvious.)

I whittled this down to the bare minimum the JIT needs, which is it calls getClassAttrib for each method argument and currently the only type loading implication is a check of ContainsGCPointers.

MichalStrehovsky · 2016-11-03T00:01:50Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/MethodCodeNode.cs

@@ -30,6 +30,22 @@ public MethodCodeNode(MethodDesc method)
            _method = method;
        }

+        /// <summary>
+        /// Validates that it will be possible 


I think you accidentally

MichalStrehovsky · 2016-11-03T00:02:54Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ReadyToRunHelperNode.cs

+                        // This way we can fail generating code for the method that references the EEType
+                        // and (depending on the policy), we could avoid scraping the entire compilation.
+                        TypeDesc type = (TypeDesc)target;
+                        EETypeNode.CheckCanGenerateEEType(type);


Maybe actually grabbing the EETypeNode here from the factory might be better (same "caching of answers" argument as above).

MichalStrehovsky · 2016-11-03T00:03:44Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ReadyToRunHelperNode.cs

+                case ReadyToRunHelperId.NewArr1:
+                case ReadyToRunHelperId.IsInstanceOf:
+                case ReadyToRunHelperId.CastClass:
+                case ReadyToRunHelperId.GetNonGCStaticBase:


Getting the bases only requires being able to compute the static layout

Altered the three base helpers to check we can ComputeStaticLayout instead

MichalStrehovsky · 2016-11-03T00:07:24Z

src/ILCompiler.Compiler/src/Compiler/MultiFileCompilationModuleGroup.cs

+                catch (TypeSystemException)
+                {
+                    // Swallow type load exceptions while rooting
+                    continue;


Can you add the same TODO comments we have around this catch block as we have in RyuJitCompilation? (We'll probably want (suppressible) warnings, plus a strict mode where missing references are fatal.)

MichalStrehovsky · 2016-11-04T02:04:29Z

Do we want checks in ExternEETypeSymbolNode ~~and ExternMethodSymbolNode too~~? We should fail compiling methods that would generate a reference to a symbol that won't exist.

nattress · 2016-11-04T19:27:37Z

Done. Fixed up the Unix failure, too.

MichalStrehovsky

LGTM otherwise (but I didn't spend much time reading the msbuild changes)

MichalStrehovsky · 2016-11-04T19:33:13Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/MethodCodeNode.cs

@@ -29,7 +29,7 @@ public MethodCodeNode(MethodDesc method)
            Debug.Assert(method.GetCanonMethodTarget(CanonicalFormKind.Specific) == method);
            _method = method;
        }
-
+        


Nit: unnecessary white space getting checked in. There's no reason to touch this file now.

MichalStrehovsky · 2016-11-04T19:33:52Z

src/ILCompiler.Compiler/src/Compiler/MultiFileCompilationModuleGroup.cs

@@ -122,7 +137,7 @@ public override bool ShouldShareAcrossModules(MethodDesc method)
        {
            if (method is InstantiatedMethod)
                return true;
-
+            


Nit: unnecessary white space getting checked in.

MichalStrehovsky · 2016-11-04T19:35:55Z

src/ILCompiler.Compiler/src/Compiler/MultiFileCompilationModuleGroup.cs

+
+            if (defType != null)
+            {
+                var unused = defType.ContainsGCPointers;


Does this cause any build warnings?

Maybe exposing ComputeTypeContainsGCPointers (for parity with the methods that compute layout) wouldn't be that bad.

Nope - I was sort of expecting to see a warning about that since we compile with level 4. I can expose ComputeTypeContainsGCPointers if you still wish. Setting a property to a dummy variable does look silly.

Leave it up to you. If you don't expose it, I'll probably do it myself because I tend to be OCD about these things :).

MichalStrehovsky · 2016-11-04T19:36:40Z

src/Common/src/TypeSystem/IL/Stubs/PInvokeILEmitter.cs

+            // System.Console's Interop.mincore.GetLargestConsoleWindowSize, the JIT decides it cannot
+            // generate an inline p/invoke because the return type is a struct.
+            //
+            if (type.IsValueType && !type.IsPrimitive)


Can you make sure there's an issue for this (and the issue # is linked from here) per Jan's comment?

#2144 added to the comments

MichalStrehovsky · 2016-11-04T19:37:11Z

src/Common/src/TypeSystem/IL/Stubs/TypeSystemThrowingILEmitter.cs

@@ -61,7 +61,7 @@ public static MethodIL EmitIL(MethodDesc methodThatShouldThrow, TypeSystemExcept

            foreach (var arg in exception.Arguments)
            {
-                codeStream.Emit(ILOpcode.ldstr, emitter.NewToken(arg));
+                codeStream.Emit(ILOpcode.ldstr, emitter.NewToken(arg ?? ""));


If you're not fixing this, please file an issue on me and add a link to it here.

This should be addressed in #2143

Enable compiling all framework / SDK assemblies in multi-module library mode Add a work-around to compile System.Console as a library in PInvokeILEmitter.cs System.Console has a p/invoke to GetLargestConsoleWindowSize which returns a COORD struct containing two 16 byte ints. The struct return type prevents the JIT from emitting an inline p/invoke in the IL stub we generate and instead it treats the external p/invoke target as a managed call and tries to compile it. Force p/invokes returning structs to use lazy p/invokes. These use Calli, which the JIT allows to always be inlined. For p/invokes, set CorJitFlag.CORJIT_FLG_IL_STUB JIT flag, which is required to allow us to always inline p/invokes. Address library module compilation failures due to unsuccessful type loads by checking on EEType creation whether the type can be loaded sufficiently to write out the EETypeNode. Code for this stolen from one of Michal's previous PRs :) Eagerly create EETypeNodes in ReadyToRunHelperNode so the failure point happens during the referencing method compilation, not later on when emitting the graph Check method signatures on library compilation method rooting Methods whose signatures contain types that cannot be loaded (they are in a separate, non-existent assembly for example) should be skipped since we cannot even replace their IL body with a throw helper. The JIT will fail trying to getClassAttribs on the argument types. This check is done when rooting methods instead of in MethodCodeNode's constructor because MethodCodeNodes are created during graph sweeping for things like virtual method combined dependencies. Add a test that verifies we can compile all framework assemblies to their own separate object files successfully. Don't check base types for generic type defs Only check static field layout for types with deferred cctors Move CheckCanGenerateMethod into MultiFileCompilationModuleGroup and scope its checks to just what the JIT requires (whether each type contains GC Pointers). Scope the type checking for static base ReadyToRunHelpers to just `ComputeStaticFieldLayout

dnfclas added the cla-already-signed label Nov 2, 2016

MichalStrehovsky reviewed Nov 3, 2016

View reviewed changes

MichalStrehovsky mentioned this pull request Nov 3, 2016

Don't allow nulls in ILEmitter #2129

Merged

nattress force-pushed the multimodulebuild branch from 14bcadf to 5b26373 Compare November 4, 2016 01:21

nattress force-pushed the multimodulebuild branch from 3a01b46 to 773496f Compare November 4, 2016 19:27

MichalStrehovsky reviewed Nov 4, 2016

View reviewed changes

nattress force-pushed the multimodulebuild branch from 773496f to a9ec5fd Compare November 5, 2016 02:01

nattress merged commit 91e5ce2 into dotnet:master Nov 5, 2016

nattress deleted the multimodulebuild branch June 26, 2017 18:17

Multi-module framework assembly compilation #2126

Multi-module framework assembly compilation #2126

Conversation

nattress commented Nov 2, 2016

jkotas commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalStrehovsky commented Nov 4, 2016 • edited Loading

nattress commented Nov 4, 2016

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalStrehovsky commented Nov 4, 2016 •

edited

Loading