From 748d4b39edd2a3fa394fd19c0602e65b318bb019 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?=
 <MichalStrehovsky@users.noreply.github.com>
Date: Wed, 3 Nov 2021 13:42:36 +0900
Subject: [PATCH] Detect generic recursion in generic lookups (#1699)

We currently cut off generic recursion at two spots:

1. When there's a direct call to something that causes a recursion (e.g. `Method<T>` calls `Method<Gen<T>>` and `Gen` is a struct).
2. When there's recursion in the generic dictionary (e.g. the sample above but `Gen` is a class).

There's yet another variation of this - indirect call to something that causes a recursion. Above two won't capture this because indirect calls don't go through the codepath 1, and codepath 2 is already too late (the recursion happens within the canonical code we already generated and invalidating dictionary entries is too late).

This adds one more spot to cut things off.

This is hit in Npgsql, but only on Linux, for whatever reason.
---
 .../Compiler/Compilation.cs                   |  5 +++
 .../Compiler/LazyGenerics/ModuleCycleInfo.cs  |  6 ++++
 .../nativeaot/SmokeTests/Generics/Generics.cs | 32 ++++++++++++++++++-
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Compilation.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Compilation.cs
index d649c6a6c1faba..be57f7d1b0b9f6 100644
--- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Compilation.cs
+++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Compilation.cs
@@ -317,6 +317,11 @@ public ISymbolNode ComputeConstantLookup(ReadyToRunHelperId lookupKind, object t
 
         public GenericDictionaryLookup ComputeGenericLookup(MethodDesc contextMethod, ReadyToRunHelperId lookupKind, object targetOfLookup)
         {
+            if (targetOfLookup is TypeSystemEntity typeSystemEntity)
+            {
+                _nodeFactory.TypeSystemContext.DetectGenericCycles(contextMethod, typeSystemEntity);
+            }
+
             GenericContextSource contextSource;
 
             if (contextMethod.RequiresInstMethodDescArg())
diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/LazyGenerics/ModuleCycleInfo.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/LazyGenerics/ModuleCycleInfo.cs
index 0c944395502d0a..3baab6e689fe62 100644
--- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/LazyGenerics/ModuleCycleInfo.cs
+++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/LazyGenerics/ModuleCycleInfo.cs
@@ -148,6 +148,12 @@ internal class GenericCycleDetector
 
             public void DetectCycle(TypeSystemEntity owner, TypeSystemEntity referent)
             {
+                // Not clear if generic recursion through fields is a thing
+                if (referent is FieldDesc)
+                {
+                    return;
+                }
+
                 var ownerType = owner as TypeDesc;
                 var ownerMethod = owner as MethodDesc;
                 var ownerDefinition = ownerType?.GetTypeDefinition() ?? (TypeSystemEntity)ownerMethod.GetTypicalMethodDefinition();
diff --git a/src/tests/nativeaot/SmokeTests/Generics/Generics.cs b/src/tests/nativeaot/SmokeTests/Generics/Generics.cs
index 8132859b94fd8c..0badf2be9bbe3d 100644
--- a/src/tests/nativeaot/SmokeTests/Generics/Generics.cs
+++ b/src/tests/nativeaot/SmokeTests/Generics/Generics.cs
@@ -43,6 +43,7 @@ static int Main()
         TestSimpleGenericRecursion.Run();
         TestGenericRecursionFromNpgsql.Run();
         TestRecursionInGenericVirtualMethods.Run();
+        TestRecursionThroughGenericLookups.Run();
 #if !CODEGEN_CPP
         TestNullableCasting.Run();
         TestVariantCasting.Run();
@@ -3005,7 +3006,7 @@ struct GenStruct<T> { }
 
         class GenClass<T> { }
 
-        private static object RecurseOverStruct<T>(int count) where T: new()
+        private static object RecurseOverStruct<T>(int count) where T : new()
         {
             if (count > 0)
                 RecurseOverStruct<GenStruct<T>>(count - 1);
@@ -3113,4 +3114,33 @@ public static void Run()
             s_derived.Get<object>();
         }
     }
+
+    class TestRecursionThroughGenericLookups
+    {
+        abstract class Handler<T>
+        {
+            public abstract void Write(object val);
+        }
+
+        class RangeHandler<T> : Handler<Gen<T>>
+        {
+            public override void Write(object val) { if (val is ArrayHandler<Gen<T>> h) h.Write(default); }
+        }
+
+        class ArrayHandler<T>
+        {
+            public virtual void Write(object val) { if (val is RangeHandler<T> h) h.Write(default); }
+        }
+
+        struct Gen<T>
+        {
+        }
+
+        public static void Run()
+        {
+            // There is a generic recursion in the above hierarchy. This just tests that we can compile.
+            new ArrayHandler<object>().Write(null);
+            new RangeHandler<object>().Write(default);
+        }
+    }
 }