From d75da5c84a5c667e2a433c19d05fb29897240e02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 20 Jan 2025 00:27:57 -0800 Subject: [PATCH 01/10] Update ReflectionInvokeMapNode.cs --- .../ReflectionInvokeMapNode.cs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs index ae964fd498a5da..c5901171bd81c4 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs @@ -55,9 +55,9 @@ public static void AddDependenciesDueToReflectability(ref DependencyList depende dependencies.Add(factory.MethodEntrypoint(invokeStub), "Reflection invoke"); var signature = method.Signature; - AddSignatureDependency(ref dependencies, factory, signature.ReturnType, "Reflection invoke"); + AddSignatureDependency(ref dependencies, factory, signature.ReturnType, "Reflection invoke", isOut: true); foreach (var parameterType in signature) - AddSignatureDependency(ref dependencies, factory, parameterType, "Reflection invoke"); + AddSignatureDependency(ref dependencies, factory, parameterType, "Reflection invoke", isOut: false); } if (method.OwningType.IsValueType && !method.Signature.IsStatic) @@ -96,10 +96,13 @@ public static void AddDependenciesDueToReflectability(ref DependencyList depende ReflectionVirtualInvokeMapNode.GetVirtualInvokeMapDependencies(ref dependencies, factory, method); } - internal static void AddSignatureDependency(ref DependencyList dependencies, NodeFactory factory, TypeDesc type, string reason) + internal static void AddSignatureDependency(ref DependencyList dependencies, NodeFactory factory, TypeDesc type, string reason, bool isOut) { if (type.IsByRef) + { type = ((ParameterizedType)type).ParameterType; + isOut = true; + } // Pointer runtime type handles can be created at runtime if necessary while (type.IsPointer) @@ -114,11 +117,10 @@ internal static void AddSignatureDependency(ref DependencyList dependencies, Nod if (type.IsGCPointer) return; - TypeDesc canonType = type.ConvertToCanonForm(CanonicalFormKind.Specific); - if (canonType.IsCanonicalSubtype(CanonicalFormKind.Any)) - GenericTypesTemplateMap.GetTemplateTypeDependencies(ref dependencies, factory, canonType); - else + if (isOut) dependencies.Add(factory.MaximallyConstructableType(type), reason); + else + dependencies.Add(factory.NecessaryTypeSymbol(type), reason); } public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) From a1bbcf197204a89da39ad588689b4c77672c0333 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 20 Jan 2025 00:29:16 -0800 Subject: [PATCH 02/10] Update ReflectedFieldNode.cs --- .../Compiler/DependencyAnalysis/ReflectedFieldNode.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedFieldNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedFieldNode.cs index 07f612e2aae59a..bf0467bfeaa144 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedFieldNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedFieldNode.cs @@ -97,7 +97,7 @@ public override IEnumerable GetStaticDependencies(NodeFacto } TypeDesc fieldType = _field.FieldType.NormalizeInstantiation(); - ReflectionInvokeMapNode.AddSignatureDependency(ref dependencies, factory, fieldType, "Type of the field"); + ReflectionInvokeMapNode.AddSignatureDependency(ref dependencies, factory, fieldType, "Type of the field", isOut: true); return dependencies; } From b728c839b2bd234eb7087305e57a57d69b8b71cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 20 Jan 2025 05:55:17 -0800 Subject: [PATCH 03/10] Normalize type instantiation in dependencies --- .../Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs index c5901171bd81c4..a2eb2cfc1df204 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs @@ -118,9 +118,9 @@ internal static void AddSignatureDependency(ref DependencyList dependencies, Nod return; if (isOut) - dependencies.Add(factory.MaximallyConstructableType(type), reason); + dependencies.Add(factory.MaximallyConstructableType(type.NormalizeInstantiation()), reason); else - dependencies.Add(factory.NecessaryTypeSymbol(type), reason); + dependencies.Add(factory.NecessaryTypeSymbol(type.NormalizeInstantiation()), reason); } public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) From dc34018ec09e337b8cd6924c0b71ebac9033e369 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Thu, 23 Jan 2025 14:29:12 +0100 Subject: [PATCH 04/10] Maybe we don't need #92994 Reverts #92994 (manually since there were many merge conflicts due to EETypePtr deletion.) --- .../src/System/InvokeUtils.cs | 5 +- .../System/Reflection/DynamicInvokeInfo.cs | 96 +++++++------------ .../ReflectionInvokeMapNode.cs | 9 +- 3 files changed, 40 insertions(+), 70 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs index 61a2d5367c7cb3..9cae9eb9198375 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs @@ -409,11 +409,8 @@ private static Exception CreateChangeTypeException(MethodTable* srcEEType, Metho } internal static ArgumentException CreateChangeTypeArgumentException(MethodTable* srcEEType, MethodTable* dstEEType, bool destinationIsByRef = false) - => CreateChangeTypeArgumentException(srcEEType, Type.GetTypeFromHandle(new RuntimeTypeHandle(dstEEType)), destinationIsByRef); - - internal static ArgumentException CreateChangeTypeArgumentException(MethodTable* srcEEType, Type dstType, bool destinationIsByRef = false) { - object? destinationTypeName = dstType; + object? destinationTypeName = Type.GetTypeFromHandle(new RuntimeTypeHandle(dstEEType)); if (destinationIsByRef) destinationTypeName += "&"; return new ArgumentException(SR.Format(SR.Arg_ObjObjEx, Type.GetTypeFromHandle(new RuntimeTypeHandle(srcEEType)), destinationTypeName)); diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/DynamicInvokeInfo.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/DynamicInvokeInfo.cs index d7837bdc649e18..00fed3ec6c6f3c 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/DynamicInvokeInfo.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/DynamicInvokeInfo.cs @@ -74,21 +74,20 @@ public unsafe DynamicInvokeInfo(MethodBase method, IntPtr invokeThunk) { Transform transform = default; - var argumentType = (RuntimeType)parameters[i].ParameterType; + Type argumentType = parameters[i].ParameterType; if (argumentType.IsByRef) { _needsCopyBack = true; transform |= Transform.ByRef; - argumentType = (RuntimeType)argumentType.GetElementType()!; + argumentType = argumentType.GetElementType()!; } Debug.Assert(!argumentType.IsByRef); - // This can return a null MethodTable for reference types. - // The compiler makes sure it returns a non-null MT for everything else. - MethodTable* eeArgumentType = argumentType.ToMethodTableMayBeNull(); - if (argumentType.IsValueType) + MethodTable* eeArgumentType = argumentType.TypeHandle.ToMethodTable(); + + if (eeArgumentType->IsValueType) { - Debug.Assert(eeArgumentType->IsValueType); + Debug.Assert(argumentType.IsValueType); if (eeArgumentType->IsByRefLike) _argumentCount = ArgumentCount_NotSupported_ByRefLike; @@ -96,15 +95,15 @@ public unsafe DynamicInvokeInfo(MethodBase method, IntPtr invokeThunk) if (eeArgumentType->IsNullable) transform |= Transform.Nullable; } - else if (argumentType.IsPointer) + else if (eeArgumentType->IsPointer) { - Debug.Assert(eeArgumentType->IsPointer); + Debug.Assert(argumentType.IsPointer); transform |= Transform.Pointer; } - else if (argumentType.IsFunctionPointer) + else if (eeArgumentType->IsFunctionPointer) { - Debug.Assert(eeArgumentType->IsFunctionPointer); + Debug.Assert(argumentType.IsFunctionPointer); transform |= Transform.FunctionPointer; } @@ -122,18 +121,19 @@ public unsafe DynamicInvokeInfo(MethodBase method, IntPtr invokeThunk) { Transform transform = default; - var returnType = (RuntimeType)methodInfo.ReturnType; + Type returnType = methodInfo.ReturnType; if (returnType.IsByRef) { transform |= Transform.ByRef; - returnType = (RuntimeType)returnType.GetElementType()!; + returnType = returnType.GetElementType()!; } Debug.Assert(!returnType.IsByRef); - MethodTable* eeReturnType = returnType.ToMethodTableMayBeNull(); - if (returnType.IsValueType) + MethodTable* eeReturnType = returnType.TypeHandle.ToMethodTable(); + + if (eeReturnType->IsValueType) { - Debug.Assert(eeReturnType->IsValueType); + Debug.Assert(returnType.IsValueType); if (returnType != typeof(void)) { @@ -152,17 +152,17 @@ public unsafe DynamicInvokeInfo(MethodBase method, IntPtr invokeThunk) _argumentCount = ArgumentCount_NotSupported; // ByRef to void return } } - else if (returnType.IsPointer) + else if (eeReturnType->IsPointer) { - Debug.Assert(eeReturnType->IsPointer); + Debug.Assert(returnType.IsPointer); transform |= Transform.Pointer; if ((transform & Transform.ByRef) == 0) transform |= Transform.AllocateReturnBox; } - else if (returnType.IsFunctionPointer) + else if (eeReturnType->IsFunctionPointer) { - Debug.Assert(eeReturnType->IsFunctionPointer); + Debug.Assert(returnType.IsFunctionPointer); transform |= Transform.FunctionPointer; if ((transform & Transform.ByRef) == 0) @@ -585,12 +585,6 @@ private unsafe ref byte InvokeDirectWithFewArguments( return defaultValue; } - private void ThrowForNeverValidNonNullArgument(MethodTable* srcEEType, int index) - { - Debug.Assert(index != 0 || _isStatic); - throw InvokeUtils.CreateChangeTypeArgumentException(srcEEType, Method.GetParametersAsSpan()[index - (_isStatic ? 0 : 1)].ParameterType, destinationIsByRef: false); - } - private unsafe void CheckArguments( Span copyOfParameters, void* byrefParameters, @@ -630,25 +624,16 @@ private unsafe void CheckArguments( MethodTable* srcEEType = arg.GetMethodTable(); MethodTable* dstEEType = argumentInfo.Type; - if (srcEEType != dstEEType) - { - // Destination type can be null if we don't have a MethodTable for this type. This means one cannot - // possibly pass a valid non-null object instance here. - if (dstEEType == null) - { - ThrowForNeverValidNonNullArgument(srcEEType, i); - } - - if (!(RuntimeImports.AreTypesAssignable(srcEEType, dstEEType) || - (dstEEType->IsInterface && arg is System.Runtime.InteropServices.IDynamicInterfaceCastable castable + if (!(srcEEType == dstEEType || + RuntimeImports.AreTypesAssignable(srcEEType, dstEEType) || + (dstEEType->IsInterface && arg is System.Runtime.InteropServices.IDynamicInterfaceCastable castable && castable.IsInterfaceImplemented(new RuntimeTypeHandle(dstEEType), throwIfNotImplemented: false)))) - { - // ByRefs have to be exact match - if ((argumentInfo.Transform & Transform.ByRef) != 0) - throw InvokeUtils.CreateChangeTypeArgumentException(srcEEType, argumentInfo.Type, destinationIsByRef: true); + { + // ByRefs have to be exact match + if ((argumentInfo.Transform & Transform.ByRef) != 0) + throw InvokeUtils.CreateChangeTypeArgumentException(srcEEType, argumentInfo.Type, destinationIsByRef: true); - arg = InvokeUtils.CheckArgumentConversions(arg, argumentInfo.Type, InvokeUtils.CheckArgumentSemantics.DynamicInvoke, binderBundle); - } + arg = InvokeUtils.CheckArgumentConversions(arg, argumentInfo.Type, InvokeUtils.CheckArgumentSemantics.DynamicInvoke, binderBundle); } if ((argumentInfo.Transform & Transform.Reference) == 0) @@ -707,25 +692,16 @@ private unsafe void CheckArguments( MethodTable* srcEEType = arg.GetMethodTable(); MethodTable* dstEEType = argumentInfo.Type; - if (srcEEType != dstEEType) - { - // Destination type can be null if we don't have a MethodTable for this type. This means one cannot - // possibly pass a valid non-null object instance here. - if (dstEEType == null) - { - ThrowForNeverValidNonNullArgument(srcEEType, i); - } - - if (!(RuntimeImports.AreTypesAssignable(srcEEType, dstEEType) || - (dstEEType->IsInterface && arg is System.Runtime.InteropServices.IDynamicInterfaceCastable castable + if (!(srcEEType == dstEEType || + RuntimeImports.AreTypesAssignable(srcEEType, dstEEType) || + (dstEEType->IsInterface && arg is System.Runtime.InteropServices.IDynamicInterfaceCastable castable && castable.IsInterfaceImplemented(new RuntimeTypeHandle(dstEEType), throwIfNotImplemented: false)))) - { - // ByRefs have to be exact match - if ((argumentInfo.Transform & Transform.ByRef) != 0) - throw InvokeUtils.CreateChangeTypeArgumentException(srcEEType, argumentInfo.Type, destinationIsByRef: true); + { + // ByRefs have to be exact match + if ((argumentInfo.Transform & Transform.ByRef) != 0) + throw InvokeUtils.CreateChangeTypeArgumentException(srcEEType, argumentInfo.Type, destinationIsByRef: true); - arg = InvokeUtils.CheckArgumentConversions(arg, argumentInfo.Type, InvokeUtils.CheckArgumentSemantics.DynamicInvoke, binderBundle: null); - } + arg = InvokeUtils.CheckArgumentConversions(arg, argumentInfo.Type, InvokeUtils.CheckArgumentSemantics.DynamicInvoke, binderBundle: null); } if ((argumentInfo.Transform & Transform.Reference) == 0) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs index a2eb2cfc1df204..a6627663b6f9ff 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs @@ -112,12 +112,9 @@ internal static void AddSignatureDependency(ref DependencyList dependencies, Nod if (type.IsPrimitive || type.IsVoid) return; - // Reflection doesn't need the ability to generate MethodTables out of thin air for reference types. - // Skip generating the dependencies. - if (type.IsGCPointer) - return; - - if (isOut) + // Reflection might need to create boxed instances of valuetypes as part of reflection invocation. + // Non-valuetypes are only needed for the purposes of casting/type checks. + if (isOut && !type.IsGCPointer) dependencies.Add(factory.MaximallyConstructableType(type.NormalizeInstantiation()), reason); else dependencies.Add(factory.NecessaryTypeSymbol(type.NormalizeInstantiation()), reason); From fff9a054d47a2321e87bb5b310644b7b5b1458c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Thu, 23 Jan 2025 14:29:20 +0100 Subject: [PATCH 05/10] Beef up test --- .../SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs b/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs index 032c3a07d7dea6..1de11a8bb8effc 100644 --- a/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs +++ b/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs @@ -132,8 +132,13 @@ public static void Run() { MethodInfo mi = typeof(TestReflectionInvokeSignatures).GetMethod(nameof(Invoke2)); - mi.Invoke(null, new object[1]); + var args = new object[1]; + mi.Invoke(null, args); ThrowIfNotPresent(typeof(TestReflectionInvokeSignatures), nameof(Allocated1)); + if (args[0].GetType().Name != nameof(Allocated1)) + throw new Exception(); + if (!args[0].ToString().Contains(nameof(Allocated1))) + throw new Exception(); } } } From 4885566ad154adaf0fa696dde6654dc12246df7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Thu, 6 Feb 2025 07:58:58 +0100 Subject: [PATCH 06/10] Update ReflectionInvokeMapNode.cs --- .../Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs index a6627663b6f9ff..abac8d89e481c0 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs @@ -114,7 +114,10 @@ internal static void AddSignatureDependency(ref DependencyList dependencies, Nod // Reflection might need to create boxed instances of valuetypes as part of reflection invocation. // Non-valuetypes are only needed for the purposes of casting/type checks. - if (isOut && !type.IsGCPointer) + // If this is a non-exact type, we need the type loader template to get the type handle. + if (type.IsCanonicalSubtype(CanonicalFormKind.Any)) + GenericTypesTemplateMap.GetTemplateTypeDependencies(ref dependencies, factory, type.NormalizeInstantiation()); + else if (isOut && !type.IsGCPointer) dependencies.Add(factory.MaximallyConstructableType(type.NormalizeInstantiation()), reason); else dependencies.Add(factory.NecessaryTypeSymbol(type.NormalizeInstantiation()), reason); From cd4225d77e5ed739f7e249215bb03600a06c58ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Fri, 7 Feb 2025 10:49:35 +0100 Subject: [PATCH 07/10] Adjust tests --- ...thPreserveFieldsHasBackingFieldsOfPropertiesRemoved.cs | 3 ++- .../Mono.Linker.Tests.Cases/Reflection/ObjectGetType.cs | 8 ++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkXml/TypeWithPreserveFieldsHasBackingFieldsOfPropertiesRemoved.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkXml/TypeWithPreserveFieldsHasBackingFieldsOfPropertiesRemoved.cs index f516a7945f9abf..f84a97d4ed6388 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkXml/TypeWithPreserveFieldsHasBackingFieldsOfPropertiesRemoved.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkXml/TypeWithPreserveFieldsHasBackingFieldsOfPropertiesRemoved.cs @@ -45,7 +45,7 @@ interface IDog string Name { get; set; } } - [Kept (By = Tool.Trimmer)] + [Kept] interface IFoo { @@ -62,6 +62,7 @@ interface IFoo3 int Bar3 { get; set; } } + [Kept (By = Tool.NativeAot)] class Cat { } diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ObjectGetType.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ObjectGetType.cs index 0152323e9ef3ca..8c6d53fbe55204 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ObjectGetType.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ObjectGetType.cs @@ -1230,8 +1230,8 @@ class DerivedFromMethodsBase : MethodAnnotatedBase void PrivateMethod () { } } - [Kept(By = Tool.Trimmer /* only used in a method signature, not legitimate to keep beyond IL-level trimming */)] - [KeptBaseType (typeof (MethodAnnotatedBase), By = Tool.Trimmer)] + [Kept] + [KeptBaseType (typeof (MethodAnnotatedBase))] class AnotherMethodsDerived : MethodAnnotatedBase { [Kept(By = Tool.Trimmer)] @@ -1343,9 +1343,9 @@ interface INestedInterface void InterfaceMethod (); } - [Kept (By = Tool.Trimmer /* only used in a method signature, not legitimate to keep beyond IL-level trimming */)] + [Kept] [KeptMember (".ctor()", By = Tool.Trimmer)] - [KeptBaseType (typeof (AnnotatedBase), By = Tool.Trimmer)] + [KeptBaseType (typeof (AnnotatedBase))] class AnotherAnnotatedType : AnnotatedBase { [Kept (By = Tool.Trimmer)] From 30bab660b8f7b07273d03f59bec93976c9474ec0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 10 Feb 2025 07:07:47 +0100 Subject: [PATCH 08/10] Is this still a saving? --- .../DependencyAnalysis/ReflectionInvokeMapNode.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs index abac8d89e481c0..69b4cd6e7b3411 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs @@ -115,12 +115,14 @@ internal static void AddSignatureDependency(ref DependencyList dependencies, Nod // Reflection might need to create boxed instances of valuetypes as part of reflection invocation. // Non-valuetypes are only needed for the purposes of casting/type checks. // If this is a non-exact type, we need the type loader template to get the type handle. + // If this is a generic type we need maximally constructable form since we don't keep track of + // necessary types in the generic types hashtable. if (type.IsCanonicalSubtype(CanonicalFormKind.Any)) GenericTypesTemplateMap.GetTemplateTypeDependencies(ref dependencies, factory, type.NormalizeInstantiation()); - else if (isOut && !type.IsGCPointer) - dependencies.Add(factory.MaximallyConstructableType(type.NormalizeInstantiation()), reason); + else if ((isOut && !type.IsGCPointer) || type.HasInstantiation) + dependencies.Add(factory.MaximallyConstructableType(type), reason); else - dependencies.Add(factory.NecessaryTypeSymbol(type.NormalizeInstantiation()), reason); + dependencies.Add(factory.NecessaryTypeSymbol(type), reason); } public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) From 89c7c2e9df57f9f9e38f6839698102a5d7849245 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Tue, 4 Mar 2025 14:17:09 +0100 Subject: [PATCH 09/10] Revert "Is this still a saving?" This reverts commit 30bab660b8f7b07273d03f59bec93976c9474ec0. --- .../DependencyAnalysis/ReflectionInvokeMapNode.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs index 69b4cd6e7b3411..abac8d89e481c0 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs @@ -115,14 +115,12 @@ internal static void AddSignatureDependency(ref DependencyList dependencies, Nod // Reflection might need to create boxed instances of valuetypes as part of reflection invocation. // Non-valuetypes are only needed for the purposes of casting/type checks. // If this is a non-exact type, we need the type loader template to get the type handle. - // If this is a generic type we need maximally constructable form since we don't keep track of - // necessary types in the generic types hashtable. if (type.IsCanonicalSubtype(CanonicalFormKind.Any)) GenericTypesTemplateMap.GetTemplateTypeDependencies(ref dependencies, factory, type.NormalizeInstantiation()); - else if ((isOut && !type.IsGCPointer) || type.HasInstantiation) - dependencies.Add(factory.MaximallyConstructableType(type), reason); + else if (isOut && !type.IsGCPointer) + dependencies.Add(factory.MaximallyConstructableType(type.NormalizeInstantiation()), reason); else - dependencies.Add(factory.NecessaryTypeSymbol(type), reason); + dependencies.Add(factory.NecessaryTypeSymbol(type.NormalizeInstantiation()), reason); } public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) From c3cf2ba3211713a0235f18c87eab5f31c19753f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 5 Mar 2025 07:57:15 +0100 Subject: [PATCH 10/10] Fix generic recursion --- .../GenericCycleDetection/GraphBuilder.cs | 26 ++++++++++++++ .../GenericCycleDetection/ModuleCycleInfo.cs | 6 ++-- .../DependencyAnalysis/ReflectedFieldNode.cs | 2 +- .../ReflectionInvokeMapNode.cs | 35 ++++++++++++------- 4 files changed, 53 insertions(+), 16 deletions(-) diff --git a/src/coreclr/tools/Common/Compiler/GenericCycleDetection/GraphBuilder.cs b/src/coreclr/tools/Common/Compiler/GenericCycleDetection/GraphBuilder.cs index dda635a8b8c271..8ac16e3bb3c4c7 100644 --- a/src/coreclr/tools/Common/Compiler/GenericCycleDetection/GraphBuilder.cs +++ b/src/coreclr/tools/Common/Compiler/GenericCycleDetection/GraphBuilder.cs @@ -9,6 +9,8 @@ using Internal.TypeSystem; using Internal.TypeSystem.Ecma; +using Debug = System.Diagnostics.Debug; + namespace ILCompiler { internal static partial class LazyGenericsSupport @@ -207,6 +209,30 @@ public GraphBuilder(EcmaModule assembly) } } } + + if (isGenericType) + { + Instantiation typeContext = default; + + foreach (FieldDefinitionHandle fieldHandle in typeDefinition.GetFields()) + { + try + { + var ecmaField = (EcmaField)assembly.GetObject(fieldHandle); + + if (typeContext.IsNull) + { + typeContext = ecmaField.OwningType.Instantiation; + Debug.Assert(!typeContext.IsNull); + } + + ProcessTypeReference(ecmaField.FieldType, typeContext, default); + } + catch (TypeSystemException) + { + } + } + } } return; } diff --git a/src/coreclr/tools/Common/Compiler/GenericCycleDetection/ModuleCycleInfo.cs b/src/coreclr/tools/Common/Compiler/GenericCycleDetection/ModuleCycleInfo.cs index 5d0a4f36cc1d04..35f6b083e41f1c 100644 --- a/src/coreclr/tools/Common/Compiler/GenericCycleDetection/ModuleCycleInfo.cs +++ b/src/coreclr/tools/Common/Compiler/GenericCycleDetection/ModuleCycleInfo.cs @@ -223,10 +223,10 @@ public void DetectCycle(TypeSystemEntity owner, TypeSystemEntity referent) if (_depthCutoff < 0) return; - // Not clear if generic recursion through fields is a thing - if (referent is FieldDesc) + // Fields don't introduce more genericness than their owning type, so treat as their owning type + if (referent is FieldDesc referentField) { - return; + referent = referentField.OwningType; } var ownerType = owner as TypeDesc; diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedFieldNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedFieldNode.cs index bf0467bfeaa144..f5359a5f6e57b5 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedFieldNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedFieldNode.cs @@ -97,7 +97,7 @@ public override IEnumerable GetStaticDependencies(NodeFacto } TypeDesc fieldType = _field.FieldType.NormalizeInstantiation(); - ReflectionInvokeMapNode.AddSignatureDependency(ref dependencies, factory, fieldType, "Type of the field", isOut: true); + ReflectionInvokeMapNode.AddSignatureDependency(ref dependencies, factory, _field, fieldType, "Type of the field", isOut: true); return dependencies; } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs index abac8d89e481c0..f0786d96ac9f71 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs @@ -55,9 +55,9 @@ public static void AddDependenciesDueToReflectability(ref DependencyList depende dependencies.Add(factory.MethodEntrypoint(invokeStub), "Reflection invoke"); var signature = method.Signature; - AddSignatureDependency(ref dependencies, factory, signature.ReturnType, "Reflection invoke", isOut: true); + AddSignatureDependency(ref dependencies, factory, method, signature.ReturnType, "Reflection invoke", isOut: true); foreach (var parameterType in signature) - AddSignatureDependency(ref dependencies, factory, parameterType, "Reflection invoke", isOut: false); + AddSignatureDependency(ref dependencies, factory, method, parameterType, "Reflection invoke", isOut: false); } if (method.OwningType.IsValueType && !method.Signature.IsStatic) @@ -96,7 +96,7 @@ public static void AddDependenciesDueToReflectability(ref DependencyList depende ReflectionVirtualInvokeMapNode.GetVirtualInvokeMapDependencies(ref dependencies, factory, method); } - internal static void AddSignatureDependency(ref DependencyList dependencies, NodeFactory factory, TypeDesc type, string reason, bool isOut) + internal static void AddSignatureDependency(ref DependencyList dependencies, NodeFactory factory, TypeSystemEntity referent, TypeDesc type, string reason, bool isOut) { if (type.IsByRef) { @@ -112,15 +112,26 @@ internal static void AddSignatureDependency(ref DependencyList dependencies, Nod if (type.IsPrimitive || type.IsVoid) return; - // Reflection might need to create boxed instances of valuetypes as part of reflection invocation. - // Non-valuetypes are only needed for the purposes of casting/type checks. - // If this is a non-exact type, we need the type loader template to get the type handle. - if (type.IsCanonicalSubtype(CanonicalFormKind.Any)) - GenericTypesTemplateMap.GetTemplateTypeDependencies(ref dependencies, factory, type.NormalizeInstantiation()); - else if (isOut && !type.IsGCPointer) - dependencies.Add(factory.MaximallyConstructableType(type.NormalizeInstantiation()), reason); - else - dependencies.Add(factory.NecessaryTypeSymbol(type.NormalizeInstantiation()), reason); + try + { + factory.TypeSystemContext.DetectGenericCycles(type, referent); + + // Reflection might need to create boxed instances of valuetypes as part of reflection invocation. + // Non-valuetypes are only needed for the purposes of casting/type checks. + // If this is a non-exact type, we need the type loader template to get the type handle. + if (type.IsCanonicalSubtype(CanonicalFormKind.Any)) + GenericTypesTemplateMap.GetTemplateTypeDependencies(ref dependencies, factory, type.NormalizeInstantiation()); + else if (isOut && !type.IsGCPointer) + dependencies.Add(factory.MaximallyConstructableType(type.NormalizeInstantiation()), reason); + else + dependencies.Add(factory.NecessaryTypeSymbol(type.NormalizeInstantiation()), reason); + } + catch (TypeSystemException) + { + // It's fine to continue compiling if there's a problem getting these. There's going to be a MissingMetadata + // exception when actually trying to invoke this and the exception will be different than the one we'd get with + // a JIT, but that's fine, we don't need to be bug-for-bug compatible. + } } public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)