From b8c929665c8e28d125829023e19a4165845f209d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Rylek?= Date: Fri, 3 Apr 2020 19:48:08 +0200 Subject: [PATCH] Crossgen2 fixes to enable composite build with shared framework (#34431) These changes involve targeted fixes to issues seen due to the new build mode and generic signature encoding fixes identified in my offline investigation with JanV. While I locally see the tests passing with just a handful of failures, I still seem unable to make the tests run in the lab; I keep investigating that but I believe it's useful to merge this delta in to let us parallelize follow-up efforts. Thanks Tomas --- .../src/tools/Common/Compiler/TypeExtensions.cs | 10 ++++++++++ .../ReadyToRun/InliningInfoNode.cs | 9 +++++++++ .../ReadyToRun/SignatureBuilder.cs | 14 +++++++++++--- .../ReadyToRun/SignatureContext.cs | 4 +++- .../Compiler/MethodExtensions.cs | 12 ++++++++++++ .../ReadyToRunCompilationModuleGroupBase.cs | 3 +-- .../JitInterface/CorInfoImpl.ReadyToRun.cs | 2 +- .../crossgen2/crossgen2/CommandLineOptions.cs | 2 ++ .../src/tools/crossgen2/crossgen2/Program.cs | 11 +++++++---- 9 files changed, 56 insertions(+), 11 deletions(-) diff --git a/src/coreclr/src/tools/Common/Compiler/TypeExtensions.cs b/src/coreclr/src/tools/Common/Compiler/TypeExtensions.cs index 03fe6874ba595..86f46938e5587 100644 --- a/src/coreclr/src/tools/Common/Compiler/TypeExtensions.cs +++ b/src/coreclr/src/tools/Common/Compiler/TypeExtensions.cs @@ -502,5 +502,15 @@ private static TypeDesc GetTypeThatMeetsConstraints(GenericParameterDesc generic return constrainedType ?? genericParam.Context.GetWellKnownType(WellKnownType.Object); } + + /// + /// Return true when the type in question is marked with the NonVersionable attribute. + /// + /// Type to check + /// True when the type is marked with the non-versionable custom attribute, false otherwise. + public static bool IsNonVersionable(this MetadataType type) + { + return type.HasCustomAttribute("System.Runtime.Versioning", "NonVersionableAttribute"); + } } } diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/InliningInfoNode.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/InliningInfoNode.cs index 89eb57a871e06..e9f62a82fc1ce 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/InliningInfoNode.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/InliningInfoNode.cs @@ -66,6 +66,15 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) continue; } + if (!factory.CompilationModuleGroup.VersionsWithMethodBody(inlinee)) + { + // We cannot record inlining info across version bubble as cross-bubble assemblies + // are not guaranteed to preserve token values. Only non-versionable methods may be + // inlined across the version bubble. + Debug.Assert(inlinee.IsNonVersionable()); + continue; + } + if (!inlineeToInliners.TryGetValue(ecmaInlineeDefinition, out HashSet inliners)) { inliners = new HashSet(); diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs index 4d76fb7cb8988..a61fea8413042 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureBuilder.cs @@ -310,6 +310,12 @@ public void EmitTypeSignature(TypeDesc typeDesc, SignatureContext context) case TypeFlags.ValueType: case TypeFlags.Nullable: case TypeFlags.Enum: + if (typeDesc.IsWellKnownType(WellKnownType.TypedReference)) + { + EmitElementType(CorElementType.ELEMENT_TYPE_TYPEDBYREF); + return; + } + { ModuleToken token = context.GetModuleTokenForType((EcmaType)typeDesc); EmitModuleOverride(token.Module, context); @@ -345,10 +351,11 @@ private void EmitInstantiatedTypeSignature(InstantiatedType type, SignatureConte EmitModuleOverride(targetModule, context); EmitElementType(CorElementType.ELEMENT_TYPE_GENERICINST); EmitTypeSignature(type.GetTypeDefinition(), context.InnerContext(targetModule)); + SignatureContext outerContext = context.OuterContext; EmitUInt((uint)type.Instantiation.Length); for (int paramIndex = 0; paramIndex < type.Instantiation.Length; paramIndex++) { - EmitTypeSignature(type.Instantiation[paramIndex], context); + EmitTypeSignature(type.Instantiation[paramIndex], outerContext); } } @@ -530,9 +537,10 @@ private void EmitMethodSpecificationSignature(MethodWithToken method, { Instantiation instantiation = method.Method.Instantiation; EmitUInt((uint)instantiation.Length); + SignatureContext outerContext = context.OuterContext; for (int typeParamIndex = 0; typeParamIndex < instantiation.Length; typeParamIndex++) { - EmitTypeSignature(instantiation[typeParamIndex], context); + EmitTypeSignature(instantiation[typeParamIndex], outerContext); } } } @@ -616,7 +624,7 @@ public SignatureContext EmitFixup(NodeFactory factory, ReadyToRunFixupKind fixup { EmitByte((byte)(fixupKind | ReadyToRunFixupKind.ModuleOverride)); EmitUInt((uint)factory.ManifestMetadataTable.ModuleToIndex(targetModule)); - return outerContext.InnerContext(targetModule); + return new SignatureContext(targetModule, outerContext.Resolver); } } } diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureContext.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureContext.cs index 901cc90a28235..02c28f585835a 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureContext.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureContext.cs @@ -32,6 +32,8 @@ public class SignatureContext /// public readonly ModuleTokenResolver Resolver; + public SignatureContext OuterContext => new SignatureContext(GlobalContext, Resolver); + public SignatureContext(EcmaModule context, ModuleTokenResolver resolver) { GlobalContext = context; @@ -53,7 +55,7 @@ public SignatureContext InnerContext(EcmaModule innerContext) public EcmaModule GetTargetModule(TypeDesc type) { - if (type.IsPrimitive || type.IsString || type.IsObject) + if (type.IsPrimitive || type.IsString || type.IsObject || type.IsWellKnownType(WellKnownType.TypedReference)) { return LocalContext; } diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/MethodExtensions.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/MethodExtensions.cs index e2d4aa7cbfa2b..1f1d3da49893a 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/MethodExtensions.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/MethodExtensions.cs @@ -33,5 +33,17 @@ public static bool IsSuppressGCTransition(this MethodDesc method) return method.HasCustomAttribute("System.Runtime.InteropServices", "SuppressGCTransitionAttribute"); } + + /// + /// Return true when the method is marked as non-versionable. Non-versionable methods + /// may be freely inlined into ReadyToRun images even when they don't reside in the + /// same version bubble as the module being compiled. + /// + /// Method to check + /// True when the method is marked as non-versionable, false otherwise. + public static bool IsNonVersionable(this MethodDesc method) + { + return method.HasCustomAttribute("System.Runtime.Versioning", "NonVersionableAttribute"); + } } } diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs index 031764ac1c8e2..7d55bc55e32b4 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs @@ -153,8 +153,7 @@ public sealed override bool CanInline(MethodDesc callerMethod, MethodDesc callee // (because otherwise we may not be able to encode its tokens) // and if the callee is either in the same version bubble or is marked as non-versionable. bool canInline = VersionsWithMethodBody(callerMethod) && - (VersionsWithMethodBody(calleeMethod) || - calleeMethod.HasCustomAttribute("System.Runtime.Versioning", "NonVersionableAttribute")); + (VersionsWithMethodBody(calleeMethod) || calleeMethod.IsNonVersionable()); return canInline; } diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index 8aa4adc279930..bd99070f774a2 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -2061,7 +2061,7 @@ private bool IsLayoutFixedInCurrentVersionBubble(TypeDesc type) } // Valuetypes with non-versionable attribute are candidates for fixed layout. Reject the rest. - return type is MetadataType metadataType && metadataType.HasCustomAttribute("System.Runtime.Versioning", "NonVersionableAttribute"); + return type is MetadataType metadataType && metadataType.IsNonVersionable(); } return true; diff --git a/src/coreclr/src/tools/crossgen2/crossgen2/CommandLineOptions.cs b/src/coreclr/src/tools/crossgen2/crossgen2/CommandLineOptions.cs index 93791b1c0d365..236e1dfeea9bb 100644 --- a/src/coreclr/src/tools/crossgen2/crossgen2/CommandLineOptions.cs +++ b/src/coreclr/src/tools/crossgen2/crossgen2/CommandLineOptions.cs @@ -44,6 +44,8 @@ public class CommandLineOptions public string[] CodegenOptions { get; set; } + public bool CompositeOrInputBubble => Composite || InputBubble; + public static Command RootCommand() { // For some reason, arity caps at 255 by default diff --git a/src/coreclr/src/tools/crossgen2/crossgen2/Program.cs b/src/coreclr/src/tools/crossgen2/crossgen2/Program.cs index ecb8954d0ff9b..9b8b4ce5de2f8 100644 --- a/src/coreclr/src/tools/crossgen2/crossgen2/Program.cs +++ b/src/coreclr/src/tools/crossgen2/crossgen2/Program.cs @@ -81,7 +81,7 @@ private void ProcessCommandLine() if (_commandLineOptions.CompileBubbleGenerics) { - if (!_commandLineOptions.InputBubble) + if (!_commandLineOptions.CompositeOrInputBubble) { Console.WriteLine(SR.WarningIgnoringBubbleGenerics); _commandLineOptions.CompileBubbleGenerics = false; @@ -223,7 +223,7 @@ private int Run() rootingModules.Add(module); versionBubbleModulesHash.Add(module); - if (!_commandLineOptions.InputBubble) + if (!_commandLineOptions.CompositeOrInputBubble) { break; } @@ -335,9 +335,12 @@ private int Run() // For non-single-method compilations add compilation roots. foreach (var module in rootingModules) { - compilationRoots.Add(new ReadyToRunRootProvider(module, profileDataManager, _commandLineOptions.Partial)); + compilationRoots.Add(new ReadyToRunRootProvider( + module, + profileDataManager, + profileDrivenPartialNGen: _commandLineOptions.Partial)); - if (!_commandLineOptions.InputBubble) + if (!_commandLineOptions.CompositeOrInputBubble) { break; }