Skip to content

Commit 666e918

Browse files
Downgrade MethodTables used in reflection invoke (#111610)
When we make a method reflection-callable, we have to make sure the reflection stack can get type handles of types within the method signature. The type handles are used as part of reflection activation to do castability checks (is it valid to call the method with this parameter?) and to do boxing of return values. In the past, what we did: * If the type has a canonical form, generate type loader template for it. * Otherwise, generate a constructed MethodTable. This was too much, so we restricted it in #92994 into: * If the type is a GC pointer, don't generate anything. Update reflection stack to be able to deal with a null type handle if and only if this is a reference type and nobody actually allocated such reference type. * Otherwise do what we did above. This is still a bit too much, as demonstrated in #111544 (comment). In this PR, I'm restricting this further to: * If the type is in the canonical form (not just "has canonical form" - "_is_ canonical form"), generate type loader template. * If the type is an out valuetype parameter, generate constructed method table (since reflection stack is going to end up boxing) * Generate necessary (unconstructed) method table otherwise (since we're only going to do castability checks and necessary method tables are exactly for that) I'm deleting the "GC pointer could have null methodtable" complication since it doesn't help much in practice once we downgraded to necessary method tables (that revert is in dc34018 and rt-sz measurements show it really doesn't affect much). Cc @dotnet/ilc-contrib
1 parent a015c51 commit 666e918

File tree

9 files changed

+105
-87
lines changed

9 files changed

+105
-87
lines changed

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -409,11 +409,8 @@ private static Exception CreateChangeTypeException(MethodTable* srcEEType, Metho
409409
}
410410

411411
internal static ArgumentException CreateChangeTypeArgumentException(MethodTable* srcEEType, MethodTable* dstEEType, bool destinationIsByRef = false)
412-
=> CreateChangeTypeArgumentException(srcEEType, Type.GetTypeFromHandle(new RuntimeTypeHandle(dstEEType)), destinationIsByRef);
413-
414-
internal static ArgumentException CreateChangeTypeArgumentException(MethodTable* srcEEType, Type dstType, bool destinationIsByRef = false)
415412
{
416-
object? destinationTypeName = dstType;
413+
object? destinationTypeName = Type.GetTypeFromHandle(new RuntimeTypeHandle(dstEEType));
417414
if (destinationIsByRef)
418415
destinationTypeName += "&";
419416
return new ArgumentException(SR.Format(SR.Arg_ObjObjEx, Type.GetTypeFromHandle(new RuntimeTypeHandle(srcEEType)), destinationTypeName));

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/DynamicInvokeInfo.cs

Lines changed: 36 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -74,37 +74,36 @@ public unsafe DynamicInvokeInfo(MethodBase method, IntPtr invokeThunk)
7474
{
7575
Transform transform = default;
7676

77-
var argumentType = (RuntimeType)parameters[i].ParameterType;
77+
Type argumentType = parameters[i].ParameterType;
7878
if (argumentType.IsByRef)
7979
{
8080
_needsCopyBack = true;
8181
transform |= Transform.ByRef;
82-
argumentType = (RuntimeType)argumentType.GetElementType()!;
82+
argumentType = argumentType.GetElementType()!;
8383
}
8484
Debug.Assert(!argumentType.IsByRef);
8585

86-
// This can return a null MethodTable for reference types.
87-
// The compiler makes sure it returns a non-null MT for everything else.
88-
MethodTable* eeArgumentType = argumentType.ToMethodTableMayBeNull();
89-
if (argumentType.IsValueType)
86+
MethodTable* eeArgumentType = argumentType.TypeHandle.ToMethodTable();
87+
88+
if (eeArgumentType->IsValueType)
9089
{
91-
Debug.Assert(eeArgumentType->IsValueType);
90+
Debug.Assert(argumentType.IsValueType);
9291

9392
if (eeArgumentType->IsByRefLike)
9493
_argumentCount = ArgumentCount_NotSupported_ByRefLike;
9594

9695
if (eeArgumentType->IsNullable)
9796
transform |= Transform.Nullable;
9897
}
99-
else if (argumentType.IsPointer)
98+
else if (eeArgumentType->IsPointer)
10099
{
101-
Debug.Assert(eeArgumentType->IsPointer);
100+
Debug.Assert(argumentType.IsPointer);
102101

103102
transform |= Transform.Pointer;
104103
}
105-
else if (argumentType.IsFunctionPointer)
104+
else if (eeArgumentType->IsFunctionPointer)
106105
{
107-
Debug.Assert(eeArgumentType->IsFunctionPointer);
106+
Debug.Assert(argumentType.IsFunctionPointer);
108107

109108
transform |= Transform.FunctionPointer;
110109
}
@@ -122,18 +121,19 @@ public unsafe DynamicInvokeInfo(MethodBase method, IntPtr invokeThunk)
122121
{
123122
Transform transform = default;
124123

125-
var returnType = (RuntimeType)methodInfo.ReturnType;
124+
Type returnType = methodInfo.ReturnType;
126125
if (returnType.IsByRef)
127126
{
128127
transform |= Transform.ByRef;
129-
returnType = (RuntimeType)returnType.GetElementType()!;
128+
returnType = returnType.GetElementType()!;
130129
}
131130
Debug.Assert(!returnType.IsByRef);
132131

133-
MethodTable* eeReturnType = returnType.ToMethodTableMayBeNull();
134-
if (returnType.IsValueType)
132+
MethodTable* eeReturnType = returnType.TypeHandle.ToMethodTable();
133+
134+
if (eeReturnType->IsValueType)
135135
{
136-
Debug.Assert(eeReturnType->IsValueType);
136+
Debug.Assert(returnType.IsValueType);
137137

138138
if (returnType != typeof(void))
139139
{
@@ -152,17 +152,17 @@ public unsafe DynamicInvokeInfo(MethodBase method, IntPtr invokeThunk)
152152
_argumentCount = ArgumentCount_NotSupported; // ByRef to void return
153153
}
154154
}
155-
else if (returnType.IsPointer)
155+
else if (eeReturnType->IsPointer)
156156
{
157-
Debug.Assert(eeReturnType->IsPointer);
157+
Debug.Assert(returnType.IsPointer);
158158

159159
transform |= Transform.Pointer;
160160
if ((transform & Transform.ByRef) == 0)
161161
transform |= Transform.AllocateReturnBox;
162162
}
163-
else if (returnType.IsFunctionPointer)
163+
else if (eeReturnType->IsFunctionPointer)
164164
{
165-
Debug.Assert(eeReturnType->IsFunctionPointer);
165+
Debug.Assert(returnType.IsFunctionPointer);
166166

167167
transform |= Transform.FunctionPointer;
168168
if ((transform & Transform.ByRef) == 0)
@@ -585,12 +585,6 @@ private unsafe ref byte InvokeDirectWithFewArguments(
585585
return defaultValue;
586586
}
587587

588-
private void ThrowForNeverValidNonNullArgument(MethodTable* srcEEType, int index)
589-
{
590-
Debug.Assert(index != 0 || _isStatic);
591-
throw InvokeUtils.CreateChangeTypeArgumentException(srcEEType, Method.GetParametersAsSpan()[index - (_isStatic ? 0 : 1)].ParameterType, destinationIsByRef: false);
592-
}
593-
594588
private unsafe void CheckArguments(
595589
Span<object?> copyOfParameters,
596590
void* byrefParameters,
@@ -630,25 +624,16 @@ private unsafe void CheckArguments(
630624
MethodTable* srcEEType = arg.GetMethodTable();
631625
MethodTable* dstEEType = argumentInfo.Type;
632626

633-
if (srcEEType != dstEEType)
634-
{
635-
// Destination type can be null if we don't have a MethodTable for this type. This means one cannot
636-
// possibly pass a valid non-null object instance here.
637-
if (dstEEType == null)
638-
{
639-
ThrowForNeverValidNonNullArgument(srcEEType, i);
640-
}
641-
642-
if (!(RuntimeImports.AreTypesAssignable(srcEEType, dstEEType) ||
643-
(dstEEType->IsInterface && arg is System.Runtime.InteropServices.IDynamicInterfaceCastable castable
627+
if (!(srcEEType == dstEEType ||
628+
RuntimeImports.AreTypesAssignable(srcEEType, dstEEType) ||
629+
(dstEEType->IsInterface && arg is System.Runtime.InteropServices.IDynamicInterfaceCastable castable
644630
&& castable.IsInterfaceImplemented(new RuntimeTypeHandle(dstEEType), throwIfNotImplemented: false))))
645-
{
646-
// ByRefs have to be exact match
647-
if ((argumentInfo.Transform & Transform.ByRef) != 0)
648-
throw InvokeUtils.CreateChangeTypeArgumentException(srcEEType, argumentInfo.Type, destinationIsByRef: true);
631+
{
632+
// ByRefs have to be exact match
633+
if ((argumentInfo.Transform & Transform.ByRef) != 0)
634+
throw InvokeUtils.CreateChangeTypeArgumentException(srcEEType, argumentInfo.Type, destinationIsByRef: true);
649635

650-
arg = InvokeUtils.CheckArgumentConversions(arg, argumentInfo.Type, InvokeUtils.CheckArgumentSemantics.DynamicInvoke, binderBundle);
651-
}
636+
arg = InvokeUtils.CheckArgumentConversions(arg, argumentInfo.Type, InvokeUtils.CheckArgumentSemantics.DynamicInvoke, binderBundle);
652637
}
653638

654639
if ((argumentInfo.Transform & Transform.Reference) == 0)
@@ -707,25 +692,16 @@ private unsafe void CheckArguments(
707692
MethodTable* srcEEType = arg.GetMethodTable();
708693
MethodTable* dstEEType = argumentInfo.Type;
709694

710-
if (srcEEType != dstEEType)
711-
{
712-
// Destination type can be null if we don't have a MethodTable for this type. This means one cannot
713-
// possibly pass a valid non-null object instance here.
714-
if (dstEEType == null)
715-
{
716-
ThrowForNeverValidNonNullArgument(srcEEType, i);
717-
}
718-
719-
if (!(RuntimeImports.AreTypesAssignable(srcEEType, dstEEType) ||
720-
(dstEEType->IsInterface && arg is System.Runtime.InteropServices.IDynamicInterfaceCastable castable
695+
if (!(srcEEType == dstEEType ||
696+
RuntimeImports.AreTypesAssignable(srcEEType, dstEEType) ||
697+
(dstEEType->IsInterface && arg is System.Runtime.InteropServices.IDynamicInterfaceCastable castable
721698
&& castable.IsInterfaceImplemented(new RuntimeTypeHandle(dstEEType), throwIfNotImplemented: false))))
722-
{
723-
// ByRefs have to be exact match
724-
if ((argumentInfo.Transform & Transform.ByRef) != 0)
725-
throw InvokeUtils.CreateChangeTypeArgumentException(srcEEType, argumentInfo.Type, destinationIsByRef: true);
699+
{
700+
// ByRefs have to be exact match
701+
if ((argumentInfo.Transform & Transform.ByRef) != 0)
702+
throw InvokeUtils.CreateChangeTypeArgumentException(srcEEType, argumentInfo.Type, destinationIsByRef: true);
726703

727-
arg = InvokeUtils.CheckArgumentConversions(arg, argumentInfo.Type, InvokeUtils.CheckArgumentSemantics.DynamicInvoke, binderBundle: null);
728-
}
704+
arg = InvokeUtils.CheckArgumentConversions(arg, argumentInfo.Type, InvokeUtils.CheckArgumentSemantics.DynamicInvoke, binderBundle: null);
729705
}
730706

731707
if ((argumentInfo.Transform & Transform.Reference) == 0)

src/coreclr/tools/Common/Compiler/GenericCycleDetection/GraphBuilder.cs

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
using Internal.TypeSystem;
1010
using Internal.TypeSystem.Ecma;
1111

12+
using Debug = System.Diagnostics.Debug;
13+
1214
namespace ILCompiler
1315
{
1416
internal static partial class LazyGenericsSupport
@@ -207,6 +209,30 @@ public GraphBuilder(EcmaModule assembly)
207209
}
208210
}
209211
}
212+
213+
if (isGenericType)
214+
{
215+
Instantiation typeContext = default;
216+
217+
foreach (FieldDefinitionHandle fieldHandle in typeDefinition.GetFields())
218+
{
219+
try
220+
{
221+
var ecmaField = (EcmaField)assembly.GetObject(fieldHandle);
222+
223+
if (typeContext.IsNull)
224+
{
225+
typeContext = ecmaField.OwningType.Instantiation;
226+
Debug.Assert(!typeContext.IsNull);
227+
}
228+
229+
ProcessTypeReference(ecmaField.FieldType, typeContext, default);
230+
}
231+
catch (TypeSystemException)
232+
{
233+
}
234+
}
235+
}
210236
}
211237
return;
212238
}

src/coreclr/tools/Common/Compiler/GenericCycleDetection/ModuleCycleInfo.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -223,10 +223,10 @@ public void DetectCycle(TypeSystemEntity owner, TypeSystemEntity referent)
223223
if (_depthCutoff < 0)
224224
return;
225225

226-
// Not clear if generic recursion through fields is a thing
227-
if (referent is FieldDesc)
226+
// Fields don't introduce more genericness than their owning type, so treat as their owning type
227+
if (referent is FieldDesc referentField)
228228
{
229-
return;
229+
referent = referentField.OwningType;
230230
}
231231

232232
var ownerType = owner as TypeDesc;

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedFieldNode.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ public override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFacto
9797
}
9898

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

102102
return dependencies;
103103
}

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,9 @@ public static void AddDependenciesDueToReflectability(ref DependencyList depende
5555
dependencies.Add(factory.MethodEntrypoint(invokeStub), "Reflection invoke");
5656

5757
var signature = method.Signature;
58-
AddSignatureDependency(ref dependencies, factory, signature.ReturnType, "Reflection invoke");
58+
AddSignatureDependency(ref dependencies, factory, method, signature.ReturnType, "Reflection invoke", isOut: true);
5959
foreach (var parameterType in signature)
60-
AddSignatureDependency(ref dependencies, factory, parameterType, "Reflection invoke");
60+
AddSignatureDependency(ref dependencies, factory, method, parameterType, "Reflection invoke", isOut: false);
6161
}
6262

6363
if (method.OwningType.IsValueType && !method.Signature.IsStatic)
@@ -96,10 +96,13 @@ public static void AddDependenciesDueToReflectability(ref DependencyList depende
9696
ReflectionVirtualInvokeMapNode.GetVirtualInvokeMapDependencies(ref dependencies, factory, method);
9797
}
9898

99-
internal static void AddSignatureDependency(ref DependencyList dependencies, NodeFactory factory, TypeDesc type, string reason)
99+
internal static void AddSignatureDependency(ref DependencyList dependencies, NodeFactory factory, TypeSystemEntity referent, TypeDesc type, string reason, bool isOut)
100100
{
101101
if (type.IsByRef)
102+
{
102103
type = ((ParameterizedType)type).ParameterType;
104+
isOut = true;
105+
}
103106

104107
// Pointer runtime type handles can be created at runtime if necessary
105108
while (type.IsPointer)
@@ -109,16 +112,26 @@ internal static void AddSignatureDependency(ref DependencyList dependencies, Nod
109112
if (type.IsPrimitive || type.IsVoid)
110113
return;
111114

112-
// Reflection doesn't need the ability to generate MethodTables out of thin air for reference types.
113-
// Skip generating the dependencies.
114-
if (type.IsGCPointer)
115-
return;
116-
117-
TypeDesc canonType = type.ConvertToCanonForm(CanonicalFormKind.Specific);
118-
if (canonType.IsCanonicalSubtype(CanonicalFormKind.Any))
119-
GenericTypesTemplateMap.GetTemplateTypeDependencies(ref dependencies, factory, canonType);
120-
else
121-
dependencies.Add(factory.MaximallyConstructableType(type), reason);
115+
try
116+
{
117+
factory.TypeSystemContext.DetectGenericCycles(type, referent);
118+
119+
// Reflection might need to create boxed instances of valuetypes as part of reflection invocation.
120+
// Non-valuetypes are only needed for the purposes of casting/type checks.
121+
// If this is a non-exact type, we need the type loader template to get the type handle.
122+
if (type.IsCanonicalSubtype(CanonicalFormKind.Any))
123+
GenericTypesTemplateMap.GetTemplateTypeDependencies(ref dependencies, factory, type.NormalizeInstantiation());
124+
else if (isOut && !type.IsGCPointer)
125+
dependencies.Add(factory.MaximallyConstructableType(type.NormalizeInstantiation()), reason);
126+
else
127+
dependencies.Add(factory.NecessaryTypeSymbol(type.NormalizeInstantiation()), reason);
128+
}
129+
catch (TypeSystemException)
130+
{
131+
// It's fine to continue compiling if there's a problem getting these. There's going to be a MissingMetadata
132+
// exception when actually trying to invoke this and the exception will be different than the one we'd get with
133+
// a JIT, but that's fine, we don't need to be bug-for-bug compatible.
134+
}
122135
}
123136

124137
public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)

src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DeadCodeElimination.cs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,8 +133,13 @@ public static void Run()
133133

134134
{
135135
MethodInfo mi = typeof(TestReflectionInvokeSignatures).GetMethod(nameof(Invoke2));
136-
mi.Invoke(null, new object[1]);
136+
var args = new object[1];
137+
mi.Invoke(null, args);
137138
ThrowIfNotPresent(typeof(TestReflectionInvokeSignatures), nameof(Allocated1));
139+
if (args[0].GetType().Name != nameof(Allocated1))
140+
throw new Exception();
141+
if (!args[0].ToString().Contains(nameof(Allocated1)))
142+
throw new Exception();
138143
}
139144
}
140145
}

src/tools/illink/test/Mono.Linker.Tests.Cases/LinkXml/TypeWithPreserveFieldsHasBackingFieldsOfPropertiesRemoved.cs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ interface IDog
4545
string Name { get; set; }
4646
}
4747

48-
[Kept (By = Tool.Trimmer)]
48+
[Kept]
4949
interface IFoo<T>
5050
{
5151

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

65+
[Kept (By = Tool.NativeAot)]
6566
class Cat
6667
{
6768
}

src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ObjectGetType.cs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1230,8 +1230,8 @@ class DerivedFromMethodsBase : MethodAnnotatedBase
12301230
void PrivateMethod () { }
12311231
}
12321232

1233-
[Kept(By = Tool.Trimmer /* only used in a method signature, not legitimate to keep beyond IL-level trimming */)]
1234-
[KeptBaseType (typeof (MethodAnnotatedBase), By = Tool.Trimmer)]
1233+
[Kept]
1234+
[KeptBaseType (typeof (MethodAnnotatedBase))]
12351235
class AnotherMethodsDerived : MethodAnnotatedBase
12361236
{
12371237
[Kept(By = Tool.Trimmer)]
@@ -1343,9 +1343,9 @@ interface INestedInterface
13431343
void InterfaceMethod ();
13441344
}
13451345

1346-
[Kept (By = Tool.Trimmer /* only used in a method signature, not legitimate to keep beyond IL-level trimming */)]
1346+
[Kept]
13471347
[KeptMember (".ctor()", By = Tool.Trimmer)]
1348-
[KeptBaseType (typeof (AnnotatedBase), By = Tool.Trimmer)]
1348+
[KeptBaseType (typeof (AnnotatedBase))]
13491349
class AnotherAnnotatedType : AnnotatedBase
13501350
{
13511351
[Kept (By = Tool.Trimmer)]

0 commit comments

Comments
 (0)