From a32209edd1dfbe7b9e528b7eb02d19932ff3674e Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 8 Jul 2021 15:12:07 -0700 Subject: [PATCH 1/2] Open types can exist as entries in interface map - While invalid via the ECMA spec, the runtime currently represents a type explicitly instantiated over its own generic type parameters via the open type MethodTable - This is not strictly correct, as per the spec, these should be represented via an instantiated type, but changing that detail at this time is considered highly risky - This conflicts with the perf optimization around partialy interface loading which uses the open type of an interface to represent a type instantiated in the curiously recurring fashion. - The fix is to detect types instantiated over generic variables, and make them ineligible for the optimization, and to detect those cases where the optimization is ineligible, and revert back to the non-optimized behavior Fixes #55323 --- src/coreclr/vm/methodtable.cpp | 5 ++-- src/coreclr/vm/methodtable.h | 3 ++- src/coreclr/vm/methodtable.inl | 2 +- src/coreclr/vm/methodtablebuilder.cpp | 11 ++++++--- .../CuriouslyRecurringThroughInterface.cs | 23 +++++++++++++++++++ .../CuriouslyRecurringThroughInterface.csproj | 11 +++++++++ 6 files changed, 48 insertions(+), 7 deletions(-) create mode 100644 src/tests/Loader/classloader/generics/Instantiation/Positive/CuriouslyRecurringThroughInterface.cs create mode 100644 src/tests/Loader/classloader/generics/Instantiation/Positive/CuriouslyRecurringThroughInterface.csproj diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 834d3cfc8c40bb..2604b779f4d9cd 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -1577,6 +1577,7 @@ BOOL MethodTable::CanCastByVarianceToInterfaceOrDelegate(MethodTable *pTargetMT, // Shortcut for generic approx type scenario if (pMTInterfaceMapOwner != NULL && + !pMTInterfaceMapOwner->ContainsGenericVariables() && IsSpecialMarkerTypeForGenericCasting() && GetTypeDefRid() == pTargetMT->GetTypeDefRid() && GetModule() == pTargetMT->GetModule() && @@ -1603,7 +1604,7 @@ BOOL MethodTable::CanCastByVarianceToInterfaceOrDelegate(MethodTable *pTargetMT, for (DWORD i = 0; i < inst.GetNumArgs(); i++) { TypeHandle thArg = inst[i]; - if (IsSpecialMarkerTypeForGenericCasting() && pMTInterfaceMapOwner) + if (IsSpecialMarkerTypeForGenericCasting() && pMTInterfaceMapOwner && pMTInterfaceMapOwner->ContainsGenericVariables()) { thArg = pMTInterfaceMapOwner; } @@ -9820,7 +9821,7 @@ PTR_MethodTable MethodTable::InterfaceMapIterator::GetInterface(MethodTable* pMT CONTRACT_END; MethodTable *pResult = m_pMap->GetMethodTable(); - if (pResult->IsSpecialMarkerTypeForGenericCasting()) + if (pResult->IsSpecialMarkerTypeForGenericCasting() && pMTOwner->ContainsGenericVariables()) { TypeHandle ownerAsInst[MaxGenericParametersForSpecialMarkerType]; for (DWORD i = 0; i < MaxGenericParametersForSpecialMarkerType; i++) diff --git a/src/coreclr/vm/methodtable.h b/src/coreclr/vm/methodtable.h index 4063e50b7b61c7..df712a378d4ece 100644 --- a/src/coreclr/vm/methodtable.h +++ b/src/coreclr/vm/methodtable.h @@ -2245,7 +2245,8 @@ class MethodTable { if (pCurrentMethodTable->HasSameTypeDefAs(pMT) && pMT->HasInstantiation() && - pCurrentMethodTable->IsSpecialMarkerTypeForGenericCasting() && + pCurrentMethodTable->IsSpecialMarkerTypeForGenericCasting() && + !pMTOwner->ContainsGenericVariables() && pMT->GetInstantiation().ContainsAllOneType(pMTOwner)) { exactMatch = true; diff --git a/src/coreclr/vm/methodtable.inl b/src/coreclr/vm/methodtable.inl index a3adc702cbe3d6..b1af313a29556b 100644 --- a/src/coreclr/vm/methodtable.inl +++ b/src/coreclr/vm/methodtable.inl @@ -1571,7 +1571,7 @@ FORCEINLINE BOOL MethodTable::ImplementsInterfaceInline(MethodTable *pInterface) while (--numInterfaces); // Second scan, looking for the curiously recurring generic scenario - if (pInterface->HasInstantiation() && pInterface->GetInstantiation().ContainsAllOneType(this)) + if (pInterface->HasInstantiation() && !ContainsGenericVariables() && pInterface->GetInstantiation().ContainsAllOneType(this)) { numInterfaces = GetNumInterfaces(); pInfo = GetInterfaceMap(); diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 89926ffdaae020..154c642cf40d71 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -9101,8 +9101,13 @@ MethodTableBuilder::LoadExactInterfaceMap(MethodTable *pMT) MethodTable **pExactMTs = (MethodTable**) _alloca(sizeof(MethodTable *) * nInterfacesCount); BOOL duplicates; bool retry = false; - bool retryWithExactInterfaces = !pMT->IsValueType() || pMT->IsSharedByGenericInstantiations(); // Always use exact loading behavior with classes or shared generics, as they have to deal with inheritance, and the - // inexact matching logic for classes would be more complex to write. + + // Always use exact loading behavior with classes or shared generics, as they have to deal with inheritance, and the + // inexact matching logic for classes would be more complex to write. + // Also always use the exact loading behavior with any generic that contains generic variables, as the open type is used + // to represent a type instantiated over its own generic variables, and the special marker type is currently the open type + // and we make this case distinguishable by simply disallowing the optimization in those cases. + bool retryWithExactInterfaces = !pMT->IsValueType() || pMT->IsSharedByGenericInstantiations() || pMT->ContainsGenericVariables(); DWORD nAssigned = 0; do @@ -9132,7 +9137,7 @@ MethodTableBuilder::LoadExactInterfaceMap(MethodTable *pMT) (const Substitution*)0, retryWithExactInterfaces ? NULL : pMT).GetMethodTable(); - bool uninstGenericCase = pNewIntfMT->IsSpecialMarkerTypeForGenericCasting(); + bool uninstGenericCase = !retryWithExactInterfaces && pNewIntfMT->IsSpecialMarkerTypeForGenericCasting(); duplicates |= InsertMethodTable(pNewIntfMT, pExactMTs, nInterfacesCount, &nAssigned); diff --git a/src/tests/Loader/classloader/generics/Instantiation/Positive/CuriouslyRecurringThroughInterface.cs b/src/tests/Loader/classloader/generics/Instantiation/Positive/CuriouslyRecurringThroughInterface.cs new file mode 100644 index 00000000000000..e8a1139f53b911 --- /dev/null +++ b/src/tests/Loader/classloader/generics/Instantiation/Positive/CuriouslyRecurringThroughInterface.cs @@ -0,0 +1,23 @@ +namespace CuriouslyRecurringPatternThroughInterface +{ + interface IGeneric + { + } + interface ICuriouslyRecurring : IGeneric> + { + } + class CuriouslyRecurringThroughInterface : ICuriouslyRecurring + { + } + + class Program + { + static object _o; + static int Main(string[] args) + { + // Test that the a generic using a variant of the curiously recurring pattern involving an interface can be loaded. + _o = typeof(CuriouslyRecurringThroughInterface); + return 100; + } + } +} \ No newline at end of file diff --git a/src/tests/Loader/classloader/generics/Instantiation/Positive/CuriouslyRecurringThroughInterface.csproj b/src/tests/Loader/classloader/generics/Instantiation/Positive/CuriouslyRecurringThroughInterface.csproj new file mode 100644 index 00000000000000..b566f023697970 --- /dev/null +++ b/src/tests/Loader/classloader/generics/Instantiation/Positive/CuriouslyRecurringThroughInterface.csproj @@ -0,0 +1,11 @@ + + + true + Exe + BuildAndRun + 1 + + + + + \ No newline at end of file From de48a70d03ea3f82c442f397851729cd143bd840 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 13 Jul 2021 10:57:30 -0700 Subject: [PATCH 2/2] Get the polarity right on ContainsGenericVariables checks --- src/coreclr/vm/methodtable.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 2604b779f4d9cd..8bdc5f0c2a892b 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -1604,7 +1604,7 @@ BOOL MethodTable::CanCastByVarianceToInterfaceOrDelegate(MethodTable *pTargetMT, for (DWORD i = 0; i < inst.GetNumArgs(); i++) { TypeHandle thArg = inst[i]; - if (IsSpecialMarkerTypeForGenericCasting() && pMTInterfaceMapOwner && pMTInterfaceMapOwner->ContainsGenericVariables()) + if (IsSpecialMarkerTypeForGenericCasting() && pMTInterfaceMapOwner && !pMTInterfaceMapOwner->ContainsGenericVariables()) { thArg = pMTInterfaceMapOwner; } @@ -9821,7 +9821,7 @@ PTR_MethodTable MethodTable::InterfaceMapIterator::GetInterface(MethodTable* pMT CONTRACT_END; MethodTable *pResult = m_pMap->GetMethodTable(); - if (pResult->IsSpecialMarkerTypeForGenericCasting() && pMTOwner->ContainsGenericVariables()) + if (pResult->IsSpecialMarkerTypeForGenericCasting() && !pMTOwner->ContainsGenericVariables()) { TypeHandle ownerAsInst[MaxGenericParametersForSpecialMarkerType]; for (DWORD i = 0; i < MaxGenericParametersForSpecialMarkerType; i++)