Fix analysis issue in LDTOKEN dependencies (#89757)
authorMichal Strehovský <MichalStrehovsky@users.noreply.github.com>
Thu, 3 Aug 2023 00:29:40 +0000 (09:29 +0900)
committerGitHub <noreply@github.com>
Thu, 3 Aug 2023 00:29:40 +0000 (09:29 +0900)
This was found in Pri0 testing. The LDTOKEN data structures didn't have enough information for generic method instantiation arguments. The Pri0 test was around generic virtual methods, but this is a more general issue so adding an extra test for that.

I'm also changing how `NodeFactory.ReflectedMethod` works - it will now require being called with canonical things explicitly (and asserts if it isn't) instead of canonicalizing behind the scenes. This was obscuring the problem.

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/CustomAttributeBasedDependencyAlgorithm.cs
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/RootingHelpers.cs
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/RootingServiceProvider.cs
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs
src/tests/nativeaot/SmokeTests/Reflection/Reflection.cs

index 79c0cf0..d11569a 100644 (file)
@@ -479,7 +479,7 @@ namespace ILCompiler.Dataflow
                                         && systemTypeValue.RepresentedType.Type.GetParameterlessConstructor() is MethodDesc ctorMethod
                                         && !reflectionMarker.Factory.MetadataManager.IsReflectionBlocked(ctorMethod))
                                     {
-                                        reflectionMarker.Dependencies.Add(reflectionMarker.Factory.ReflectedMethod(ctorMethod), "Marshal API");
+                                        reflectionMarker.Dependencies.Add(reflectionMarker.Factory.ReflectedMethod(ctorMethod.GetCanonMethodTarget(CanonicalFormKind.Specific)), "Marshal API");
                                     }
                                 }
                             }
index 77b41a5..68e8b5f 100644 (file)
@@ -128,7 +128,7 @@ namespace ILCompiler.DependencyAnalysis
                     // Make a new list in case we need to abort.
                     var caDependencies = factory.MetadataManager.GetDependenciesForCustomAttribute(factory, constructor, decodedValue, parent) ?? new DependencyList();
 
-                    caDependencies.Add(factory.ReflectedMethod(constructor), "Attribute constructor");
+                    caDependencies.Add(factory.ReflectedMethod(constructor.GetCanonMethodTarget(CanonicalFormKind.Specific)), "Attribute constructor");
                     caDependencies.Add(factory.ReflectedType(constructor.OwningType), "Attribute type");
 
                     if (AddDependenciesFromCustomAttributeBlob(caDependencies, factory, constructor.OwningType, decodedValue))
@@ -234,7 +234,7 @@ namespace ILCompiler.DependencyAnalysis
                             setterMethod = factory.TypeSystemContext.GetMethodForInstantiatedType(setterMethod, (InstantiatedType)attributeType);
                         }
 
-                        dependencies.Add(factory.ReflectedMethod(setterMethod), "Custom attribute blob");
+                        dependencies.Add(factory.ReflectedMethod(setterMethod.GetCanonMethodTarget(CanonicalFormKind.Specific)), "Custom attribute blob");
                     }
 
                     return true;
index 6f17af2..23bc01a 100644 (file)
@@ -985,9 +985,6 @@ namespace ILCompiler.DependencyAnalysis
         private NodeCache<MethodDesc, ReflectedMethodNode> _reflectedMethods;
         public ReflectedMethodNode ReflectedMethod(MethodDesc method)
         {
-            // We track reflectability at canonical method body level
-            method = method.GetCanonMethodTarget(CanonicalFormKind.Specific);
-
             return _reflectedMethods.GetOrAdd(method);
         }
 
index 2e72c95..038c1ee 100644 (file)
@@ -186,7 +186,7 @@ namespace ILCompiler
             }
 
             dependencies ??= new DependencyList();
-            dependencies.Add(factory.ReflectedMethod(method), reason);
+            dependencies.Add(factory.ReflectedMethod(method.GetCanonMethodTarget(CanonicalFormKind.Specific)), reason);
 
             return true;
         }
index 12a590d..f6b5624 100644 (file)
@@ -52,7 +52,7 @@ namespace ILCompiler
             if (!_factory.MetadataManager.IsReflectionBlocked(method))
             {
                 _factory.TypeSystemContext.EnsureLoadableMethod(method);
-                _rootAdder(_factory.ReflectedMethod(method), reason);
+                _rootAdder(_factory.ReflectedMethod(method.GetCanonMethodTarget(CanonicalFormKind.Specific)), reason);
             }
         }
 
index e5784e0..e680bf8 100644 (file)
@@ -286,7 +286,7 @@ namespace ILCompiler
                 if (!IsReflectionBlocked(invokeMethod))
                 {
                     dependencies ??= new DependencyList();
-                    dependencies.Add(factory.ReflectedMethod(invokeMethod), "Delegate invoke method is always reflectable");
+                    dependencies.Add(factory.ReflectedMethod(invokeMethod.GetCanonMethodTarget(CanonicalFormKind.Specific)), "Delegate invoke method is always reflectable");
                 }
             }
 
@@ -507,7 +507,7 @@ namespace ILCompiler
 
                     dependencies ??= new CombinedDependencyList();
                     dependencies.Add(new DependencyNodeCore<NodeFactory>.CombinedDependencyListEntry(
-                        factory.ReflectedMethod(reflectedMethod),
+                        factory.ReflectedMethod(reflectedMethod.GetCanonMethodTarget(CanonicalFormKind.Specific)),
                         factory.ReflectedMethod(reflectedMethod.GetTypicalMethodDefinition()),
                         "Methods have same reflectability"));
                 }
@@ -538,7 +538,18 @@ namespace ILCompiler
             dependencies ??= new DependencyList();
 
             if (!IsReflectionBlocked(method))
-                dependencies.Add(factory.ReflectedMethod(method), "LDTOKEN method");
+            {
+                MethodDesc canonicalMethod = method.GetCanonMethodTarget(CanonicalFormKind.Specific);
+                dependencies.Add(factory.ReflectedMethod(canonicalMethod), "LDTOKEN method");
+
+                if (canonicalMethod != method)
+                {
+                    foreach (TypeDesc instArg in method.Instantiation)
+                    {
+                        dependencies.Add(factory.ReflectedType(instArg), "LDTOKEN method");
+                    }
+                }
+            }
         }
 
         public override void GetDependenciesDueToDelegateCreation(ref DependencyList dependencies, NodeFactory factory, MethodDesc target)
@@ -546,7 +557,7 @@ namespace ILCompiler
             if (!IsReflectionBlocked(target))
             {
                 dependencies ??= new DependencyList();
-                dependencies.Add(factory.ReflectedMethod(target), "Target of a delegate");
+                dependencies.Add(factory.ReflectedMethod(target.GetCanonMethodTarget(CanonicalFormKind.Specific)), "Target of a delegate");
 
                 if (target.IsVirtual)
                     dependencies.Add(factory.DelegateTargetVirtualMethod(target.GetCanonMethodTarget(CanonicalFormKind.Specific)), "Target of a delegate");
@@ -631,7 +642,7 @@ namespace ILCompiler
                 if (method.IsAbstract && GetMetadataCategory(method) != 0)
                 {
                     dependencies ??= new DependencyList();
-                    dependencies.Add(factory.ReflectedMethod(method), "Abstract reflectable method");
+                    dependencies.Add(factory.ReflectedMethod(method.GetCanonMethodTarget(CanonicalFormKind.Specific)), "Abstract reflectable method");
                 }
             }
         }
index b2ad549..8a77223 100644 (file)
@@ -65,6 +65,8 @@ internal static class ReflectionTest
         TestIsValueTypeWithoutTypeHandle.Run();
         TestMdArrayLoad.Run();
         TestByRefTypeLoad.Run();
+        TestGenericLdtoken.Run();
+        TestAbstractGenericLdtoken.Run();
 
         //
         // Mostly functionality tests
@@ -2402,6 +2404,59 @@ internal static class ReflectionTest
         }
     }
 
+    class TestGenericLdtoken
+    {
+        class Base
+        {
+            [MethodImpl(MethodImplOptions.NoInlining)]
+            public static Base GetInstance() => new Base();
+            public virtual Type GrabType<T>() => typeof(T);
+        }
+
+        class Derived : Base
+        {
+            [MethodImpl(MethodImplOptions.NoInlining)]
+            public static new Base GetInstance() => new Derived();
+            public override Type GrabType<T>() => typeof(T);
+        }
+
+        class Generic<T> { }
+
+        class Atom { }
+
+        public static void Run()
+        {
+            Expression<Func<Type>> grabType = () => Base.GetInstance().GrabType<Generic<Atom>>();
+            Console.WriteLine(grabType.Compile()().FullName);
+        }
+    }
+
+    class TestAbstractGenericLdtoken
+    {
+        abstract class Base
+        {
+            [MethodImpl(MethodImplOptions.NoInlining)]
+            public static Base GetInstance() => null;
+            public abstract Type GrabType<T>();
+        }
+
+        class Generic<T> { }
+
+        class Atom { }
+
+        public static void Run()
+        {
+            Expression<Func<Type>> grabType = () => Base.GetInstance().GrabType<Generic<Atom>>();
+            try
+            {
+                grabType.Compile()();
+            }
+            catch (NullReferenceException)
+            {
+            }
+        }
+    }
+
     class TestEntryPoint
     {
         public static void Run()