Skip to content

Commit e3f6dd9

Browse files
MichalStrehovskyMichalStrehovsky
authored
Make EnumAllVirtualSlots return slots for interfaces (#121438)
Calling `EnumAllVirtualSlots` on classes returns all `newslot` methods that exist in the class hierarchy. Calling this on interfaces produced an empty enumeration. For runtime-async, it would be convenient if this returned the logical slots on interfaces. While interfaces don't have physical slots, they still have logical slots, so it's not a big stretch. Doing this now simplifies things in some places, and requires an extra `if` check in other places. But it will be really convenient for runtime-async.
1 parent 5668ab9 commit e3f6dd9

File tree

6 files changed

+90
-108
lines changed

6 files changed

+90
-108
lines changed

‎src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs‎

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -972,25 +972,26 @@ public override IEnumerable<MethodDesc> ComputeAllVirtualSlots(TypeDesc type)
972972

973973
// Enumerate all possible virtual slots of a type
974974
public static IEnumerable<MethodDesc> EnumAllVirtualSlots(MetadataType type)
975+
{
976+
return type.IsInterface ? type.GetAllVirtualMethods() : EnumAllVirtualSlotsOnClass(type);
977+
}
978+
private static IEnumerable<MethodDesc> EnumAllVirtualSlotsOnClass(MetadataType type)
975979
{
976980
MethodDescHashtable alreadyEnumerated = new MethodDescHashtable();
977-
if (!type.IsInterface)
981+
do
978982
{
979-
do
983+
foreach (MethodDesc m in type.GetAllVirtualMethods())
980984
{
981-
foreach (MethodDesc m in type.GetAllVirtualMethods())
985+
MethodDesc possibleVirtual = FindSlotDefiningMethodForVirtualMethod(m);
986+
if (!alreadyEnumerated.Contains(possibleVirtual))
982987
{
983-
MethodDesc possibleVirtual = FindSlotDefiningMethodForVirtualMethod(m);
984-
if (!alreadyEnumerated.Contains(possibleVirtual))
985-
{
986-
alreadyEnumerated.AddOrGetExisting(possibleVirtual);
987-
yield return possibleVirtual;
988-
}
988+
alreadyEnumerated.AddOrGetExisting(possibleVirtual);
989+
yield return possibleVirtual;
989990
}
991+
}
990992

991-
type = type.BaseType;
992-
} while (type != null);
993-
}
993+
type = type.BaseType;
994+
} while (type != null);
994995
}
995996

996997
/// <summary>

‎src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs‎

Lines changed: 47 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ private static VirtualMethodAnalysisFlags AnalyzeVirtualMethods(TypeDesc type)
141141
//
142142
foreach (DefType interfaceImpl in defType.RuntimeInterfaces)
143143
{
144-
foreach (MethodDesc method in interfaceImpl.GetAllVirtualMethods())
144+
foreach (MethodDesc method in interfaceImpl.EnumAllVirtualSlots())
145145
{
146146
if (!method.HasInstantiation)
147147
continue;
@@ -369,56 +369,59 @@ public override IEnumerable<CombinedDependencyListEntry> GetConditionalStaticDep
369369
// If we're producing a full vtable, none of the dependencies are conditional.
370370
if (!factory.VTable(defType).HasKnownVirtualMethodUse)
371371
{
372-
bool isNonInterfaceAbstractType = !defType.IsInterface && ((MetadataType)defType).IsAbstract;
373-
374-
foreach (MethodDesc decl in defType.EnumAllVirtualSlots())
372+
if (!defType.IsInterface)
375373
{
376-
// Generic virtual methods are tracked by an orthogonal mechanism.
377-
if (decl.HasInstantiation)
378-
continue;
374+
bool isAbstractType = ((MetadataType)defType).IsAbstract;
379375

380-
MethodDesc impl = defType.FindVirtualFunctionTargetMethodOnObjectType(decl);
381-
bool implOwnerIsAbstract = ((MetadataType)impl.OwningType).IsAbstract;
382-
383-
// We add a conditional dependency in two situations:
384-
// 1. The implementation is on this type. This is pretty obvious.
385-
// 2. The implementation comes from an abstract base type. We do this
386-
// because abstract types only request a TentativeMethodEntrypoint of the implementation.
387-
// The actual method body of this entrypoint might still be trimmed away.
388-
// We don't need to do this for implementations from non-abstract bases since
389-
// non-abstract types will create a hard conditional reference to their virtual
390-
// method implementations.
391-
//
392-
// We also skip abstract methods since they don't have a body to refer to.
393-
if ((impl.OwningType == defType || implOwnerIsAbstract) && !impl.IsAbstract)
376+
foreach (MethodDesc decl in defType.EnumAllVirtualSlots())
394377
{
395-
MethodDesc canonImpl = impl.GetCanonMethodTarget(CanonicalFormKind.Specific);
378+
// Generic virtual methods are tracked by an orthogonal mechanism.
379+
if (decl.HasInstantiation)
380+
continue;
396381

397-
// If this is an abstract type, only request a tentative entrypoint (whose body
398-
// might just be stubbed out). This lets us avoid generating method bodies for
399-
// virtual method on abstract types that are overridden in all their children.
382+
MethodDesc impl = defType.FindVirtualFunctionTargetMethodOnObjectType(decl);
383+
bool implOwnerIsAbstract = ((MetadataType)impl.OwningType).IsAbstract;
384+
385+
// We add a conditional dependency in two situations:
386+
// 1. The implementation is on this type. This is pretty obvious.
387+
// 2. The implementation comes from an abstract base type. We do this
388+
// because abstract types only request a TentativeMethodEntrypoint of the implementation.
389+
// The actual method body of this entrypoint might still be trimmed away.
390+
// We don't need to do this for implementations from non-abstract bases since
391+
// non-abstract types will create a hard conditional reference to their virtual
392+
// method implementations.
400393
//
401-
// We don't do this if the method can be placed in the sealed vtable since
402-
// those can never be overridden by children anyway.
403-
bool canUseTentativeMethod = isNonInterfaceAbstractType
404-
&& !decl.CanMethodBeInSealedVTable(factory)
405-
&& factory.CompilationModuleGroup.AllowVirtualMethodOnAbstractTypeOptimization(canonImpl);
406-
IMethodNode implNode = canUseTentativeMethod ?
407-
factory.TentativeMethodEntrypoint(canonImpl, impl.OwningType.IsValueType) :
408-
factory.MethodEntrypoint(canonImpl, impl.OwningType.IsValueType);
409-
result.Add(new CombinedDependencyListEntry(implNode, factory.VirtualMethodUse(decl), "Virtual method"));
394+
// We also skip abstract methods since they don't have a body to refer to.
395+
if ((impl.OwningType == defType || implOwnerIsAbstract) && !impl.IsAbstract)
396+
{
397+
MethodDesc canonImpl = impl.GetCanonMethodTarget(CanonicalFormKind.Specific);
398+
399+
// If this is an abstract type, only request a tentative entrypoint (whose body
400+
// might just be stubbed out). This lets us avoid generating method bodies for
401+
// virtual method on abstract types that are overridden in all their children.
402+
//
403+
// We don't do this if the method can be placed in the sealed vtable since
404+
// those can never be overridden by children anyway.
405+
bool canUseTentativeMethod = isAbstractType
406+
&& !decl.CanMethodBeInSealedVTable(factory)
407+
&& factory.CompilationModuleGroup.AllowVirtualMethodOnAbstractTypeOptimization(canonImpl);
408+
IMethodNode implNode = canUseTentativeMethod ?
409+
factory.TentativeMethodEntrypoint(canonImpl, impl.OwningType.IsValueType) :
410+
factory.MethodEntrypoint(canonImpl, impl.OwningType.IsValueType);
411+
result.Add(new CombinedDependencyListEntry(implNode, factory.VirtualMethodUse(decl), "Virtual method"));
412+
413+
result.Add(new CombinedDependencyListEntry(
414+
factory.AddressTakenMethodEntrypoint(canonImpl, impl.OwningType.IsValueType),
415+
factory.DelegateTargetVirtualMethod(decl.GetCanonMethodTarget(CanonicalFormKind.Specific)), "Slot is a delegate target"));
416+
}
410417

411-
result.Add(new CombinedDependencyListEntry(
412-
factory.AddressTakenMethodEntrypoint(canonImpl, impl.OwningType.IsValueType),
413-
factory.DelegateTargetVirtualMethod(decl.GetCanonMethodTarget(CanonicalFormKind.Specific)), "Slot is a delegate target"));
414-
}
418+
if (impl.OwningType == defType)
419+
{
420+
factory.MetadataManager.NoteOverridingMethod(decl, impl);
421+
}
415422

416-
if (impl.OwningType == defType)
417-
{
418-
factory.MetadataManager.NoteOverridingMethod(decl, impl);
423+
factory.MetadataManager.GetDependenciesForOverridingMethod(ref result, factory, decl, impl);
419424
}
420-
421-
factory.MetadataManager.GetDependenciesForOverridingMethod(ref result, factory, decl, impl);
422425
}
423426

424427
Debug.Assert(
@@ -448,7 +451,7 @@ public override IEnumerable<CombinedDependencyListEntry> GetConditionalStaticDep
448451

449452
bool isVariantInterfaceImpl = VariantInterfaceMethodUseNode.IsVariantInterfaceImplementation(factory, _type, interfaceType);
450453

451-
foreach (MethodDesc interfaceMethod in interfaceType.GetAllVirtualMethods())
454+
foreach (MethodDesc interfaceMethod in interfaceType.EnumAllVirtualSlots())
452455
{
453456
// Generic virtual methods are tracked by an orthogonal mechanism.
454457
if (interfaceMethod.HasInstantiation)

‎src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NativeLayoutVertexNode.cs‎

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1055,30 +1055,6 @@ public override Vertex WriteVertex(NodeFactory factory)
10551055

10561056
return SetSavedVertex(layoutInfo);
10571057
}
1058-
1059-
private static IEnumerable<MethodDesc> EnumVirtualSlotsDeclaredOnType(TypeDesc declType)
1060-
{
1061-
// VirtualMethodUse of Foo<SomeType>.Method will bring in VirtualMethodUse
1062-
// of Foo<__Canon>.Method. This in turn should bring in Foo<OtherType>.Method.
1063-
DefType defType = declType.GetClosestDefType();
1064-
1065-
Debug.Assert(!declType.IsInterface);
1066-
1067-
IEnumerable<MethodDesc> allSlots = defType.EnumAllVirtualSlots();
1068-
1069-
foreach (var method in allSlots)
1070-
{
1071-
// Generic virtual methods are tracked by an orthogonal mechanism.
1072-
if (method.HasInstantiation)
1073-
continue;
1074-
1075-
// Current type doesn't define this slot. Another VTableSlice will take care of this.
1076-
if (method.OwningType != defType)
1077-
continue;
1078-
1079-
yield return method;
1080-
}
1081-
}
10821058
}
10831059

10841060
public abstract class NativeLayoutGenericDictionarySlotNode : NativeLayoutVertexNode

‎src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeGVMEntriesNode.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ public InterfaceGVMEntryInfo(MethodDesc callingMethod, MethodDesc implementation
4646
public TypeGVMEntriesNode(TypeDesc associatedType)
4747
{
4848
Debug.Assert(associatedType.IsTypeDefinition);
49+
Debug.Assert(!associatedType.IsInterface);
4950
_associatedType = associatedType;
5051
}
5152

‎src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/VTableSliceNode.cs‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,7 @@ protected static MethodDesc[] ComputeSlots(TypeDesc type)
3333
bool isObjectType = type.IsObject;
3434
DefType defType = type.GetClosestDefType();
3535

36-
IEnumerable<MethodDesc> allSlots = type.IsInterface ?
37-
type.GetAllVirtualMethods() : defType.EnumAllVirtualSlots();
36+
IEnumerable<MethodDesc> allSlots = defType.EnumAllVirtualSlots();
3837

3938
foreach (var method in allSlots)
4039
{
@@ -224,8 +223,7 @@ public override IEnumerable<CombinedDependencyListEntry> GetConditionalStaticDep
224223
// of Foo<__Canon>.Method. This in turn should bring in Foo<OtherType>.Method.
225224
DefType defType = _type.GetClosestDefType();
226225

227-
IEnumerable<MethodDesc> allSlots = _type.IsInterface ?
228-
_type.GetAllVirtualMethods() : defType.EnumAllVirtualSlots();
226+
IEnumerable<MethodDesc> allSlots = defType.EnumAllVirtualSlots();
229227

230228
foreach (var method in allSlots)
231229
{

‎src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs‎

Lines changed: 27 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -494,40 +494,43 @@ Task<bool> ValidateTypeWorkerHelper(TypeDesc typeToCheckForSkipValidation)
494494
}
495495
}
496496

497-
foreach (var virtualMethod in type.EnumAllVirtualSlots())
497+
if (!type.IsInterface)
498498
{
499-
var implementationMethod = virtualMethodAlgorithm.FindVirtualFunctionTargetMethodOnObjectType(virtualMethod, type);
500-
501-
if (implementationMethod != null)
499+
foreach (var virtualMethod in type.EnumAllVirtualSlots())
502500
{
503-
// Validate that for every override involving generic methods that the generic method constraints are matching
504-
if (!CompareMethodConstraints(virtualMethod, implementationMethod))
505-
{
506-
AddTypeValidationError(type, $"Virtual method '{virtualMethod}' overridden by method '{implementationMethod}' which does not have matching generic constraints");
507-
return false;
508-
}
501+
var implementationMethod = virtualMethodAlgorithm.FindVirtualFunctionTargetMethodOnObjectType(virtualMethod, type);
509502

510-
// Validate that if the decl method for the virtual is not on the immediate base type, that the intermediate type did not establish a
511-
// covariant return type which requires the implementation method to specify a more specific base type
512-
if ((virtualMethod.OwningType != type.BaseType) && (virtualMethod.OwningType != type) && (baseTypeVirtualMethodAlgorithm != null))
503+
if (implementationMethod != null)
513504
{
514-
var implementationOnBaseType = baseTypeVirtualMethodAlgorithm.FindVirtualFunctionTargetMethodOnObjectType(virtualMethod, type.BaseType);
515-
if (!implementationMethod.Signature.ApplySubstitution(type.Instantiation).EquivalentWithCovariantReturnType(implementationOnBaseType.Signature.ApplySubstitution(type.Instantiation)))
505+
// Validate that for every override involving generic methods that the generic method constraints are matching
506+
if (!CompareMethodConstraints(virtualMethod, implementationMethod))
516507
{
517-
AddTypeValidationError(type, $"Virtual method '{virtualMethod}' overridden by method '{implementationMethod}' does not satisfy the covariant return type introduced with '{implementationOnBaseType}'");
508+
AddTypeValidationError(type, $"Virtual method '{virtualMethod}' overridden by method '{implementationMethod}' which does not have matching generic constraints");
518509
return false;
519510
}
511+
512+
// Validate that if the decl method for the virtual is not on the immediate base type, that the intermediate type did not establish a
513+
// covariant return type which requires the implementation method to specify a more specific base type
514+
if ((virtualMethod.OwningType != type.BaseType) && (virtualMethod.OwningType != type) && (baseTypeVirtualMethodAlgorithm != null))
515+
{
516+
var implementationOnBaseType = baseTypeVirtualMethodAlgorithm.FindVirtualFunctionTargetMethodOnObjectType(virtualMethod, type.BaseType);
517+
if (!implementationMethod.Signature.ApplySubstitution(type.Instantiation).EquivalentWithCovariantReturnType(implementationOnBaseType.Signature.ApplySubstitution(type.Instantiation)))
518+
{
519+
AddTypeValidationError(type, $"Virtual method '{virtualMethod}' overridden by method '{implementationMethod}' does not satisfy the covariant return type introduced with '{implementationOnBaseType}'");
520+
return false;
521+
}
522+
}
520523
}
521-
}
522524

523-
// Validate that all virtual static methods are actually implemented if the type is not abstract
524-
// Validate that all virtual instance methods are actually implemented if the type is not abstract
525-
if (!type.IsAbstract)
526-
{
527-
if (implementationMethod == null || implementationMethod.IsAbstract)
525+
// Validate that all virtual static methods are actually implemented if the type is not abstract
526+
// Validate that all virtual instance methods are actually implemented if the type is not abstract
527+
if (!type.IsAbstract)
528528
{
529-
AddTypeValidationError(type, $"Interface method '{virtualMethod}' does not have implementation");
530-
return false;
529+
if (implementationMethod == null || implementationMethod.IsAbstract)
530+
{
531+
AddTypeValidationError(type, $"Interface method '{virtualMethod}' does not have implementation");
532+
return false;
533+
}
531534
}
532535
}
533536
}

0 commit comments

Comments
 (0)