Skip to content

Commit

Permalink
Check method can compile before rooting (#729)
Browse files Browse the repository at this point in the history 
* 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)
  • Loading branch information
Fadi Hanna authored Dec 13, 2019
1 parent 26bb3b1 commit b65649e
Show file tree
Hide file tree
Showing 11 changed files with 85 additions and 23 deletions.
20 changes: 18 additions & 2 deletions src/coreclr/src/tools/Common/Compiler/CompilerTypeSystemContext.Validation.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,17 @@ public void EnsureLoadableType(TypeDesc type)
_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<TypeDesc, TypeDesc>
{
protected override bool CompareKeyToValue(TypeDesc key, TypeDesc value) => key == value;
Expand Down Expand Up @@ -96,8 +107,8 @@ private static TypeDesc EnsureLoadableTypeUncached(TypeDesc type)
// 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;
}
Expand All @@ -114,6 +125,11 @@ private static TypeDesc EnsureLoadableTypeUncached(TypeDesc type)
((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
Expand Down
4 changes: 4 additions & 0 deletions ...en2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DelegateCtorSignature.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ public DelegateCtorSignature(
_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;
Expand Down
3 changes: 3 additions & 0 deletions ...sgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/FieldFixupSignature.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ public FieldFixupSignature(ReadyToRunFixupKind fixupKind, FieldDesc fieldDesc, S
_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;
Expand Down
16 changes: 16 additions & 0 deletions ...n2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/GenericLookupSignature.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,22 @@ public GenericLookupSignature(
_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;
Expand Down
5 changes: 5 additions & 0 deletions ...gen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodFixupSignature.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ public MethodFixupSignature(
_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;
Expand Down
10 changes: 5 additions & 5 deletions ...sgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ModuleTokenResolver.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,14 @@ public class ModuleTokenResolver

private readonly CompilationModuleGroup _compilationModuleGroup;

private readonly CompilerTypeSystemContext _typeSystemContext;

private Func<EcmaModule, int> _moduleIndexLookup;

public CompilerTypeSystemContext CompilerContext { get; }

public ModuleTokenResolver(CompilationModuleGroup compilationModuleGroup, CompilerTypeSystemContext typeSystemContext)
{
_compilationModuleGroup = compilationModuleGroup;
_typeSystemContext = typeSystemContext;
CompilerContext = typeSystemContext;
}

public void SetModuleIndexLookup(Func<EcmaModule, int> moduleIndexLookup)
Expand Down Expand Up @@ -103,7 +103,7 @@ public ModuleToken GetModuleTokenForField(FieldDesc field, bool throwIfNotFound
FieldDesc canonField = field;
if (owningCanonType != field.OwningType)
{
canonField = _typeSystemContext.GetFieldForInstantiatedType(field.GetTypicalFieldDefinition(), (InstantiatedType)owningCanonType);
canonField = CompilerContext.GetFieldForInstantiatedType(field.GetTypicalFieldDefinition(), (InstantiatedType)owningCanonType);
}

ModuleToken token;
Expand Down Expand Up @@ -160,7 +160,7 @@ public void AddModuleTokenForField(FieldDesc field, ModuleToken token)
FieldDesc canonField = field;
if (owningCanonType != field.OwningType)
{
canonField = _typeSystemContext.GetFieldForInstantiatedType(field.GetTypicalFieldDefinition(), (InstantiatedType)owningCanonType);
canonField = CompilerContext.GetFieldForInstantiatedType(field.GetTypicalFieldDefinition(), (InstantiatedType)owningCanonType);
}

_fieldToRefTokens[canonField] = token;
Expand Down
3 changes: 3 additions & 0 deletions ...n2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/NewArrayFixupSignature.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ public NewArrayFixupSignature(ArrayType arrayType, SignatureContext signatureCon
{
_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;
Expand Down
3 changes: 3 additions & 0 deletions ...2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/NewObjectFixupSignature.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ public NewObjectFixupSignature(TypeDesc typeDesc, SignatureContext signatureCont
{
_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;
Expand Down
3 changes: 3 additions & 0 deletions ...ssgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ public TypeFixupSignature(ReadyToRunFixupKind fixupKind, TypeDesc typeDesc, Sign
_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;
Expand Down
18 changes: 14 additions & 4 deletions ...reclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunLibraryRootProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

using Internal.TypeSystem.Ecma;
using Internal.TypeSystem;
using Internal.JitInterface;

namespace ILCompiler
{
Expand Down Expand Up @@ -63,8 +64,11 @@ public void AddCompilationRoots(IRootingServiceProvider rootProvider)
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)
{
Expand Down Expand Up @@ -124,8 +128,11 @@ private void RootMethods(TypeDesc type, string reason, IRootingServiceProvider r

try
{
CheckCanGenerateMethod(methodToRoot);
rootProvider.AddCompilationRoot(methodToRoot, reason);
if (!CorInfoImpl.ShouldSkipCompilation(method))
{
CheckCanGenerateMethod(methodToRoot);
rootProvider.AddCompilationRoot(methodToRoot, reason);
}
}
catch (TypeSystemException)
{
Expand All @@ -143,6 +150,9 @@ private void RootMethods(TypeDesc type, string reason, IRootingServiceProvider r
/// </summary>
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
Expand Down
23 changes: 11 additions & 12 deletions src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,31 +171,30 @@ private static mdToken FindGenericMethodArgTypeSpec(EcmaModule module)
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;
Expand All @@ -211,7 +210,7 @@ public void CompileMethod(IReadyToRunMethodCodeNode methodCodeNodeNeedingCode)

try
{
if (!ShouldSkipCompilation(methodCodeNodeNeedingCode))
if (!ShouldSkipCompilation(MethodBeingCompiled))
{
CompileMethodInternal(methodCodeNodeNeedingCode);
codeGotPublished = true;
Expand Down

0 comments on commit b65649e

Please sign in to comment.