From b65649eb8e28c9a58bb561059363869b9fd84748 Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Thu, 12 Dec 2019 22:10:29 -0500 Subject: [PATCH] Check method can compile before rooting (#729) * Check if a method should be skipped from compilation, and if so, do not root it. * Harden various signature nodes against type system exceptions Use existing validation API in CompilerTypeSystemContext to validate types on the various signature-emitting nodes: if it throws a TypeSystem exception due to a malformed type/method, the exception will propagate, get handled, and cause the current method being compiled to abort and be skipped (otherwise, the compiler will crash while emitting these problematic signatures) --- .../CompilerTypeSystemContext.Validation.cs | 20 +++++++++++++++++-- .../ReadyToRun/DelegateCtorSignature.cs | 4 ++++ .../ReadyToRun/FieldFixupSignature.cs | 3 +++ .../ReadyToRun/GenericLookupSignature.cs | 16 +++++++++++++++ .../ReadyToRun/MethodFixupSignature.cs | 5 +++++ .../ReadyToRun/ModuleTokenResolver.cs | 10 +++++----- .../ReadyToRun/NewArrayFixupSignature.cs | 3 +++ .../ReadyToRun/NewObjectFixupSignature.cs | 3 +++ .../ReadyToRun/TypeFixupSignature.cs | 3 +++ .../Compiler/ReadyToRunLibraryRootProvider.cs | 18 +++++++++++++---- .../JitInterface/CorInfoImpl.ReadyToRun.cs | 23 +++++++++++----------- 11 files changed, 85 insertions(+), 23 deletions(-) diff --git a/src/coreclr/src/tools/Common/Compiler/CompilerTypeSystemContext.Validation.cs b/src/coreclr/src/tools/Common/Compiler/CompilerTypeSystemContext.Validation.cs index 5031485..bf72510 100644 --- a/src/coreclr/src/tools/Common/Compiler/CompilerTypeSystemContext.Validation.cs +++ b/src/coreclr/src/tools/Common/Compiler/CompilerTypeSystemContext.Validation.cs @@ -21,6 +21,17 @@ namespace ILCompiler _validTypes.GetOrCreateValue(type); } + public void EnsureLoadableMethod(MethodDesc method) + { + EnsureLoadableType(method.OwningType); + + if (method.HasInstantiation) + { + foreach (var instType in method.Instantiation) + EnsureLoadableType(instType); + } + } + class ValidTypeHashTable : LockFreeReaderHashtable { protected override bool CompareKeyToValue(TypeDesc key, TypeDesc value) => key == value; @@ -96,8 +107,8 @@ namespace ILCompiler // Validate classes, structs, enums, interfaces, and delegates Debug.Assert(type.IsDefType); - // Don't validate generic definitons - if (type.IsGenericDefinition) + // Don't validate generic definitons or RuntimeDeterminiedTypes + if (type.IsGenericDefinition || type.IsRuntimeDeterminedType) { return type; } @@ -114,6 +125,11 @@ namespace ILCompiler ((CompilerTypeSystemContext)type.Context).EnsureLoadableType(intf.NormalizeInstantiation()); } + if (type.BaseType != null) + { + ((CompilerTypeSystemContext)type.Context).EnsureLoadableType(type.BaseType); + } + var defType = (DefType)type; // Ensure we can compute the type layout diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DelegateCtorSignature.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DelegateCtorSignature.cs index 338c7bb..0c9c177 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DelegateCtorSignature.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DelegateCtorSignature.cs @@ -29,6 +29,10 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun _targetMethod = targetMethod; _methodToken = methodToken; _signatureContext = signatureContext; + + // Ensure types in signature are loadable and resolvable, otherwise we'll fail later while emitting the signature + signatureContext.Resolver.CompilerContext.EnsureLoadableType(delegateType); + signatureContext.Resolver.CompilerContext.EnsureLoadableMethod(targetMethod.Method); } public override int ClassCode => 99885741; diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/FieldFixupSignature.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/FieldFixupSignature.cs index 1b1d931..d5f828c 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/FieldFixupSignature.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/FieldFixupSignature.cs @@ -25,6 +25,9 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun _fixupKind = fixupKind; _fieldDesc = fieldDesc; _signatureContext = signatureContext; + + // Ensure types in signature are loadable and resolvable, otherwise we'll fail later while emitting the signature + signatureContext.Resolver.CompilerContext.EnsureLoadableType(fieldDesc.OwningType); } public override int ClassCode => 271828182; diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/GenericLookupSignature.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/GenericLookupSignature.cs index f23f9b4..1136952 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/GenericLookupSignature.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/GenericLookupSignature.cs @@ -46,6 +46,22 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun _fieldArgument = fieldArgument; _methodContext = methodContext; _signatureContext = signatureContext; + + // Ensure types in signature are loadable and resolvable, otherwise we'll fail later while emitting the signature + if (typeArgument != null) + { + signatureContext.Resolver.CompilerContext.EnsureLoadableType(typeArgument); + } + if (fieldArgument != null) + { + signatureContext.Resolver.CompilerContext.EnsureLoadableType(fieldArgument.OwningType); + } + if (methodArgument != null) + { + signatureContext.Resolver.CompilerContext.EnsureLoadableMethod(methodArgument.Method); + if (methodArgument.ConstrainedType != null) + signatureContext.Resolver.CompilerContext.EnsureLoadableType(methodArgument.ConstrainedType); + } } public override int ClassCode => 258608008; diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodFixupSignature.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodFixupSignature.cs index a4c9105..cef3ab7 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodFixupSignature.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodFixupSignature.cs @@ -37,6 +37,11 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun _signatureContext = signatureContext; _isUnboxingStub = isUnboxingStub; _isInstantiatingStub = isInstantiatingStub; + + // Ensure types in signature are loadable and resolvable, otherwise we'll fail later while emitting the signature + signatureContext.Resolver.CompilerContext.EnsureLoadableMethod(method.Method); + if (method.ConstrainedType != null) + signatureContext.Resolver.CompilerContext.EnsureLoadableType(method.ConstrainedType); } public MethodDesc Method => _method.Method; diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ModuleTokenResolver.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ModuleTokenResolver.cs index 6769f8d..0261bc4 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ModuleTokenResolver.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ModuleTokenResolver.cs @@ -32,14 +32,14 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun private readonly CompilationModuleGroup _compilationModuleGroup; - private readonly CompilerTypeSystemContext _typeSystemContext; - private Func _moduleIndexLookup; + public CompilerTypeSystemContext CompilerContext { get; } + public ModuleTokenResolver(CompilationModuleGroup compilationModuleGroup, CompilerTypeSystemContext typeSystemContext) { _compilationModuleGroup = compilationModuleGroup; - _typeSystemContext = typeSystemContext; + CompilerContext = typeSystemContext; } public void SetModuleIndexLookup(Func moduleIndexLookup) @@ -103,7 +103,7 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun FieldDesc canonField = field; if (owningCanonType != field.OwningType) { - canonField = _typeSystemContext.GetFieldForInstantiatedType(field.GetTypicalFieldDefinition(), (InstantiatedType)owningCanonType); + canonField = CompilerContext.GetFieldForInstantiatedType(field.GetTypicalFieldDefinition(), (InstantiatedType)owningCanonType); } ModuleToken token; @@ -160,7 +160,7 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun FieldDesc canonField = field; if (owningCanonType != field.OwningType) { - canonField = _typeSystemContext.GetFieldForInstantiatedType(field.GetTypicalFieldDefinition(), (InstantiatedType)owningCanonType); + canonField = CompilerContext.GetFieldForInstantiatedType(field.GetTypicalFieldDefinition(), (InstantiatedType)owningCanonType); } _fieldToRefTokens[canonField] = token; diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/NewArrayFixupSignature.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/NewArrayFixupSignature.cs index 99107cc..baf10db 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/NewArrayFixupSignature.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/NewArrayFixupSignature.cs @@ -18,6 +18,9 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun { _arrayType = arrayType; _signatureContext = signatureContext; + + // Ensure types in signature are loadable and resolvable, otherwise we'll fail later while emitting the signature + signatureContext.Resolver.CompilerContext.EnsureLoadableType(arrayType); } public override int ClassCode => 815543321; diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/NewObjectFixupSignature.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/NewObjectFixupSignature.cs index d72e307..bafd5fe 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/NewObjectFixupSignature.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/NewObjectFixupSignature.cs @@ -18,6 +18,9 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun { _typeDesc = typeDesc; _signatureContext = signatureContext; + + // Ensure types in signature are loadable and resolvable, otherwise we'll fail later while emitting the signature + signatureContext.Resolver.CompilerContext.EnsureLoadableType(typeDesc); } public override int ClassCode => 551247760; diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs index 90366c1..86b115a 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs @@ -24,6 +24,9 @@ namespace ILCompiler.DependencyAnalysis.ReadyToRun _fixupKind = fixupKind; _typeDesc = typeDesc; _signatureContext = signatureContext; + + // Ensure types in signature are loadable and resolvable, otherwise we'll fail later while emitting the signature + signatureContext.Resolver.CompilerContext.EnsureLoadableType(typeDesc); } public override int ClassCode => 255607008; diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunLibraryRootProvider.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunLibraryRootProvider.cs index bff9ba0..f62be46 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunLibraryRootProvider.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunLibraryRootProvider.cs @@ -6,6 +6,7 @@ using System; using Internal.TypeSystem.Ecma; using Internal.TypeSystem; +using Internal.JitInterface; namespace ILCompiler { @@ -63,8 +64,11 @@ namespace ILCompiler if (containsSignatureVariables) continue; - CheckCanGenerateMethod(method); - rootProvider.AddCompilationRoot(method, "Profile triggered method"); + if (!CorInfoImpl.ShouldSkipCompilation(method)) + { + CheckCanGenerateMethod(method); + rootProvider.AddCompilationRoot(method, "Profile triggered method"); + } } catch (TypeSystemException) { @@ -124,8 +128,11 @@ namespace ILCompiler try { - CheckCanGenerateMethod(methodToRoot); - rootProvider.AddCompilationRoot(methodToRoot, reason); + if (!CorInfoImpl.ShouldSkipCompilation(method)) + { + CheckCanGenerateMethod(methodToRoot); + rootProvider.AddCompilationRoot(methodToRoot, reason); + } } catch (TypeSystemException) { @@ -143,6 +150,9 @@ namespace ILCompiler /// public static void CheckCanGenerateMethod(MethodDesc method) { + // Ensure the method is loadable + ((CompilerTypeSystemContext)method.Context).EnsureLoadableMethod(method); + MethodSignature signature = method.Signature; // Vararg methods are not supported in .NET Core 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 1fafa05..ccd4147 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 @@ -171,31 +171,30 @@ namespace Internal.JitInterface throw new NotSupportedException(); } - private bool ShouldSkipCompilation(IMethodNode methodCodeNodeNeedingCode) + public static bool ShouldSkipCompilation(MethodDesc methodNeedingCode) { - MethodDesc method = methodCodeNodeNeedingCode.Method; - if (method.IsAggressiveOptimization) + if (methodNeedingCode.IsAggressiveOptimization) { return true; } - if (HardwareIntrinsicHelpers.IsHardwareIntrinsic(method)) + if (HardwareIntrinsicHelpers.IsHardwareIntrinsic(methodNeedingCode)) { return true; } - if (MethodBeingCompiled.IsAbstract) + if (methodNeedingCode.IsAbstract) { return true; } - if (MethodBeingCompiled.OwningType.IsDelegate && ( - MethodBeingCompiled.IsConstructor || - MethodBeingCompiled.Name == "BeginInvoke" || - MethodBeingCompiled.Name == "Invoke" || - MethodBeingCompiled.Name == "EndInvoke")) + if (methodNeedingCode.OwningType.IsDelegate && ( + methodNeedingCode.IsConstructor || + methodNeedingCode.Name == "BeginInvoke" || + methodNeedingCode.Name == "Invoke" || + methodNeedingCode.Name == "EndInvoke")) { // Special methods on delegate types return true; } - if (method.HasCustomAttribute("System.Runtime", "BypassReadyToRunAttribute")) + if (methodNeedingCode.HasCustomAttribute("System.Runtime", "BypassReadyToRunAttribute")) { // This is a quick workaround to opt specific methods out of ReadyToRun compilation to work around bugs. return true; @@ -211,7 +210,7 @@ namespace Internal.JitInterface try { - if (!ShouldSkipCompilation(methodCodeNodeNeedingCode)) + if (!ShouldSkipCompilation(MethodBeingCompiled)) { CompileMethodInternal(methodCodeNodeNeedingCode); codeGotPublished = true; -- 2.7.4