From cdeb24246f7fdaad2d6566f9d4b07e27233e1aea 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 cc9535ca13f6ed..30da943a8317ce 100644 --- a/src/coreclr/vm/comdelegate.cpp +++ b/src/coreclr/vm/comdelegate.cpp @@ -2406,16 +2406,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 722383e058dd7f..87d4b7cd8d4195 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 9641005ec0aefd..a1f4e41ef9d3b9 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 8cff26f064e657..fca3ae4f4b00dd 100644 --- a/src/coreclr/vm/typedesc.h +++ b/src/coreclr/vm/typedesc.h @@ -278,6 +278,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. @@ -286,6 +294,8 @@ struct cdac_data class TypeVarTypeDesc : public TypeDesc { + const DWORD WhichMask = 0xC0000000; + const DWORD WhichShift = 30; public: #ifndef DACCESS_COMPILE @@ -309,7 +319,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 @@ -349,16 +359,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, @@ -386,7 +396,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 1df074a0dc8b4f..7007458071547f 100644 --- a/src/coreclr/vm/typehandle.cpp +++ b/src/coreclr/vm/typehandle.cpp @@ -571,8 +571,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 @@ -1371,8 +1371,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 799719ff8a774addb5e43bbd894beacb5ebdd352 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 82a4f0c39daed0743e29db08152a2423c5270ba2 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 87d4b7cd8d4195..98b57b40e3fc27 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 e482bb7982a3f37654b090fd305671c3c0089a0b 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 98b57b40e3fc27..0e2e6283d07824 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 1b7451594a8b11208245f27b34c809e4988b4cb7 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 fca3ae4f4b00dd..57f0b9b2328d60 100644 --- a/src/coreclr/vm/typedesc.h +++ b/src/coreclr/vm/typedesc.h @@ -294,8 +294,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 957c89cb12ffac9f9152732fd7374d186b7ad558 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 0e2e6283d07824..0fe0561a60cf58 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]; @@ -8537,6 +8537,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()) @@ -8823,7 +8829,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 5dd3f85bdd5920..0affeb2abf74d3 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 1e8b5c87893c2c..6a3a038fa14b30 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -417,7 +417,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, @@ -9602,12 +9602,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(); @@ -9639,7 +9639,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(); @@ -9647,7 +9655,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()) @@ -9702,7 +9710,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 3de06739f4d7e81713484e30933b3d4ca5a0a119 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 0fe0561a60cf58..f27184f9405d9a 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 0affeb2abf74d3..b40e94560e0cf5 100644 --- a/src/coreclr/vm/methodtable.h +++ b/src/coreclr/vm/methodtable.h @@ -2298,7 +2298,7 @@ class MethodTable pMT->HasInstantiation() && pCurrentMethodTable->IsSpecialMarkerTypeForGenericCasting() && !pMTOwner->GetAuxiliaryData()->MayHaveOpenInterfacesInInterfaceMap() && - pMT->GetInstantiation().ContainsAllOneType(pMTOwner)) + pMT->GetInstantiation().ContainsAllOneType(pMTOwner->GetSpecialInstantiationType())) { exactMatch = true; #ifndef DACCESS_COMPILE @@ -2314,6 +2314,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 f6aa4746666839..06cd420ccc8d1a 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 9b4ccbe7223c6a64fb3cc9c9a9dc37a2d71e7c59 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 f27184f9405d9a..7d06db520067f9 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(); @@ -8325,6 +8349,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)) @@ -8371,19 +8410,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 6a3a038fa14b30..127a3b62d2be37 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -2240,6 +2240,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 87757d0206f66bf9fc97bf7a84a3449aada0b4d7 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 e400fc704e593860e1e0b47f84aff66267a28d3d 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 127a3b62d2be37..3622dee82c4bc3 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -9607,9 +9607,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) { @@ -9642,7 +9639,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 a4b1aa72df90a83075bb5d4263c82be45057d8e1 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 57f0b9b2328d60..08a2074f2e3039 100644 --- a/src/coreclr/vm/typedesc.h +++ b/src/coreclr/vm/typedesc.h @@ -282,7 +282,7 @@ enum class WhichConstraintsToLoad { All = 0, TypeOrMethodVarsAndNonInterfacesOnly = 1, - TypeOrMethodVarsOnly = 2, + Invalid = 2, None = 3, }; @@ -294,8 +294,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 @@ -359,7 +359,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 9a3adb7a6d91a808c2bb13d894591c956c99c2ba 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 7d06db520067f9..6a440b89ecc8be 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 292ee2b35922bc544d30745daa1cbe779cd56e1e 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 e6ef9cad2c10f9..c9eea05dedb2ec 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 a96bcffae4fe1a..66b1ff66c2ee8a 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 7a23d0cd0b6325..ef0e4f3659aa8a 100644 --- a/src/coreclr/vm/method.hpp +++ b/src/coreclr/vm/method.hpp @@ -480,9 +480,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 6a440b89ecc8be..334cb6736fd432 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 3622dee82c4bc3..bcb27bf2697c28 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -12728,7 +12728,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 f64bcaccebd13bf7f68603554b9141628704ac56 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 ef0e4f3659aa8a..4cb42eaa6e23c6 100644 --- a/src/coreclr/vm/method.hpp +++ b/src/coreclr/vm/method.hpp @@ -479,10 +479,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 334cb6736fd432..950cf61722f8c6 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 c5a78e6ca0c9765f8f51ef2d7c7e09c8bc2a2f49 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 950cf61722f8c6..63de5f4a088558 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 0f16b472f04d92ae242f9c1d7268dea6a2bfcb4c 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 c9eea05dedb2ec..57dd3bbea81924 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 0ae569a0ff2651397b2d7352a767d8c8b0d4df0d 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 adfe056bdbe20b..5bd1b6e7ff4cf7 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 3cea27da8fbcee..c203207e25c1ce 100644 --- a/src/coreclr/vm/ceeload.h +++ b/src/coreclr/vm/ceeload.h @@ -676,6 +676,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; @@ -1490,6 +1492,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 66b1ff66c2ee8a..9209e652830c02 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 63de5f4a088558..9729e9e9318e99 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 bcb27bf2697c28..fa97feb6ea8e87 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -1409,11 +1409,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(); @@ -12729,11 +12726,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;