Fix the TypeLoadException CPAOT compilation bucket (dotnet#7360)

trylek · web-flow · commit 7c3fb3829d44 · 2019-04-29T21:05:31.000+02:00
I have fixed this by constructing TypeFixupSignature instances
using a new helper method TypeSignature (much like what we're
doing for methods) which calls the CheckCanGenerateEEType method
upfront. This properly throws the exception to JIT so that it
gets caught in compileMethodInternal and just suppresses jitting
of a particular method rather than tearing down the entire
compilation process.

Based on Michal's suggestion I'm proposing to remove the check
from the constructors of AvailableType and ExternalTypeNode as
with my change it becomes duplicative and it ultimately causes
more harm than good by crashing the compiler when hitting
an invalid type (as happens e.g. in negative CoreCLR typeloader
tests).

Thanks

Tomas
diff --git a/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ExternalTypeNode.cs b/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ExternalTypeNode.cs
@@ -18,13 +18,6 @@ class ExternalTypeNode : DependencyNodeCore<NodeFactory>, IEETypeNode
         public ExternalTypeNode(NodeFactory factory, TypeDesc type)
         {
             _type = type;
-
-            //
-            // This check encodes rules specific to CoreRT. Ie, no function pointer classes allowed.
-            // Eventually we will hit situations where this check fails when it shouldn't and we'll need to 
-            // split the logic. It's a good sanity check for the time being though.
-            //
-            EETypeNode.CheckCanGenerateEEType(factory, type);
         }
 
         public TypeDesc Type => _type;
diff --git a/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRun/AvailableType.cs b/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRun/AvailableType.cs
@@ -18,13 +18,6 @@ public class AvailableType : DependencyNodeCore<NodeFactory>, IEETypeNode
         public AvailableType(NodeFactory factory, TypeDesc type)
         {
             _type = type;
-
-            //
-            // This check encodes rules specific to CoreRT. Ie, no function pointer classes allowed.
-            // Eventually we will hit situations where this check fails when it shouldn't and we'll need to 
-            // split the logic. It's a good sanity check for the time being though.
-            //
-            EETypeNode.CheckCanGenerateEEType(factory, type);
         }
 
         public TypeDesc Type => _type;
diff --git a/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs b/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs
@@ -295,6 +295,28 @@ public MethodFixupSignature MethodSignature(
             return signature;
         }
 
+        private readonly Dictionary<ReadyToRunFixupKind, Dictionary<TypeDesc, TypeFixupSignature>> _typeSignatures =
+            new Dictionary<ReadyToRunFixupKind, Dictionary<TypeDesc, TypeFixupSignature>>();
+
+        public TypeFixupSignature TypeSignature(ReadyToRunFixupKind fixupKind, TypeDesc typeDesc, SignatureContext signatureContext)
+        {
+            Dictionary<TypeDesc, TypeFixupSignature> perFixupKindMap;
+            if (!_typeSignatures.TryGetValue(fixupKind, out perFixupKindMap))
+            {
+                perFixupKindMap = new Dictionary<TypeDesc, TypeFixupSignature>();
+                _typeSignatures.Add(fixupKind, perFixupKindMap);
+            }
+
+            TypeFixupSignature signature;
+            if (!perFixupKindMap.TryGetValue(typeDesc, out signature))
+            {
+                EETypeNode.CheckCanGenerateEEType(this, typeDesc);
+                signature = new TypeFixupSignature(fixupKind, typeDesc, signatureContext);
+                perFixupKindMap.Add(typeDesc, signature);
+            }
+            return signature;
+        }
+
         public override void AttachToDependencyGraph(DependencyAnalyzerBase<NodeFactory> graph)
         {
             Header = new HeaderNode(Target);
@@ -493,7 +515,7 @@ protected override ISymbolNode CreateGenericLookupFromDictionaryNode(ReadyToRunG
                 this,
                 HelperImports,
                 GetGenericStaticHelper(helperKey.HelperId),
-                new TypeFixupSignature(
+                TypeSignature(
                     ReadyToRunFixupKind.READYTORUN_FIXUP_Invalid,
                     (TypeDesc)helperKey.Target,
                     InputModuleContext));
@@ -505,7 +527,7 @@ protected override ISymbolNode CreateGenericLookupFromTypeNode(ReadyToRunGeneric
                 this,
                 HelperImports,
                 GetGenericStaticHelper(helperKey.HelperId),
-                new TypeFixupSignature(
+                TypeSignature(
                     ReadyToRunFixupKind.READYTORUN_FIXUP_Invalid,
                     (TypeDesc)helperKey.Target,
                     InputModuleContext));
diff --git a/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs b/src/ILCompiler.ReadyToRun/src/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs
@@ -150,7 +150,7 @@ private ISymbolNode CreateGCStaticBaseHelper(TypeDesc type, SignatureContext sig
                 _codegenNodeFactory,
                 _codegenNodeFactory.HelperImports,
                 ILCompiler.DependencyAnalysis.ReadyToRun.ReadyToRunHelper.READYTORUN_HELPER_DelayLoad_Helper,
-                new TypeFixupSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_StaticBaseGC, type, signatureContext));
+                _codegenNodeFactory.TypeSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_StaticBaseGC, type, signatureContext));
         }
 
         private ISymbolNode CreateNonGCStaticBaseHelper(TypeDesc type, SignatureContext signatureContext)
@@ -159,7 +159,7 @@ private ISymbolNode CreateNonGCStaticBaseHelper(TypeDesc type, SignatureContext
                 _codegenNodeFactory,
                 _codegenNodeFactory.HelperImports,
                 ILCompiler.DependencyAnalysis.ReadyToRun.ReadyToRunHelper.READYTORUN_HELPER_DelayLoad_Helper,
-                new TypeFixupSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_StaticBaseNonGC, type, signatureContext));
+                _codegenNodeFactory.TypeSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_StaticBaseNonGC, type, signatureContext));
         }
 
         private ISymbolNode CreateThreadGcStaticBaseHelper(TypeDesc type, SignatureContext signatureContext)
@@ -168,7 +168,7 @@ private ISymbolNode CreateThreadGcStaticBaseHelper(TypeDesc type, SignatureConte
                 _codegenNodeFactory,
                 _codegenNodeFactory.HelperImports,
                 ILCompiler.DependencyAnalysis.ReadyToRun.ReadyToRunHelper.READYTORUN_HELPER_DelayLoad_Helper,
-                new TypeFixupSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_ThreadStaticBaseGC, type, signatureContext));
+                _codegenNodeFactory.TypeSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_ThreadStaticBaseGC, type, signatureContext));
         }
 
         private ISymbolNode CreateThreadNonGcStaticBaseHelper(TypeDesc type, SignatureContext signatureContext)
@@ -177,7 +177,7 @@ private ISymbolNode CreateThreadNonGcStaticBaseHelper(TypeDesc type, SignatureCo
                 _codegenNodeFactory,
                 _codegenNodeFactory.HelperImports,
                 ILCompiler.DependencyAnalysis.ReadyToRun.ReadyToRunHelper.READYTORUN_HELPER_DelayLoad_Helper,
-                new TypeFixupSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_ThreadStaticBaseNonGC, type, signatureContext));
+                _codegenNodeFactory.TypeSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_ThreadStaticBaseNonGC, type, signatureContext));
         }
 
         private ISymbolNode CreateIsInstanceOfHelper(TypeDesc type, SignatureContext signatureContext)
@@ -186,7 +186,7 @@ private ISymbolNode CreateIsInstanceOfHelper(TypeDesc type, SignatureContext sig
                 _codegenNodeFactory,
                 _codegenNodeFactory.HelperImports,
                 ILCompiler.DependencyAnalysis.ReadyToRun.ReadyToRunHelper.READYTORUN_HELPER_DelayLoad_Helper,
-                new TypeFixupSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_IsInstanceOf, type, signatureContext));
+                _codegenNodeFactory.TypeSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_IsInstanceOf, type, signatureContext));
         }
 
         private ISymbolNode CreateCastClassHelper(TypeDesc type, SignatureContext signatureContext)
@@ -195,14 +195,14 @@ private ISymbolNode CreateCastClassHelper(TypeDesc type, SignatureContext signat
                 _codegenNodeFactory,
                 _codegenNodeFactory.HelperImports,
                 ILCompiler.DependencyAnalysis.ReadyToRun.ReadyToRunHelper.READYTORUN_HELPER_DelayLoad_Helper_Obj,
-                new TypeFixupSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_ChkCast, type, signatureContext));
+                _codegenNodeFactory.TypeSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_ChkCast, type, signatureContext));
         }
 
         private ISymbolNode CreateTypeHandleHelper(TypeDesc type, SignatureContext signatureContext)
         {
             return new PrecodeHelperImport(
                 _codegenNodeFactory,
-                new TypeFixupSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_TypeHandle, type, signatureContext));
+                _codegenNodeFactory.TypeSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_TypeHandle, type, signatureContext));
         }
 
         private ISymbolNode CreateMethodHandleHelper(MethodWithToken method, SignatureContext signatureContext)
@@ -255,14 +255,14 @@ private ISymbolNode CreateCctorTrigger(TypeDesc type, SignatureContext signature
                 _codegenNodeFactory,
                 _codegenNodeFactory.DispatchImports,
                 ILCompiler.DependencyAnalysis.ReadyToRun.ReadyToRunHelper.READYTORUN_HELPER_DelayLoad_Helper,
-                new TypeFixupSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_CctorTrigger, type, signatureContext));
+                _codegenNodeFactory.TypeSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_CctorTrigger, type, signatureContext));
         }
 
         private ISymbolNode CreateTypeDictionary(TypeDesc type, SignatureContext signatureContext)
         {
             return new PrecodeHelperImport(
                 _codegenNodeFactory,
-                new TypeFixupSignature(
+                _codegenNodeFactory.TypeSignature(
                     ReadyToRunFixupKind.READYTORUN_FIXUP_TypeDictionary,
                     type,
                     signatureContext));
@@ -323,7 +323,7 @@ public ISymbolNode FieldBaseOffset(TypeDesc typeDesc, SignatureContext signature
             {
                 result = new PrecodeHelperImport(
                     _codegenNodeFactory,
-                    new TypeFixupSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_FieldBaseOffset, typeDesc, signatureContext));
+                    _codegenNodeFactory.TypeSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_FieldBaseOffset, typeDesc, signatureContext));
                 _fieldBaseOffsetCache.Add(typeDesc, result);
             }
             return result;
@@ -734,7 +734,7 @@ public ISymbolNode ConstructedTypeSymbol(TypeDesc type, SignatureContext signatu
             {
                 symbol = new PrecodeHelperImport(
                     _codegenNodeFactory,
-                    new TypeFixupSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_TypeDictionary, type, signatureContext));
+                    _codegenNodeFactory.TypeSignature(ReadyToRunFixupKind.READYTORUN_FIXUP_TypeDictionary, type, signatureContext));
                 _constructedTypeSymbols.Add(type, symbol);
             }
             return symbol;

Original file line number	Diff line number	Diff line change
`@@ -18,13 +18,6 @@ class ExternalTypeNode : DependencyNodeCore<NodeFactory>, IEETypeNode`
`18`	`18`	`public ExternalTypeNode(NodeFactory factory, TypeDesc type)`
`19`	`19`	`{`
`20`	`20`	`_type = type;`
`21`		`-`
`22`		`- //`
`23`		`- // This check encodes rules specific to CoreRT. Ie, no function pointer classes allowed.`
`24`		`- // Eventually we will hit situations where this check fails when it shouldn't and we'll need to`
`25`		`- // split the logic. It's a good sanity check for the time being though.`
`26`		`- //`
`27`		`- EETypeNode.CheckCanGenerateEEType(factory, type);`
`28`	`21`	`}`
`29`	`22`
`30`	`23`	`public TypeDesc Type => _type;`
Original file line number	Diff line number	Diff line change
`@@ -18,13 +18,6 @@ public class AvailableType : DependencyNodeCore<NodeFactory>, IEETypeNode`
`18`	`18`	`public AvailableType(NodeFactory factory, TypeDesc type)`
`19`	`19`	`{`
`20`	`20`	`_type = type;`
`21`		`-`
`22`		`- //`
`23`		`- // This check encodes rules specific to CoreRT. Ie, no function pointer classes allowed.`
`24`		`- // Eventually we will hit situations where this check fails when it shouldn't and we'll need to`
`25`		`- // split the logic. It's a good sanity check for the time being though.`
`26`		`- //`
`27`		`- EETypeNode.CheckCanGenerateEEType(factory, type);`
`28`	`21`	`}`
`29`	`22`
`30`	`23`	`public TypeDesc Type => _type;`