From f48053308eac2f7b4bcc2dc690be9f253d594734 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 24 Jun 2021 17:39:30 -0700 Subject: [PATCH 1/5] Change interface map layout in two interesting ways 1. For interface maps defined in System.Private.CoreLib, rely on the C# compiler to prevent any ambiguous duplicates, and to find the full interface expansion, instead of expanding it within the type loader See code marked with #SpecialCorelibInterfaceExpansionAlgorithm 2. For interface map expansion that follows the curiously recurring generic pattern, place the open instantiation of the type in the interface map instead of the the exact instantiation, and update all places in the runtime which consider the interface map to deal with that change (Mostly by being able to not resovle to an exact interface, but in some cases there is a helper which will do exactly that) --- src/coreclr/vm/class.cpp | 3 +- src/coreclr/vm/classcompat.cpp | 6 +- src/coreclr/vm/clsload.cpp | 7 +- src/coreclr/vm/clsload.hpp | 3 +- src/coreclr/vm/comcallablewrapper.cpp | 7 +- src/coreclr/vm/cominterfacemarshaler.cpp | 2 +- src/coreclr/vm/compile.cpp | 4 +- src/coreclr/vm/generics.cpp | 6 +- src/coreclr/vm/jitinterface.cpp | 2 +- src/coreclr/vm/marshalnative.cpp | 2 +- src/coreclr/vm/methodtable.cpp | 84 +++++++--- src/coreclr/vm/methodtable.h | 62 +++++++- src/coreclr/vm/methodtable.inl | 27 ++++ src/coreclr/vm/methodtablebuilder.cpp | 178 ++++++++++++++++------ src/coreclr/vm/methodtablebuilder.h | 9 +- src/coreclr/vm/runtimecallablewrapper.cpp | 6 +- src/coreclr/vm/runtimehandles.cpp | 2 +- src/coreclr/vm/siginfo.cpp | 40 +++-- src/coreclr/vm/siginfo.hpp | 3 +- src/coreclr/vm/typehandle.cpp | 2 +- src/coreclr/vm/typehandle.h | 10 ++ 21 files changed, 356 insertions(+), 109 deletions(-) diff --git a/src/coreclr/vm/class.cpp b/src/coreclr/vm/class.cpp index 6f2da9db3c29d3..61e8d1ef8c5eec 100644 --- a/src/coreclr/vm/class.cpp +++ b/src/coreclr/vm/class.cpp @@ -1902,7 +1902,8 @@ TypeHandle MethodTable::GetDefItfForComClassItf() InterfaceMapIterator it = IterateInterfaceMap(); if (it.Next()) { - return TypeHandle(it.GetInterface()); + // Can use GetInterfaceApprox, as there are no generic default interfaces + return TypeHandle(it.GetInterfaceApprox()); } else { diff --git a/src/coreclr/vm/classcompat.cpp b/src/coreclr/vm/classcompat.cpp index 1870e6f3dd0a2d..be0ba15b2f53db 100644 --- a/src/coreclr/vm/classcompat.cpp +++ b/src/coreclr/vm/classcompat.cpp @@ -1241,7 +1241,11 @@ VOID MethodTableBuilder::BuildInteropVTable_ExpandInterface(InterfaceInfo_t *pIn if (pNewInterface->GetNumInterfaces() != 0) { MethodTable::InterfaceMapIterator it = pNewInterface->IterateInterfaceMap(); while (it.Next()) { - BuildInteropVTable_ExpandInterface(pInterfaceMap, it.GetInterface(), + MethodTable *pItf = it.GetInterfaceApprox(); + if (pItf->HasInstantiation() || pItf->IsGenericTypeDefinition()) + continue; + + BuildInteropVTable_ExpandInterface(pInterfaceMap, pItf, pwInterfaceListSize, pdwMaxInterfaceMethods, FALSE); } } diff --git a/src/coreclr/vm/clsload.cpp b/src/coreclr/vm/clsload.cpp index 62eccc0995ffe7..77a1a7c215c8a7 100644 --- a/src/coreclr/vm/clsload.cpp +++ b/src/coreclr/vm/clsload.cpp @@ -2281,7 +2281,8 @@ TypeHandle ClassLoader::LoadTypeDefOrRefOrSpecThrowing(Module *pModule, LoadTypesFlag fLoadTypes/*=LoadTypes*/ , ClassLoadLevel level /* = CLASS_LOADED */, BOOL dropGenericArgumentLevel /* = FALSE */, - const Substitution *pSubst) + const Substitution *pSubst, + MethodTable *pMTInterfaceMapOwner) { CONTRACT(TypeHandle) { @@ -2315,7 +2316,7 @@ TypeHandle ClassLoader::LoadTypeDefOrRefOrSpecThrowing(Module *pModule, } SigPointer sigptr(pSig, cSig); TypeHandle typeHnd = sigptr.GetTypeHandleThrowing(pModule, pTypeContext, fLoadTypes, - level, dropGenericArgumentLevel, pSubst); + level, dropGenericArgumentLevel, pSubst, (const ZapSig::Context *)0, pMTInterfaceMapOwner); #ifndef DACCESS_COMPILE if ((fNotFoundAction == ThrowIfNotFound) && typeHnd.IsNull()) pModule->GetAssembly()->ThrowTypeLoadException(pInternalImport, typeDefOrRefOrSpec, @@ -5294,7 +5295,7 @@ BOOL ClassLoader::CanAccessFamily( while (it.Next()) { // We only loosely check if they are of the same generic type - if (it.GetInterface()->HasSameTypeDefAs(pTargetClass)) + if (it.HasSameTypeDefAs(pTargetClass)) return TRUE; } } diff --git a/src/coreclr/vm/clsload.hpp b/src/coreclr/vm/clsload.hpp index b947d4a65bfe35..195fb4f62cdcb4 100644 --- a/src/coreclr/vm/clsload.hpp +++ b/src/coreclr/vm/clsload.hpp @@ -680,7 +680,8 @@ class ClassLoader LoadTypesFlag fLoadTypes = LoadTypes, ClassLoadLevel level = CLASS_LOADED, BOOL dropGenericArgumentLevel = FALSE, - const Substitution *pSubst = NULL /* substitution to apply if the token is a type spec with generic variables */ ); + const Substitution *pSubst = NULL /* substitution to apply if the token is a type spec with generic variables */, + MethodTable *pMTInterfaceMapOwner = NULL); // Load constructed types by providing their constituents static TypeHandle LoadPointerOrByrefTypeThrowing(CorElementType typ, diff --git a/src/coreclr/vm/comcallablewrapper.cpp b/src/coreclr/vm/comcallablewrapper.cpp index dfee278b891537..72355417b0c807 100644 --- a/src/coreclr/vm/comcallablewrapper.cpp +++ b/src/coreclr/vm/comcallablewrapper.cpp @@ -2520,12 +2520,13 @@ static IUnknown * GetComIPFromCCW_HandleExtendsCOMObject( MethodTable::InterfaceMapIterator intIt = pMT->IterateInterfaceMapFrom(intfIndex); // If the number of slots is 0, then no need to proceed - if (intIt.GetInterface()->GetNumVirtuals() != 0) + if (intIt.GetInterfaceApprox()->GetNumVirtuals() != 0) { MethodDesc *pClsMD = NULL; + _ASSERTE(!intIt.GetInterfaceApprox()->HasInstantiation()); // Find the implementation for the first slot of the interface - DispatchSlot impl(pMT->FindDispatchSlot(intIt.GetInterface()->GetTypeID(), 0, FALSE /* throwOnConflict */)); + DispatchSlot impl(pMT->FindDispatchSlot(intIt.GetInterfaceApprox()->GetTypeID(), 0, FALSE /* throwOnConflict */)); CONSISTENCY_CHECK(!impl.IsNull()); // Get the MethodDesc for this slot in the class @@ -3991,7 +3992,7 @@ ComCallWrapperTemplate::CCWInterfaceMapIterator::CCWInterfaceMapIterator(TypeHan MethodTable::InterfaceMapIterator it = pMT->IterateInterfaceMap(); while (it.Next()) { - MethodTable *pItfMT = it.GetInterface(); + MethodTable *pItfMT = it.GetInterface(pMT); AppendInterface(pItfMT); } diff --git a/src/coreclr/vm/cominterfacemarshaler.cpp b/src/coreclr/vm/cominterfacemarshaler.cpp index 13b29066d8c13b..1bee6fa1f18530 100644 --- a/src/coreclr/vm/cominterfacemarshaler.cpp +++ b/src/coreclr/vm/cominterfacemarshaler.cpp @@ -425,7 +425,7 @@ VOID COMInterfaceMarshaler::EnsureCOMInterfacesSupported(OBJECTREF oref, MethodT while (it.Next()) { - MethodTable *pItfMT = it.GetInterface(); + MethodTable *pItfMT = it.GetInterfaceApprox(); if (!pItfMT) COMPlusThrow(kInvalidCastException, IDS_EE_CANNOT_COERCE_COMOBJECT); diff --git a/src/coreclr/vm/compile.cpp b/src/coreclr/vm/compile.cpp index 486844c7f79a48..b36840a3f43637 100644 --- a/src/coreclr/vm/compile.cpp +++ b/src/coreclr/vm/compile.cpp @@ -5235,7 +5235,7 @@ void CEEPreloader::ExpandTypeDependencies(TypeHandle th) MethodTable::InterfaceMapIterator intIterator = pMT->IterateInterfaceMap(); while (intIterator.Next()) { - TriageTypeForZap(intIterator.GetInterface(), TRUE); + TriageTypeForZap(intIterator.GetInterfaceApprox(), TRUE); } // Make sure approx types for all fields are saved @@ -5423,7 +5423,7 @@ void CEEPreloader::TriageTypeFromSoftBoundModule(TypeHandle th, Module * pSoftBo MethodTable::InterfaceMapIterator intIterator = pMT->IterateInterfaceMap(); while (intIterator.Next()) { - TriageTypeFromSoftBoundModule(intIterator.GetInterface(), pSoftBoundModule); + TriageTypeFromSoftBoundModule(intIterator.GetInterfaceApprox(), pSoftBoundModule); } // It does not seem worth it to reject the remaining items diff --git a/src/coreclr/vm/generics.cpp b/src/coreclr/vm/generics.cpp index b616b84099c30a..35f5ebd66d7344 100644 --- a/src/coreclr/vm/generics.cpp +++ b/src/coreclr/vm/generics.cpp @@ -705,7 +705,11 @@ BOOL RecursionGraph::CheckForIllegalRecursion() MethodTable::InterfaceMapIterator it = pMT->IterateInterfaceMap(); while (it.Next()) { - AddDependency(it.GetInterface()); + MethodTable *pItfApprox = it.GetInterfaceApprox(); + if (!pItfApprox->IsTypicalTypeDefinition()) + { + AddDependency(pItfApprox); + } } // Check all owned nodes for expanding cycles. The edges recorded above must all diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 8483c1fb1dc4fa..e2dd3b838c60c4 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -8973,7 +8973,7 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info) int canonicallyMatchingInterfacesFound = 0; while (it.Next()) { - if (it.GetInterface()->GetCanonicalMethodTable() == pOwnerMT) + if (it.GetInterface(pObjMT)->GetCanonicalMethodTable() == pOwnerMT) { canonicallyMatchingInterfacesFound++; if (canonicallyMatchingInterfacesFound > 1) diff --git a/src/coreclr/vm/marshalnative.cpp b/src/coreclr/vm/marshalnative.cpp index b28f34aa266002..59106d70f1fc93 100644 --- a/src/coreclr/vm/marshalnative.cpp +++ b/src/coreclr/vm/marshalnative.cpp @@ -1064,7 +1064,7 @@ FCIMPL2(Object*, MarshalNative::InternalCreateWrapperOfType, Object* objUNSAFE, MethodTable::InterfaceMapIterator it = pNewWrapMT->IterateInterfaceMap(); while (it.Next()) { - MethodTable *pItfMT = it.GetInterface(); + MethodTable *pItfMT = it.GetInterfaceApprox(); // ComImport interfaces cannot be generic if (pItfMT->IsComImport()) { if (!Object::SupportsInterface(gc.obj, pItfMT)) diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index bb44e829905079..cbc7b0e24a953b 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()->CanCastByVarianceToInterfaceOrDelegate(pTargetMT, pVisited)) + if (it.GetInterface(this)->CanCastByVarianceToInterfaceOrDelegate(pTargetMT, pVisited)) return TRUE; } } @@ -1998,7 +1998,7 @@ MethodTable::Debug_DumpInterfaceMap( InterfaceMapIterator it(this); while (it.Next()) { - MethodTable *pInterfaceMT = it.GetInterface(); + MethodTable *pInterfaceMT = it.GetInterfaceApprox(); //LF_ALWAYS allowed here because this is controlled by special env var code:EEConfig::ShouldDumpOnClassLoad LOG((LF_ALWAYS, LL_ALWAYS, @@ -4337,7 +4337,7 @@ BOOL MethodTable::ComputeNeedsRestoreWorker(DataImage *image, TypeHandleList *pV InterfaceMapIterator it = IterateInterfaceMap(); while (it.Next()) { - if (!image->CanPrerestoreEagerBindToMethodTable(it.GetInterface(), pVisited)) + if (!image->CanPrerestoreEagerBindToMethodTable(it.GetInterface(this), pVisited)) { UPDATE_RESTORE_REASON(InterfaceIsGeneric); return TRUE; @@ -5389,7 +5389,7 @@ void MethodTable::DoFullyLoad(Generics::RecursionGraph * const pVisited, const MethodTable::InterfaceMapIterator it = IterateInterfaceMap(); while (it.Next()) { - it.GetInterface()->DoFullyLoad(&locals.newVisited, level, pPending, &locals.fBailed, pInstContext); + it.GetInterfaceApprox()->DoFullyLoad(&locals.newVisited, level, pPending, &locals.fBailed, pInstContext); if (fNeedAccessChecks) { @@ -5399,7 +5399,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(), IDS_CLASSLOAD_INTERFACE_NO_ACCESS); + DoAccessibilityCheck(this, it.GetInterface(this, CLASS_LOAD_APPROXPARENTS), IDS_CLASSLOAD_INTERFACE_NO_ACCESS); } } } @@ -6225,8 +6225,7 @@ MethodTable::FindEncodedMapDispatchEntry( DispatchMapEntry * pCurEntry = it.Entry(); if (pCurEntry->GetSlotNumber() == slotNumber) { - MethodTable * pCurEntryType = LookupDispatchMapType(pCurEntry->GetTypeID()); - if (pCurEntryType == dispatchTokenType) + if (DispatchMapTypeMatchesMethodTable(pCurEntry->GetTypeID(), dispatchTokenType)) { *pEntry = *pCurEntry; return TRUE; @@ -6751,8 +6750,6 @@ BOOL MethodTable::FindDefaultInterfaceImplementation( } CONTRACT_END; #ifdef FEATURE_DEFAULT_INTERFACES - InterfaceMapIterator it = this->IterateInterfaceMap(); - CQuickArray candidates; unsigned candidatesCount = 0; @@ -6795,7 +6792,8 @@ BOOL MethodTable::FindDefaultInterfaceImplementation( MethodTable::InterfaceMapIterator it = pMT->IterateInterfaceMapFrom(dwParentInterfaces); while (!it.Finished()) { - MethodTable *pCurMT = it.GetInterface(); + 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. + MethodDesc *pCurMD = NULL; if (TryGetCandidateImplementation(pCurMT, pInterfaceMD, pInterfaceMT, allowVariance, &pCurMD)) { @@ -7033,7 +7031,7 @@ BOOL MethodTable::ImplementsInterfaceWithSameSlotsAsParent(MethodTable *pItfMT, for (; it.IsValid(); it.Next()) { DispatchMapEntry *pCurEntry = it.Entry(); - if (LookupDispatchMapType(pCurEntry->GetTypeID()) == pItfMT) + if (DispatchMapTypeMatchesMethodTable(pCurEntry->GetTypeID(), pItfMT)) { // this class and its parents up to pParentMT must have no mappings for the interface return FALSE; @@ -7083,7 +7081,7 @@ BOOL MethodTable::HasSameInterfaceImplementationAsParent(MethodTable *pItfMT, Me for (; it.IsValid(); it.Next()) { DispatchMapEntry *pCurEntry = it.Entry(); - if (LookupDispatchMapType(pCurEntry->GetTypeID()) == pItfMT) + if (DispatchMapTypeMatchesMethodTable(pCurEntry->GetTypeID(), pItfMT)) { UINT32 ifaceSlot = pCurEntry->GetSlotNumber(); if (!bitMask.TestBit(ifaceSlot)) @@ -7115,6 +7113,7 @@ BOOL MethodTable::HasSameInterfaceImplementationAsParent(MethodTable *pItfMT, Me #endif // !DACCESS_COMPILE //========================================================================================== +#ifndef DACCESS_COMPILE MethodTable * MethodTable::LookupDispatchMapType(DispatchMapTypeID typeID) { CONTRACTL { @@ -7125,7 +7124,21 @@ MethodTable * MethodTable::LookupDispatchMapType(DispatchMapTypeID typeID) _ASSERTE(!typeID.IsThisClass()); InterfaceMapIterator intIt = IterateInterfaceMapFrom(typeID.GetInterfaceNum()); - return intIt.GetInterface(); + return intIt.GetInterface(this); +} +#endif // DACCESS_COMPILE + +//========================================================================================== +bool MethodTable::DispatchMapTypeMatchesMethodTable(DispatchMapTypeID typeID, MethodTable* pMT) +{ + CONTRACTL { + WRAPPER(THROWS); + GC_TRIGGERS; + } CONTRACTL_END; + + _ASSERTE(!typeID.IsThisClass()); + InterfaceMapIterator intIt = IterateInterfaceMapFrom(typeID.GetInterfaceNum()); + return intIt.CurrentInterfaceMatches(this, pMT); } //========================================================================================== @@ -8311,7 +8324,7 @@ MethodTable::MethodDataInterfaceImpl::PopulateNextLevel() MethodTable::InterfaceMapIterator it = m_pImpl->GetImplMethodTable()->IterateInterfaceMap(); while (it.Next()) { - if (pDeclMT == it.GetInterface()) + if (it.CurrentInterfaceMatches(m_pImpl->GetImplMethodTable(), pDeclMT)) { // We found the interface INDEBUG(dbg_fInterfaceFound = TRUE); DispatchMapTypeID declTypeID = DispatchMapTypeID::InterfaceClassID(it.GetIndex()); @@ -9217,7 +9230,7 @@ MethodTable::ResolveVirtualStaticMethod(MethodTable* pInterfaceType, MethodDesc* InterfaceMapIterator it = IterateInterfaceMap(); while (it.Next()) { - if (pInterfaceTypeCanonical == it.GetInterface()) + if (it.CurrentInterfaceMatches(this, pInterfaceTypeCanonical)) { canonicalEquivalentFound = true; break; @@ -9243,13 +9256,13 @@ MethodTable::ResolveVirtualStaticMethod(MethodTable* pInterfaceType, MethodDesc* MethodTable::InterfaceMapIterator it = IterateInterfaceMap(); while (it.Next()) { - if (it.GetInterface() == pInterfaceType) + if (it.CurrentInterfaceMatches(this, pInterfaceType)) { // This is the variant interface check logic, skip the continue; } - if (!it.GetInterface()->HasSameTypeDefAs(pInterfaceType)) + if (!it.HasSameTypeDefAs(pInterfaceType)) { // Variance matches require a typedef match // Equivalence isn't sufficient, and is uninteresting as equivalent interfaces cannot have static virtuals. @@ -9258,22 +9271,24 @@ MethodTable::ResolveVirtualStaticMethod(MethodTable* pInterfaceType, MethodDesc* BOOL equivalentOrVariantCompatible; + MethodTable *pItfInMap = it.GetInterface(this, CLASS_LOAD_APPROXPARENTS); + if (allowVariantMatches) { - equivalentOrVariantCompatible = it.GetInterface()->CanCastTo(pInterfaceType, NULL); + equivalentOrVariantCompatible = pItfInMap->CanCastTo(pInterfaceType, NULL); } else { // When performing override checking to ensure that a concrete type is valid, require the implementation // actually implement the exact or equivalent interface. - equivalentOrVariantCompatible = it.GetInterface()->IsEquivalentTo(pInterfaceType); + equivalentOrVariantCompatible = pItfInMap->IsEquivalentTo(pInterfaceType); } if (equivalentOrVariantCompatible) { // Variant or equivalent matching interface found // Attempt to resolve on variance matched interface - pMD = pMT->TryResolveVirtualStaticMethodOnThisType(it.GetInterface(), pInterfaceMD, verifyImplemented); + pMD = pMT->TryResolveVirtualStaticMethodOnThisType(pItfInMap, pInterfaceMD, verifyImplemented); if (pMD != nullptr) { return pMD; @@ -9448,9 +9463,10 @@ MethodTable::VerifyThatAllVirtualStaticMethodsAreImplemented() InterfaceMapIterator it = IterateInterfaceMap(); while (it.Next()) { - MethodTable *pInterfaceMT = it.GetInterface(); + MethodTable *pInterfaceMT = it.GetInterfaceApprox(); if (pInterfaceMT->HasVirtualStaticMethods()) { + pInterfaceMT = it.GetInterface(this, CLASS_LOAD_EXACTPARENTS); for (MethodIterator it(pInterfaceMT); it.IsValid(); it.Next()) { MethodDesc *pMD = it.GetMethodDesc(); @@ -9527,7 +9543,7 @@ MethodTable::TryResolveConstraintMethodApprox( DWORD cPotentialMatchingInterfaces = 0; while (it.Next()) { - TypeHandle thPotentialInterfaceType(it.GetInterface()); + TypeHandle thPotentialInterfaceType(it.GetInterface(pCanonMT)); if (thPotentialInterfaceType.AsMethodTable()->GetCanonicalMethodTable() == thInterfaceType.AsMethodTable()->GetCanonicalMethodTable()) { @@ -9778,6 +9794,30 @@ EEClassNativeLayoutInfo const* MethodTable::EnsureNativeLayoutInfoInitialized() #endif } +#ifndef DACCESS_COMPILE +PTR_MethodTable MethodTable::InterfaceMapIterator::GetInterface(MethodTable* pMTOwner, ClassLoadLevel loadLevel /*= CLASS_LOADED*/) +{ + CONTRACT(PTR_MethodTable) + { + GC_TRIGGERS; + THROWS; + PRECONDITION(m_i != (DWORD) -1 && m_i < m_count); + POSTCONDITION(CheckPointer(RETVAL)); + } + CONTRACT_END; + + MethodTable *pResult = m_pMap->GetMethodTable(); + if (pResult->IsGenericTypeDefinition()) + { + TypeHandle ownerAsInst(pMTOwner); + Instantiation inst(&ownerAsInst, 1); + pResult = ClassLoader::LoadGenericInstantiationThrowing(pResult->GetModule(), pResult->GetCl(), inst).AsMethodTable(); + SetInterface(pResult); + } + RETURN (m_pMap->GetMethodTable()); +} +#endif // DACCESS_COMPILE + #ifdef FEATURE_READYTORUN_COMPILER static BOOL ComputeIsLayoutFixedInCurrentVersionBubble(MethodTable * pMT) diff --git a/src/coreclr/vm/methodtable.h b/src/coreclr/vm/methodtable.h index 3d0ee06b37f34b..35ce8d90fe756c 100644 --- a/src/coreclr/vm/methodtable.h +++ b/src/coreclr/vm/methodtable.h @@ -2171,8 +2171,16 @@ class MethodTable return (m_i == m_count); } - // Get the interface at the current position - inline PTR_MethodTable GetInterface() +#ifndef DACCESS_COMPILE + // Get the interface at the current position. This GetInterfaceMethod + // will ensure that the exact correct instantiation of the interface + // is found, even if the MethodTable in the interface map is the generic + // approximation + PTR_MethodTable GetInterface(MethodTable* pMTOwner, ClassLoadLevel loadLevel = CLASS_LOADED); +#endif + + // Get the interface at the current position, with whatever its normal load level is + inline PTR_MethodTable GetInterfaceApprox() { CONTRACT(PTR_MethodTable) { @@ -2187,6 +2195,53 @@ class MethodTable RETURN (m_pMap->GetMethodTable()); } + inline bool CurrentInterfaceMatches(MethodTable* pMTOwner, MethodTable* pMT) + { + CONTRACT(bool) + { + GC_NOTRIGGER; + NOTHROW; + SUPPORTS_DAC; + PRECONDITION(m_i != (DWORD) -1 && m_i < m_count); + } + CONTRACT_END; + + bool exactMatch = m_pMap->GetMethodTable() == pMT; + if (!exactMatch) + { + if (m_pMap->GetMethodTable()->HasSameTypeDefAs(pMT) && + pMT->HasInstantiation() && + m_pMap->GetMethodTable()->IsGenericTypeDefinition() && + pMT->GetInstantiation().ContainsAllOneType(pMTOwner)) + { + exactMatch = true; +#ifndef DACCESS_COMPILE + // We match exactly, and have an actual pMT loaded. Insert + // the searched for interface if it is fully loaded, so that + // future checks are more efficient + if (pMT->IsFullyLoaded()) + SetInterface(pMT); +#endif + } + } + + RETURN (exactMatch); + } + + inline bool HasSameTypeDefAs(MethodTable* pMT) + { + CONTRACT(bool) + { + GC_NOTRIGGER; + NOTHROW; + SUPPORTS_DAC; + PRECONDITION(m_i != (DWORD) -1 && m_i < m_count); + } + CONTRACT_END; + + RETURN (m_pMap->GetMethodTable()->HasSameTypeDefAs(pMT)); + } + #ifndef DACCESS_COMPILE void SetInterface(MethodTable *pMT) { @@ -2390,7 +2445,10 @@ class MethodTable UINT32 GetTypeID(); + // Will return either the dispatch map type. May trigger type loader in order to get + // exact result. MethodTable *LookupDispatchMapType(DispatchMapTypeID typeID); + bool DispatchMapTypeMatchesMethodTable(DispatchMapTypeID typeID, MethodTable* pMT); MethodDesc *GetIntroducingMethodDesc(DWORD slotNumber); diff --git a/src/coreclr/vm/methodtable.inl b/src/coreclr/vm/methodtable.inl index 88ebe105bda5cf..0f0efdc2d017f9 100644 --- a/src/coreclr/vm/methodtable.inl +++ b/src/coreclr/vm/methodtable.inl @@ -1570,6 +1570,33 @@ 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)) + { + numInterfaces = GetNumInterfaces(); + pInfo = GetInterfaceMap(); + + do + { + if (pInfo->GetMethodTable()->HasSameTypeDefAs(pInterface) && pInfo->GetMethodTable()->IsGenericTypeDefinition()) + { + // 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 + // of -1 to indicate this. We cannot take for granted that every instance of this + // COM object has this interface so FindInterface on these interfaces is made to fail. + // + // However, we are only considering the statically available slots here + // (m_wNumInterface doesn't contain the dynamic slots), so we can safely + // ignore this detail. + pInfo->SetMethodTable(pInterface); + return TRUE; + } + pInfo++; + } + while (--numInterfaces); + } + return FALSE; } diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 9ccb3d78f86ecf..bd32feb86db72e 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -412,15 +412,22 @@ MethodTableBuilder::ExpandApproxInterface( bmtInterface->dwInterfaceMapSize++; - // 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, - // not the stack. - InterfaceDeclarationScope declaredItfScope(declScope.fIsInterfaceDeclaredOnParent, false); - ExpandApproxDeclaredInterfaces( - bmtInterface, - bmtTypeHandle(pNewItfType), - declaredItfScope - COMMA_INDEBUG(dbg_pClassMT)); + // Checking for further expanded interfaces isn't necessary for the system module, as we can rely on the C# compiler + // 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()) + { + // 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, + // not the stack. + InterfaceDeclarationScope declaredItfScope(declScope.fIsInterfaceDeclaredOnParent, false); + ExpandApproxDeclaredInterfaces( + bmtInterface, + bmtTypeHandle(pNewItfType), + declaredItfScope + COMMA_INDEBUG(dbg_pClassMT)); + } } // MethodTableBuilder::ExpandApproxInterface //******************************************************************************* @@ -9022,6 +9029,18 @@ void MethodTableBuilder::CopyExactParentSlots(MethodTable *pMT, MethodTable *pAp } } // MethodTableBuilder::CopyExactParentSlots +bool InstantiationIsAllTypeVariables(const Instantiation &inst) +{ + for (auto i = inst.GetNumArgs(); i > 0;) + { + TypeHandle th = inst[--i]; + if (!th.IsGenericVariable()) + return false; + } + + return true; +} + //******************************************************************************* /* static */ void @@ -9038,7 +9057,7 @@ MethodTableBuilder::LoadExactInterfaceMap(MethodTable *pMT) MethodTable::InterfaceMapIterator it = pMT->IterateInterfaceMap(); while (it.Next()) { - if (it.GetInterface()->HasInstantiation()) + if (it.GetInterfaceApprox()->HasInstantiation()) { hasInstantiatedInterfaces = TRUE; break; @@ -9080,41 +9099,91 @@ MethodTableBuilder::LoadExactInterfaceMap(MethodTable *pMT) // (e) If duplicates found then use the slow metadata-based technique code:#LoadExactInterfaceMap_Algorithm2 DWORD nInterfacesCount = pMT->GetNumInterfaces(); 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. + DWORD nAssigned = 0; - BOOL duplicates = false; - if (pParentMT != NULL) + do { - MethodTable::InterfaceMapIterator parentIt = pParentMT->IterateInterfaceMap(); - while (parentIt.Next()) + nAssigned = 0; + retry = false; + duplicates = false; + if (pParentMT != NULL) { - duplicates |= InsertMethodTable(parentIt.GetInterface(), pExactMTs, nInterfacesCount, &nAssigned); + MethodTable::InterfaceMapIterator parentIt = pParentMT->IterateInterfaceMap(); + while (parentIt.Next()) + { + duplicates |= InsertMethodTable(parentIt.GetInterface(pParentMT, CLASS_LOAD_EXACTPARENTS), pExactMTs, nInterfacesCount, &nAssigned); + } } - } - InterfaceImplEnum ie(pMT->GetModule(), pMT->GetCl(), NULL); - while ((hr = ie.Next()) == S_OK) - { - MethodTable *pNewIntfMT = ClassLoader::LoadTypeDefOrRefOrSpecThrowing(pMT->GetModule(), - ie.CurrentToken(), - &typeContext, - ClassLoader::ThrowIfNotFound, - ClassLoader::FailIfUninstDefOrRef, - ClassLoader::LoadTypes, - CLASS_LOAD_EXACTPARENTS, - TRUE).GetMethodTable(); - - duplicates |= InsertMethodTable(pNewIntfMT, pExactMTs, nInterfacesCount, &nAssigned); - MethodTable::InterfaceMapIterator intIt = pNewIntfMT->IterateInterfaceMap(); - while (intIt.Next()) + InterfaceImplEnum ie(pMT->GetModule(), pMT->GetCl(), NULL); + while ((hr = ie.Next()) == S_OK) { - duplicates |= InsertMethodTable(intIt.GetInterface(), pExactMTs, nInterfacesCount, &nAssigned); + // TODO This feels like code that will need intesreting special casing + MethodTable *pNewIntfMT = ClassLoader::LoadTypeDefOrRefOrSpecThrowing(pMT->GetModule(), + ie.CurrentToken(), + &typeContext, + ClassLoader::ThrowIfNotFound, + ClassLoader::FailIfUninstDefOrRef, + ClassLoader::LoadTypes, + CLASS_LOAD_EXACTPARENTS, + TRUE, + (const Substitution*)0, + retryWithExactInterfaces ? NULL : pMT).GetMethodTable(); + + bool uninstGenericCase = pNewIntfMT->IsGenericTypeDefinition(); + + 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()) + { + MethodTable::InterfaceMapIterator intIt = pNewIntfMT->IterateInterfaceMap(); + while (intIt.Next()) + { + MethodTable *pItfPossiblyApprox = intIt.GetInterfaceApprox(); + if (uninstGenericCase && pItfPossiblyApprox->HasInstantiation() && pItfPossiblyApprox->ContainsGenericVariables()) + { + // We allow a limited set of interface generic shapes with type variables. In particular, we require the instantiations to be exactly type variables + // the current type + + if (InstantiationIsAllTypeVariables(pItfPossiblyApprox->GetInstantiation())) + { + pItfPossiblyApprox = ClassLoader::LoadTypeDefThrowing(pItfPossiblyApprox->GetModule(), pItfPossiblyApprox->GetCl(), ClassLoader::ThrowIfNotFound, ClassLoader::PermitUninstDefOrRef, 0, CLASS_LOAD_EXACTPARENTS).AsMethodTable(); + } + else + { + retry = true; + break; + } + } + duplicates |= InsertMethodTable(intIt.GetInterface(pNewIntfMT, CLASS_LOAD_EXACTPARENTS), pExactMTs, nInterfacesCount, &nAssigned); + } + } + + if (retry) + break; } - } + + if (retry) + { + retryWithExactInterfaces = true; + } + } while (retry); + if (FAILED(hr)) { pMT->GetAssembly()->ThrowTypeLoadException(pMT->GetMDImport(), pMT->GetCl(), IDS_CLASSLOAD_BADFORMAT); } #ifdef _DEBUG - duplicates |= CLRConfig::GetConfigValue(CLRConfig::INTERNAL_AlwaysUseMetadataInterfaceMapLayout); + if (!pMT->GetModule()->IsSystem()) + { + + duplicates |= CLRConfig::GetConfigValue(CLRConfig::INTERNAL_AlwaysUseMetadataInterfaceMapLayout); + } //#InjectInterfaceDuplicates_LoadExactInterfaceMap // If we are injecting duplicates also for non-generic interfaces in check builds, we have to use @@ -9122,6 +9191,10 @@ MethodTableBuilder::LoadExactInterfaceMap(MethodTable *pMT) // Has to be in sync with code:#InjectInterfaceDuplicates_Main. duplicates |= pMT->Debug_HasInjectedInterfaceDuplicates(); #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()); + CONSISTENCY_CHECK(duplicates || (nAssigned == pMT->GetNumInterfaces())); if (duplicates) { @@ -9181,7 +9254,8 @@ MethodTableBuilder::LoadExactInterfaceMap(MethodTable *pMT) pParentMT, pParentSubstForTypeLoad, pParentSubstForComparing, - pStackingAllocator); + pStackingAllocator, + retryWithExactInterfaces ? NULL : pMT); } #ifdef _DEBUG //#ExactInterfaceMap_SupersetOfParent @@ -9204,7 +9278,7 @@ MethodTableBuilder::LoadExactInterfaceMap(MethodTable *pMT) } else { // It is not canonical instantiation, we can compare MethodTables - _ASSERTE(parentInterfacesIterator.GetInterface() == bmtExactInterface.pExactMTs[nInterfaceIndex]); + _ASSERTE(parentInterfacesIterator.GetInterfaceApprox() == bmtExactInterface.pExactMTs[nInterfaceIndex]); } nInterfaceIndex++; } @@ -9237,7 +9311,8 @@ MethodTableBuilder::LoadExactInterfaceMap(MethodTable *pMT) pMT->GetCl(), NULL, NULL, - pStackingAllocator + pStackingAllocator, + retryWithExactInterfaces ? NULL : pMT COMMA_INDEBUG(pMT)); CONSISTENCY_CHECK(bmtExactInterface.nAssigned == pMT->GetNumInterfaces()); @@ -9405,9 +9480,8 @@ MethodTableBuilder::LoadExactInterfaceMap(MethodTable *pMT) while (thisIt.Next()) { #ifdef _DEBUG - MethodTable*pOldMT = thisIt.GetInterface(); MethodTable *pNewMT = pExactMTs[i]; - CONSISTENCY_CHECK(pOldMT->HasSameTypeDefAs(pNewMT)); + CONSISTENCY_CHECK(thisIt.HasSameTypeDefAs(pNewMT)); #endif // _DEBUG thisIt.SetInterface(pExactMTs[i]); i++; @@ -9422,7 +9496,8 @@ MethodTableBuilder::ExpandExactInheritedInterfaces( MethodTable * pMT, const Substitution * pSubstForTypeLoad, Substitution * pSubstForComparing, - StackingAllocator * pStackingAllocator) + StackingAllocator * pStackingAllocator, + MethodTable * pMTInterfaceMapOwner) { STANDARD_VM_CONTRACT; @@ -9449,7 +9524,8 @@ MethodTableBuilder::ExpandExactInheritedInterfaces( pParentMT, pParentSubstForTypeLoad, pParentSubstForComparing, - pStackingAllocator); + pStackingAllocator, + pMTInterfaceMapOwner); } ExpandExactDeclaredInterfaces( bmtInfo, @@ -9457,7 +9533,8 @@ MethodTableBuilder::ExpandExactInheritedInterfaces( pMT->GetCl(), pSubstForTypeLoad, pSubstForComparing, - pStackingAllocator + pStackingAllocator, + pMTInterfaceMapOwner COMMA_INDEBUG(pMT)); // Restore type's subsitution chain for comparing interfaces @@ -9473,7 +9550,8 @@ MethodTableBuilder::ExpandExactDeclaredInterfaces( mdToken typeDef, const Substitution * pSubstForTypeLoad, Substitution * pSubstForComparing, - StackingAllocator * pStackingAllocator + StackingAllocator * pStackingAllocator, + MethodTable * pMTInterfaceMapOwner COMMA_INDEBUG(MethodTable * dbg_pClassMT)) { STANDARD_VM_CONTRACT; @@ -9491,7 +9569,8 @@ MethodTableBuilder::ExpandExactDeclaredInterfaces( ClassLoader::LoadTypes, CLASS_LOAD_EXACTPARENTS, TRUE, - pSubstForTypeLoad).GetMethodTable(); + pSubstForTypeLoad, + pMTInterfaceMapOwner).GetMethodTable(); Substitution ifaceSubstForTypeLoad(ie.CurrentToken(), pModule, pSubstForTypeLoad); Substitution ifaceSubstForComparing(ie.CurrentToken(), pModule, pSubstForComparing); @@ -9500,7 +9579,8 @@ MethodTableBuilder::ExpandExactDeclaredInterfaces( pInterface, &ifaceSubstForTypeLoad, &ifaceSubstForComparing, - pStackingAllocator + pStackingAllocator, + pMTInterfaceMapOwner COMMA_INDEBUG(dbg_pClassMT)); } if (FAILED(hr)) @@ -9516,7 +9596,8 @@ MethodTableBuilder::ExpandExactInterface( MethodTable * pIntf, const Substitution * pSubstForTypeLoad_OnStack, // Allocated on stack! const Substitution * pSubstForComparing_OnStack, // Allocated on stack! - StackingAllocator * pStackingAllocator + StackingAllocator * pStackingAllocator, + MethodTable * pMTInterfaceMapOwner COMMA_INDEBUG(MethodTable * dbg_pClassMT)) { STANDARD_VM_CONTRACT; @@ -9563,7 +9644,8 @@ MethodTableBuilder::ExpandExactInterface( pIntf->GetCl(), pSubstForTypeLoad, &bmtInfo->pInterfaceSubstitution[n], - pStackingAllocator + pStackingAllocator, + pMTInterfaceMapOwner COMMA_INDEBUG(dbg_pClassMT)); } // MethodTableBuilder::ExpandExactInterface @@ -10716,7 +10798,7 @@ MethodTableBuilder::SetupMethodTable2( MethodTable::InterfaceMapIterator intIt = pMT->IterateInterfaceMap(); while (intIt.Next()) { - MethodTable* pIntfMT = intIt.GetInterface(); + MethodTable* pIntfMT = intIt.GetInterface(pMT, pMT->GetLoadLevel()); if (pIntfMT->GetNumVirtuals() != 0) { BOOL hasComImportMethod = FALSE; diff --git a/src/coreclr/vm/methodtablebuilder.h b/src/coreclr/vm/methodtablebuilder.h index 63cc9c856c1ace..e3b861293a32a2 100644 --- a/src/coreclr/vm/methodtablebuilder.h +++ b/src/coreclr/vm/methodtablebuilder.h @@ -2454,7 +2454,8 @@ class MethodTableBuilder MethodTable * pIntf, const Substitution * pSubstForTypeLoad_OnStack, // Allocated on stack! const Substitution * pSubstForComparing_OnStack, // Allocated on stack! - StackingAllocator * pStackingAllocator + StackingAllocator * pStackingAllocator, + MethodTable * pMTInterfaceMapOwner COMMA_INDEBUG(MethodTable * dbg_pClassMT)); public: @@ -2465,7 +2466,8 @@ class MethodTableBuilder mdToken typeDef, const Substitution * pSubstForTypeLoad, Substitution * pSubstForComparing, - StackingAllocator * pStackingAllocator + StackingAllocator * pStackingAllocator, + MethodTable * pMTInterfaceMapOwner COMMA_INDEBUG(MethodTable * dbg_pClassMT)); static void @@ -2474,7 +2476,8 @@ class MethodTableBuilder MethodTable * pParentMT, const Substitution * pSubstForTypeLoad, Substitution * pSubstForComparing, - StackingAllocator * pStackingAllocator); + StackingAllocator * pStackingAllocator, + MethodTable * pMTInterfaceMapOwner); public: // -------------------------------------------------------------------------------------------- diff --git a/src/coreclr/vm/runtimecallablewrapper.cpp b/src/coreclr/vm/runtimecallablewrapper.cpp index 17f58b0f80b660..5f961840743b92 100644 --- a/src/coreclr/vm/runtimecallablewrapper.cpp +++ b/src/coreclr/vm/runtimecallablewrapper.cpp @@ -2548,7 +2548,11 @@ BOOL ComObject::SupportsInterface(OBJECTREF oref, MethodTable* pIntfTable) MethodTable::InterfaceMapIterator it = pIntfTable->IterateInterfaceMap(); while (it.Next()) { - bSupportsItf = Object::SupportsInterface(oref, it.GetInterface()); + MethodTable *pItf = it.GetInterfaceApprox(); + if (pItf->HasInstantiation() || pItf->IsGenericTypeDefinition()) + continue; + + bSupportsItf = Object::SupportsInterface(oref, pItf); if (!bSupportsItf) break; } diff --git a/src/coreclr/vm/runtimehandles.cpp b/src/coreclr/vm/runtimehandles.cpp index 736f36758eed37..62d27295861b32 100644 --- a/src/coreclr/vm/runtimehandles.cpp +++ b/src/coreclr/vm/runtimehandles.cpp @@ -906,7 +906,7 @@ FCIMPL1(PtrArray*, RuntimeTypeHandle::GetInterfaces, ReflectClassBaseObject *pTy MethodTable::InterfaceMapIterator it = typeHandle.GetMethodTable()->IterateInterfaceMap(); while (it.Next()) { - OBJECTREF refInterface = it.GetInterface()->GetManagedClassObject(); + OBJECTREF refInterface = it.GetInterface(typeHandle.GetMethodTable())->GetManagedClassObject(); refRetVal->SetAt(i, refInterface); _ASSERTE(refRetVal->GetAt(i) != NULL); i++; diff --git a/src/coreclr/vm/siginfo.cpp b/src/coreclr/vm/siginfo.cpp index ff958846b8b084..956f80cc18e0a2 100644 --- a/src/coreclr/vm/siginfo.cpp +++ b/src/coreclr/vm/siginfo.cpp @@ -989,7 +989,8 @@ TypeHandle SigPointer::GetTypeHandleThrowing( BOOL dropGenericArgumentLevel/*=FALSE*/, const Substitution * pSubst/*=NULL*/, // ZapSigContext is only set when decoding zapsigs - const ZapSig::Context * pZapSigContext) const + const ZapSig::Context * pZapSigContext, + MethodTable *pMTInterfaceMapOwner) const { CONTRACT(TypeHandle) { @@ -1414,20 +1415,29 @@ TypeHandle SigPointer::GetTypeHandleThrowing( break; } - // Group together the current signature type context and substitution chain, which - // we may later use to instantiate constraints of type arguments that turn out to be - // typespecs, i.e. generic types. - InstantiationContext instContext(pTypeContext, pSubst); - - // Now make the instantiated type - // The class loader will check the arity - // When we know it was correctly computed at NGen time, we ask the class loader to skip that check. - thRet = (ClassLoader::LoadGenericInstantiationThrowing(pGenericTypeModule, - tkGenericType, - Instantiation(thisinst, ntypars), - fLoadTypes, level, - &instContext, - pZapSigContext && pZapSigContext->externalTokens == ZapSig::NormalTokens)); + Instantiation genericLoadInst(thisinst, ntypars); + + if (pMTInterfaceMapOwner != NULL && genericLoadInst.ContainsAllOneType(pMTInterfaceMapOwner)) + { + thRet = ClassLoader::LoadTypeDefThrowing(pGenericTypeModule, tkGenericType, ClassLoader::ThrowIfNotFound, ClassLoader::PermitUninstDefOrRef, 0, level); + } + else + { + // Group together the current signature type context and substitution chain, which + // we may later use to instantiate constraints of type arguments that turn out to be + // typespecs, i.e. generic types. + InstantiationContext instContext(pTypeContext, pSubst); + + // Now make the instantiated type + // The class loader will check the arity + // When we know it was correctly computed at NGen time, we ask the class loader to skip that check. + thRet = (ClassLoader::LoadGenericInstantiationThrowing(pGenericTypeModule, + tkGenericType, + genericLoadInst, + fLoadTypes, level, + &instContext, + pZapSigContext && pZapSigContext->externalTokens == ZapSig::NormalTokens)); + } break; } diff --git a/src/coreclr/vm/siginfo.hpp b/src/coreclr/vm/siginfo.hpp index 86809e4d27f243..7489cd349f0562 100644 --- a/src/coreclr/vm/siginfo.hpp +++ b/src/coreclr/vm/siginfo.hpp @@ -253,7 +253,8 @@ class SigPointer : public SigParser ClassLoadLevel level = CLASS_LOADED, BOOL dropGenericArgumentLevel = FALSE, const Substitution *pSubst = NULL, - const ZapSig::Context *pZapSigContext = NULL) const; + const ZapSig::Context *pZapSigContext = NULL, + MethodTable *pMTInterfaceMapOwner = NULL) const; public: //------------------------------------------------------------------------ diff --git a/src/coreclr/vm/typehandle.cpp b/src/coreclr/vm/typehandle.cpp index b198c87467c928..81550fd3d3b004 100644 --- a/src/coreclr/vm/typehandle.cpp +++ b/src/coreclr/vm/typehandle.cpp @@ -740,7 +740,7 @@ TypeHandle TypeHandle::MergeClassWithInterface(TypeHandle tClass, TypeHandle tIn MethodTable::InterfaceMapIterator intIt = pMTInterface->IterateInterfaceMap(); while (intIt.Next()) { - MethodTable *pMT = intIt.GetInterface(); + MethodTable *pMT = intIt.GetInterface(pMTInterface); if (pMTClass->ImplementsEquivalentInterface(pMT)) { // Found a common interface. If there are multiple common interfaces, then diff --git a/src/coreclr/vm/typehandle.h b/src/coreclr/vm/typehandle.h index e623d4a8cec844..339d5f397b172e 100644 --- a/src/coreclr/vm/typehandle.h +++ b/src/coreclr/vm/typehandle.h @@ -756,6 +756,16 @@ class Instantiation return m_pArgs; } + bool ContainsAllOneType(TypeHandle th) + { + for (auto i = GetNumArgs(); i > 0;) + { + if ((*this)[--i] != th) + return false; + } + return true; + } + private: // Note that for DAC builds, m_pArgs may be host allocated buffer, not a copy of an object marshalled by DAC. FixupPointer * m_pArgs; From 0f0e2493cfebd45e81b4e8fe1f90edfd931dadc3 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 22 Jun 2021 18:39:24 -0700 Subject: [PATCH 2/5] Adjust casting for new NonTrivial cases --- .../src/System/Runtime/CompilerServices/CastHelpers.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 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 cb8800994aa787..00699faf27e9d6 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) +// 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 { goto slowPath; } - obj = null; +// obj = null; } done: From a615e9aa023372b1602ed0c592b4532da7b0e086 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 22 Jun 2021 18:39:30 -0700 Subject: [PATCH 3/5] Fix build issues --- src/coreclr/vm/jitinterface.cpp | 5 +++-- src/coreclr/vm/methodtable.inl | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index e2dd3b838c60c4..a1e4d93d881de4 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -14301,9 +14301,10 @@ BOOL LoadDynamicInfoEntry(Module *currentModule, MethodTable::InterfaceMapIterator it = thImpl.GetMethodTable()->IterateInterfaceMap(); while (it.Next()) { - if (pInterfaceTypeCanonical == it.GetInterface()->GetCanonicalMethodTable()) + MethodTable *pItfInMap = it.GetInterface(thImpl.GetMethodTable()); + if (pInterfaceTypeCanonical == pItfInMap->GetCanonicalMethodTable()) { - pDeclMethod = MethodDesc::FindOrCreateAssociatedMethodDesc(pDeclMethod, it.GetInterface(), FALSE, pDeclMethod->GetMethodInstantiation(), FALSE, TRUE); + pDeclMethod = MethodDesc::FindOrCreateAssociatedMethodDesc(pDeclMethod, pItfInMap, FALSE, pDeclMethod->GetMethodInstantiation(), FALSE, TRUE); break; } } diff --git a/src/coreclr/vm/methodtable.inl b/src/coreclr/vm/methodtable.inl index 0f0efdc2d017f9..c2dfad0a9f29a1 100644 --- a/src/coreclr/vm/methodtable.inl +++ b/src/coreclr/vm/methodtable.inl @@ -1589,7 +1589,9 @@ FORCEINLINE BOOL MethodTable::ImplementsInterfaceInline(MethodTable *pInterface) // However, we are only considering the statically available slots here // (m_wNumInterface doesn't contain the dynamic slots), so we can safely // ignore this detail. +#ifndef DACCESS_COMPILE pInfo->SetMethodTable(pInterface); +#endif return TRUE; } pInfo++; From 1a32f138001f12a4f952af14aca8478cc17a5698 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 23 Jun 2021 15:43:05 -0700 Subject: [PATCH 4/5] 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 00699faf27e9d6..cb8800994aa787 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 6e3d8b57f504d3..5cd77f67d6f061 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 50a14d10c47302..4cc6ab1de2aeb8 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 be0ba15b2f53db..ffd5b0fa983bba 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 72355417b0c807..8b95dac8cdd773 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 cbc7b0e24a953b..e23b4be978a960 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 35ce8d90fe756c..1231de3e4dfe6c 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 c2dfad0a9f29a1..688fc2630bf214 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 bd32feb86db72e..3ba6a2ea6aeea0 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) From 33774b165c5fdbc9c7d02d5f0c263807a6531ecd Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Fri, 25 Jun 2021 15:15:04 -0700 Subject: [PATCH 5/5] Code review comments --- src/coreclr/vm/methodtablebuilder.cpp | 7 +++---- src/coreclr/vm/siginfo.cpp | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 3ba6a2ea6aeea0..0d372a4659b6c6 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() && IsValueClass())) + 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, @@ -9146,9 +9146,8 @@ MethodTableBuilder::LoadExactInterfaceMap(MethodTable *pMT) MethodTable *pItfPossiblyApprox = intIt.GetInterfaceApprox(); if (uninstGenericCase && pItfPossiblyApprox->HasInstantiation() && pItfPossiblyApprox->ContainsGenericVariables()) { - // We allow a limited set of interface generic shapes with type variables. In particular, we require the instantiations to be exactly type variables - // the current type - + // We allow a limited set of interface generic shapes with type variables. In particular, we require the + // instantiations to be exactly simple type variables if (InstantiationIsAllTypeVariables(pItfPossiblyApprox->GetInstantiation())) { pItfPossiblyApprox = ClassLoader::LoadTypeDefThrowing(pItfPossiblyApprox->GetModule(), pItfPossiblyApprox->GetCl(), ClassLoader::ThrowIfNotFound, ClassLoader::PermitUninstDefOrRef, 0, CLASS_LOAD_EXACTPARENTS).AsMethodTable(); diff --git a/src/coreclr/vm/siginfo.cpp b/src/coreclr/vm/siginfo.cpp index 956f80cc18e0a2..2d0889c53240c0 100644 --- a/src/coreclr/vm/siginfo.cpp +++ b/src/coreclr/vm/siginfo.cpp @@ -990,7 +990,7 @@ TypeHandle SigPointer::GetTypeHandleThrowing( const Substitution * pSubst/*=NULL*/, // ZapSigContext is only set when decoding zapsigs const ZapSig::Context * pZapSigContext, - MethodTable *pMTInterfaceMapOwner) const + MethodTable * pMTInterfaceMapOwner) const { CONTRACT(TypeHandle) {