Fix failure to generate ngen pdb in crossgen

src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.Validation.cs

@@ -1,6 +1,8 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System;
+using System.Collections.Generic;
 using Internal.TypeSystem;
 
 using Debug = System.Diagnostics.Debug;
@@ -11,13 +13,180 @@ namespace ILCompiler
     // in unpredictable spots.
     public partial class CompilerTypeSystemContext
     {
+        [ThreadStatic]
+        private static List<TypeLoadabilityCheckInProgress> t_typeLoadCheckInProgressStack;
+        private static List<TypeDesc> EmptyList = new List<TypeDesc>();
+        private readonly ValidTypeHashTable _validTypes = new ValidTypeHashTable();
+
         /// <summary>
         /// Ensures that the type can be fully loaded. The method will throw one of the type system
         /// exceptions if the type is not loadable.
         /// </summary>
         public void EnsureLoadableType(TypeDesc type)
         {
-            _validTypes.GetOrCreateValue(type);
+            if (type == null)
+                return;
+
+            if (_validTypes.Contains(type))
+                return;
+
+            // Use a scheme where we push a stack of types in the process of loading
+            // When the stack pops, without throwing an exception, the type will be marked as being detected as successfully loadable.
+            // We need this complex scheme, as types can have circular dependencies. In addition, due to circular references, we can
+            // be forced to move when a type is successfully marked as loaded up the stack to an earlier call to EnsureLoadableType
+            //
+            // For example, consider the following case:
+            // interface IInterface<T> {}
+            // interface IPassThruInterface<T> : IInterface<T> {}
+            // interface ISimpleInterface {}
+            // class A<T> : IInterface<A<T>>, IPassThruInterface<A<T>>, ISimpleInterface {}
+            // class B : A<B> {}
+            //
+            // We call EnsureLoadableType on B
+            //
+            // This will generate the following interesting stacks of calls to EnsureLoadableType
+            //
+            // B -> A<B> -> B
+            //  This stack indicates that A<B> can only be considered loadable if B is considered loadable, so we must defer marking
+            //  A<B> as loadable until we finish processing B.
+            //
+            // B -> A<B> -> ISimpleInterface
+            //  Since examining ISimpleInterface does not have any dependency on B or A<B>, it can be marked as loadable as soon
+            //  as we finish processing it.
+            //
+            // B -> A<B> -> IInterface<A<B>> -> A<B>
+            //  This stack indicates that IInterface<A<B>> can be considered loadable if A<B> is considered loadable. We must defer
+            //  marking IInterface<A<B>> as loadable until we are able to mark A<B> as loadable. Based on the stack above, that can
+            //  only happen once B is considered loadable.
+            //
+            // B -> A<B> -> IPassthruInterface<A<B>> -> IInterface<A<B>>
+            //  This stack indicates that IPassthruInterface<A<B>> can be considered loadable if IInterface<A<B>> is considered
+            //  loadable. If this happens after the IInterface<A<B>> is marked as being loadable once B is considered loadable
+            //  then we will push the loadibility marking to the B level at this point. OR we will continue to recurse and the logic
+            //  will note that IInterface<A<B>> needs A<B> needs B which will move the marking up at that point.
+
+            if (PushTypeLoadInProgress(type))
+                return;
+
+            bool threwException = true;
+            try
+            {
+                EnsureLoadableTypeUncached(type);
+                threwException = false;
+            }
+            finally
+            {
+                PopTypeLoadabilityCheckInProgress(threwException);
+            }
+        }
+
+        private sealed class TypeLoadabilityCheckInProgress
+        {
+            public TypeDesc TypeInLoadabilityCheck;
+            public bool MarkTypeAsSuccessfullyLoadedIfNoExceptionThrown;
+            public List<TypeDesc> OtherTypesToMarkAsSuccessfullyLoaded;
+
+            public void AddToOtherTypesToMarkAsSuccessfullyLoaded(TypeDesc type)
+            {
+                if (OtherTypesToMarkAsSuccessfullyLoaded == EmptyList)
+                {
+                    OtherTypesToMarkAsSuccessfullyLoaded = new List<TypeDesc>();
+                }
+
+                Debug.Assert(!OtherTypesToMarkAsSuccessfullyLoaded.Contains(type));
+                OtherTypesToMarkAsSuccessfullyLoaded.Add(type);
+            }
+        }
+
+        // Returns true to indicate the type should be considered to be loadable (although it might not be, actually safety may require more code to execute)
+        private static bool PushTypeLoadInProgress(TypeDesc type)
+        {
+            t_typeLoadCheckInProgressStack ??= new List<TypeLoadabilityCheckInProgress>();
+
+            // Walk stack to see if the specified type is already in the process of being type checked.
+            int typeLoadCheckInProgressStackOffset = -1;
+            bool checkingMode = false; // Checking for match on TypeLoadabilityCheck field or in OtherTypesToMarkAsSuccessfullyLoaded. (true for OtherTypesToMarkAsSuccessfullyLoaded)
+            for (int typeCheckDepth = t_typeLoadCheckInProgressStack.Count - 1; typeCheckDepth >= 0; typeCheckDepth--)
+            {
+                if (t_typeLoadCheckInProgressStack[typeCheckDepth].TypeInLoadabilityCheck == type)
+                {
+                    // The stack contains the interesting type.
+                    if (t_typeLoadCheckInProgressStack[typeCheckDepth].MarkTypeAsSuccessfullyLoadedIfNoExceptionThrown)
+                    {
+                        // And this is the level where the type is known to be successfully loaded.
+                        typeLoadCheckInProgressStackOffset = typeCheckDepth;
+                        break;
+                    }
+                }
+                else if (t_typeLoadCheckInProgressStack[typeCheckDepth].OtherTypesToMarkAsSuccessfullyLoaded.Contains(type))
+                {
+                    // We've found where the type will be marked as successfully loaded.
+                    typeLoadCheckInProgressStackOffset = typeCheckDepth;
+                    break;
+                }
+            }
+
+            if (checkingMode)
+            {
+                // If we enabled checkingMode we should always have found the type
+                Debug.Assert(typeLoadCheckInProgressStackOffset != -1);
+            }
+
+            if (typeLoadCheckInProgressStackOffset == -1)
+            {
+                // The type is not already in the process of being checked for loadability, so return false to indicate that normal load checking should begin
+                TypeLoadabilityCheckInProgress typeCheckInProgress = new TypeLoadabilityCheckInProgress();
+                typeCheckInProgress.TypeInLoadabilityCheck = type;
+                typeCheckInProgress.OtherTypesToMarkAsSuccessfullyLoaded = EmptyList;
+                typeCheckInProgress.MarkTypeAsSuccessfullyLoadedIfNoExceptionThrown = true;
+                t_typeLoadCheckInProgressStack.Add(typeCheckInProgress);
+                return false;
+            }
+
+            // Move timing of when types are considered loaded back to the point at which we mark this type as loaded
+            var typeLoadCheckToAddTo = t_typeLoadCheckInProgressStack[typeLoadCheckInProgressStackOffset];
+            for (int typeCheckDepth = t_typeLoadCheckInProgressStack.Count - 1; typeCheckDepth > typeLoadCheckInProgressStackOffset; typeCheckDepth--)
+            {
+                if (t_typeLoadCheckInProgressStack[typeCheckDepth].MarkTypeAsSuccessfullyLoadedIfNoExceptionThrown)
+                {
+                    typeLoadCheckToAddTo.AddToOtherTypesToMarkAsSuccessfullyLoaded(t_typeLoadCheckInProgressStack[typeCheckDepth].TypeInLoadabilityCheck);
+                    t_typeLoadCheckInProgressStack[typeCheckDepth].MarkTypeAsSuccessfullyLoadedIfNoExceptionThrown = false;
+                }
+
+                foreach (var typeToMove in t_typeLoadCheckInProgressStack[typeCheckDepth].OtherTypesToMarkAsSuccessfullyLoaded)
+                {
+                    typeLoadCheckToAddTo.AddToOtherTypesToMarkAsSuccessfullyLoaded(typeToMove);
+                }
+
+                t_typeLoadCheckInProgressStack[typeCheckDepth].OtherTypesToMarkAsSuccessfullyLoaded = EmptyList;
+            }
+
+            // We are going to report that the type should be considered to be loadable at this stage
+            return true;
+        }
+
+        private void PopTypeLoadabilityCheckInProgress(bool exceptionThrown)
+        {
+            Debug.Assert(EmptyList.Count == 0);
+            var typeLoadabilityCheck = t_typeLoadCheckInProgressStack[t_typeLoadCheckInProgressStack.Count - 1];
+            t_typeLoadCheckInProgressStack.RemoveAt(t_typeLoadCheckInProgressStack.Count - 1);
+
+            if (!exceptionThrown)
+            {
+                if (!typeLoadabilityCheck.MarkTypeAsSuccessfullyLoadedIfNoExceptionThrown)
+                {
+                    Debug.Assert(typeLoadabilityCheck.OtherTypesToMarkAsSuccessfullyLoaded.Count == 0);
+                }
+
+                if (typeLoadabilityCheck.MarkTypeAsSuccessfullyLoadedIfNoExceptionThrown)
+                {
+                    _validTypes.GetOrCreateValue(typeLoadabilityCheck.TypeInLoadabilityCheck);
+                    foreach (var type in typeLoadabilityCheck.OtherTypesToMarkAsSuccessfullyLoaded)
+                    {
+                        _validTypes.GetOrCreateValue(type);
+                    }
+                }
+            }
         }
 
         public void EnsureLoadableMethod(MethodDesc method)
@@ -37,11 +206,10 @@ private sealed class ValidTypeHashTable : LockFreeReaderHashtable<TypeDesc, Type
         {
             protected override bool CompareKeyToValue(TypeDesc key, TypeDesc value) => key == value;
             protected override bool CompareValueToValue(TypeDesc value1, TypeDesc value2) => value1 == value2;
-            protected override TypeDesc CreateValueFromKey(TypeDesc key) => EnsureLoadableTypeUncached(key);
+            protected override TypeDesc CreateValueFromKey(TypeDesc key) => key;
             protected override int GetKeyHashCode(TypeDesc key) => key.GetHashCode();
             protected override int GetValueHashCode(TypeDesc value) => value.GetHashCode();
         }
-        private readonly ValidTypeHashTable _validTypes = new ValidTypeHashTable();
 
         private static TypeDesc EnsureLoadableTypeUncached(TypeDesc type)
         {
@@ -53,7 +221,11 @@ private static TypeDesc EnsureLoadableTypeUncached(TypeDesc type)
                 }
             }
 
-            if (type.IsParameterizedType)
+            if (type.IsGenericParameter)
+            {
+                // Generic parameters don't need validation
+            }
+            else if (type.IsParameterizedType)
             {
                 // Validate parameterized types
                 var parameterizedType = (ParameterizedType)type;
@@ -161,16 +333,14 @@ private static TypeDesc EnsureLoadableTypeUncached(TypeDesc type)
                     {
                         ThrowHelper.ThrowTypeLoadException(ExceptionStringID.ClassLoadGeneral, type);
                     }
+
+                    ((CompilerTypeSystemContext)type.Context).EnsureLoadableType(typeArg);
                 }
 
-                // Don't validate constraints with crossgen2 - the type system is not set up correctly
-                // and doesn't see generic interfaces on arrays.
-#if !READYTORUN
                 if (!defType.IsCanonicalSubtype(CanonicalFormKind.Any) && !defType.CheckConstraints())
                 {
                     ThrowHelper.ThrowTypeLoadException(ExceptionStringID.ClassLoadGeneral, type);
                 }
-#endif
 
                 // Check the type doesn't have bogus MethodImpls or overrides and we can get the finalizer.
                 defType.GetFinalizer();

src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunLibraryRootProvider.cs

@@ -119,6 +119,8 @@ private static void CheckTypeCanBeUsedInSignature(TypeDesc type)
                     ThrowHelper.ThrowTypeLoadException(ExceptionStringID.ClassLoadGeneral, type);
                 }
             }
+
+            ((CompilerTypeSystemContext)type.Context).EnsureLoadableType(type);
         }
 
         private static Instantiation GetInstantiationThatMeetsConstraints(Instantiation definition)

src/tests/readytorun/regressions/GitHub_89337/GitHub_89337.cs

@@ -0,0 +1,22 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+// This is the C# file used to generate GitHub_89337.il
+
+class GitHub_89337
+{
+    class Generic<T>
+    {
+    }
+
+    class Derived : System.Xml.NameTable { }
+    static Generic<Derived>? Passthru(Generic<Derived>? param)
+    {
+        return param;
+    }
+
+    static int Main()
+    {
+        return 100;
+    }
+}