From 1d77e2f8f3a816fa9b78e46e6a44e7074d6f84a9 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 6 Oct 2025 15:47:37 -0700 Subject: [PATCH 01/17] Reduce constraint loading by not forcing constraints to be loaded eagerly - Reduce the set of constraints that need to be loaded for Bounds and cast checking TODO: There is a path in InitTypeContext which I have #ifdef'd out as I don't understand the comment The accessibility checking needs full loading now, as we don't have a scheme to just look at all the typedefs in a signature, and instead need to do a load of the full type and work from there. This could be revisited. --- src/coreclr/vm/comdelegate.cpp | 8 +- src/coreclr/vm/genmeth.cpp | 12 +- src/coreclr/vm/methodtable.cpp | 6 +- src/coreclr/vm/runtimehandles.cpp | 2 +- src/coreclr/vm/typectxt.cpp | 9 +- src/coreclr/vm/typedesc.cpp | 212 ++++++++++++++++++++++++------ src/coreclr/vm/typedesc.h | 22 +++- src/coreclr/vm/typehandle.cpp | 6 +- 8 files changed, 206 insertions(+), 71 deletions(-) diff --git a/src/coreclr/vm/comdelegate.cpp b/src/coreclr/vm/comdelegate.cpp index 5d59f3298e9a18..669a97e10f8483 100644 --- a/src/coreclr/vm/comdelegate.cpp +++ b/src/coreclr/vm/comdelegate.cpp @@ -2403,16 +2403,16 @@ static bool IsLocationAssignable(TypeHandle fromHandle, TypeHandle toHandle, boo // We need to check whether constraints of fromHandle have been loaded, because the // CanCastTo operation might have made its decision without enumerating constraints // (e.g. when toHandle is System.Object). - if (!fromHandleVar->ConstraintsLoaded()) - fromHandleVar->LoadConstraints(CLASS_DEPENDENCIES_LOADED); + if (!fromHandleVar->ConstraintsLoaded(WhichConstraintsToLoad::TypeOrMethodVarsAndNonInterfacesOnly)) + fromHandleVar->LoadConstraints(CLASS_DEPENDENCIES_LOADED, WhichConstraintsToLoad::TypeOrMethodVarsAndNonInterfacesOnly); if (toHandle.IsGenericVariable()) { TypeVarTypeDesc *toHandleVar = toHandle.AsGenericVariable(); // Constraints of toHandleVar were not touched by CanCastTo. - if (!toHandleVar->ConstraintsLoaded()) - toHandleVar->LoadConstraints(CLASS_DEPENDENCIES_LOADED); + if (!toHandleVar->ConstraintsLoaded(WhichConstraintsToLoad::TypeOrMethodVarsAndNonInterfacesOnly)) + toHandleVar->LoadConstraints(CLASS_DEPENDENCIES_LOADED, WhichConstraintsToLoad::TypeOrMethodVarsAndNonInterfacesOnly); // Both handles are type variables. The following table lists all possible combinations. // diff --git a/src/coreclr/vm/genmeth.cpp b/src/coreclr/vm/genmeth.cpp index f1d9129f62e13a..749d7436cb0e7b 100644 --- a/src/coreclr/vm/genmeth.cpp +++ b/src/coreclr/vm/genmeth.cpp @@ -1536,7 +1536,7 @@ BOOL Bounded(TypeVarTypeDesc *tyvar, DWORD depth) { } DWORD numConstraints; - TypeHandle *constraints = tyvar->GetConstraints(&numConstraints, CLASS_DEPENDENCIES_LOADED); + TypeHandle *constraints = tyvar->GetConstraints(&numConstraints, CLASS_DEPENDENCIES_LOADED, WhichConstraintsToLoad::TypeOrMethodVarsOnly); for (unsigned i = 0; i < numConstraints; i++) { TypeHandle constraint = constraints[i]; @@ -1570,19 +1570,13 @@ void MethodDesc::LoadConstraintsForTypicalMethodDefinition(BOOL *pfHasCircularCl // Force a load of the constraints on the type parameters Instantiation classInst = GetClassInstantiation(); - for (DWORD i = 0; i < classInst.GetNumArgs(); i++) - { - TypeVarTypeDesc* tyvar = classInst[i].AsGenericVariable(); - _ASSERTE(tyvar != NULL); - tyvar->LoadConstraints(level); - } Instantiation methodInst = GetMethodInstantiation(); for (DWORD i = 0; i < methodInst.GetNumArgs(); i++) { TypeVarTypeDesc* tyvar = methodInst[i].AsGenericVariable(); _ASSERTE(tyvar != NULL); - tyvar->LoadConstraints(level); + tyvar->LoadConstraints(level, WhichConstraintsToLoad::All); // Use the All here, since DoAccessibilityCheckForConstraints needs it, athough really, we only need to walk the typedefs/refs in the constraints VOID DoAccessibilityCheckForConstraints(MethodTable *pAskingMT, TypeVarTypeDesc *pTyVar, UINT resIDWhy); DoAccessibilityCheckForConstraints(GetMethodTable(), tyvar, E_ACCESSDENIED); @@ -1662,8 +1656,6 @@ BOOL MethodDesc::SatisfiesMethodConstraints(TypeHandle thParent, BOOL fThrowIfNo _ASSERTE(tyvar != NULL); _ASSERTE(TypeFromToken(tyvar->GetTypeOrMethodDef()) == mdtMethodDef); - tyvar->LoadConstraints(); //TODO: is this necessary for anything but the typical method? - // Pass in the InstatiationContext so constraints can be correctly evaluated // if this is an instantiation where the type variable is in its open position if (!tyvar->SatisfiesConstraints(&typeContext,thArg, typicalInstMatchesMethodInst ? &instContext : NULL)) diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 56f704997fa332..e95805ea154dde 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -4214,7 +4214,7 @@ VOID DoAccessibilityCheckForConstraints(MethodTable *pAskingMT, TypeVarTypeDesc CONTRACTL_END; DWORD numConstraints; - TypeHandle *pthConstraints = pTyVar->GetCachedConstraints(&numConstraints); + TypeHandle *pthConstraints = pTyVar->GetCachedConstraints(&numConstraints, WhichConstraintsToLoad::All); for (DWORD cidx = 0; cidx < numConstraints; cidx++) { TypeHandle thConstraint = pthConstraints[cidx]; @@ -4582,12 +4582,14 @@ void MethodTable::DoFullyLoad(Generics::RecursionGraph * const pVisited, const BOOL Bounded(TypeVarTypeDesc *tyvar, DWORD depth); TypeVarTypeDesc *pTyVar = formalParams[i].AsGenericVariable(); - pTyVar->LoadConstraints(CLASS_DEPENDENCIES_LOADED); if (!Bounded(pTyVar, formalParams.GetNumArgs())) { COMPlusThrow(kTypeLoadException, VER_E_CIRCULAR_VAR_CONSTRAINTS); } + // For typical type definitions, we have to load the constraints here because they may not + // have been loaded yet. In principle we could change DoAccessibilityCheckForConstraints to walk the typedefs/refs in the constraint instead + pTyVar->LoadConstraints(CLASS_DEPENDENCIES_LOADED, WhichConstraintsToLoad::All); DoAccessibilityCheckForConstraints(this, pTyVar, E_ACCESSDENIED); } } diff --git a/src/coreclr/vm/runtimehandles.cpp b/src/coreclr/vm/runtimehandles.cpp index 9b2e05cb658a99..0219c1aed68770 100644 --- a/src/coreclr/vm/runtimehandles.cpp +++ b/src/coreclr/vm/runtimehandles.cpp @@ -612,7 +612,7 @@ extern "C" void QCALLTYPE RuntimeTypeHandle_GetConstraints(QCall::TypeHandle pTy TypeVarTypeDesc* pGenericVariable = typeHandle.AsGenericVariable(); DWORD dwCount; - constraints = pGenericVariable->GetConstraints(&dwCount); + constraints = pGenericVariable->GetConstraints(&dwCount, CLASS_LOADED, WhichConstraintsToLoad::All); GCX_COOP(); retTypeArray.Set(CopyRuntimeTypeHandles(constraints, dwCount, CLASS__TYPE)); diff --git a/src/coreclr/vm/typectxt.cpp b/src/coreclr/vm/typectxt.cpp index 1abaf9441bd674..fc04b676bfa3bc 100644 --- a/src/coreclr/vm/typectxt.cpp +++ b/src/coreclr/vm/typectxt.cpp @@ -95,12 +95,12 @@ TypeHandle GetDeclaringMethodTableFromTypeVarTypeDesc(TypeVarTypeDesc *pTypeVar, // This currently should only happen in cases where we've already loaded the constraints. // Currently, the only known case where use this code is reflection over methods exposed on a TypeVariable. - _ASSERTE(pTypeVar->ConstraintsLoaded()); + _ASSERTE(pTypeVar->ConstraintsLoaded(WhichConstraintsToLoad::All)); - if (pTypeVar->ConstraintsLoaded()) + if (pTypeVar->ConstraintsLoaded(WhichConstraintsToLoad::All)) { DWORD cConstraints; - TypeHandle *pTypeHandles = pTypeVar->GetCachedConstraints(&cConstraints); + TypeHandle *pTypeHandles = pTypeVar->GetConstraints(&cConstraints, CLASS_DEPENDENCIES_LOADED, WhichConstraintsToLoad::All); for (DWORD iConstraint = 0; iConstraint < cConstraints; iConstraint++) { if (pTypeHandles[iConstraint].IsGenericVariable()) @@ -146,7 +146,8 @@ void SigTypeContext::InitTypeContext(MethodDesc *md, TypeHandle declaringType, I // factor this with the work above if (declaringType.IsGenericVariable()) { - declaringType = GetDeclaringMethodTableFromTypeVarTypeDesc(declaringType.AsGenericVariable(), md); + _ASSERTE(FALSE); +// declaringType = GetDeclaringMethodTableFromTypeVarTypeDesc(declaringType.AsGenericVariable(), md); } if (declaringType.IsNull()) diff --git a/src/coreclr/vm/typedesc.cpp b/src/coreclr/vm/typedesc.cpp index e37d34355bdb16..540c10a87fbd85 100644 --- a/src/coreclr/vm/typedesc.cpp +++ b/src/coreclr/vm/typedesc.cpp @@ -343,7 +343,7 @@ BOOL TypeDesc::CanCastTo(TypeHandle toTypeHnd, TypeHandlePairList *pVisited) else { DWORD numConstraints; - TypeHandle* constraints = tyvar->GetConstraints(&numConstraints, CLASS_DEPENDENCIES_LOADED); + TypeHandle* constraints = tyvar->GetConstraints(&numConstraints, CLASS_DEPENDENCIES_LOADED, WhichConstraintsToLoad::All); if (constraints != NULL) { @@ -410,8 +410,8 @@ BOOL TypeDesc::CanCastParam(TypeHandle fromParam, TypeHandle toParam, TypeHandle { TypeVarTypeDesc* varFromParam = fromParam.AsGenericVariable(); - if (!varFromParam->ConstraintsLoaded()) - varFromParam->LoadConstraints(CLASS_DEPENDENCIES_LOADED); + if (!varFromParam->ConstraintsLoaded(WhichConstraintsToLoad::TypeOrMethodVarsAndNonInterfacesOnly)) + varFromParam->LoadConstraints(CLASS_DEPENDENCIES_LOADED, WhichConstraintsToLoad::TypeOrMethodVarsAndNonInterfacesOnly); if (!varFromParam->ConstrainedAsObjRef()) return FALSE; @@ -713,34 +713,39 @@ TypeHandle TypeVarTypeDesc::LoadOwnerType() return genericType; } -TypeHandle* TypeVarTypeDesc::GetCachedConstraints(DWORD *pNumConstraints) +TypeHandle* TypeVarTypeDesc::GetCachedConstraints(DWORD *pNumConstraints, WhichConstraintsToLoad which) { LIMITED_METHOD_CONTRACT; PRECONDITION(CheckPointer(pNumConstraints)); - PRECONDITION(m_numConstraints != (DWORD) -1); - *pNumConstraints = m_numConstraints; + DWORD numConstraints = m_numConstraintsWithFlags; + WhichConstraintsToLoad whichCurrent = (WhichConstraintsToLoad)(numConstraints >> WhichShift); + bool prevWhichInsufficient = whichCurrent > which; + _ASSERTE(!prevWhichInsufficient); + + *pNumConstraints = numConstraints & ~WhichMask; return m_constraints; } - - - -TypeHandle* TypeVarTypeDesc::GetConstraints(DWORD *pNumConstraints, ClassLoadLevel level /* = CLASS_LOADED */) +TypeHandle* TypeVarTypeDesc::GetConstraints(DWORD *pNumConstraints, ClassLoadLevel level, WhichConstraintsToLoad which) { WRAPPER_NO_CONTRACT; PRECONDITION(CheckPointer(pNumConstraints)); PRECONDITION(level == CLASS_DEPENDENCIES_LOADED || level == CLASS_LOADED); - if (m_numConstraints == (DWORD) -1) - LoadConstraints(level); + DWORD numConstraints = m_numConstraintsWithFlags; + WhichConstraintsToLoad whichCurrent = (WhichConstraintsToLoad)(numConstraints >> WhichShift); + bool prevWhichInsufficient = whichCurrent > which; - *pNumConstraints = m_numConstraints; + if (prevWhichInsufficient) + LoadConstraints(level, which); + + *pNumConstraints = m_numConstraintsWithFlags & ~WhichMask; return m_constraints; } -void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level /* = CLASS_LOADED */) +void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level, WhichConstraintsToLoad which) { CONTRACTL { @@ -755,12 +760,24 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level /* = CLASS_LOADED */) CONTRACTL_END; _ASSERTE(((INT_PTR)&m_constraints) % sizeof(m_constraints) == 0); - _ASSERTE(((INT_PTR)&m_numConstraints) % sizeof(m_numConstraints) == 0); + _ASSERTE(((INT_PTR)&m_numConstraintsWithFlags) % sizeof(m_numConstraintsWithFlags) == 0); + + DWORD numConstraints; - DWORD numConstraints = m_numConstraints; + // If we have already loaded the constraints, and the previously loaded constraints are sufficient for the current request, return. - if (numConstraints == (DWORD) -1) + do { + numConstraints = m_numConstraintsWithFlags; + DWORD initialNumConstraints = numConstraints; + WhichConstraintsToLoad whichCurrent = (WhichConstraintsToLoad)(numConstraints >> WhichShift); + bool prevWhichInsufficient = whichCurrent > which; + + if (!prevWhichInsufficient) + { + break; + } + IMDInternalImport* pInternalImport = GetModule()->GetMDImport(); HENUMInternalHolder hEnum(pInternalImport); @@ -787,7 +804,7 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level /* = CLASS_LOADED */) bool foundResult = false; if (!GetModule()->m_pTypeGenericInfoMap->HasConstraints(defToken, &foundResult) && foundResult) { - m_numConstraints = 0; + m_numConstraintsWithFlags = 0; return; } @@ -801,11 +818,24 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level /* = CLASS_LOADED */) numConstraints = pInternalImport->EnumGetCount(&hEnum); if (numConstraints != 0) { + numConstraints = (numConstraints & ~WhichMask) | (((DWORD)which) << WhichShift); + LoaderAllocator* pAllocator = GetModule()->GetLoaderAllocator(); // If there is a single class constraint we place it at index 0 of the array - AllocMemHolder constraints - (pAllocator->GetLowFrequencyHeap()->AllocMem(S_SIZE_T(numConstraints) * S_SIZE_T(sizeof(TypeHandle)))); + AllocMemHolder constraintAlloc; + TypeHandle *constraints; + + if (whichCurrent == WhichConstraintsToLoad::None) + { + constraintAlloc = (pAllocator->GetLowFrequencyHeap()->AllocMem(S_SIZE_T(numConstraints) * S_SIZE_T(sizeof(TypeHandle)))); + constraints = (TypeHandle*)constraintAlloc; + } + else + { + constraints = m_constraints; + } + bool loadedAllConstraints = true; DWORD i = 0; while (pInternalImport->EnumNext(&hEnum, &tkConstraint)) { @@ -816,14 +846,86 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level /* = CLASS_LOADED */) GetModule()->GetAssembly()->ThrowTypeLoadException(pInternalImport, pMT->GetCl(), IDS_CLASSLOAD_BADFORMAT); } _ASSERTE(tkParam == GetToken()); - TypeHandle thConstraint = ClassLoader::LoadTypeDefOrRefOrSpecThrowing(GetModule(), tkConstraintType, - &typeContext, - ClassLoader::ThrowIfNotFound, - ClassLoader::FailIfUninstDefOrRef, - ClassLoader::LoadTypes, - level); + TypeHandle thConstraint; + + bool loadConstraint; + if (TypeFromToken(tkConstraintType) == mdtTypeSpec && which != WhichConstraintsToLoad::All) + { + ULONG cSig; + PCCOR_SIGNATURE pSig; - constraints[i++] = thConstraint; + if (FAILED(pInternalImport->GetTypeSpecFromToken(tkConstraintType, &pSig, &cSig))) + { + GetModule()->GetAssembly()->ThrowTypeLoadException(pInternalImport, pMT->GetCl(), IDS_CLASSLOAD_BADFORMAT); + } + + SigPointer investigatePtr(pSig, cSig); + CorElementType elemType; + IfFailThrow(investigatePtr.GetElemType(&elemType)); + if (elemType == ELEMENT_TYPE_VAR || elemType == ELEMENT_TYPE_MVAR) + { + // We can always load variable constraints + loadConstraint = true; + } + else if (which != WhichConstraintsToLoad::TypeOrMethodVarsOnly) + { + _ASSERTE(which == WhichConstraintsToLoad::TypeOrMethodVarsAndNonInterfacesOnly); + + if (elemType != ELEMENT_TYPE_GENERICINST) + { + // We don't know if its a class or interface, but it isn't generic, so finding out is the same as loading + // it, so just allow the load to occur. + loadConstraint = true; + } + else + { + IfFailThrow(investigatePtr.GetElemType(&elemType)); + _ASSERTE(elemType == ELEMENT_TYPE_CLASS + || elemType == ELEMENT_TYPE_VALUETYPE); + mdToken tkInvestigate; + + IfFailThrow(investigatePtr.GetToken(&tkInvestigate)); + + TypeHandle thUninstantiated = ClassLoader::LoadTypeDefOrRefOrSpecThrowing(GetModule(), tkInvestigate, + &typeContext, + ClassLoader::ThrowIfNotFound, + ClassLoader::PermitUninstDefOrRef, + ClassLoader::LoadTypes, + level); + + if (thUninstantiated.IsInterface()) + { + loadConstraint = false; + } + else + { + loadConstraint = true; + } + } + } + else + { + loadConstraint = false; + } + } + else + { + loadConstraint = true; + } + + if (loadConstraint) + { + thConstraint = ClassLoader::LoadTypeDefOrRefOrSpecThrowing(GetModule(), tkConstraintType, + &typeContext, + ClassLoader::ThrowIfNotFound, + ClassLoader::FailIfUninstDefOrRef, + ClassLoader::LoadTypes, + level); + } + else + { + loadedAllConstraints = false; + } // Method type constraints behave contravariantly // (cf Bounded polymorphism e.g. see @@ -832,6 +934,7 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level /* = CLASS_LOADED */) { ULONG cSig; PCCOR_SIGNATURE pSig; + if (FAILED(pInternalImport->GetTypeSpecFromToken(tkConstraintType, &pSig, &cSig))) { GetModule()->GetAssembly()->ThrowTypeLoadException(pInternalImport, pMT->GetCl(), IDS_CLASSLOAD_BADFORMAT); @@ -845,20 +948,40 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level /* = CLASS_LOADED */) GetModule()->GetAssembly()->ThrowTypeLoadException(pInternalImport, pMT->GetCl(), IDS_CLASSLOAD_VARIANCE_IN_CONSTRAINT); } } + + if (!thConstraint.IsNull()) + VolatileStore((TADDR*)&constraints[i++], thConstraint.AsTAddr()); } - if (InterlockedCompareExchangeT(&m_constraints, constraints.operator->(), NULL) == NULL) + if (whichCurrent == WhichConstraintsToLoad::None) { - constraints.SuppressRelease(); + if (InterlockedCompareExchangeT(&m_constraints, constraintAlloc.operator->(), NULL) == NULL) + { + constraintAlloc.SuppressRelease(); + } + } + + if (loadedAllConstraints) + { + // We loaded all the constraints, so we can update the number we're going to store to indicate that we're storing all of them + numConstraints = numConstraints & ~WhichMask; } } - m_numConstraints = numConstraints; - } + if (InterlockedCompareExchangeT(&m_numConstraintsWithFlags, numConstraints, initialNumConstraints) != initialNumConstraints) + { + // Retry if another thread set the number of constraints while we were working + continue; + } + break; + } while (true); - for (DWORD i = 0; i < numConstraints; i++) + for (DWORD i = 0; i < (numConstraints & ~WhichMask); i++) { - ClassLoader::EnsureLoaded(m_constraints[i], level); + TypeHandle constraint = m_constraints[i]; + if (constraint.IsNull()) + continue; + ClassLoader::EnsureLoaded(constraint, level); } } @@ -869,7 +992,7 @@ BOOL TypeVarTypeDesc::ConstrainedAsObjRef() NOTHROW; GC_NOTRIGGER; MODE_ANY; - PRECONDITION(ConstraintsLoaded()); + PRECONDITION(ConstraintsLoaded(WhichConstraintsToLoad::TypeOrMethodVarsAndNonInterfacesOnly)); } CONTRACTL_END; @@ -903,12 +1026,17 @@ BOOL TypeVarTypeDesc::ConstrainedAsObjRefHelper() CONTRACTL_END; DWORD dwNumConstraints = 0; - TypeHandle* constraints = GetCachedConstraints(&dwNumConstraints); + TypeHandle* constraints = GetCachedConstraints(&dwNumConstraints, WhichConstraintsToLoad::TypeOrMethodVarsAndNonInterfacesOnly); for (DWORD i = 0; i < dwNumConstraints; i++) { TypeHandle constraint = constraints[i]; + // This might be null if we didn't load an interface constraint + // Interface constraints do not contribute to this calculation, so we didn't need them + if (constraint.IsNull()) + continue; + if (constraint.IsGenericVariable() && constraint.AsGenericVariable()->ConstrainedAsObjRefHelper()) return TRUE; @@ -936,7 +1064,7 @@ BOOL TypeVarTypeDesc::ConstrainedAsValueType() NOTHROW; GC_NOTRIGGER; MODE_ANY; - PRECONDITION(ConstraintsLoaded()); + PRECONDITION(ConstraintsLoaded(WhichConstraintsToLoad::TypeOrMethodVarsAndNonInterfacesOnly)); } CONTRACTL_END; @@ -953,12 +1081,16 @@ BOOL TypeVarTypeDesc::ConstrainedAsValueType() return TRUE; DWORD dwNumConstraints = 0; - TypeHandle* constraints = GetCachedConstraints(&dwNumConstraints); + TypeHandle* constraints = GetCachedConstraints(&dwNumConstraints, WhichConstraintsToLoad::TypeOrMethodVarsAndNonInterfacesOnly); for (DWORD i = 0; i < dwNumConstraints; i++) { TypeHandle constraint = constraints[i]; + // This might be null if we didn't load an interface constraint + if (constraint.IsNull()) + continue; + if (constraint.IsGenericVariable()) { if (constraint.AsGenericVariable()->ConstrainedAsValueType()) @@ -1669,12 +1801,12 @@ TypeVarTypeDesc::EnumMemoryRegions(CLRDataEnumMemoryFlags flags) GetModule()->EnumMemoryRegions(flags, true); } - if (m_numConstraints != (DWORD)-1) + if (m_numConstraintsWithFlags != (DWORD)-1) { PTR_TypeHandle constraint = m_constraints; - for (DWORD i = 0; i < m_numConstraints; i++) + for (DWORD i = 0; i < (m_numConstraintsWithFlags & ~WhichMask); i++) { - if (constraint.IsValid()) + if (constraint.IsValid() && !constraint->IsNull()) { constraint->EnumMemoryRegions(flags); } diff --git a/src/coreclr/vm/typedesc.h b/src/coreclr/vm/typedesc.h index 1edcb3f1ae0971..e42e9c42977f87 100644 --- a/src/coreclr/vm/typedesc.h +++ b/src/coreclr/vm/typedesc.h @@ -283,6 +283,14 @@ struct cdac_data static constexpr size_t TypeArg = offsetof(ParamTypeDesc, m_Arg); }; +enum class WhichConstraintsToLoad +{ + All = 0, + TypeOrMethodVarsAndNonInterfacesOnly = 1, + TypeOrMethodVarsOnly = 2, + None = 3, +}; + /*************************************************************************/ // These are for verification of generic code and reflection over generic code. // Each TypeVarTypeDesc represents a class or method type variable, as specified by a GenericParam entry. @@ -291,6 +299,8 @@ struct cdac_data class TypeVarTypeDesc : public TypeDesc { + const DWORD WhichMask = 0xC0000000; + const DWORD WhichShift = 30; public: #ifndef DACCESS_COMPILE @@ -314,7 +324,7 @@ class TypeVarTypeDesc : public TypeDesc m_token = token; m_index = index; m_constraints = NULL; - m_numConstraints = (DWORD)-1; + m_numConstraintsWithFlags = (DWORD)-1; } #endif // #ifndef DACCESS_COMPILE @@ -354,16 +364,16 @@ class TypeVarTypeDesc : public TypeDesc MethodDesc * LoadOwnerMethod(); TypeHandle LoadOwnerType(); - BOOL ConstraintsLoaded() { LIMITED_METHOD_CONTRACT; return m_numConstraints != (DWORD)-1; } + BOOL ConstraintsLoaded(WhichConstraintsToLoad which) { LIMITED_METHOD_CONTRACT; if (((m_numConstraintsWithFlags & WhichMask) >> WhichShift) <= (DWORD)which) return TRUE; return FALSE; } // Return NULL if no constraints are specified // Return an array of type handles if constraints are specified, // with the number of constraints returned in pNumConstraints - TypeHandle* GetCachedConstraints(DWORD *pNumConstraints); - TypeHandle* GetConstraints(DWORD *pNumConstraints, ClassLoadLevel level = CLASS_LOADED); + TypeHandle* GetCachedConstraints(DWORD *pNumConstraints, WhichConstraintsToLoad which); + TypeHandle* GetConstraints(DWORD *pNumConstraints, ClassLoadLevel level, WhichConstraintsToLoad which); // Load the constraints if not already loaded - void LoadConstraints(ClassLoadLevel level = CLASS_LOADED); + void LoadConstraints(ClassLoadLevel level, WhichConstraintsToLoad which); // Check the constraints on this type parameter hold in the supplied context for the supplied type BOOL SatisfiesConstraints(SigTypeContext *pTypeContext, TypeHandle thArg, @@ -391,7 +401,7 @@ class TypeVarTypeDesc : public TypeDesc mdToken m_typeOrMethodDef; // Constraints, determined on first call to GetConstraints - Volatile m_numConstraints; // -1 until number has been determined + Volatile m_numConstraintsWithFlags; // -1 until number has been determined. Bit 31 and 30 bits are WhichConstraintsToLoad PTR_TypeHandle m_constraints; // token for GenericParam entry diff --git a/src/coreclr/vm/typehandle.cpp b/src/coreclr/vm/typehandle.cpp index 05b80e1fd662fa..32ff89f91d3a89 100644 --- a/src/coreclr/vm/typehandle.cpp +++ b/src/coreclr/vm/typehandle.cpp @@ -582,8 +582,8 @@ BOOL TypeHandle::IsBoxedAndCanCastTo(TypeHandle type, TypeHandlePairList *pPairL { TypeVarTypeDesc* varFromParam = AsGenericVariable(); - if (!varFromParam->ConstraintsLoaded()) - varFromParam->LoadConstraints(CLASS_DEPENDENCIES_LOADED); + if (!varFromParam->ConstraintsLoaded(WhichConstraintsToLoad::TypeOrMethodVarsAndNonInterfacesOnly)) + varFromParam->LoadConstraints(CLASS_DEPENDENCIES_LOADED, WhichConstraintsToLoad::TypeOrMethodVarsAndNonInterfacesOnly); // A generic type parameter cannot be compatible with another type // as it could be substitued with a valuetype. However, if it is @@ -1382,8 +1382,6 @@ BOOL TypeHandle::SatisfiesClassConstraints() const _ASSERTE(tyvar != NULL); _ASSERTE(TypeFromToken(tyvar->GetTypeOrMethodDef()) == mdtTypeDef); - tyvar->LoadConstraints(); //TODO: is this necessary for anything but the typical class? - if (!tyvar->SatisfiesConstraints(&typeContext, thArg)) { return FALSE; From bb4a3d2f16c378537b9bdd34f3eca921d3d4b5d0 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 6 Oct 2025 16:31:49 -0700 Subject: [PATCH 02/17] Clear up the issues around GetDeclaringMethodTableFromTypeVarTypeDesc --- src/coreclr/vm/typectxt.cpp | 19 +++++++++++++------ src/coreclr/vm/typedesc.cpp | 13 ++++++++++--- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/coreclr/vm/typectxt.cpp b/src/coreclr/vm/typectxt.cpp index fc04b676bfa3bc..74c390b165a1d0 100644 --- a/src/coreclr/vm/typectxt.cpp +++ b/src/coreclr/vm/typectxt.cpp @@ -91,7 +91,10 @@ void SigTypeContext::InitTypeContext(MethodDesc *md, TypeHandle declaringType, S #ifndef DACCESS_COMPILE TypeHandle GetDeclaringMethodTableFromTypeVarTypeDesc(TypeVarTypeDesc *pTypeVar, MethodDesc *pMD) { - LIMITED_METHOD_CONTRACT; + CONTRACTL { + THROWS; + GC_TRIGGERS; + } CONTRACTL_END; // This currently should only happen in cases where we've already loaded the constraints. // Currently, the only known case where use this code is reflection over methods exposed on a TypeVariable. @@ -129,10 +132,10 @@ TypeHandle GetDeclaringMethodTableFromTypeVarTypeDesc(TypeVarTypeDesc *pTypeVar, void SigTypeContext::InitTypeContext(MethodDesc *md, TypeHandle declaringType, Instantiation exactMethodInst, SigTypeContext *pRes) { + // This method has an unusual contract for SigTypeContext so that it can be used to load type with a declaringType which is a generic variable. CONTRACTL { - NOTHROW; - GC_NOTRIGGER; - FORBID_FAULT; + THROWS; + GC_TRIGGERS; PRECONDITION(CheckPointer(md)); } CONTRACTL_END; @@ -146,8 +149,12 @@ void SigTypeContext::InitTypeContext(MethodDesc *md, TypeHandle declaringType, I // factor this with the work above if (declaringType.IsGenericVariable()) { - _ASSERTE(FALSE); -// declaringType = GetDeclaringMethodTableFromTypeVarTypeDesc(declaringType.AsGenericVariable(), md); + declaringType.AsGenericVariable()->LoadConstraints(CLASS_LOADED, WhichConstraintsToLoad::All); + + // This can only happen for reflection over type variables. Notably the logic which is used to enumerate the locals + // of a MethodBody which was found by reflection over a type variable. This only needs to be non-null + // in the case where the the type variable is constrained to implement a generic class or struct. + declaringType = GetDeclaringMethodTableFromTypeVarTypeDesc(declaringType.AsGenericVariable(), md); } if (declaringType.IsNull()) diff --git a/src/coreclr/vm/typedesc.cpp b/src/coreclr/vm/typedesc.cpp index 540c10a87fbd85..aad6f4270df771 100644 --- a/src/coreclr/vm/typedesc.cpp +++ b/src/coreclr/vm/typedesc.cpp @@ -729,9 +729,16 @@ TypeHandle* TypeVarTypeDesc::GetCachedConstraints(DWORD *pNumConstraints, WhichC TypeHandle* TypeVarTypeDesc::GetConstraints(DWORD *pNumConstraints, ClassLoadLevel level, WhichConstraintsToLoad which) { - WRAPPER_NO_CONTRACT; - PRECONDITION(CheckPointer(pNumConstraints)); - PRECONDITION(level == CLASS_DEPENDENCIES_LOADED || level == CLASS_LOADED); + CONTRACTL + { + THROWS; + GC_TRIGGERS; + MODE_ANY; + } + CONTRACTL_END; + + _ASSERTE(CheckPointer(pNumConstraints)); + _ASSERTE(level == CLASS_DEPENDENCIES_LOADED || level == CLASS_LOADED); DWORD numConstraints = m_numConstraintsWithFlags; WhichConstraintsToLoad whichCurrent = (WhichConstraintsToLoad)(numConstraints >> WhichShift); From db83d474308f46ff08b49bc0b5cfc4933e0138e4 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 6 Oct 2025 17:05:31 -0700 Subject: [PATCH 03/17] Tweak, and add comments... This is DRAFT as we now have an unused state WhichConstraintsToLoad::TypeOrMethodVarsOnly which we never use. (It could be used for the Bounds algorithm, but that isn't actually used without doing the more expensive logic which does access validation) --- src/coreclr/vm/genmeth.cpp | 4 +- src/coreclr/vm/methodtable.cpp | 126 ++++++++++++++++++++++++++++++++- src/coreclr/vm/typectxt.cpp | 47 +++++------- src/coreclr/vm/typedesc.cpp | 11 ++- 4 files changed, 153 insertions(+), 35 deletions(-) diff --git a/src/coreclr/vm/genmeth.cpp b/src/coreclr/vm/genmeth.cpp index 749d7436cb0e7b..2166fb1e1c42e1 100644 --- a/src/coreclr/vm/genmeth.cpp +++ b/src/coreclr/vm/genmeth.cpp @@ -1536,7 +1536,7 @@ BOOL Bounded(TypeVarTypeDesc *tyvar, DWORD depth) { } DWORD numConstraints; - TypeHandle *constraints = tyvar->GetConstraints(&numConstraints, CLASS_DEPENDENCIES_LOADED, WhichConstraintsToLoad::TypeOrMethodVarsOnly); + TypeHandle *constraints = tyvar->GetConstraints(&numConstraints, CLASS_DEPENDENCIES_LOADED, WhichConstraintsToLoad::TypeOrMethodVarsAndNonInterfacesOnly); for (unsigned i = 0; i < numConstraints; i++) { TypeHandle constraint = constraints[i]; @@ -1576,8 +1576,6 @@ void MethodDesc::LoadConstraintsForTypicalMethodDefinition(BOOL *pfHasCircularCl { TypeVarTypeDesc* tyvar = methodInst[i].AsGenericVariable(); _ASSERTE(tyvar != NULL); - tyvar->LoadConstraints(level, WhichConstraintsToLoad::All); // Use the All here, since DoAccessibilityCheckForConstraints needs it, athough really, we only need to walk the typedefs/refs in the constraints - VOID DoAccessibilityCheckForConstraints(MethodTable *pAskingMT, TypeVarTypeDesc *pTyVar, UINT resIDWhy); DoAccessibilityCheckForConstraints(GetMethodTable(), tyvar, E_ACCESSDENIED); } diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index e95805ea154dde..40ec20a0727d3b 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -4204,6 +4204,87 @@ VOID DoAccessibilityCheckForConstraint(MethodTable *pAskingMT, TypeHandle thCons } +VOID DoAccessibilityCheckForConstraintSignature(Module *pModule, SigPointer *pSigPtr, MethodTable *pAskingMT, UINT resIDWhy) +{ + CONTRACTL + { + THROWS; + GC_TRIGGERS; + } + CONTRACTL_END; + + CorElementType elemType; + IfFailThrow(sigPtr.GetElemType(&elemType)); + switch (elemType) + { + case ELEMENT_TYPE_CLASS: + case ELEMENT_TYPE_VALUETYPE: + { + mdToken typeDefOrRef; + IfFailThrow(sigPtr.GetToken(&typeDefOrRef)); + + TypeHandle th = ClassLoader::LoadTypeDefOrRefThrowing(pModule, typeDefOrRef, ClassLoader::ThrowIfNotFound, ClassLoader::PermitUninstDefOrRef, tdNoTypes, CLASS_DEPENDENCIES_LOADED); + DoAccessibilityCheckForConstraint(pAskingMT, th, resIDWhy); + break; + } + case ELEMENT_TYPE_GENERICINST: + { + IfFailThrow(sigPtr.GetElemType(&elemType)); + if (!(elemType == ELEMENT_TYPE_CLASS || elemType == ELEMENT_TYPE_VALUETYPE)) + { + COMPlusThrow(kTypeLoadException, IDS_CLASSLOAD_BADFORMAT); + } + mdToken typeDefOrRef; + IfFailThrow(sigPtr.GetToken(&typeDefOrRef)); + TypeHandle th = ClassLoader::LoadTypeDefOrRefThrowing(pModule, typeDefOrRef, ClassLoader::ThrowIfNotFound, ClassLoader::PermitUninstDefOrRef, tdNoTypes, CLASS_DEPENDENCIES_LOADED); + DoAccessibilityCheckForConstraint(pAskingMT, th, resIDWhy); + ULONG numArgs; + IfFailThrow(sigPtr.GetData(&numArgs)); + for (ULONG i = 0; i < numArgs; i++) + { + DoAccessibilityCheckForConstraintSignature(pModule, sigPtr, pAskingMT, resIDWhy); + } + break; + } + case ELEMENT_TYPE_SZARRAY: + case ELEMENT_TYPE_BYREF: + case ELEMENT_TYPE_PTR: + DoAccessibilityCheckForConstraintSignature(pModule, sigPtr, pAskingMT, resIDWhy); + break; + case ELEMENT_TYPE_ARRAY: + { + IfFailThrow(sigPtr.GetElemType(&elemType)); + DoAccessibilityCheckForConstraintSignature(pModule, sigPtr, pAskingMT, resIDWhy); + ULONG rank; + IfFailThrow(sigPtr.GetData(&rank)); + ULONG numSizes; + IfFailThrow(sigPtr.GetData(&numSizes)); + for (ULONG i = 0; i < numSizes; i++) + { + ULONG size; + IfFailThrow(sigPtr.GetData(&size)); + } + ULONG numLoBounds; + IfFailThrow(sigPtr.GetData(&numLoBounds)); + for (ULONG i = 0; i < numLoBounds; i++) + { + ULONG loBound; + IfFailThrow(sigPtr.GetData(&loBound)); + } + break; + } + case ELEMENT_TYPE_FNPTR: + { + // Function pointers can't be in constraint signatures + COMPlusThrow(kTypeLoadException, IDS_CLASSLOAD_BADFORMAT); + } + default: + // Primitive types and such. Nothing to check + break; + } +} + + VOID DoAccessibilityCheckForConstraints(MethodTable *pAskingMT, TypeVarTypeDesc *pTyVar, UINT resIDWhy) { CONTRACTL @@ -4214,12 +4295,52 @@ VOID DoAccessibilityCheckForConstraints(MethodTable *pAskingMT, TypeVarTypeDesc CONTRACTL_END; DWORD numConstraints; - TypeHandle *pthConstraints = pTyVar->GetCachedConstraints(&numConstraints, WhichConstraintsToLoad::All); + TypeHandle *pthConstraints = pTyVar->GetConstraints(&numConstraints, CLASS_DEPENDENCIES_LOADED, WhichConstraintsToLoad::TypeOrMethodVarsAndNonInterfacesOnly); for (DWORD cidx = 0; cidx < numConstraints; cidx++) { TypeHandle thConstraint = pthConstraints[cidx]; - DoAccessibilityCheckForConstraint(pAskingMT, thConstraint, resIDWhy); + if (thConstraint.IsNull()) + { + // This is a constraint which we didn't load above. Instead of doing the full load, just iterate the signature of the constraint to make sure all of the TypeDefs/Refs are accessible + Module *pModule = pTyVar->GetModule(); + + IMDInternalImport* pInternalImport = pModule->GetMDImport(); + HENUMInternalHolder hEnum(pInternalImport); + mdGenericParamConstraint tkConstraint; + hEnum.EnumInit(mdtGenericParamConstraint, pTyVar->GetToken()); + DWORD i = 0; + while (pInternalImport->EnumNext(&hEnum, &tkConstraint)) + { + _ASSERTE(i <= numConstraints); + if (i == cidx) + { + // We've found the constraint we're looking for. + mdToken tkConstraintType, tkParam; + if (FAILED(pInternalImport->GetGenericParamConstraintProps(tkConstraint, &tkParam, &tkConstraintType))) + { + GetModule()->GetAssembly()->ThrowTypeLoadException(pInternalImport, pMT->GetCl(), IDS_CLASSLOAD_BADFORMAT); + } + _ASSERTE(tkParam == pTyVar->GetToken()); + + _ASSERTE(TypeFromToken(tkConstraintType) == mdtTypeSpec); + ULONG cSig; + PCCOR_SIGNATURE pSig; + + if (FAILED(pInternalImport->GetTypeSpecFromToken(tkConstraintType, &pSig, &cSig))) + { + GetModule()->GetAssembly()->ThrowTypeLoadException(pInternalImport, pMT->GetCl(), IDS_CLASSLOAD_BADFORMAT); + } + SigPointer sigPtr(pSig, cSig); + DoAccessibilityCheckForConstraintSignature(pModule, &sigPtr, pAskingMT, resIDWhy); + break; + } + } + } + else + { + DoAccessibilityCheckForConstraint(pAskingMT, thConstraint, resIDWhy); + } } } @@ -4589,7 +4710,6 @@ void MethodTable::DoFullyLoad(Generics::RecursionGraph * const pVisited, const // For typical type definitions, we have to load the constraints here because they may not // have been loaded yet. In principle we could change DoAccessibilityCheckForConstraints to walk the typedefs/refs in the constraint instead - pTyVar->LoadConstraints(CLASS_DEPENDENCIES_LOADED, WhichConstraintsToLoad::All); DoAccessibilityCheckForConstraints(this, pTyVar, E_ACCESSDENIED); } } diff --git a/src/coreclr/vm/typectxt.cpp b/src/coreclr/vm/typectxt.cpp index 74c390b165a1d0..33ad0f0861a026 100644 --- a/src/coreclr/vm/typectxt.cpp +++ b/src/coreclr/vm/typectxt.cpp @@ -96,34 +96,31 @@ TypeHandle GetDeclaringMethodTableFromTypeVarTypeDesc(TypeVarTypeDesc *pTypeVar, GC_TRIGGERS; } CONTRACTL_END; - // This currently should only happen in cases where we've already loaded the constraints. - // Currently, the only known case where use this code is reflection over methods exposed on a TypeVariable. - _ASSERTE(pTypeVar->ConstraintsLoaded(WhichConstraintsToLoad::All)); + // This can only happen for reflection over type variables. Notably the logic which is used to enumerate the locals + // of a MethodBody which was found by reflection over a type variable. This only needs to be non-null + // in the case where the the type variable is constrained to implement a generic class or struct. - if (pTypeVar->ConstraintsLoaded(WhichConstraintsToLoad::All)) + DWORD cConstraints; + TypeHandle *pTypeHandles = pTypeVar->GetConstraints(&cConstraints, CLASS_DEPENDENCIES_LOADED, WhichConstraintsToLoad::All); + for (DWORD iConstraint = 0; iConstraint < cConstraints; iConstraint++) { - DWORD cConstraints; - TypeHandle *pTypeHandles = pTypeVar->GetConstraints(&cConstraints, CLASS_DEPENDENCIES_LOADED, WhichConstraintsToLoad::All); - for (DWORD iConstraint = 0; iConstraint < cConstraints; iConstraint++) + if (pTypeHandles[iConstraint].IsGenericVariable()) { - if (pTypeHandles[iConstraint].IsGenericVariable()) - { - TypeHandle th = GetDeclaringMethodTableFromTypeVarTypeDesc(pTypeHandles[iConstraint].AsGenericVariable(), pMD); - if (!th.IsNull()) - return th; - } - else + TypeHandle th = GetDeclaringMethodTableFromTypeVarTypeDesc(pTypeHandles[iConstraint].AsGenericVariable(), pMD); + if (!th.IsNull()) + return th; + } + else + { + MethodTable *pMT = pTypeHandles[iConstraint].GetMethodTable(); + while (pMT != NULL) { - MethodTable *pMT = pTypeHandles[iConstraint].GetMethodTable(); - while (pMT != NULL) + if (pMT == pMD->GetMethodTable()) { - if (pMT == pMD->GetMethodTable()) - { - return TypeHandle(pMT); - } - - pMT = pMT->GetParentMethodTable(); + return TypeHandle(pMT); } + + pMT = pMT->GetParentMethodTable(); } } } @@ -146,14 +143,8 @@ void SigTypeContext::InitTypeContext(MethodDesc *md, TypeHandle declaringType, I } else { - // factor this with the work above if (declaringType.IsGenericVariable()) { - declaringType.AsGenericVariable()->LoadConstraints(CLASS_LOADED, WhichConstraintsToLoad::All); - - // This can only happen for reflection over type variables. Notably the logic which is used to enumerate the locals - // of a MethodBody which was found by reflection over a type variable. This only needs to be non-null - // in the case where the the type variable is constrained to implement a generic class or struct. declaringType = GetDeclaringMethodTableFromTypeVarTypeDesc(declaringType.AsGenericVariable(), md); } diff --git a/src/coreclr/vm/typedesc.cpp b/src/coreclr/vm/typedesc.cpp index aad6f4270df771..6f87feb367837e 100644 --- a/src/coreclr/vm/typedesc.cpp +++ b/src/coreclr/vm/typedesc.cpp @@ -771,7 +771,16 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level, WhichConstraintsToLo DWORD numConstraints; - // If we have already loaded the constraints, and the previously loaded constraints are sufficient for the current request, return. + // If we have already loaded the constraints, and the previously loaded constraints are sufficient for the current request, skip the logic to find more constraints. + // Otherwise we need to load more constraints, and then possibly actually force them to be loaded to the appropriate load level. + // NOTE: + // The WhichConstraintsToLoad enum is ordered from most constraints to least constraints, so we can use a simple greater-than comparison to determine + // if the previously loaded constraints are sufficient for the current request. + // There are 4 possible values for WhichConstraintsToLoad: + // All - Load all constraints + // TypeOrMethodVarsAndNonInterfacesOnly - Load all constraints except interface constraints that are not type or method variables. The code MAY load other constraints. This is used when loading constraints for the purpose of type safety checks. + // TypeOrMethodVarsOnly - Load only type or method variables. (Needed for the Bounded algorithm) + // None - Load no constraints. This is the initial state. do { From c0ae70db06e7226a378420f4c2b686e7198fa853 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 6 Oct 2025 17:10:13 -0700 Subject: [PATCH 04/17] Fix build errors --- src/coreclr/vm/methodtable.cpp | 48 +++++++++++++++++----------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 40ec20a0727d3b..7771a0fc424ad1 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -4214,14 +4214,14 @@ VOID DoAccessibilityCheckForConstraintSignature(Module *pModule, SigPointer *pSi CONTRACTL_END; CorElementType elemType; - IfFailThrow(sigPtr.GetElemType(&elemType)); + IfFailThrow(pSigPtr->GetElemType(&elemType)); switch (elemType) { case ELEMENT_TYPE_CLASS: case ELEMENT_TYPE_VALUETYPE: { mdToken typeDefOrRef; - IfFailThrow(sigPtr.GetToken(&typeDefOrRef)); + IfFailThrow(pSigPtr->GetToken(&typeDefOrRef)); TypeHandle th = ClassLoader::LoadTypeDefOrRefThrowing(pModule, typeDefOrRef, ClassLoader::ThrowIfNotFound, ClassLoader::PermitUninstDefOrRef, tdNoTypes, CLASS_DEPENDENCIES_LOADED); DoAccessibilityCheckForConstraint(pAskingMT, th, resIDWhy); @@ -4229,47 +4229,47 @@ VOID DoAccessibilityCheckForConstraintSignature(Module *pModule, SigPointer *pSi } case ELEMENT_TYPE_GENERICINST: { - IfFailThrow(sigPtr.GetElemType(&elemType)); + IfFailThrow(pSigPtr->GetElemType(&elemType)); if (!(elemType == ELEMENT_TYPE_CLASS || elemType == ELEMENT_TYPE_VALUETYPE)) { COMPlusThrow(kTypeLoadException, IDS_CLASSLOAD_BADFORMAT); } mdToken typeDefOrRef; - IfFailThrow(sigPtr.GetToken(&typeDefOrRef)); + IfFailThrow(pSigPtr->GetToken(&typeDefOrRef)); TypeHandle th = ClassLoader::LoadTypeDefOrRefThrowing(pModule, typeDefOrRef, ClassLoader::ThrowIfNotFound, ClassLoader::PermitUninstDefOrRef, tdNoTypes, CLASS_DEPENDENCIES_LOADED); DoAccessibilityCheckForConstraint(pAskingMT, th, resIDWhy); - ULONG numArgs; - IfFailThrow(sigPtr.GetData(&numArgs)); - for (ULONG i = 0; i < numArgs; i++) + uint32_t numArgs; + IfFailThrow(pSigPtr->GetData(&numArgs)); + for (uint32_t i = 0; i < numArgs; i++) { - DoAccessibilityCheckForConstraintSignature(pModule, sigPtr, pAskingMT, resIDWhy); + DoAccessibilityCheckForConstraintSignature(pModule, pSigPtr, pAskingMT, resIDWhy); } break; } case ELEMENT_TYPE_SZARRAY: case ELEMENT_TYPE_BYREF: case ELEMENT_TYPE_PTR: - DoAccessibilityCheckForConstraintSignature(pModule, sigPtr, pAskingMT, resIDWhy); + DoAccessibilityCheckForConstraintSignature(pModule, pSigPtr, pAskingMT, resIDWhy); break; case ELEMENT_TYPE_ARRAY: { - IfFailThrow(sigPtr.GetElemType(&elemType)); - DoAccessibilityCheckForConstraintSignature(pModule, sigPtr, pAskingMT, resIDWhy); - ULONG rank; - IfFailThrow(sigPtr.GetData(&rank)); - ULONG numSizes; - IfFailThrow(sigPtr.GetData(&numSizes)); + IfFailThrow(pSigPtr->GetElemType(&elemType)); + DoAccessibilityCheckForConstraintSignature(pModule, pSigPtr, pAskingMT, resIDWhy); + uint32_t rank; + IfFailThrow(pSigPtr->GetData(&rank)); + uint32_t numSizes; + IfFailThrow(pSigPtr->GetData(&numSizes)); for (ULONG i = 0; i < numSizes; i++) { - ULONG size; - IfFailThrow(sigPtr.GetData(&size)); + uint32_t size; + IfFailThrow(pSigPtr->GetData(&size)); } - ULONG numLoBounds; - IfFailThrow(sigPtr.GetData(&numLoBounds)); - for (ULONG i = 0; i < numLoBounds; i++) + uint32_t numLoBounds; + IfFailThrow(pSigPtr->GetData(&numLoBounds)); + for (uint32_t i = 0; i < numLoBounds; i++) { - ULONG loBound; - IfFailThrow(sigPtr.GetData(&loBound)); + uint32_t loBound; + IfFailThrow(pSigPtr->GetData(&loBound)); } break; } @@ -4319,7 +4319,7 @@ VOID DoAccessibilityCheckForConstraints(MethodTable *pAskingMT, TypeVarTypeDesc mdToken tkConstraintType, tkParam; if (FAILED(pInternalImport->GetGenericParamConstraintProps(tkConstraint, &tkParam, &tkConstraintType))) { - GetModule()->GetAssembly()->ThrowTypeLoadException(pInternalImport, pMT->GetCl(), IDS_CLASSLOAD_BADFORMAT); + ThrowHR(COR_E_BADIMAGEFORMAT); } _ASSERTE(tkParam == pTyVar->GetToken()); @@ -4329,7 +4329,7 @@ VOID DoAccessibilityCheckForConstraints(MethodTable *pAskingMT, TypeVarTypeDesc if (FAILED(pInternalImport->GetTypeSpecFromToken(tkConstraintType, &pSig, &cSig))) { - GetModule()->GetAssembly()->ThrowTypeLoadException(pInternalImport, pMT->GetCl(), IDS_CLASSLOAD_BADFORMAT); + ThrowHR(COR_E_BADIMAGEFORMAT); } SigPointer sigPtr(pSig, cSig); DoAccessibilityCheckForConstraintSignature(pModule, &sigPtr, pAskingMT, resIDWhy); From 1f742a3cacc446e2ecf75461d5881dc8cebf36aa Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 7 Oct 2025 11:55:54 -0700 Subject: [PATCH 05/17] Fix bugs making it not work properly --- src/coreclr/vm/typedesc.cpp | 8 ++++++-- src/coreclr/vm/typedesc.h | 4 ++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/coreclr/vm/typedesc.cpp b/src/coreclr/vm/typedesc.cpp index 6f87feb367837e..0a87e9caa01982 100644 --- a/src/coreclr/vm/typedesc.cpp +++ b/src/coreclr/vm/typedesc.cpp @@ -843,7 +843,7 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level, WhichConstraintsToLo if (whichCurrent == WhichConstraintsToLoad::None) { - constraintAlloc = (pAllocator->GetLowFrequencyHeap()->AllocMem(S_SIZE_T(numConstraints) * S_SIZE_T(sizeof(TypeHandle)))); + constraintAlloc = (pAllocator->GetLowFrequencyHeap()->AllocMem(S_SIZE_T(numConstraints & ~WhichMask) * S_SIZE_T(sizeof(TypeHandle)))); constraints = (TypeHandle*)constraintAlloc; } else @@ -966,7 +966,11 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level, WhichConstraintsToLo } if (!thConstraint.IsNull()) - VolatileStore((TADDR*)&constraints[i++], thConstraint.AsTAddr()); + { + VolatileStore((TADDR*)&constraints[i], thConstraint.AsTAddr()); + } + + i++; } if (whichCurrent == WhichConstraintsToLoad::None) diff --git a/src/coreclr/vm/typedesc.h b/src/coreclr/vm/typedesc.h index e42e9c42977f87..8636a253c6c188 100644 --- a/src/coreclr/vm/typedesc.h +++ b/src/coreclr/vm/typedesc.h @@ -299,8 +299,8 @@ enum class WhichConstraintsToLoad class TypeVarTypeDesc : public TypeDesc { - const DWORD WhichMask = 0xC0000000; - const DWORD WhichShift = 30; + static const DWORD WhichMask = 0xC0000000; + static const DWORD WhichShift = 30; public: #ifndef DACCESS_COMPILE From 4e4a395d287f11a5280259980b7606d2a9d04352 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 30 Nov 2023 16:20:57 -0800 Subject: [PATCH 06/17] Rework the special instantiation type logic to be more effective - Currently it only handles interfaces defined on valuetypes - Change it to work for interfaces that are required implementation of other interfaces - (In that case, if the interface is generic, the special instantiation type is the first type parameter of the interface, not anything else.) This change removes some safety checks that I can't find the rationale for. This may cause some entertaining failures in CI --- src/coreclr/vm/clsload.cpp | 4 ++-- src/coreclr/vm/clsload.hpp | 2 +- src/coreclr/vm/methodtable.cpp | 12 +++++++++--- src/coreclr/vm/methodtable.h | 18 +++++++++++++++++- src/coreclr/vm/methodtablebuilder.cpp | 20 ++++++++++++++------ src/coreclr/vm/siginfo.cpp | 19 ++++++++++++++----- src/coreclr/vm/siginfo.hpp | 2 +- 7 files changed, 58 insertions(+), 19 deletions(-) diff --git a/src/coreclr/vm/clsload.cpp b/src/coreclr/vm/clsload.cpp index 72ca527baa01c4..7a49c4e11a91f3 100644 --- a/src/coreclr/vm/clsload.cpp +++ b/src/coreclr/vm/clsload.cpp @@ -1755,7 +1755,7 @@ TypeHandle ClassLoader::LoadTypeDefOrRefOrSpecThrowing(Module *pModule, ClassLoadLevel level /* = CLASS_LOADED */, BOOL dropGenericArgumentLevel /* = FALSE */, const Substitution *pSubst, - MethodTable *pMTInterfaceMapOwner) + TypeHandle thSpecialInterfaceInstantiationType) { CONTRACT(TypeHandle) { @@ -1789,7 +1789,7 @@ TypeHandle ClassLoader::LoadTypeDefOrRefOrSpecThrowing(Module *pModule, } SigPointer sigptr(pSig, cSig); TypeHandle typeHnd = sigptr.GetTypeHandleThrowing(pModule, pTypeContext, fLoadTypes, - level, dropGenericArgumentLevel, pSubst, (const ZapSig::Context *)0, pMTInterfaceMapOwner); + level, dropGenericArgumentLevel, pSubst, (const ZapSig::Context *)0, thSpecialInterfaceInstantiationType); #ifndef DACCESS_COMPILE if ((fNotFoundAction == ThrowIfNotFound) && typeHnd.IsNull()) pModule->GetAssembly()->ThrowTypeLoadException(pInternalImport, typeDefOrRefOrSpec, diff --git a/src/coreclr/vm/clsload.hpp b/src/coreclr/vm/clsload.hpp index c50dd4a5fc7987..9e494369f0ce0b 100644 --- a/src/coreclr/vm/clsload.hpp +++ b/src/coreclr/vm/clsload.hpp @@ -656,7 +656,7 @@ class ClassLoader ClassLoadLevel level = CLASS_LOADED, BOOL dropGenericArgumentLevel = FALSE, const Substitution *pSubst = NULL /* substitution to apply if the token is a type spec with generic variables */, - MethodTable *pMTInterfaceMapOwner = NULL); + TypeHandle thSpecialInterfaceInstantiationType = TypeHandle()); // Load constructed types by providing their constituents static TypeHandle LoadPointerOrByrefTypeThrowing(CorElementType typ, diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 7771a0fc424ad1..741450b59aa030 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -1265,7 +1265,7 @@ BOOL MethodTable::CanCastByVarianceToInterfaceOrDelegate(MethodTable *pTargetMT, IsSpecialMarkerTypeForGenericCasting() && GetTypeDefRid() == pTargetMT->GetTypeDefRid() && GetModule() == pTargetMT->GetModule() && - pTargetMT->GetInstantiation().ContainsAllOneType(pMTInterfaceMapOwner)) + pTargetMT->GetInstantiation().ContainsAllOneType(pMTInterfaceMapOwner->GetSpecialInstantiationType())) { return TRUE; } @@ -1290,7 +1290,7 @@ BOOL MethodTable::CanCastByVarianceToInterfaceOrDelegate(MethodTable *pTargetMT, TypeHandle thArg = inst[i]; if (IsSpecialMarkerTypeForGenericCasting() && pMTInterfaceMapOwner && !pMTInterfaceMapOwner->GetAuxiliaryData()->MayHaveOpenInterfacesInInterfaceMap()) { - thArg = pMTInterfaceMapOwner; + thArg = pMTInterfaceMapOwner->GetSpecialInstantiationType(); } TypeHandle thTargetArg = targetInst[i]; @@ -8510,6 +8510,12 @@ MethodTable::TryResolveConstraintMethodApprox( DWORD cPotentialMatchingInterfaces = 0; while (it.Next()) { + // If the approx type doesn't match by type handle, then it clearly can't match + // by canonical type. This avoids force loading the interface and breaking the + // special interface map type scenario + if (!it.GetInterfaceApprox()->HasSameTypeDefAs(thInterfaceType.AsMethodTable())) + continue; + TypeHandle thPotentialInterfaceType(it.GetInterface(pCanonMT)); if (thPotentialInterfaceType.AsMethodTable()->GetCanonicalMethodTable() == thInterfaceType.AsMethodTable()->GetCanonicalMethodTable()) @@ -8796,7 +8802,7 @@ PTR_MethodTable MethodTable::InterfaceMapIterator::GetInterface(MethodTable* pMT { TypeHandle ownerAsInst[MaxGenericParametersForSpecialMarkerType]; for (DWORD i = 0; i < MaxGenericParametersForSpecialMarkerType; i++) - ownerAsInst[i] = pMTOwner; + ownerAsInst[i] = pMTOwner->GetSpecialInstantiationType(); _ASSERTE(pResult->GetInstantiation().GetNumArgs() <= MaxGenericParametersForSpecialMarkerType); Instantiation inst(ownerAsInst, pResult->GetInstantiation().GetNumArgs()); diff --git a/src/coreclr/vm/methodtable.h b/src/coreclr/vm/methodtable.h index e161f01e0855a2..d4cdb93488a97b 100644 --- a/src/coreclr/vm/methodtable.h +++ b/src/coreclr/vm/methodtable.h @@ -1432,8 +1432,11 @@ class MethodTable // of the fully loaded type. This is to reduce the amount of type loading // performed at process startup. // + // When placed on a valuetype or a non-generic interface, the special marker will indicate that the interface should be considered instantiated over the valuetype + // When placed on an interface, the special marker will indicate that the interface should be considered instantiated over the first generic parameter of the interface + // // The current rule is that these interfaces can only appear - // on valuetypes that are not shared generic, and that the special + // on valuetypes and interfaces that are not shared generic, and that the special // marker type is the open generic type. // inline bool IsSpecialMarkerTypeForGenericCasting() @@ -1441,6 +1444,19 @@ class MethodTable return IsGenericTypeDefinition(); } + // See comment on IsSpecialMarkerTypeForGenericCasting for details + inline TypeHandle GetSpecialInstantiationType() + { + if (IsInterface() && HasInstantiation()) + { + return GetInstantiation()[0]; + } + else + { + return TypeHandle(this); + } + } + static const DWORD MaxGenericParametersForSpecialMarkerType = 8; static BOOL ComputeContainsGenericVariables(Instantiation inst); diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 7070a5a3054a2c..34519e19d5fc40 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -413,7 +413,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() || IsInterface()))) { // 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, @@ -9647,12 +9647,12 @@ MethodTableBuilder::LoadExactInterfaceMap(MethodTable *pMT) BOOL duplicates; bool retry = false; - // Always use exact loading behavior with classes or shared generics, as they have to deal with inheritance, and the + // Always use exact loading behavior with normal classes or shared generics, as they have to deal with inheritance, and the // inexact matching logic for classes would be more complex to write. // Also always use the exact loading behavior with any generic that contains generic variables, as the open type is used // to represent a type instantiated over its own generic variables, and the special marker type is currently the open type // and we make this case distinguishable by simply disallowing the optimization in those cases. - bool retryWithExactInterfaces = !pMT->IsValueType() || pMT->IsSharedByGenericInstantiations() || pMT->ContainsGenericVariables(); + bool retryWithExactInterfaces = !(pMT->IsValueType() || pMT->IsInterface()) || pMT->IsSharedByGenericInstantiations(); if (retryWithExactInterfaces) { pMT->GetAuxiliaryDataForWrite()->SetMayHaveOpenInterfacesInInterfaceMap(); @@ -9684,7 +9684,15 @@ MethodTableBuilder::LoadExactInterfaceMap(MethodTable *pMT) CLASS_LOAD_EXACTPARENTS, TRUE, (const Substitution*)0, - retryWithExactInterfaces ? NULL : pMT).GetMethodTable(); + retryWithExactInterfaces ? NULL : pMT->GetSpecialInstantiationType()).GetMethodTable(); + + // When checking to possibly load the special instantiation type, if we load a type which ISN'T the type instantiatiated over the special instantiation type, but it IS IsSpecialMarkerTypeForGenericCasting + // the ClassLoader::LoadTypeDefOrRefOrSpecThrowing function will return System.Object's MT, and we need to detect that, and abort use of the special path. + if (!pNewIntfMT->IsInterface() && !retry) + { + retry = true; + break; + } bool uninstGenericCase = !retryWithExactInterfaces && pNewIntfMT->IsSpecialMarkerTypeForGenericCasting(); @@ -9692,7 +9700,7 @@ MethodTableBuilder::LoadExactInterfaceMap(MethodTable *pMT) // 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() && pMT->IsValueType())) + if (!(pMT->GetModule()->IsSystem() && (pMT->IsValueType() || pMT->IsInterface()))) { MethodTable::InterfaceMapIterator intIt = pNewIntfMT->IterateInterfaceMap(); while (intIt.Next()) @@ -9747,7 +9755,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() && pMT->IsValueType())); + _ASSERTE(!duplicates || !(pMT->GetModule()->IsSystem() && (pMT->IsValueType() || pMT->IsInterface()))); CONSISTENCY_CHECK(duplicates || (nAssigned == pMT->GetNumInterfaces())); if (duplicates) diff --git a/src/coreclr/vm/siginfo.cpp b/src/coreclr/vm/siginfo.cpp index e23a17418e89f1..ad8a84c443778f 100644 --- a/src/coreclr/vm/siginfo.cpp +++ b/src/coreclr/vm/siginfo.cpp @@ -1103,7 +1103,7 @@ TypeHandle SigPointer::GetTypeHandleThrowing( const Substitution * pSubst/*=NULL*/, // ZapSigContext is only set when decoding zapsigs const ZapSig::Context * pZapSigContext, - MethodTable * pMTInterfaceMapOwner, + TypeHandle thSpecialInterfaceInstantiationType, HandleRecursiveGenericsForFieldLayoutLoad *pRecursiveFieldGenericHandling) const { CONTRACT(TypeHandle) @@ -1133,7 +1133,7 @@ TypeHandle SigPointer::GetTypeHandleThrowing( // Zap sig context must be NULL, as this can only happen in the type loader itself _ASSERTE(pZapSigContext == NULL); // Similarly with the pMTInterfaceMapOwner logic - _ASSERTE(pMTInterfaceMapOwner == NULL); + _ASSERTE(thSpecialInterfaceInstantiationType.IsNull()); // This may throw an exception using the FullModule _ASSERTE(pModule->IsFullModule()); @@ -1589,7 +1589,7 @@ TypeHandle SigPointer::GetTypeHandleThrowing( argDrop, pSubst, pZapSigContext, - NULL, + TypeHandle(), pRecursiveFieldGenericHandling); if (typeHnd.IsNull()) { @@ -1616,7 +1616,7 @@ TypeHandle SigPointer::GetTypeHandleThrowing( Instantiation genericLoadInst(thisinst, ntypars); - if (pMTInterfaceMapOwner != NULL && genericLoadInst.ContainsAllOneType(pMTInterfaceMapOwner)) + if (!thSpecialInterfaceInstantiationType.IsNull() && genericLoadInst.ContainsAllOneType(thSpecialInterfaceInstantiationType)) { thRet = ClassLoader::LoadTypeDefThrowing(pGenericTypeModule, tkGenericType, ClassLoader::ThrowIfNotFound, ClassLoader::PermitUninstDefOrRef, 0, level); } @@ -1637,7 +1637,16 @@ TypeHandle SigPointer::GetTypeHandleThrowing( &instContext, pZapSigContext && pZapSigContext->externalTokens == ZapSig::NormalTokens)); - if (!handlingRecursiveGenericFieldScenario) + if (!thFound.IsNull() && !thSpecialInterfaceInstantiationType.IsNull() && !thFound.IsTypeDesc() && thFound.AsMethodTable()->IsSpecialMarkerTypeForGenericCasting()) + { + // We are trying to load an interface instantiation, and we have a concept of the special marker type enabled, but + // the loaded type is not the expected type we should be looking for to return a special marker type, but the normal load has + // found a type which claims to be a special marker type. In this case return something else (object) to indicate that + // we found an invalid situation and this function should be retried without the special marker type logic enabled. + thRet = TypeHandle(CoreLibBinder::GetElementType(ELEMENT_TYPE_OBJECT)); + break; + } + else if (!handlingRecursiveGenericFieldScenario) { thRet = thFound; break; diff --git a/src/coreclr/vm/siginfo.hpp b/src/coreclr/vm/siginfo.hpp index 7b6dc043d639b9..ec0bce7f58bc0d 100644 --- a/src/coreclr/vm/siginfo.hpp +++ b/src/coreclr/vm/siginfo.hpp @@ -253,7 +253,7 @@ class SigPointer : public SigParser BOOL dropGenericArgumentLevel = FALSE, const Substitution *pSubst = NULL, const ZapSig::Context *pZapSigContext = NULL, - MethodTable *pMTInterfaceMapOwner = NULL, + TypeHandle thSpecialInterfaceInstantiationType = NULL, HandleRecursiveGenericsForFieldLayoutLoad *pRecursiveFieldGenericHandling = NULL ) const; From bef91a89847911921409350c404ff057230e0c5c Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 7 Oct 2025 16:33:23 -0700 Subject: [PATCH 07/17] Fix interface matching bug found in testing --- src/coreclr/vm/methodtable.cpp | 67 ++++++++++++++++++++++++++++++---- src/coreclr/vm/methodtable.h | 4 +- src/coreclr/vm/methodtable.inl | 2 +- 3 files changed, 64 insertions(+), 9 deletions(-) diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 741450b59aa030..bf072c6da893c2 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -1753,6 +1753,63 @@ NOINLINE BOOL MethodTable::ImplementsInterface(MethodTable *pInterface) return ImplementsInterfaceInline(pInterface); } +bool MethodTable::InterfaceMapIterator::CurrentInterfaceEquivalentTo(MethodTable* pMTOwner, MethodTable* pMT) +{ + CONTRACTL + { + THROWS; + GC_TRIGGERS; + PRECONDITION(pMT->IsInterface()); // class we are looking up should be an interface + } + CONTRACTL_END; + + MethodTable *pCurrentMethodTable = m_pMap->GetMethodTable(); + + if (pCurrentMethodTable == pMT) + return true; + + if (pCurrentMethodTable->IsSpecialMarkerTypeForGenericCasting() && !pMTOwner->GetAuxiliaryData()->MayHaveOpenInterfacesInInterfaceMap() && pCurrentMethodTable->HasSameTypeDefAs(pMT)) + { + // Any matches need to use the special marker type logic + if (!pMTOwner->GetAuxiliaryData()->MayHaveOpenInterfacesInInterfaceMap()) + { + TypeHandle pSpecialInstantiationType = pCurrentMethodTable->GetSpecialInstantiationType(); + // If we reach here, we are trying to do a compare with a value in the interface map which is a special marker type + // First check for exact match. + if (pMT->GetInstantiation().ContainsAllOneType(pSpecialInstantiationType)) + { + // 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 +#ifndef DACCESS_COMPILE + if (pMT->IsFullyLoaded()) + SetInterface(pMT); +#endif + return true; + } + else + { + // We don't match exactly, but we may still be equivalent + for (DWORD i = 0; i < pMT->GetNumGenericArgs(); i++) + { + TypeHandle arg = pMT->GetInstantiation()[i]; + if (!arg.IsEquivalentTo(pSpecialInstantiationType)) + { + return false; + } + } + return true; + } + } + return false; + } + else + { + // We don't have a special marker type in the interface map, so we need to do the normal equivalence check + return pCurrentMethodTable->IsEquivalentTo(pMT); + } +} + //========================================================================================== BOOL MethodTable::ImplementsEquivalentInterface(MethodTable *pInterface) { @@ -1778,16 +1835,12 @@ BOOL MethodTable::ImplementsEquivalentInterface(MethodTable *pInterface) if (numInterfaces == 0) return FALSE; - InterfaceInfo_t *pInfo = GetInterfaceMap(); - - do + InterfaceMapIterator it = IterateInterfaceMap(); + while (it.Next()) { - if (pInfo->GetMethodTable()->IsEquivalentTo(pInterface)) + if (it.CurrentInterfaceEquivalentTo(this, pInterface)) return TRUE; - - pInfo++; } - while (--numInterfaces); return FALSE; } diff --git a/src/coreclr/vm/methodtable.h b/src/coreclr/vm/methodtable.h index d4cdb93488a97b..99311d9e03dca8 100644 --- a/src/coreclr/vm/methodtable.h +++ b/src/coreclr/vm/methodtable.h @@ -2296,7 +2296,7 @@ class MethodTable pMT->HasInstantiation() && pCurrentMethodTable->IsSpecialMarkerTypeForGenericCasting() && !pMTOwner->GetAuxiliaryData()->MayHaveOpenInterfacesInInterfaceMap() && - pMT->GetInstantiation().ContainsAllOneType(pMTOwner)) + pMT->GetInstantiation().ContainsAllOneType(pMTOwner->GetSpecialInstantiationType())) { exactMatch = true; #ifndef DACCESS_COMPILE @@ -2312,6 +2312,8 @@ class MethodTable RETURN (exactMatch); } + bool CurrentInterfaceEquivalentTo(MethodTable* pMTOwner, MethodTable* pMT); + inline bool HasSameTypeDefAs(MethodTable* pMT) { CONTRACT(bool) diff --git a/src/coreclr/vm/methodtable.inl b/src/coreclr/vm/methodtable.inl index 6bdf3aa40a0bc2..ce8ad69210eb6b 100644 --- a/src/coreclr/vm/methodtable.inl +++ b/src/coreclr/vm/methodtable.inl @@ -1339,7 +1339,7 @@ FORCEINLINE BOOL MethodTable::ImplementsInterfaceInline(MethodTable *pInterface) while (--numInterfaces); // Second scan, looking for the curiously recurring generic scenario - if (pInterface->HasInstantiation() && !GetAuxiliaryData()->MayHaveOpenInterfacesInInterfaceMap() && pInterface->GetInstantiation().ContainsAllOneType(this)) + if (pInterface->HasInstantiation() && !GetAuxiliaryData()->MayHaveOpenInterfacesInInterfaceMap() && pInterface->GetInstantiation().ContainsAllOneType(this->GetSpecialInstantiationType())) { numInterfaces = GetNumInterfaces(); pInfo = GetInterfaceMap(); From 2bdfc3ce772cdc29ed63efe06334fb53940c9382 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 8 Oct 2025 09:36:23 -0700 Subject: [PATCH 08/17] Optimize virtual static and default interface method resolution logic - Tweak the logic so that it will attempt to avoid loading types if they are cannot have implementations of the method we're looking for --- src/coreclr/vm/methodtable.cpp | 164 +++++++++++++++----------- src/coreclr/vm/methodtablebuilder.cpp | 3 + 2 files changed, 99 insertions(+), 68 deletions(-) diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index bf072c6da893c2..165d38cc649cd1 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -5752,6 +5752,27 @@ namespace } } +// Looking only at the typedef details of pMT, determine if it might have a candidate implementation +bool InterfaceMayHaveCandidateImplementation(MethodTable *pMT, MethodDesc *pInterfaceMD) +{ + LIMITED_METHOD_CONTRACT; + + MethodTable *pInterfaceMT = pInterfaceMD->GetMethodTable(); + + // If a method is defined on pMT and isn't abstract, then it might have a default implementation on that type if it isn't abstract + if (pMT->HasSameTypeDefAs(pInterfaceMT)) + { + return !pInterfaceMD->IsAbstract(); + } + + // If a pMT has MethodImpl records then its possible that it could override the interface method + if (pMT->GetClass()->ContainsMethodImpls()) + return true; + + // Otherwise the type pMT cannot possibly have a candidate implementation for pInterfaceMD + return false; +} + // Find the default interface implementation method for interface dispatch // It is either the interface method with default interface method implementation, // or an most specific interface with an explicit methodimpl overriding the method @@ -5785,7 +5806,7 @@ BOOL MethodTable::FindDefaultInterfaceImplementation( // Check the current method table itself MethodDesc *candidateMaybe = NULL; - if (IsInterface() && TryGetCandidateImplementation(this, pInterfaceMD, pInterfaceMT, findDefaultImplementationFlags, &candidateMaybe, level)) + if (IsInterface() && InterfaceMayHaveCandidateImplementation(this, pInterfaceMD) && TryGetCandidateImplementation(this, pInterfaceMD, pInterfaceMT, findDefaultImplementationFlags, &candidateMaybe, level)) { _ASSERTE(candidateMaybe != NULL); @@ -5822,74 +5843,77 @@ BOOL MethodTable::FindDefaultInterfaceImplementation( MethodTable::InterfaceMapIterator it = pMT->IterateInterfaceMapFrom(dwParentInterfaces); while (!it.Finished()) { - MethodTable *pCurMT = it.GetInterface(pMT, level); - - MethodDesc *pCurMD = NULL; - if (TryGetCandidateImplementation(pCurMT, pInterfaceMD, pInterfaceMT, findDefaultImplementationFlags, &pCurMD, level)) + if (InterfaceMayHaveCandidateImplementation(it.GetInterfaceApprox(), pInterfaceMD)) { - // - // Found a match. But is it a more specific match (we want most specific interfaces) - // - _ASSERTE(pCurMD != NULL); - bool needToInsert = true; - bool seenMoreSpecific = false; + MethodTable *pCurMT = it.GetInterface(pMT, level); - // We need to maintain the invariant that the candidates are always the most specific - // in all path scaned so far. There might be multiple incompatible candidates - for (unsigned i = 0; i < candidatesCount; ++i) + MethodDesc *pCurMD = NULL; + if (TryGetCandidateImplementation(pCurMT, pInterfaceMD, pInterfaceMT, findDefaultImplementationFlags, &pCurMD, level)) { - MethodTable *pCandidateMT = candidates[i].pMT; - if (pCandidateMT == NULL) - continue; - - if (pCandidateMT == pCurMT) + // + // Found a match. But is it a more specific match (we want most specific interfaces) + // + _ASSERTE(pCurMD != NULL); + bool needToInsert = true; + bool seenMoreSpecific = false; + + // We need to maintain the invariant that the candidates are always the most specific + // in all path scaned so far. There might be multiple incompatible candidates + for (unsigned i = 0; i < candidatesCount; ++i) { - // A dup - we are done - needToInsert = false; - break; - } + MethodTable *pCandidateMT = candidates[i].pMT; + if (pCandidateMT == NULL) + continue; - if (allowVariance && pCandidateMT->HasSameTypeDefAs(pCurMT)) - { - // Variant match on the same type - this is a tie - } - else if (pCurMT->CanCastToInterface(pCandidateMT)) - { - // pCurMT is a more specific choice than IFoo/IBar both overrides IBlah : - if (!seenMoreSpecific) + if (pCandidateMT == pCurMT) { - seenMoreSpecific = true; - candidates[i].pMT = pCurMT; - candidates[i].pMD = pCurMD; + // A dup - we are done + needToInsert = false; + break; } - else + + if (allowVariance && pCandidateMT->HasSameTypeDefAs(pCurMT)) { - candidates[i].pMT = NULL; - candidates[i].pMD = NULL; + // Variant match on the same type - this is a tie } + else if (pCurMT->CanCastToInterface(pCandidateMT)) + { + // pCurMT is a more specific choice than IFoo/IBar both overrides IBlah : + if (!seenMoreSpecific) + { + seenMoreSpecific = true; + candidates[i].pMT = pCurMT; + candidates[i].pMD = pCurMD; + } + else + { + candidates[i].pMT = NULL; + candidates[i].pMD = NULL; + } - needToInsert = false; - } - else if (pCandidateMT->CanCastToInterface(pCurMT)) - { - // pCurMT is less specific - we don't need to scan more entries as this entry can - // represent pCurMT (other entries are incompatible with pCurMT) - needToInsert = false; - break; + needToInsert = false; + } + else if (pCandidateMT->CanCastToInterface(pCurMT)) + { + // pCurMT is less specific - we don't need to scan more entries as this entry can + // represent pCurMT (other entries are incompatible with pCurMT) + needToInsert = false; + break; + } + else + { + // pCurMT is incompatible - keep scanning + } } - else + + if (needToInsert) { - // pCurMT is incompatible - keep scanning + ASSERT(candidatesCount < candidates.Size()); + candidates[candidatesCount].pMT = pCurMT; + candidates[candidatesCount].pMD = pCurMD; + candidatesCount++; } } - - if (needToInsert) - { - ASSERT(candidatesCount < candidates.Size()); - candidates[candidatesCount].pMT = pCurMT; - candidates[candidatesCount].pMD = pCurMD; - candidatesCount++; - } } it.Next(); @@ -8298,6 +8322,21 @@ MethodTable::TryResolveVirtualStaticMethodOnThisType(MethodTable* pInterfaceType // entries, let's reset the count and just break out. (Should we throw?) break; } + + LPCUTF8 szMember = NULL; + PCCOR_SIGNATURE pSigMember = NULL; + DWORD cSigMember = 0; + if (TypeFromToken(methodDecl) == mdtMemberRef) + { + IfFailThrow(pMDInternalImport->GetNameAndSigOfMemberRef(methodDecl, &pSigMember, &cSigMember, &szMember)); + + // Do a quick name check to avoid excess use of FindMethod and the type load below + if (strcmp(szMember, pInterfaceMD->GetName()) != 0) + { + continue; + } + } + mdToken tkParent; hr = pMDInternalImport->GetParentToken(methodDecl, &tkParent); if (FAILED(hr)) @@ -8344,19 +8383,8 @@ MethodTable::TryResolveVirtualStaticMethodOnThisType(MethodTable* pInterfaceType } else if (TypeFromToken(methodDecl) == mdtMemberRef) { - LPCUTF8 szMember; - PCCOR_SIGNATURE pSig; - DWORD cSig; - - IfFailThrow(pMDInternalImport->GetNameAndSigOfMemberRef(methodDecl, &pSig, &cSig, &szMember)); - - // Do a quick name check to avoid excess use of FindMethod - if (strcmp(szMember, pInterfaceMD->GetName()) != 0) - { - continue; - } - - pMethodDecl = MemberLoader::FindMethod(pInterfaceMT, szMember, pSig, cSig, GetModule()); + // We've already gotten the szMember, pSigMember, and cSigMember as a result of the early out check above. + pMethodDecl = MemberLoader::FindMethod(pInterfaceMT, szMember, pSigMember, cSigMember, GetModule()); } else { diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 34519e19d5fc40..60bab20d28881c 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -2237,6 +2237,9 @@ MethodTableBuilder::EnumerateMethodImpls() bmtMethod->dwNumberMethodImpls = hEnumMethodImpl.EnumMethodImplGetCount(); bmtMethod->dwNumberInexactMethodImplCandidates = 0; + if (bmtMethod->dwNumberMethodImpls != 0) + GetHalfBakedClass()->SetContainsMethodImpls(); + // This is the first pass. In this we will simply enumerate the token pairs and fill in // the data structures. In addition, we'll sort the list and eliminate duplicates. if (bmtMethod->dwNumberMethodImpls > 0) From 3b937fdf540fa29ad70090445fe83b92f30fcd72 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 8 Oct 2025 14:53:18 -0700 Subject: [PATCH 09/17] Update loader/classloader/generics/ByRefLike/ValidateNegative test - This test was no longer actually forcing the type which is supposed to throw a TypeLoadException to load within the test - Tweak the test to actually force the offending type to be loaded --- .../classloader/generics/ByRefLike/InvalidCSharpNegative.il | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tests/Loader/classloader/generics/ByRefLike/InvalidCSharpNegative.il b/src/tests/Loader/classloader/generics/ByRefLike/InvalidCSharpNegative.il index 08767e372ea460..eb5649cf33470e 100644 --- a/src/tests/Loader/classloader/generics/ByRefLike/InvalidCSharpNegative.il +++ b/src/tests/Loader/classloader/generics/ByRefLike/InvalidCSharpNegative.il @@ -106,6 +106,7 @@ { ldtoken class InvalidCSharpNegative.GenericDerivedInterface_Invalid`1 call class [System.Runtime]System.Type [System.Runtime]System.Type::GetTypeFromHandle(valuetype [System.Runtime]System.RuntimeTypeHandle) + callvirt instance class [System.Runtime]System.Type[] [System.Runtime]System.Type::GetInterfaces() callvirt instance string [System.Runtime]System.Object::ToString() ret } From 64d10ff155b4e2a8fbd3eb38cd2c91bda6e91254 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 8 Oct 2025 16:38:52 -0700 Subject: [PATCH 10/17] Pass around the type I want passed around --- src/coreclr/vm/clsload.cpp | 4 ++-- src/coreclr/vm/clsload.hpp | 2 +- src/coreclr/vm/methodtablebuilder.cpp | 5 +---- src/coreclr/vm/siginfo.cpp | 10 +++++----- src/coreclr/vm/siginfo.hpp | 2 +- 5 files changed, 10 insertions(+), 13 deletions(-) diff --git a/src/coreclr/vm/clsload.cpp b/src/coreclr/vm/clsload.cpp index 7a49c4e11a91f3..72ca527baa01c4 100644 --- a/src/coreclr/vm/clsload.cpp +++ b/src/coreclr/vm/clsload.cpp @@ -1755,7 +1755,7 @@ TypeHandle ClassLoader::LoadTypeDefOrRefOrSpecThrowing(Module *pModule, ClassLoadLevel level /* = CLASS_LOADED */, BOOL dropGenericArgumentLevel /* = FALSE */, const Substitution *pSubst, - TypeHandle thSpecialInterfaceInstantiationType) + MethodTable *pMTInterfaceMapOwner) { CONTRACT(TypeHandle) { @@ -1789,7 +1789,7 @@ TypeHandle ClassLoader::LoadTypeDefOrRefOrSpecThrowing(Module *pModule, } SigPointer sigptr(pSig, cSig); TypeHandle typeHnd = sigptr.GetTypeHandleThrowing(pModule, pTypeContext, fLoadTypes, - level, dropGenericArgumentLevel, pSubst, (const ZapSig::Context *)0, thSpecialInterfaceInstantiationType); + level, dropGenericArgumentLevel, pSubst, (const ZapSig::Context *)0, pMTInterfaceMapOwner); #ifndef DACCESS_COMPILE if ((fNotFoundAction == ThrowIfNotFound) && typeHnd.IsNull()) pModule->GetAssembly()->ThrowTypeLoadException(pInternalImport, typeDefOrRefOrSpec, diff --git a/src/coreclr/vm/clsload.hpp b/src/coreclr/vm/clsload.hpp index 9e494369f0ce0b..c50dd4a5fc7987 100644 --- a/src/coreclr/vm/clsload.hpp +++ b/src/coreclr/vm/clsload.hpp @@ -656,7 +656,7 @@ class ClassLoader ClassLoadLevel level = CLASS_LOADED, BOOL dropGenericArgumentLevel = FALSE, const Substitution *pSubst = NULL /* substitution to apply if the token is a type spec with generic variables */, - TypeHandle thSpecialInterfaceInstantiationType = TypeHandle()); + MethodTable *pMTInterfaceMapOwner = NULL); // Load constructed types by providing their constituents static TypeHandle LoadPointerOrByrefTypeThrowing(CorElementType typ, diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 60bab20d28881c..4e8e11bbe639b5 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -9652,9 +9652,6 @@ MethodTableBuilder::LoadExactInterfaceMap(MethodTable *pMT) // Always use exact loading behavior with normal classes or shared generics, as they have to deal with inheritance, and the // inexact matching logic for classes would be more complex to write. - // Also always use the exact loading behavior with any generic that contains generic variables, as the open type is used - // to represent a type instantiated over its own generic variables, and the special marker type is currently the open type - // and we make this case distinguishable by simply disallowing the optimization in those cases. bool retryWithExactInterfaces = !(pMT->IsValueType() || pMT->IsInterface()) || pMT->IsSharedByGenericInstantiations(); if (retryWithExactInterfaces) { @@ -9687,7 +9684,7 @@ MethodTableBuilder::LoadExactInterfaceMap(MethodTable *pMT) CLASS_LOAD_EXACTPARENTS, TRUE, (const Substitution*)0, - retryWithExactInterfaces ? NULL : pMT->GetSpecialInstantiationType()).GetMethodTable(); + retryWithExactInterfaces ? NULL : pMT).GetMethodTable(); // When checking to possibly load the special instantiation type, if we load a type which ISN'T the type instantiatiated over the special instantiation type, but it IS IsSpecialMarkerTypeForGenericCasting // the ClassLoader::LoadTypeDefOrRefOrSpecThrowing function will return System.Object's MT, and we need to detect that, and abort use of the special path. diff --git a/src/coreclr/vm/siginfo.cpp b/src/coreclr/vm/siginfo.cpp index ad8a84c443778f..023b85030f0e0a 100644 --- a/src/coreclr/vm/siginfo.cpp +++ b/src/coreclr/vm/siginfo.cpp @@ -1103,7 +1103,7 @@ TypeHandle SigPointer::GetTypeHandleThrowing( const Substitution * pSubst/*=NULL*/, // ZapSigContext is only set when decoding zapsigs const ZapSig::Context * pZapSigContext, - TypeHandle thSpecialInterfaceInstantiationType, + MethodTable* pMTInterfaceMapOwner, HandleRecursiveGenericsForFieldLayoutLoad *pRecursiveFieldGenericHandling) const { CONTRACT(TypeHandle) @@ -1133,7 +1133,7 @@ TypeHandle SigPointer::GetTypeHandleThrowing( // Zap sig context must be NULL, as this can only happen in the type loader itself _ASSERTE(pZapSigContext == NULL); // Similarly with the pMTInterfaceMapOwner logic - _ASSERTE(thSpecialInterfaceInstantiationType.IsNull()); + _ASSERTE(pMTInterfaceMapOwner == NULL); // This may throw an exception using the FullModule _ASSERTE(pModule->IsFullModule()); @@ -1589,7 +1589,7 @@ TypeHandle SigPointer::GetTypeHandleThrowing( argDrop, pSubst, pZapSigContext, - TypeHandle(), + NULL, pRecursiveFieldGenericHandling); if (typeHnd.IsNull()) { @@ -1616,7 +1616,7 @@ TypeHandle SigPointer::GetTypeHandleThrowing( Instantiation genericLoadInst(thisinst, ntypars); - if (!thSpecialInterfaceInstantiationType.IsNull() && genericLoadInst.ContainsAllOneType(thSpecialInterfaceInstantiationType)) + if (pMTInterfaceMapOwner != NULL && genericLoadInst.ContainsAllOneType(pMTInterfaceMapOwner->GetSpecialInstantiationType())) { thRet = ClassLoader::LoadTypeDefThrowing(pGenericTypeModule, tkGenericType, ClassLoader::ThrowIfNotFound, ClassLoader::PermitUninstDefOrRef, 0, level); } @@ -1637,7 +1637,7 @@ TypeHandle SigPointer::GetTypeHandleThrowing( &instContext, pZapSigContext && pZapSigContext->externalTokens == ZapSig::NormalTokens)); - if (!thFound.IsNull() && !thSpecialInterfaceInstantiationType.IsNull() && !thFound.IsTypeDesc() && thFound.AsMethodTable()->IsSpecialMarkerTypeForGenericCasting()) + if (!thFound.IsNull() && pMTInterfaceMapOwner != NULL && !thFound.IsTypeDesc() && thFound.AsMethodTable()->IsSpecialMarkerTypeForGenericCasting()) { // We are trying to load an interface instantiation, and we have a concept of the special marker type enabled, but // the loaded type is not the expected type we should be looking for to return a special marker type, but the normal load has diff --git a/src/coreclr/vm/siginfo.hpp b/src/coreclr/vm/siginfo.hpp index ec0bce7f58bc0d..fa063529d4be10 100644 --- a/src/coreclr/vm/siginfo.hpp +++ b/src/coreclr/vm/siginfo.hpp @@ -253,7 +253,7 @@ class SigPointer : public SigParser BOOL dropGenericArgumentLevel = FALSE, const Substitution *pSubst = NULL, const ZapSig::Context *pZapSigContext = NULL, - TypeHandle thSpecialInterfaceInstantiationType = NULL, + MethodTable* pMTInterfaceMapOwner = NULL, HandleRecursiveGenericsForFieldLayoutLoad *pRecursiveFieldGenericHandling = NULL ) const; From 9eef0b3dcdcfb6ec28f1d021479b7577aa8e14f9 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 8 Oct 2025 16:46:45 -0700 Subject: [PATCH 11/17] Remove WhichConstraintsToLoad::TypeOrMethodVarsOnly --- src/coreclr/vm/typedesc.cpp | 32 +++++++++++++------------------- src/coreclr/vm/typedesc.h | 8 ++++---- 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/src/coreclr/vm/typedesc.cpp b/src/coreclr/vm/typedesc.cpp index 0a87e9caa01982..99c872ec2b0dc5 100644 --- a/src/coreclr/vm/typedesc.cpp +++ b/src/coreclr/vm/typedesc.cpp @@ -719,11 +719,11 @@ TypeHandle* TypeVarTypeDesc::GetCachedConstraints(DWORD *pNumConstraints, WhichC PRECONDITION(CheckPointer(pNumConstraints)); DWORD numConstraints = m_numConstraintsWithFlags; - WhichConstraintsToLoad whichCurrent = (WhichConstraintsToLoad)(numConstraints >> WhichShift); + WhichConstraintsToLoad whichCurrent = (WhichConstraintsToLoad)(numConstraints >> WhichConstraintsLoadedShift); bool prevWhichInsufficient = whichCurrent > which; _ASSERTE(!prevWhichInsufficient); - *pNumConstraints = numConstraints & ~WhichMask; + *pNumConstraints = numConstraints & ~WhichConstraintsLoadedMask; return m_constraints; } @@ -741,13 +741,13 @@ TypeHandle* TypeVarTypeDesc::GetConstraints(DWORD *pNumConstraints, ClassLoadLev _ASSERTE(level == CLASS_DEPENDENCIES_LOADED || level == CLASS_LOADED); DWORD numConstraints = m_numConstraintsWithFlags; - WhichConstraintsToLoad whichCurrent = (WhichConstraintsToLoad)(numConstraints >> WhichShift); + WhichConstraintsToLoad whichCurrent = (WhichConstraintsToLoad)(numConstraints >> WhichConstraintsLoadedShift); bool prevWhichInsufficient = whichCurrent > which; if (prevWhichInsufficient) LoadConstraints(level, which); - *pNumConstraints = m_numConstraintsWithFlags & ~WhichMask; + *pNumConstraints = m_numConstraintsWithFlags & ~WhichConstraintsLoadedMask; return m_constraints; } @@ -776,17 +776,16 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level, WhichConstraintsToLo // NOTE: // The WhichConstraintsToLoad enum is ordered from most constraints to least constraints, so we can use a simple greater-than comparison to determine // if the previously loaded constraints are sufficient for the current request. - // There are 4 possible values for WhichConstraintsToLoad: + // There are 3 possible values for WhichConstraintsToLoad: // All - Load all constraints // TypeOrMethodVarsAndNonInterfacesOnly - Load all constraints except interface constraints that are not type or method variables. The code MAY load other constraints. This is used when loading constraints for the purpose of type safety checks. - // TypeOrMethodVarsOnly - Load only type or method variables. (Needed for the Bounded algorithm) // None - Load no constraints. This is the initial state. do { numConstraints = m_numConstraintsWithFlags; DWORD initialNumConstraints = numConstraints; - WhichConstraintsToLoad whichCurrent = (WhichConstraintsToLoad)(numConstraints >> WhichShift); + WhichConstraintsToLoad whichCurrent = (WhichConstraintsToLoad)(numConstraints >> WhichConstraintsLoadedShift); bool prevWhichInsufficient = whichCurrent > which; if (!prevWhichInsufficient) @@ -834,7 +833,7 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level, WhichConstraintsToLo numConstraints = pInternalImport->EnumGetCount(&hEnum); if (numConstraints != 0) { - numConstraints = (numConstraints & ~WhichMask) | (((DWORD)which) << WhichShift); + numConstraints = (numConstraints & ~WhichConstraintsLoadedMask) | (((DWORD)which) << WhichConstraintsLoadedShift); LoaderAllocator* pAllocator = GetModule()->GetLoaderAllocator(); // If there is a single class constraint we place it at index 0 of the array @@ -843,7 +842,7 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level, WhichConstraintsToLo if (whichCurrent == WhichConstraintsToLoad::None) { - constraintAlloc = (pAllocator->GetLowFrequencyHeap()->AllocMem(S_SIZE_T(numConstraints & ~WhichMask) * S_SIZE_T(sizeof(TypeHandle)))); + constraintAlloc = (pAllocator->GetLowFrequencyHeap()->AllocMem(S_SIZE_T(numConstraints & ~WhichConstraintsLoadedMask) * S_SIZE_T(sizeof(TypeHandle)))); constraints = (TypeHandle*)constraintAlloc; } else @@ -867,6 +866,7 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level, WhichConstraintsToLo bool loadConstraint; if (TypeFromToken(tkConstraintType) == mdtTypeSpec && which != WhichConstraintsToLoad::All) { + _ASSERTE(which == WhichConstraintsToLoad::TypeOrMethodVarsAndNonInterfacesOnly); ULONG cSig; PCCOR_SIGNATURE pSig; @@ -883,10 +883,8 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level, WhichConstraintsToLo // We can always load variable constraints loadConstraint = true; } - else if (which != WhichConstraintsToLoad::TypeOrMethodVarsOnly) + else { - _ASSERTE(which == WhichConstraintsToLoad::TypeOrMethodVarsAndNonInterfacesOnly); - if (elemType != ELEMENT_TYPE_GENERICINST) { // We don't know if its a class or interface, but it isn't generic, so finding out is the same as loading @@ -919,10 +917,6 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level, WhichConstraintsToLo } } } - else - { - loadConstraint = false; - } } else { @@ -984,7 +978,7 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level, WhichConstraintsToLo if (loadedAllConstraints) { // We loaded all the constraints, so we can update the number we're going to store to indicate that we're storing all of them - numConstraints = numConstraints & ~WhichMask; + numConstraints = numConstraints & ~WhichConstraintsLoadedMask; } } @@ -996,7 +990,7 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level, WhichConstraintsToLo break; } while (true); - for (DWORD i = 0; i < (numConstraints & ~WhichMask); i++) + for (DWORD i = 0; i < (numConstraints & ~WhichConstraintsLoadedMask); i++) { TypeHandle constraint = m_constraints[i]; if (constraint.IsNull()) @@ -1824,7 +1818,7 @@ TypeVarTypeDesc::EnumMemoryRegions(CLRDataEnumMemoryFlags flags) if (m_numConstraintsWithFlags != (DWORD)-1) { PTR_TypeHandle constraint = m_constraints; - for (DWORD i = 0; i < (m_numConstraintsWithFlags & ~WhichMask); i++) + for (DWORD i = 0; i < (m_numConstraintsWithFlags & ~WhichConstraintsLoadedMask); i++) { if (constraint.IsValid() && !constraint->IsNull()) { diff --git a/src/coreclr/vm/typedesc.h b/src/coreclr/vm/typedesc.h index 8636a253c6c188..da2dffb380209e 100644 --- a/src/coreclr/vm/typedesc.h +++ b/src/coreclr/vm/typedesc.h @@ -287,7 +287,7 @@ enum class WhichConstraintsToLoad { All = 0, TypeOrMethodVarsAndNonInterfacesOnly = 1, - TypeOrMethodVarsOnly = 2, + Invalid = 2, None = 3, }; @@ -299,8 +299,8 @@ enum class WhichConstraintsToLoad class TypeVarTypeDesc : public TypeDesc { - static const DWORD WhichMask = 0xC0000000; - static const DWORD WhichShift = 30; + static const DWORD WhichConstraintsLoadedMask = 0xC0000000; + static const DWORD WhichConstraintsLoadedShift = 30; public: #ifndef DACCESS_COMPILE @@ -364,7 +364,7 @@ class TypeVarTypeDesc : public TypeDesc MethodDesc * LoadOwnerMethod(); TypeHandle LoadOwnerType(); - BOOL ConstraintsLoaded(WhichConstraintsToLoad which) { LIMITED_METHOD_CONTRACT; if (((m_numConstraintsWithFlags & WhichMask) >> WhichShift) <= (DWORD)which) return TRUE; return FALSE; } + BOOL ConstraintsLoaded(WhichConstraintsToLoad which) { LIMITED_METHOD_CONTRACT; if (((m_numConstraintsWithFlags & WhichConstraintsLoadedMask) >> WhichConstraintsLoadedShift) <= (DWORD)which) return TRUE; return FALSE; } // Return NULL if no constraints are specified // Return an array of type handles if constraints are specified, From 4a6a81cef2c9c00c25698d2e3f70dc5e2ea0b0c2 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 8 Oct 2025 16:56:54 -0700 Subject: [PATCH 12/17] Fix out of date comment --- src/coreclr/vm/methodtable.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 165d38cc649cd1..a7eaee911c8a36 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -4761,8 +4761,6 @@ void MethodTable::DoFullyLoad(Generics::RecursionGraph * const pVisited, const COMPlusThrow(kTypeLoadException, VER_E_CIRCULAR_VAR_CONSTRAINTS); } - // For typical type definitions, we have to load the constraints here because they may not - // have been loaded yet. In principle we could change DoAccessibilityCheckForConstraints to walk the typedefs/refs in the constraint instead DoAccessibilityCheckForConstraints(this, pTyVar, E_ACCESSDENIED); } } From 533e8cad82ff18eda4115d0685175331b0be6db6 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Fri, 10 Oct 2025 15:23:39 -0700 Subject: [PATCH 13/17] - Add support for doing circularity checks on type and method generic parameters to the TypeValidationChecker in crossgen2 - Add support for doing variance safety checks to the type loader in crossgen2 - Remove the circularity of type variables checks happening in generic method load, as it is redundant - Put all of the variance safety checks in the runtime under the SkipTypeValidation flag as crossgen2 can now do it reliably --- .../ReadyToRun/TypeValidationChecker.cs | 172 +++++++++++++++++- src/coreclr/vm/class.cpp | 2 +- src/coreclr/vm/genmeth.cpp | 15 +- src/coreclr/vm/method.hpp | 5 +- src/coreclr/vm/methodtable.cpp | 7 +- src/coreclr/vm/methodtablebuilder.cpp | 7 +- src/coreclr/vm/typedesc.cpp | 7 +- 7 files changed, 186 insertions(+), 29 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs index 0f4cfe2b0a0c97..7eac27c50ae63b 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using System.Reflection; using System.Text; @@ -133,6 +134,18 @@ Task ValidateTypeWorkerHelper(TypeDesc typeToCheckForSkipValidation) AddTypeValidationError(type, $"Interface type {interfaceType} failed validation"); return false; } + + if (!interfaceType.IsInterface) + { + AddTypeValidationError(type, $"Runtime interface {interfaceType} is not an interface"); + return false; + } + + if (interfaceType.HasInstantiation) + { + // Interfaces behave covariantly + ValidateVarianceInType(type.Instantiation, interfaceType, Internal.TypeSystem.GenericVariance.Covariant); + } } // Validate that each interface type explicitly implemented on this type is accessible to this type -- UNIMPLEMENTED @@ -293,14 +306,59 @@ Task ValidateTypeWorkerHelper(TypeDesc typeToCheckForSkipValidation) AddTypeValidationError(type, $"'{method}' is an runtime-impl generic method"); return false; } - // Validate that generic variance is properly respected in method signatures -- UNIMPLEMENTED - // Validate that there are no cyclical method constraints -- UNIMPLEMENTED + + + // Validate that generic variance is properly respected in method signatures + if (type.IsInterface && method.IsVirtual && !method.Signature.IsStatic && type.HasInstantiation) + { + if (!ValidateVarianceInSignature(type.Instantiation, method.Signature, Internal.TypeSystem.GenericVariance.Covariant, Internal.TypeSystem.GenericVariance.Contravariant)) + { + AddTypeValidationError(type, $"'{method}' has invalid variance in signature"); + return false; + } + } + + foreach (var genericParameterType in method.Instantiation) + { + foreach (var constraint in ((GenericParameterDesc)genericParameterType).TypeConstraints) + { + if (!ValidateVarianceInType(type.Instantiation, constraint, Internal.TypeSystem.GenericVariance.Contravariant)) + { + AddTypeValidationError(type, $"'{method}' has invalid variance in generic parameter constraint {constraint} on type {genericParameterType}"); + return false; + } + } + } + + if (!BoundedCheckForInstantiation(method.Instantiation)) + { + AddTypeValidationError(type, $"'{method}' has cyclical generic parameter constraints"); + return false; + } // Validate that constraints are all acccessible to the method using them -- UNIMPLEMENTED } // Generic class special rules // Validate that a generic class cannot be a ComImport class, or a ComEventInterface class -- UNIMPLEMENTED - // Validate that there are no cyclical class or method constraints, and that constraints are all acccessible to the type using them -- UNIMPLEMENTED + + foreach (var genericParameterType in type.Instantiation) + { + foreach (var constraint in ((GenericParameterDesc)genericParameterType).TypeConstraints) + { + if (!ValidateVarianceInType(type.Instantiation, constraint, Internal.TypeSystem.GenericVariance.Contravariant)) + { + AddTypeValidationError(type, $"'{genericParameterType}' has invalid variance in generic parameter constraint {constraint}"); + return false; + } + } + } + + // Validate that there are no cyclical class or method constraints, and that constraints are all acccessible to the type using them + if (!BoundedCheckForInstantiation(type.Instantiation)) + { + AddTypeValidationError(type, $"'{type}' has cyclical generic parameter constraints"); + return false; + } // Override rules // Validate that each override results does not violate accessibility rules -- UNIMPLEMENTED @@ -579,6 +637,114 @@ async Task ValidateTypeHelperFunctionPointerType(FunctionPointerType funct } return true; } + + static bool BoundedCheckForInstantiation(Instantiation instantiation) + { + foreach (var type in instantiation) + { + Debug.Assert(type.IsGenericParameter); + GenericParameterDesc genericParameter = (GenericParameterDesc)type; + + if (!BoundedCheckForType(genericParameter, instantiation.Length)) + return false; + } + return true; + } + + static bool BoundedCheckForType(GenericParameterDesc genericParameter, int maxDepth) + { + if (maxDepth <= 0) + return false; + foreach (var type in genericParameter.TypeConstraints) + { + if (type is GenericParameterDesc possiblyCircularGenericParameter) + { + if (possiblyCircularGenericParameter.Kind == genericParameter.Kind) + { + if (!BoundedCheckForType(possiblyCircularGenericParameter, maxDepth - 1)) + return false; + } + } + } + return true; + } + + static bool ValidateVarianceInSignature(Instantiation associatedTypeInstantiation, MethodSignature signature, Internal.TypeSystem.GenericVariance returnTypePosition, Internal.TypeSystem.GenericVariance argTypePosition) + { + for (int i = 0; i < signature.Length; i++) + { + if (!ValidateVarianceInType(associatedTypeInstantiation, signature[i], argTypePosition)) + return false; + } + + if (!ValidateVarianceInType(associatedTypeInstantiation, signature.ReturnType, returnTypePosition)) + return false; + + return true; + } + + static bool ValidateVarianceInType(Instantiation associatedTypeInstantiation, TypeDesc type, Internal.TypeSystem.GenericVariance position) + { + if (type is SignatureTypeVariable signatureTypeVar) + { + GenericParameterDesc gp = (GenericParameterDesc)associatedTypeInstantiation[signatureTypeVar.Index]; + if (gp.Variance != Internal.TypeSystem.GenericVariance.None) + { + if (position != gp.Variance) + { + // Covariant and contravariant parameters can *only* appear in resp. covariant and contravariant positions + return false; + } + } + } + else if (type is InstantiatedType instantiatedType) + { + var typeDef = instantiatedType.GetTypeDefinition(); + if (typeDef.IsValueType || position == Internal.TypeSystem.GenericVariance.None) + { + // Value types and non-variant positions require all parameters to be non-variant + foreach (TypeDesc instType in instantiatedType.Instantiation) + { + if (!ValidateVarianceInType(associatedTypeInstantiation, instType, Internal.TypeSystem.GenericVariance.None)) + return false; + } + } + else + { + int index = 0; + for (index = 0; index < typeDef.Instantiation.Length; index++) + { + Internal.TypeSystem.GenericVariance positionForParameter = ((GenericParameterDesc)typeDef.Instantiation[index]).Variance; + // If the surrounding context is contravariant then we need to flip the variance of this parameter + if (position == Internal.TypeSystem.GenericVariance.Contravariant) + { + if (positionForParameter == Internal.TypeSystem.GenericVariance.Covariant) + positionForParameter = Internal.TypeSystem.GenericVariance.Contravariant; + else if (positionForParameter == Internal.TypeSystem.GenericVariance.Contravariant) + positionForParameter = Internal.TypeSystem.GenericVariance.Covariant; + else + positionForParameter = Internal.TypeSystem.GenericVariance.None; + } + + if (!ValidateVarianceInType(associatedTypeInstantiation, instantiatedType.Instantiation[index], positionForParameter)) + return false; + } + } + } + else if (type is ParameterizedType parameterizedType) + { + // Arrays behave as covariant, byref and pointer types are non-variant + if (!ValidateVarianceInType(associatedTypeInstantiation, parameterizedType.ParameterType, (parameterizedType.IsByRef || parameterizedType.IsPointer) ? Internal.TypeSystem.GenericVariance.None : position)) + return false; + } + else if (type is FunctionPointerType functionPointerType) + { + if (!ValidateVarianceInSignature(associatedTypeInstantiation, functionPointerType.Signature, Internal.TypeSystem.GenericVariance.None, Internal.TypeSystem.GenericVariance.None)) + return false; + } + + return true; + } } } } diff --git a/src/coreclr/vm/class.cpp b/src/coreclr/vm/class.cpp index 71a03c6cf3d767..1b41ef0e5954aa 100644 --- a/src/coreclr/vm/class.cpp +++ b/src/coreclr/vm/class.cpp @@ -941,7 +941,7 @@ EEClass::CheckVarianceInSig( uint32_t cArgs; IfFailThrow(psig.GetData(&cArgs)); - // Conservatively, assume non-variance of function pointer types + // Conservatively, assume non-variance of function pointer types, if we ever change this, update the TypeValidationChecker in crossgen2 also if (!CheckVarianceInSig(numGenericArgs, pVarianceInfo, pModule, psig, gpNonVariant)) return FALSE; diff --git a/src/coreclr/vm/genmeth.cpp b/src/coreclr/vm/genmeth.cpp index 2166fb1e1c42e1..b8066a3612a56b 100644 --- a/src/coreclr/vm/genmeth.cpp +++ b/src/coreclr/vm/genmeth.cpp @@ -1554,18 +1554,16 @@ BOOL Bounded(TypeVarTypeDesc *tyvar, DWORD depth) { return TRUE; } -void MethodDesc::LoadConstraintsForTypicalMethodDefinition(BOOL *pfHasCircularClassConstraints, BOOL *pfHasCircularMethodConstraints, ClassLoadLevel level/* = CLASS_LOADED*/) +void MethodDesc::LoadConstraintsForTypicalMethodDefinition(BOOL *pfHasCircularMethodConstraints, ClassLoadLevel level/* = CLASS_LOADED*/) { CONTRACTL { THROWS; GC_TRIGGERS; MODE_ANY; PRECONDITION(IsTypicalMethodDefinition()); - PRECONDITION(CheckPointer(pfHasCircularClassConstraints)); PRECONDITION(CheckPointer(pfHasCircularMethodConstraints)); } CONTRACTL_END; - *pfHasCircularClassConstraints = FALSE; *pfHasCircularMethodConstraints = FALSE; // Force a load of the constraints on the type parameters @@ -1580,17 +1578,6 @@ void MethodDesc::LoadConstraintsForTypicalMethodDefinition(BOOL *pfHasCircularCl DoAccessibilityCheckForConstraints(GetMethodTable(), tyvar, E_ACCESSDENIED); } - // reject circular class constraints - for (DWORD i = 0; i < classInst.GetNumArgs(); i++) - { - TypeVarTypeDesc* tyvar = classInst[i].AsGenericVariable(); - _ASSERTE(tyvar != NULL); - if(!Bounded(tyvar, classInst.GetNumArgs())) - { - *pfHasCircularClassConstraints = TRUE; - } - } - // reject circular method constraints for (DWORD i = 0; i < methodInst.GetNumArgs(); i++) { diff --git a/src/coreclr/vm/method.hpp b/src/coreclr/vm/method.hpp index f56eddccf9012d..291f22ab888bba 100644 --- a/src/coreclr/vm/method.hpp +++ b/src/coreclr/vm/method.hpp @@ -477,9 +477,8 @@ class MethodDesc BOOL IsTypicalMethodDefinition() const; // Force a load of the (typical) constraints on the type parameters of a typical method definition, - // detecting cyclic bounds on class and method type parameters. - void LoadConstraintsForTypicalMethodDefinition(BOOL *pfHasCircularClassConstraints, - BOOL *pfHasCircularMethodConstraints, + // detecting cyclic bounds on method type parameters. + void LoadConstraintsForTypicalMethodDefinition(BOOL *pfHasCircularMethodConstraints, ClassLoadLevel level = CLASS_LOADED); DWORD IsClassConstructor() diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index a7eaee911c8a36..d51e0e9e556c19 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -4776,15 +4776,10 @@ void MethodTable::DoFullyLoad(Generics::RecursionGraph * const pVisited, const if (pMD->IsGenericMethodDefinition() && pMD->IsTypicalMethodDefinition()) { - BOOL fHasCircularClassConstraints = TRUE; BOOL fHasCircularMethodConstraints = TRUE; - pMD->LoadConstraintsForTypicalMethodDefinition(&fHasCircularClassConstraints, &fHasCircularMethodConstraints, CLASS_DEPENDENCIES_LOADED); + pMD->LoadConstraintsForTypicalMethodDefinition(&fHasCircularMethodConstraints, CLASS_DEPENDENCIES_LOADED); - if (fHasCircularClassConstraints) - { - COMPlusThrow(kTypeLoadException, VER_E_CIRCULAR_VAR_CONSTRAINTS); - } if (fHasCircularMethodConstraints) { COMPlusThrow(kTypeLoadException, VER_E_CIRCULAR_MVAR_CONSTRAINTS); diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 4e8e11bbe639b5..33d458ceea1dad 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -12800,7 +12800,12 @@ ClassLoader::CreateTypeHandleForTypeDefThrowing( } // Check interface for use of variant type parameters - if ((genericsInfo.pVarianceInfo != NULL) && (TypeFromToken(crInterface) == mdtTypeSpec)) + if ((genericsInfo.pVarianceInfo != NULL) && (TypeFromToken(crInterface) == mdtTypeSpec) +#ifdef FEATURE_READYTORUN + // No sanity checks for ready-to-run compiled images if possible + && (!pModule->IsReadyToRun() || !pModule->GetReadyToRunInfo()->SkipTypeValidation()) +#endif + ) { ULONG cSig; PCCOR_SIGNATURE pSig; diff --git a/src/coreclr/vm/typedesc.cpp b/src/coreclr/vm/typedesc.cpp index 99c872ec2b0dc5..d618e5e33ea61e 100644 --- a/src/coreclr/vm/typedesc.cpp +++ b/src/coreclr/vm/typedesc.cpp @@ -940,7 +940,12 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level, WhichConstraintsToLo // Method type constraints behave contravariantly // (cf Bounded polymorphism e.g. see // Cardelli & Wegner, On understanding types, data abstraction and polymorphism, Computing Surveys 17(4), Dec 1985) - if (pMT != NULL && pMT->HasVariance() && TypeFromToken(tkConstraintType) == mdtTypeSpec) + if (pMT != NULL && pMT->HasVariance() && TypeFromToken(tkConstraintType) == mdtTypeSpec +#ifdef FEATURE_READYTORUN + // No sanity checks for ready-to-run compiled images if possible + && (!GetModule()->IsReadyToRun() || !GetModule()->GetReadyToRunInfo()->SkipTypeValidation()) +#endif + ) { ULONG cSig; PCCOR_SIGNATURE pSig; From ebca7a602285e427cb0d5ea6106edcc3bdffaa0f Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Fri, 10 Oct 2025 15:47:44 -0700 Subject: [PATCH 14/17] Code review feedback --- src/coreclr/vm/genmeth.cpp | 20 +++++++++----------- src/coreclr/vm/method.hpp | 6 ++---- src/coreclr/vm/methodtable.cpp | 4 +++- src/coreclr/vm/typedesc.cpp | 3 +++ 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/coreclr/vm/genmeth.cpp b/src/coreclr/vm/genmeth.cpp index b8066a3612a56b..0a2dcc7367ea09 100644 --- a/src/coreclr/vm/genmeth.cpp +++ b/src/coreclr/vm/genmeth.cpp @@ -1554,7 +1554,7 @@ BOOL Bounded(TypeVarTypeDesc *tyvar, DWORD depth) { return TRUE; } -void MethodDesc::LoadConstraintsForTypicalMethodDefinition(BOOL *pfHasCircularMethodConstraints, ClassLoadLevel level/* = CLASS_LOADED*/) +void MethodDesc::CheckConstraintMetadataValidity(BOOL *pfHasCircularMethodConstraints) { CONTRACTL { THROWS; @@ -1564,25 +1564,23 @@ void MethodDesc::LoadConstraintsForTypicalMethodDefinition(BOOL *pfHasCircularMe PRECONDITION(CheckPointer(pfHasCircularMethodConstraints)); } CONTRACTL_END; + // In this function we explicitly check for accessibility of method type parameter constraints as + // well as explicitly do a check for circularity among method type parameter constraints. + // + // For checking the variance of the constraints we rely on the fact that both DoAccessibilityCheckForConstraints + // and Bounded will call GetConstraints on the type variables, which will in turn call + // LoadConstraints, and LoadConstraints will do the variance checking using EEClass::CheckVarianceInSig *pfHasCircularMethodConstraints = FALSE; - // Force a load of the constraints on the type parameters - Instantiation classInst = GetClassInstantiation(); - Instantiation methodInst = GetMethodInstantiation(); - for (DWORD i = 0; i < methodInst.GetNumArgs(); i++) - { - TypeVarTypeDesc* tyvar = methodInst[i].AsGenericVariable(); - _ASSERTE(tyvar != NULL); - VOID DoAccessibilityCheckForConstraints(MethodTable *pAskingMT, TypeVarTypeDesc *pTyVar, UINT resIDWhy); - DoAccessibilityCheckForConstraints(GetMethodTable(), tyvar, E_ACCESSDENIED); - } // reject circular method constraints for (DWORD i = 0; i < methodInst.GetNumArgs(); i++) { TypeVarTypeDesc* tyvar = methodInst[i].AsGenericVariable(); _ASSERTE(tyvar != NULL); + VOID DoAccessibilityCheckForConstraints(MethodTable *pAskingMT, TypeVarTypeDesc *pTyVar, UINT resIDWhy); + DoAccessibilityCheckForConstraints(GetMethodTable(), tyvar, E_ACCESSDENIED); if(!Bounded(tyvar, methodInst.GetNumArgs())) { *pfHasCircularMethodConstraints = TRUE; diff --git a/src/coreclr/vm/method.hpp b/src/coreclr/vm/method.hpp index 291f22ab888bba..e4d6f9e017f74e 100644 --- a/src/coreclr/vm/method.hpp +++ b/src/coreclr/vm/method.hpp @@ -476,10 +476,8 @@ class MethodDesc // This method can be called on a non-restored method desc BOOL IsTypicalMethodDefinition() const; - // Force a load of the (typical) constraints on the type parameters of a typical method definition, - // detecting cyclic bounds on method type parameters. - void LoadConstraintsForTypicalMethodDefinition(BOOL *pfHasCircularMethodConstraints, - ClassLoadLevel level = CLASS_LOADED); + // Validate accessibility and usage of variant generic parameters and check for cycles in the method constraints + void CheckConstraintMetadataValidity(BOOL *pfHasCircularMethodConstraints); DWORD IsClassConstructor() { diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index d51e0e9e556c19..9bbdf277790993 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -4753,6 +4753,8 @@ void MethodTable::DoFullyLoad(Generics::RecursionGraph * const pVisited, const for (DWORD i = 0; i < formalParams.GetNumArgs(); i++) { + // This call to Bounded/DoAccessibilityCheckForConstraints will also cause constraint Variance rules to be checked + // via the call to GetConstraints which will eventually call EEClass::CheckVarianceInSig BOOL Bounded(TypeVarTypeDesc *tyvar, DWORD depth); TypeVarTypeDesc *pTyVar = formalParams[i].AsGenericVariable(); @@ -4778,7 +4780,7 @@ void MethodTable::DoFullyLoad(Generics::RecursionGraph * const pVisited, const { BOOL fHasCircularMethodConstraints = TRUE; - pMD->LoadConstraintsForTypicalMethodDefinition(&fHasCircularMethodConstraints, CLASS_DEPENDENCIES_LOADED); + pMD->CheckConstraintMetadataValidity(&fHasCircularMethodConstraints); if (fHasCircularMethodConstraints) { diff --git a/src/coreclr/vm/typedesc.cpp b/src/coreclr/vm/typedesc.cpp index d618e5e33ea61e..85b19f3e3ef682 100644 --- a/src/coreclr/vm/typedesc.cpp +++ b/src/coreclr/vm/typedesc.cpp @@ -940,6 +940,9 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level, WhichConstraintsToLo // Method type constraints behave contravariantly // (cf Bounded polymorphism e.g. see // Cardelli & Wegner, On understanding types, data abstraction and polymorphism, Computing Surveys 17(4), Dec 1985) + // + // This check is NOT conditional on actually loading the constraint, since we want to run EEClass::CheckVarianceInSig + // even if we didn't load the constraint, to cause the TypeLoadException to happen at a predictable time. if (pMT != NULL && pMT->HasVariance() && TypeFromToken(tkConstraintType) == mdtTypeSpec #ifdef FEATURE_READYTORUN // No sanity checks for ready-to-run compiled images if possible From 9d8b167fbabc164ad37687fa1c913a6c690be546 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Fri, 10 Oct 2025 16:20:30 -0700 Subject: [PATCH 15/17] - Flesh out implementation of sig parser for constraint accessibility - Add test for constraints with function pointers in them --- src/coreclr/vm/methodtable.cpp | 54 ++++++++++++++- .../General/FunctionPointerConstraints.cs | 66 +++++++++++++++++++ .../General/FunctionPointerConstraints.csproj | 9 +++ 3 files changed, 126 insertions(+), 3 deletions(-) create mode 100644 src/tests/Loader/classloader/generics/Constraints/General/FunctionPointerConstraints.cs create mode 100644 src/tests/Loader/classloader/generics/Constraints/General/FunctionPointerConstraints.csproj diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 9bbdf277790993..7197c3bb34fd1f 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -4328,12 +4328,60 @@ VOID DoAccessibilityCheckForConstraintSignature(Module *pModule, SigPointer *pSi } case ELEMENT_TYPE_FNPTR: { - // Function pointers can't be in constraint signatures - COMPlusThrow(kTypeLoadException, IDS_CLASSLOAD_BADFORMAT); + uint32_t uCallConv = 0; + IfFailThrow(pSigPtr->GetData(&uCallConv)); + + if ((uCallConv & IMAGE_CEE_CS_CALLCONV_GENERIC) != 0) + { + // Generic function pointers are not allowed. + ThrowHR(COR_E_BADIMAGEFORMAT); + } + + // Get the arg count. + uint32_t cArgs = 0; + IfFailThrow(pSigPtr->GetData(&cArgs)); + + // Loop for cArgs + 1 to handle the return type and all the args + for (uint32_t i = 0; i <= cArgs; i++) + { + DoAccessibilityCheckForConstraintSignature(pModule, pSigPtr, pAskingMT, resIDWhy); + } + break; } - default: + case ELEMENT_TYPE_VOID: + case ELEMENT_TYPE_BOOLEAN: + case ELEMENT_TYPE_CHAR: + case ELEMENT_TYPE_I1: + case ELEMENT_TYPE_U1: + case ELEMENT_TYPE_I2: + case ELEMENT_TYPE_U2: + case ELEMENT_TYPE_I4: + case ELEMENT_TYPE_U4: + case ELEMENT_TYPE_I8: + case ELEMENT_TYPE_U8: + case ELEMENT_TYPE_R4: + case ELEMENT_TYPE_R8: + case ELEMENT_TYPE_I: + case ELEMENT_TYPE_U: + case ELEMENT_TYPE_OBJECT: + case ELEMENT_TYPE_STRING: + case ELEMENT_TYPE_TYPEDBYREF: // Primitive types and such. Nothing to check break; + + case ELEMENT_TYPE_VAR: + case ELEMENT_TYPE_MVAR: + { + // A generic variable. Its always accessible, but we do need to parse it + uint32_t varNumber; + IfFailThrow(pSigPtr->GetData(&varNumber)); + break; + } + + default: + // Unknown element type, bad format + ThrowHR(COR_E_BADIMAGEFORMAT); + break; } } diff --git a/src/tests/Loader/classloader/generics/Constraints/General/FunctionPointerConstraints.cs b/src/tests/Loader/classloader/generics/Constraints/General/FunctionPointerConstraints.cs new file mode 100644 index 00000000000000..2b5bfc4996421d --- /dev/null +++ b/src/tests/Loader/classloader/generics/Constraints/General/FunctionPointerConstraints.cs @@ -0,0 +1,66 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// this test has a generic type with a constraint that it needs to implement an interface constrained on a function pointer +// we want to make sure we can load such type. + +using System; +using System.Text; +using Xunit; + +public class Test_FunctionPointerConstraints { + [Fact] + public static int TestEntryPoint() + { + MyClass myClass = new MyClass(); + + string result = myClass.MyMethod(new MyClass2()); + + bool pass = result == "Func1 Func2 "; + if (pass) + { + Console.WriteLine("PASS"); + return 100; + } + else + { + Console.WriteLine($"result: {result}"); + Console.WriteLine("FAIL"); + return 101; + } + } + + public unsafe class MyClass where T : I1[]> + { + public string MyMethod(T param) + { + StringBuilder stringBuilder = new StringBuilder(); + foreach (var item in param.Get()) + { + stringBuilder.Append(item()); + } + + return stringBuilder.ToString(); + } + } + + public unsafe class MyClass2 : I1[]> + { + private static string Func1() => "Func1 "; + private static string Func2() => "Func2 "; + + public unsafe delegate*[] Get() + { + return new delegate*[] + { + &Func1, + &Func2 + }; + } + } + public interface I1 + { + U Get(); + } +} + diff --git a/src/tests/Loader/classloader/generics/Constraints/General/FunctionPointerConstraints.csproj b/src/tests/Loader/classloader/generics/Constraints/General/FunctionPointerConstraints.csproj new file mode 100644 index 00000000000000..bc4e8cf32e5ec7 --- /dev/null +++ b/src/tests/Loader/classloader/generics/Constraints/General/FunctionPointerConstraints.csproj @@ -0,0 +1,9 @@ + + + true + 1 + + + + + From d5fad7d4d3e46f84c73f7d1f47b22742823fba34 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 13 Oct 2025 13:56:57 -0700 Subject: [PATCH 16/17] - Add crossgen2 validation of interface impl variance rules --- .../ReadyToRun/TypeValidationChecker.cs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs index 7eac27c50ae63b..d240c67058926f 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeValidationChecker.cs @@ -353,6 +353,23 @@ Task ValidateTypeWorkerHelper(TypeDesc typeToCheckForSkipValidation) } } + // Validate that generic variance is properly respected in interface signatures + if (type.HasInstantiation && type.IsInterface) + { + foreach (var interfaceType in type.ExplicitlyImplementedInterfaces) + { + if (interfaceType.HasInstantiation) + { + // Interfaces behave covariantly + if (!ValidateVarianceInType(type.Instantiation, interfaceType, Internal.TypeSystem.GenericVariance.Covariant)) + { + AddTypeValidationError(type, $"'{type}' has invalid variance in in interface impl of {interfaceType}"); + return false; + } + } + } + } + // Validate that there are no cyclical class or method constraints, and that constraints are all acccessible to the type using them if (!BoundedCheckForInstantiation(type.Instantiation)) { From 12511ff0e5be68e2891d744a92acf723d8b1f820 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 13 Oct 2025 14:34:52 -0700 Subject: [PATCH 17/17] - Adjust contracts as suggested - add SkipTypeValidation helper --- src/coreclr/vm/ceeload.cpp | 6 ++++++ src/coreclr/vm/ceeload.h | 8 ++++++++ src/coreclr/vm/class.cpp | 2 +- src/coreclr/vm/genmeth.cpp | 4 +--- src/coreclr/vm/methodtable.cpp | 25 ++++--------------------- src/coreclr/vm/methodtablebuilder.cpp | 13 +++---------- src/coreclr/vm/typedesc.cpp | 6 +----- 7 files changed, 24 insertions(+), 40 deletions(-) diff --git a/src/coreclr/vm/ceeload.cpp b/src/coreclr/vm/ceeload.cpp index bb0f2037816852..e461d61863cf2f 100644 --- a/src/coreclr/vm/ceeload.cpp +++ b/src/coreclr/vm/ceeload.cpp @@ -434,10 +434,16 @@ void Module::Initialize(AllocMemTracker *pamTracker, LPCWSTR szName) AllocateMaps(); m_dwTransientFlags &= ~((DWORD)CLASSES_FREED); // Set flag indicating LookupMaps are now in a consistent and destructable state + if (IsSystem()) + m_dwPersistedFlags |= SKIP_TYPE_VALIDATION; // Skip type validation on System + #ifdef FEATURE_READYTORUN m_pNativeImage = NULL; if ((m_pReadyToRunInfo = ReadyToRunInfo::Initialize(this, pamTracker)) != NULL) { + if (m_pReadyToRunInfo->SkipTypeValidation()) + m_dwPersistedFlags |= SKIP_TYPE_VALIDATION; // Skip type validation on System + m_pNativeImage = m_pReadyToRunInfo->GetNativeImage(); if (m_pNativeImage != NULL) { diff --git a/src/coreclr/vm/ceeload.h b/src/coreclr/vm/ceeload.h index c4d02b6e02dbb6..ae539705296272 100644 --- a/src/coreclr/vm/ceeload.h +++ b/src/coreclr/vm/ceeload.h @@ -686,6 +686,8 @@ class Module : public ModuleBase RUNTIME_MARSHALLING_ENABLED_IS_CACHED = 0x00008000, //If runtime marshalling is enabled for this assembly RUNTIME_MARSHALLING_ENABLED = 0x00010000, + + SKIP_TYPE_VALIDATION = 0x00020000, }; Volatile m_dwTransientFlags; @@ -1500,6 +1502,12 @@ class Module : public ModuleBase #endif } + bool SkipTypeValidation() const + { + LIMITED_METHOD_DAC_CONTRACT; + + return (m_dwPersistedFlags & SKIP_TYPE_VALIDATION) != 0; + } #ifdef FEATURE_READYTORUN PTR_ReadyToRunInfo GetReadyToRunInfo() const { diff --git a/src/coreclr/vm/class.cpp b/src/coreclr/vm/class.cpp index 1b41ef0e5954aa..f42793017ee9b4 100644 --- a/src/coreclr/vm/class.cpp +++ b/src/coreclr/vm/class.cpp @@ -1359,7 +1359,7 @@ void ClassLoader::ValidateMethodsWithCovariantReturnTypes(MethodTable* pMT) return; // Step 1: Validate compatibility of return types on overriding methods - if (pMT->GetClass()->HasCovariantOverride() && (!pMT->GetModule()->IsReadyToRun() || !pMT->GetModule()->GetReadyToRunInfo()->SkipTypeValidation())) + if (pMT->GetClass()->HasCovariantOverride() && !pMT->GetModule()->SkipTypeValidation()) { for (WORD i = 0; i < pParentMT->GetNumVirtuals(); i++) { diff --git a/src/coreclr/vm/genmeth.cpp b/src/coreclr/vm/genmeth.cpp index 0a2dcc7367ea09..492ea84fe7e7f5 100644 --- a/src/coreclr/vm/genmeth.cpp +++ b/src/coreclr/vm/genmeth.cpp @@ -1557,9 +1557,7 @@ BOOL Bounded(TypeVarTypeDesc *tyvar, DWORD depth) { void MethodDesc::CheckConstraintMetadataValidity(BOOL *pfHasCircularMethodConstraints) { CONTRACTL { - THROWS; - GC_TRIGGERS; - MODE_ANY; + STANDARD_VM_CHECK; PRECONDITION(IsTypicalMethodDefinition()); PRECONDITION(CheckPointer(pfHasCircularMethodConstraints)); } CONTRACTL_END; diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 7197c3bb34fd1f..fa394ddb1d4fe2 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -4218,12 +4218,7 @@ static VOID DoAccessibilityCheck(MethodTable *pAskingMT, MethodTable *pTargetMT, VOID DoAccessibilityCheckForConstraint(MethodTable *pAskingMT, TypeHandle thConstraint, UINT resIDWhy) { - CONTRACTL - { - THROWS; - GC_TRIGGERS; - } - CONTRACTL_END; + STANDARD_VM_CONTRACT; if (thConstraint.IsArray()) { @@ -4259,12 +4254,7 @@ VOID DoAccessibilityCheckForConstraint(MethodTable *pAskingMT, TypeHandle thCons VOID DoAccessibilityCheckForConstraintSignature(Module *pModule, SigPointer *pSigPtr, MethodTable *pAskingMT, UINT resIDWhy) { - CONTRACTL - { - THROWS; - GC_TRIGGERS; - } - CONTRACTL_END; + STANDARD_VM_CONTRACT; CorElementType elemType; IfFailThrow(pSigPtr->GetElemType(&elemType)); @@ -4388,12 +4378,7 @@ VOID DoAccessibilityCheckForConstraintSignature(Module *pModule, SigPointer *pSi VOID DoAccessibilityCheckForConstraints(MethodTable *pAskingMT, TypeVarTypeDesc *pTyVar, UINT resIDWhy) { - CONTRACTL - { - THROWS; - GC_TRIGGERS; - } - CONTRACTL_END; + STANDARD_VM_CONTRACT; DWORD numConstraints; TypeHandle *pthConstraints = pTyVar->GetConstraints(&numConstraints, CLASS_DEPENDENCIES_LOADED, WhichConstraintsToLoad::TypeOrMethodVarsAndNonInterfacesOnly); @@ -4609,13 +4594,11 @@ void MethodTable::DoFullyLoad(Generics::RecursionGraph * const pVisited, const bool fNeedsSanityChecks = true; -#ifdef FEATURE_READYTORUN Module * pModule = GetModule(); // No sanity checks for ready-to-run compiled images if possible - if (pModule->IsSystem() || (pModule->IsReadyToRun() && pModule->GetReadyToRunInfo()->SkipTypeValidation())) + if (pModule->SkipTypeValidation()) fNeedsSanityChecks = false; -#endif bool fNeedAccessChecks = (level == CLASS_LOADED) && fNeedsSanityChecks && diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 33d458ceea1dad..f36a4c3f6d1d30 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -1405,11 +1405,8 @@ MethodTableBuilder::BuildMethodTableThrowing( #endif // _DEBUG // If this is CoreLib, then don't perform some sanity checks on the layout - bmtProp->fNoSanityChecks = pModule->IsSystem() || -#ifdef FEATURE_READYTORUN - // No sanity checks for ready-to-run compiled images if possible - (pModule->IsReadyToRun() && pModule->GetReadyToRunInfo()->SkipTypeValidation()) || -#endif + bmtProp->fNoSanityChecks = + pModule->SkipTypeValidation() || // No sanity checks for real generic instantiations !bmtGenerics->IsTypicalTypeDefinition(); @@ -12801,11 +12798,7 @@ ClassLoader::CreateTypeHandleForTypeDefThrowing( // Check interface for use of variant type parameters if ((genericsInfo.pVarianceInfo != NULL) && (TypeFromToken(crInterface) == mdtTypeSpec) -#ifdef FEATURE_READYTORUN - // No sanity checks for ready-to-run compiled images if possible - && (!pModule->IsReadyToRun() || !pModule->GetReadyToRunInfo()->SkipTypeValidation()) -#endif - ) + && !pModule->SkipTypeValidation()) { ULONG cSig; PCCOR_SIGNATURE pSig; diff --git a/src/coreclr/vm/typedesc.cpp b/src/coreclr/vm/typedesc.cpp index 85b19f3e3ef682..09598f362c7452 100644 --- a/src/coreclr/vm/typedesc.cpp +++ b/src/coreclr/vm/typedesc.cpp @@ -944,11 +944,7 @@ void TypeVarTypeDesc::LoadConstraints(ClassLoadLevel level, WhichConstraintsToLo // This check is NOT conditional on actually loading the constraint, since we want to run EEClass::CheckVarianceInSig // even if we didn't load the constraint, to cause the TypeLoadException to happen at a predictable time. if (pMT != NULL && pMT->HasVariance() && TypeFromToken(tkConstraintType) == mdtTypeSpec -#ifdef FEATURE_READYTORUN - // No sanity checks for ready-to-run compiled images if possible - && (!GetModule()->IsReadyToRun() || !GetModule()->GetReadyToRunInfo()->SkipTypeValidation()) -#endif - ) + && !GetModule()->SkipTypeValidation()) { ULONG cSig; PCCOR_SIGNATURE pSig;