Skip to content

Downgrade MethodTables used in reflection invoke #111610

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Mar 6, 2025
Merged

Downgrade MethodTables used in reflection invoke #111610

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d75da5c
Update ReflectionInvokeMapNode.cs
MichalStrehovsky Jan 20, 2025
a1bbcf1
Update ReflectedFieldNode.cs
MichalStrehovsky Jan 20, 2025
b728c83
Normalize type instantiation in dependencies
MichalStrehovsky Jan 20, 2025
dc34018
Maybe we don't need #92994
MichalStrehovsky Jan 23, 2025
fff9a05
Beef up test
MichalStrehovsky Jan 23, 2025
4885566
Update ReflectionInvokeMapNode.cs
MichalStrehovsky Feb 6, 2025
cd4225d
Adjust tests
MichalStrehovsky Feb 7, 2025
30bab66
Is this still a saving?
MichalStrehovsky Feb 10, 2025
89c7c2e
Revert "Is this still a saving?"
MichalStrehovsky Mar 4, 2025
2bf75a0
Merge branch 'main' into MichalStrehovsky-patch-1
MichalStrehovsky Mar 4, 2025
c3cf2ba
Fix generic recursion
MichalStrehovsky Mar 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
96 changes: 36 additions & 60 deletions src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/DynamicInvokeInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,37 +74,36 @@ 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;

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;
}
Expand All @@ -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))
{
Expand All @@ -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)
Expand Down Expand Up @@ -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<object?> copyOfParameters,
void* byrefParameters,
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering: Since this conditional clause is identical to the one in the parallel method, would extracting a helper method to ensure they never diverge? Seems like this whole block doesn't reference the parameters (except by the indexed-to arg) so it should be possible to extract 682-705.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need to validate it doesn't deoptimize codegen, this code is quite performance sensitive.

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)
Expand Down
26 changes: 26 additions & 0 deletions src/coreclr/tools/Common/Compiler/GenericCycleDetection/GraphBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
using Internal.TypeSystem;
using Internal.TypeSystem.Ecma;

using Debug = System.Diagnostics.Debug;

namespace ILCompiler
{
internal static partial class LazyGenericsSupport
Expand Down Expand Up @@ -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;
}
Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/tools/Common/Compiler/GenericCycleDetection/ModuleCycleInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedFieldNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFacto
}

TypeDesc fieldType = _field.FieldType.NormalizeInstantiation();
ReflectionInvokeMapNode.AddSignatureDependency(ref dependencies, factory, fieldType, "Type of the field");
ReflectionInvokeMapNode.AddSignatureDependency(ref dependencies, factory, _field, fieldType, "Type of the field", isOut: true);

return dependencies;
}
Expand Down
39 changes: 26 additions & 13 deletions ...eclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, method, signature.ReturnType, "Reflection invoke", isOut: true);
foreach (var parameterType in signature)
AddSignatureDependency(ref dependencies, factory, parameterType, "Reflection invoke");
AddSignatureDependency(ref dependencies, factory, method, parameterType, "Reflection invoke", isOut: false);
}

if (method.OwningType.IsValueType && !method.Signature.IsStatic)
Expand Down Expand Up @@ -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, TypeSystemEntity referent, 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)
Expand All @@ -109,16 +112,26 @@ 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;

TypeDesc canonType = type.ConvertToCanonForm(CanonicalFormKind.Specific);
if (canonType.IsCanonicalSubtype(CanonicalFormKind.Any))
GenericTypesTemplateMap.GetTemplateTypeDependencies(ref dependencies, factory, canonType);
else
dependencies.Add(factory.MaximallyConstructableType(type), 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)
Expand Down
7 changes: 6 additions & 1 deletion src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,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();
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion ...o.Linker.Tests.Cases/LinkXml/TypeWithPreserveFieldsHasBackingFieldsOfPropertiesRemoved.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ interface IDog
string Name { get; set; }
}

[Kept (By = Tool.Trimmer)]
[Kept]
interface IFoo<T>
{

Expand All @@ -62,6 +62,7 @@ interface IFoo3<T, K, J>
int Bar3 { get; set; }
}

[Kept (By = Tool.NativeAot)]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is not actually testing anything relevant, so I'm not worried about "regression" here.

  • In ILLinker, looks like preserve=fields is going to skip over compiler generated backing fields. Not sure where that logic is but we clearly don't do that in native AOT and we do preserve them. Won't lose my sleep over that.
  • The test infrastructure doesn't check field preservation on native AOT side since we don't really "trim" fields (we vacate space for them no matter what, there's leeway whether reflection metadata is generated).

So the only observable effect in this test is whether the type of the field survived and now it by design survives if the field was a target of reflection.

class Cat
{
}
Expand Down
8 changes: 4 additions & 4 deletions src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ObjectGetType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -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)]
Expand Down
Loading