Skip to content

Commit

Permalink
Fix issues noted through code review and testing
Browse files Browse the repository at this point in the history 
- Re-enable fast path for NonTrivialCasting for interface misses
- Adjust SetInterface usage to only occur for scenarios where the found type if fully loaded
- Only use #SpecialCorelibInterfaceExpansionAlgorithm on ValueTypes, turns out derived types have all the fun problems
- Adjust casting logic to work with the approx interface and owner pair. This avoid the need to load types in CanCastTo, which is important as CanCastTo is used heavily during type load in scenario where further type loading is prohibited
- Give checks for the special marker type a unique name instead of just re-using IsGenericTypeDefinition. Use IsSpecialMarkerTypeForGenericCasting instead.
  • Loading branch information
@davidwrighton
davidwrighton committed Jun 25, 2021
1 parent a615e9a commit 1a32f13
Show file tree
Hide file tree
Showing 9 changed files with 60 additions and 31 deletions.
4 changes: 2 additions & 2 deletions src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs
View file
Expand Up @@ -257,12 +257,12 @@ private static CastResult TryGet(nuint source, nuint target)
}

extra:
// if (mt->NonTrivialInterfaceCast) Remove the NonTrivialInterfaceCast check for now as the uninstantiated interface being used for the curiously recurring pattern scenario makes more cases NonTrivial
if (mt->NonTrivialInterfaceCast)
{
goto slowPath;
}

// obj = null;
obj = null;
}

done:
Expand Down
3 changes: 2 additions & 1 deletion ...eclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs
View file
Expand Up @@ -408,7 +408,8 @@ internal unsafe struct MethodTable
private const uint enum_flag_NonTrivialInterfaceCast = 0x00080000 // enum_flag_Category_Array
| 0x40000000 // enum_flag_ComObject
| 0x00400000 // enum_flag_ICastable;
| 0x00200000;// enum_flag_IDynamicInterfaceCastable;
| 0x00200000 // enum_flag_IDynamicInterfaceCastable;
| 0x00040000; // enum_flag_Category_ValueType

private const int DebugClassNamePtr = // adjust for debug_m_szClassName
#if DEBUG
Expand Down
4 changes: 0 additions & 4 deletions src/coreclr/vm/amd64/asmconstants.h
View file
Expand Up @@ -203,10 +203,6 @@ ASMCONSTANTS_C_ASSERT(METHODTABLE_EQUIVALENCE_FLAGS
#define METHODTABLE_EQUIVALENCE_FLAGS 0x0
#endif

#define METHODTABLE_NONTRIVIALINTERFACECAST_FLAGS (0x00080000 + 0x40000000 + 0x00400000 + 0x00200000)
ASMCONSTANTS_C_ASSERT(METHODTABLE_NONTRIVIALINTERFACECAST_FLAGS
== MethodTable::enum_flag_NonTrivialInterfaceCast);

#define MethodTable__enum_flag_ContainsPointers 0x01000000
ASMCONSTANTS_C_ASSERT(MethodTable__enum_flag_ContainsPointers
== MethodTable::enum_flag_ContainsPointers);
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/classcompat.cpp
View file
Expand Up @@ -1242,7 +1242,7 @@ VOID MethodTableBuilder::BuildInteropVTable_ExpandInterface(InterfaceInfo_t *pIn
MethodTable::InterfaceMapIterator it = pNewInterface->IterateInterfaceMap();
while (it.Next()) {
MethodTable *pItf = it.GetInterfaceApprox();
if (pItf->HasInstantiation() || pItf->IsGenericTypeDefinition())
if (pItf->HasInstantiation() || pItf->IsSpecialMarkerTypeForGenericCasting())
continue;

BuildInteropVTable_ExpandInterface(pInterfaceMap, pItf,
Expand Down
7 changes: 4 additions & 3 deletions src/coreclr/vm/comcallablewrapper.cpp
View file
Expand Up @@ -2520,13 +2520,14 @@ static IUnknown * GetComIPFromCCW_HandleExtendsCOMObject(
MethodTable::InterfaceMapIterator intIt = pMT->IterateInterfaceMapFrom(intfIndex);

// If the number of slots is 0, then no need to proceed
if (intIt.GetInterfaceApprox()->GetNumVirtuals() != 0)
MethodTable* pItf = intIt.GetInterfaceApprox();
if (pItf->GetNumVirtuals() != 0)
{
MethodDesc *pClsMD = NULL;
_ASSERTE(!intIt.GetInterfaceApprox()->HasInstantiation());
_ASSERTE(!pItf->HasInstantiation());

// Find the implementation for the first slot of the interface
DispatchSlot impl(pMT->FindDispatchSlot(intIt.GetInterfaceApprox()->GetTypeID(), 0, FALSE /* throwOnConflict */));
DispatchSlot impl(pMT->FindDispatchSlot(pItf->GetTypeID(), 0, FALSE /* throwOnConflict */));
CONSISTENCY_CHECK(!impl.IsNull());

// Get the MethodDesc for this slot in the class
Expand Down
37 changes: 27 additions & 10 deletions src/coreclr/vm/methodtable.cpp
View file
Expand Up @@ -1544,15 +1544,15 @@ BOOL MethodTable::CanCastToInterface(MethodTable *pTargetMT, TypeHandlePairList
InterfaceMapIterator it = IterateInterfaceMap();
while (it.Next())
{
if (it.GetInterface(this)->CanCastByVarianceToInterfaceOrDelegate(pTargetMT, pVisited))
if (it.GetInterfaceApprox()->CanCastByVarianceToInterfaceOrDelegate(pTargetMT, pVisited, this))
return TRUE;
}
}
return FALSE;
}

//==========================================================================================
BOOL MethodTable::CanCastByVarianceToInterfaceOrDelegate(MethodTable *pTargetMT, TypeHandlePairList *pVisited)
BOOL MethodTable::CanCastByVarianceToInterfaceOrDelegate(MethodTable *pTargetMT, TypeHandlePairList *pVisited, MethodTable* pMTInterfaceMapOwner /*= NULL*/)
{
CONTRACTL
{
Expand All @@ -1573,6 +1573,16 @@ BOOL MethodTable::CanCastByVarianceToInterfaceOrDelegate(MethodTable *pTargetMT,
return TRUE;
}

// Shortcut for generic approx type scenario
if (pMTInterfaceMapOwner != NULL &&
IsSpecialMarkerTypeForGenericCasting() &&
GetTypeDefRid() == pTargetMT->GetTypeDefRid() &&
GetModule() == pTargetMT->GetModule() &&
pTargetMT->GetInstantiation().ContainsAllOneType(pMTInterfaceMapOwner))
{
return TRUE;
}

if (GetTypeDefRid() != pTargetMT->GetTypeDefRid() || GetModule() != pTargetMT->GetModule() ||
TypeHandlePairList::Exists(pVisited, this, pTargetMT))
{
Expand All @@ -1591,6 +1601,11 @@ BOOL MethodTable::CanCastByVarianceToInterfaceOrDelegate(MethodTable *pTargetMT,
for (DWORD i = 0; i < inst.GetNumArgs(); i++)
{
TypeHandle thArg = inst[i];
if (IsSpecialMarkerTypeForGenericCasting() && pMTInterfaceMapOwner)
{
thArg = pMTInterfaceMapOwner;
}

TypeHandle thTargetArg = targetInst[i];

// If argument types are not equivalent, test them for compatibility
Expand Down Expand Up @@ -5389,7 +5404,8 @@ void MethodTable::DoFullyLoad(Generics::RecursionGraph * const pVisited, const
MethodTable::InterfaceMapIterator it = IterateInterfaceMap();
while (it.Next())
{
it.GetInterfaceApprox()->DoFullyLoad(&locals.newVisited, level, pPending, &locals.fBailed, pInstContext);
MethodTable* pItf = it.GetInterfaceApprox();
pItf->DoFullyLoad(&locals.newVisited, level, pPending, &locals.fBailed, pInstContext);

if (fNeedAccessChecks)
{
Expand All @@ -5399,7 +5415,7 @@ void MethodTable::DoFullyLoad(Generics::RecursionGraph * const pVisited, const
// A transparent type should not be allowed to implement a critical interface.
// However since this has never been enforced before we have many classes that
// violate this rule. Enforcing it now will be a breaking change.
DoAccessibilityCheck(this, it.GetInterface(this, CLASS_LOAD_APPROXPARENTS), IDS_CLASSLOAD_INTERFACE_NO_ACCESS);
DoAccessibilityCheck(this, pItf, IDS_CLASSLOAD_INTERFACE_NO_ACCESS);
}
}
}
Expand Down Expand Up @@ -6792,7 +6808,7 @@ BOOL MethodTable::FindDefaultInterfaceImplementation(
MethodTable::InterfaceMapIterator it = pMT->IterateInterfaceMapFrom(dwParentInterfaces);
while (!it.Finished())
{
MethodTable *pCurMT = it.GetInterface(pMT); // TODO! Consider adding a bit check, so that we only do the full resolution of the interface if the interface type has default method implementations on it.
MethodTable *pCurMT = it.GetInterface(pMT);

MethodDesc *pCurMD = NULL;
if (TryGetCandidateImplementation(pCurMT, pInterfaceMD, pInterfaceMT, allowVariance, &pCurMD))
Expand Down Expand Up @@ -9271,7 +9287,7 @@ MethodTable::ResolveVirtualStaticMethod(MethodTable* pInterfaceType, MethodDesc*

BOOL equivalentOrVariantCompatible;

MethodTable *pItfInMap = it.GetInterface(this, CLASS_LOAD_APPROXPARENTS);
MethodTable *pItfInMap = it.GetInterface(this, CLASS_LOAD_EXACTPARENTS);

if (allowVariantMatches)
{
Expand Down Expand Up @@ -9807,14 +9823,15 @@ PTR_MethodTable MethodTable::InterfaceMapIterator::GetInterface(MethodTable* pMT
CONTRACT_END;

MethodTable *pResult = m_pMap->GetMethodTable();
if (pResult->IsGenericTypeDefinition())
if (pResult->IsSpecialMarkerTypeForGenericCasting())
{
TypeHandle ownerAsInst(pMTOwner);
Instantiation inst(&ownerAsInst, 1);
pResult = ClassLoader::LoadGenericInstantiationThrowing(pResult->GetModule(), pResult->GetCl(), inst).AsMethodTable();
SetInterface(pResult);
pResult = ClassLoader::LoadGenericInstantiationThrowing(pResult->GetModule(), pResult->GetCl(), inst, ClassLoader::LoadTypes, loadLevel).AsMethodTable();
if (pResult->IsFullyLoaded())
SetInterface(pResult);
}
RETURN (m_pMap->GetMethodTable());
RETURN (pResult);
}
#endif // DACCESS_COMPILE

Expand Down
19 changes: 17 additions & 2 deletions src/coreclr/vm/methodtable.h
View file
Expand Up @@ -1240,6 +1240,20 @@ class MethodTable

BOOL ContainsGenericMethodVariables();

// When creating an interface map, under some circumstances the
// runtime will place the special marker type in the interface map instead
// of the fully loaded type. This is to reduce the amount of type loading
// performed at process startup.
//
// The current rule is that these interfaces can only appear
// on valuetypes that are not shared generic, and that the special
// marker type is the open generic type.
//
inline bool IsSpecialMarkerTypeForGenericCasting()
{
return IsGenericTypeDefinition();
}

static BOOL ComputeContainsGenericVariables(Instantiation inst);

inline void SetContainsGenericVariables()
Expand Down Expand Up @@ -1913,7 +1927,7 @@ class MethodTable
BOOL ArraySupportsBizarreInterface(MethodTable* pInterfaceMT, TypeHandlePairList* pVisited);
BOOL ArrayIsInstanceOf(MethodTable* pTargetMT, TypeHandlePairList* pVisited);

BOOL CanCastByVarianceToInterfaceOrDelegate(MethodTable* pTargetMT, TypeHandlePairList* pVisited);
BOOL CanCastByVarianceToInterfaceOrDelegate(MethodTable* pTargetMT, TypeHandlePairList* pVisited, MethodTable* pMTInterfaceMapOwner = NULL);

// The inline part of equivalence check.
#ifndef DACCESS_COMPILE
Expand Down Expand Up @@ -2211,7 +2225,7 @@ class MethodTable
{
if (m_pMap->GetMethodTable()->HasSameTypeDefAs(pMT) &&
pMT->HasInstantiation() &&
m_pMap->GetMethodTable()->IsGenericTypeDefinition() &&
m_pMap->GetMethodTable()->IsSpecialMarkerTypeForGenericCasting() &&
pMT->GetInstantiation().ContainsAllOneType(pMTOwner))
{
exactMatch = true;
Expand Down Expand Up @@ -3695,6 +3709,7 @@ public :
| enum_flag_ComObject
| enum_flag_ICastable
| enum_flag_IDynamicInterfaceCastable
| enum_flag_Category_ValueType

}; // enum WFLAGS_HIGH_ENUM

Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/vm/methodtable.inl
View file
Expand Up @@ -1570,7 +1570,6 @@ FORCEINLINE BOOL MethodTable::ImplementsInterfaceInline(MethodTable *pInterface)
}
while (--numInterfaces);

// TODO! Consider breaking out into seperate function!
// Second scan, looking for the curiously recurring generic scenario
if (pInterface->HasInstantiation() && pInterface->GetInstantiation().ContainsAllOneType(this))
{
Expand All @@ -1579,7 +1578,7 @@ FORCEINLINE BOOL MethodTable::ImplementsInterfaceInline(MethodTable *pInterface)

do
{
if (pInfo->GetMethodTable()->HasSameTypeDefAs(pInterface) && pInfo->GetMethodTable()->IsGenericTypeDefinition())
if (pInfo->GetMethodTable()->HasSameTypeDefAs(pInterface) && pInfo->GetMethodTable()->IsSpecialMarkerTypeForGenericCasting())
{
// Extensible RCW's need to be handled specially because they can have interfaces
// in their map that are added at runtime. These interfaces will have a start offset
Expand All @@ -1590,7 +1589,8 @@ FORCEINLINE BOOL MethodTable::ImplementsInterfaceInline(MethodTable *pInterface)
// (m_wNumInterface doesn't contain the dynamic slots), so we can safely
// ignore this detail.
#ifndef DACCESS_COMPILE
pInfo->SetMethodTable(pInterface);
if (pInterface->IsFullyLoaded())
pInfo->SetMethodTable(pInterface);
#endif
return TRUE;
}
Expand Down
9 changes: 4 additions & 5 deletions src/coreclr/vm/methodtablebuilder.cpp
View file
Expand Up @@ -416,7 +416,7 @@ MethodTableBuilder::ExpandApproxInterface(
// to have found all of the interfaces that the type implements, and to place them in the interface list itself. Also
// we can assume no ambiguous interfaces
// Code related to this is marked with #SpecialCorelibInterfaceExpansionAlgorithm
if (!GetModule()->IsSystem())
if (!(GetModule()->IsSystem() && IsValueClass()))
{
// Make sure to pass in the substitution from the new itf type created above as
// these methods assume that substitutions are allocated in the stacking heap,
Expand Down Expand Up @@ -9121,7 +9121,6 @@ MethodTableBuilder::LoadExactInterfaceMap(MethodTable *pMT)
InterfaceImplEnum ie(pMT->GetModule(), pMT->GetCl(), NULL);
while ((hr = ie.Next()) == S_OK)
{
// TODO This feels like code that will need intesreting special casing
MethodTable *pNewIntfMT = ClassLoader::LoadTypeDefOrRefOrSpecThrowing(pMT->GetModule(),
ie.CurrentToken(),
&typeContext,
Expand All @@ -9133,13 +9132,13 @@ MethodTableBuilder::LoadExactInterfaceMap(MethodTable *pMT)
(const Substitution*)0,
retryWithExactInterfaces ? NULL : pMT).GetMethodTable();

bool uninstGenericCase = pNewIntfMT->IsGenericTypeDefinition();
bool uninstGenericCase = pNewIntfMT->IsSpecialMarkerTypeForGenericCasting();

duplicates |= InsertMethodTable(pNewIntfMT, pExactMTs, nInterfacesCount, &nAssigned);

// We have a special algorithm for interface maps in CoreLib, which doesn't expand interfaces, and assumes no ambiguous
// duplicates. Code related to this is marked with #SpecialCorelibInterfaceExpansionAlgorithm
if (!pMT->GetModule()->IsSystem())
if (!(pMT->GetModule()->IsSystem() && pMT->IsValueType()))
{
MethodTable::InterfaceMapIterator intIt = pNewIntfMT->IterateInterfaceMap();
while (intIt.Next())
Expand Down Expand Up @@ -9193,7 +9192,7 @@ MethodTableBuilder::LoadExactInterfaceMap(MethodTable *pMT)
#endif
// We have a special algorithm for interface maps in CoreLib, which doesn't expand interfaces, and assumes no ambiguous
// duplicates. Code related to this is marked with #SpecialCorelibInterfaceExpansionAlgorithm
_ASSERTE(!duplicates || !pMT->GetModule()->IsSystem());
_ASSERTE(!duplicates || !(pMT->GetModule()->IsSystem() && pMT->IsValueType()));

CONSISTENCY_CHECK(duplicates || (nAssigned == pMT->GetNumInterfaces()));
if (duplicates)
Expand Down

0 comments on commit 1a32f13

Please sign in to comment.