Skip to content

Commit

Permalink
Open types can exist as entries in interface map (#55372)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
davidwrighton committed Jul 13, 2021
1 parent f531684 commit 002370b
Show file tree
Hide file tree
Showing 6 changed files with 48 additions and 7 deletions.
5 changes: 3 additions & 2 deletions src/coreclr/vm/methodtable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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() &&
Expand All @@ -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;
}
Expand Down Expand Up @@ -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++)
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/vm/methodtable.h
Original file line number Diff line number Diff line change
Expand Up @@ -2245,7 +2245,8 @@ class MethodTable
{
if (pCurrentMethodTable->HasSameTypeDefAs(pMT) &&
pMT->HasInstantiation() &&
pCurrentMethodTable->IsSpecialMarkerTypeForGenericCasting() &&
pCurrentMethodTable->IsSpecialMarkerTypeForGenericCasting() &&
!pMTOwner->ContainsGenericVariables() &&
pMT->GetInstantiation().ContainsAllOneType(pMTOwner))
{
exactMatch = true;
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/methodtable.inl
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
11 changes: 8 additions & 3 deletions src/coreclr/vm/methodtablebuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
namespace CuriouslyRecurringPatternThroughInterface
{
interface IGeneric<T_IGeneric>
{
}
interface ICuriouslyRecurring<T_ICuriouslyRecurring> : IGeneric<CuriouslyRecurringThroughInterface<T_ICuriouslyRecurring>>
{
}
class CuriouslyRecurringThroughInterface<T_CuriouslyRecurringThroughInterface> : ICuriouslyRecurring<T_CuriouslyRecurringThroughInterface>
{
}

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<int>);
return 100;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<OutputType>Exe</OutputType>
<CLRTestKind>BuildAndRun</CLRTestKind>
<CLRTestPriority>1</CLRTestPriority>
</PropertyGroup>
<ItemGroup>
<Compile Include="CuriouslyRecurringThroughInterface.cs" />
</ItemGroup>
</Project>

0 comments on commit 002370b

Please sign in to comment.