From 1a32f138001f12a4f952af14aca8478cc17a5698 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 23 Jun 2021 15:43:05 -0700 Subject: [PATCH] Fix issues noted through code review and testing - 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. --- .../Runtime/CompilerServices/CastHelpers.cs | 4 +- .../RuntimeHelpers.CoreCLR.cs | 3 +- src/coreclr/vm/amd64/asmconstants.h | 4 -- src/coreclr/vm/classcompat.cpp | 2 +- src/coreclr/vm/comcallablewrapper.cpp | 7 ++-- src/coreclr/vm/methodtable.cpp | 37 ++++++++++++++----- src/coreclr/vm/methodtable.h | 19 +++++++++- src/coreclr/vm/methodtable.inl | 6 +-- src/coreclr/vm/methodtablebuilder.cpp | 9 ++--- 9 files changed, 60 insertions(+), 31 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs index 00699faf27e9..cb8800994aa7 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs @@ -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: diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs index 6e3d8b57f504..5cd77f67d6f0 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs @@ -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 diff --git a/src/coreclr/vm/amd64/asmconstants.h b/src/coreclr/vm/amd64/asmconstants.h index 50a14d10c473..4cc6ab1de2ae 100644 --- a/src/coreclr/vm/amd64/asmconstants.h +++ b/src/coreclr/vm/amd64/asmconstants.h @@ -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); diff --git a/src/coreclr/vm/classcompat.cpp b/src/coreclr/vm/classcompat.cpp index be0ba15b2f53..ffd5b0fa983b 100644 --- a/src/coreclr/vm/classcompat.cpp +++ b/src/coreclr/vm/classcompat.cpp @@ -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, diff --git a/src/coreclr/vm/comcallablewrapper.cpp b/src/coreclr/vm/comcallablewrapper.cpp index 72355417b0c8..8b95dac8cdd7 100644 --- a/src/coreclr/vm/comcallablewrapper.cpp +++ b/src/coreclr/vm/comcallablewrapper.cpp @@ -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 diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index cbc7b0e24a95..e23b4be978a9 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -1544,7 +1544,7 @@ 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; } } @@ -1552,7 +1552,7 @@ BOOL MethodTable::CanCastToInterface(MethodTable *pTargetMT, TypeHandlePairList } //========================================================================================== -BOOL MethodTable::CanCastByVarianceToInterfaceOrDelegate(MethodTable *pTargetMT, TypeHandlePairList *pVisited) +BOOL MethodTable::CanCastByVarianceToInterfaceOrDelegate(MethodTable *pTargetMT, TypeHandlePairList *pVisited, MethodTable* pMTInterfaceMapOwner /*= NULL*/) { CONTRACTL { @@ -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)) { @@ -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 @@ -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) { @@ -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); } } } @@ -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)) @@ -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) { @@ -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 diff --git a/src/coreclr/vm/methodtable.h b/src/coreclr/vm/methodtable.h index 35ce8d90fe75..1231de3e4dfe 100644 --- a/src/coreclr/vm/methodtable.h +++ b/src/coreclr/vm/methodtable.h @@ -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() @@ -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 @@ -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; @@ -3695,6 +3709,7 @@ public : | enum_flag_ComObject | enum_flag_ICastable | enum_flag_IDynamicInterfaceCastable + | enum_flag_Category_ValueType }; // enum WFLAGS_HIGH_ENUM diff --git a/src/coreclr/vm/methodtable.inl b/src/coreclr/vm/methodtable.inl index c2dfad0a9f29..688fc2630bf2 100644 --- a/src/coreclr/vm/methodtable.inl +++ b/src/coreclr/vm/methodtable.inl @@ -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)) { @@ -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 @@ -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; } diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index bd32feb86db7..3ba6a2ea6aee 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -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, @@ -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, @@ -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()) @@ -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)