From d139d1d9c2b58cbf627994c6f09383b6e0a18ca8 Mon Sep 17 00:00:00 2001 From: Tomas Date: Wed, 7 Apr 2021 03:07:15 +0200 Subject: [PATCH 1/5] Minimalistic JIT change to enable the new constrained. opcodes --- src/coreclr/jit/importer.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 8ec1ca1108c60e..2b318b8867b83a 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -13862,7 +13862,8 @@ void Compiler::impImportBlockCode(BasicBlock* block) JITDUMP(" %08X", resolvedToken.token); - eeGetCallInfo(&resolvedToken, nullptr /* constraint typeRef*/, + eeGetCallInfo(&resolvedToken, + (prefixFlags & PREFIX_CONSTRAINED) ? &constrainedResolvedToken : nullptr, addVerifyFlag(combine(CORINFO_CALLINFO_SECURITYCHECKS, CORINFO_CALLINFO_LDFTN)), &callInfo); @@ -14033,9 +14034,9 @@ void Compiler::impImportBlockCode(BasicBlock* block) { OPCODE actualOpcode = impGetNonPrefixOpcode(codeAddr, codeEndp); - if (actualOpcode != CEE_CALLVIRT) + if (actualOpcode != CEE_CALLVIRT && actualOpcode != CEE_CALL && actualOpcode != CEE_LDFTN) { - BADCODE("constrained. has to be followed by callvirt"); + BADCODE("constrained. has to be followed by callvirt, call or ldftn"); } } From abda7c471be76137e436b87e2dc288fff13704bc Mon Sep 17 00:00:00 2001 From: Tomas Date: Tue, 20 Apr 2021 22:23:18 +0200 Subject: [PATCH 2/5] One more JIT fix for constrained. prefix --- src/coreclr/jit/fgbasic.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 7992d3b696a31c..424afbe67f08f0 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -1143,9 +1143,9 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed { OPCODE actualOpcode = impGetNonPrefixOpcode(codeAddr, codeEndp); - if (actualOpcode != CEE_CALLVIRT) + if (actualOpcode != CEE_CALLVIRT && actualOpcode != CEE_CALL && actualOpcode != CEE_LDFTN) { - BADCODE("constrained. has to be followed by callvirt"); + BADCODE("constrained. has to be followed by callvirt, call or ldftn"); } } goto OBSERVE_OPCODE; From d343b1499a7e8adfdc0257c718a24305f4d99ee0 Mon Sep 17 00:00:00 2001 From: Tomas Date: Tue, 20 Apr 2021 22:27:46 +0200 Subject: [PATCH 3/5] Runtime changes up to the point of resolution in getCallInfo I have modified the behavior based on the principle that the virtual static methods shouldn't go in the vtable. For this purpose they are treated as 'non-virtual' when building the interface slot table. I'm now trying to figure out how to traverse the MethodImpls in MethodTable during the constrained lookup, I haven't yet managed to identify whether they are already accessible via the existing MethodTable API or whether it will require adding a new table. Thanks Tomas --- src/coreclr/vm/jitinterface.cpp | 2 +- src/coreclr/vm/methodtable.cpp | 13 ++++ src/coreclr/vm/methodtable.h | 8 ++- src/coreclr/vm/methodtable.inl | 14 +++++ src/coreclr/vm/methodtablebuilder.cpp | 90 ++++++++++++++++----------- src/coreclr/vm/methodtablebuilder.h | 6 +- 6 files changed, 92 insertions(+), 41 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 11dfd7fb76107e..77f7201fe0a758 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -5118,7 +5118,7 @@ void CEEInfo::getCallInfo( TypeHandle exactType = TypeHandle(pResolvedToken->hClass); TypeHandle constrainedType; - if ((flags & CORINFO_CALLINFO_CALLVIRT) && (pConstrainedResolvedToken != NULL)) + if (pConstrainedResolvedToken != NULL) { constrainedType = TypeHandle(pConstrainedResolvedToken->hClass); } diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index a6f1048333e1ac..164c833d5810db 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -9170,6 +9170,14 @@ MethodDesc *MethodTable::GetDefaultConstructor(BOOL forceBoxedEntryPoint /* = FA FALSE /* no allowInstParam */); } +//========================================================================================== +// Finds the (non-unboxing) MethodDesc that implements the interface virtual static method pInterfaceMD. +MethodDesc * +MethodTable::ResolveVirtualStaticMethod(MethodDesc* pInterfaceMD) +{ + // TODO +} + //========================================================================================== // Finds the (non-unboxing) MethodDesc that implements the interface method pInterfaceMD. // @@ -9189,6 +9197,11 @@ MethodTable::TryResolveConstraintMethodApprox( GC_TRIGGERS; } CONTRACTL_END; + if (pInterfaceMD->IsStatic()) + { + return ResolveVirtualStaticMethod(pInterfaceMD); + } + // We can't resolve constraint calls effectively for reference types, and there's // not a lot of perf. benefit in doing it anyway. // diff --git a/src/coreclr/vm/methodtable.h b/src/coreclr/vm/methodtable.h index 4f05c6a1868c2c..d5984745910c36 100644 --- a/src/coreclr/vm/methodtable.h +++ b/src/coreclr/vm/methodtable.h @@ -1557,6 +1557,9 @@ class MethodTable inline WORD GetNumNonVirtualSlots(); + inline BOOL HasVirtualStaticMethods() const; + inline void SetHasVirtualStaticMethods(); + inline WORD GetNumVirtuals() { LIMITED_METHOD_DAC_CONTRACT; @@ -2276,6 +2279,9 @@ class MethodTable #endif // FEATURE_COMINTEROP + // Resolve virtual static interface method pInterfaceMD on this type. + MethodDesc *ResolveVirtualStaticMethod(MethodDesc* pInterfaceMD); + // Try a partial resolve of the constraint call, up to generic code sharing. // // Note that this will not necessarily resolve the call exactly, since we might be compiling @@ -3655,7 +3661,7 @@ public : enum_flag_RequiresDispatchTokenFat = 0x0200, enum_flag_HasCctor = 0x0400, - // enum_flag_unused = 0x0800, + enum_flag_HasVirtualStaticMethods = 0x0800, #ifdef FEATURE_64BIT_ALIGNMENT enum_flag_RequiresAlign8 = 0x1000, // Type requires 8-byte alignment (only set on platforms that require this and don't get it implicitly) diff --git a/src/coreclr/vm/methodtable.inl b/src/coreclr/vm/methodtable.inl index 07b2c3c361ed14..88ebe105bda5cf 100644 --- a/src/coreclr/vm/methodtable.inl +++ b/src/coreclr/vm/methodtable.inl @@ -541,6 +541,20 @@ inline INT32 MethodTable::MethodIterator::GetNumMethods() const return m_iMethods; } +//========================================================================================== +inline BOOL MethodTable::HasVirtualStaticMethods() const +{ + WRAPPER_NO_CONTRACT; + return GetFlag(enum_flag_HasVirtualStaticMethods); +} + +//========================================================================================== +inline void MethodTable::SetHasVirtualStaticMethods() +{ + WRAPPER_NO_CONTRACT; + return SetFlag(enum_flag_HasVirtualStaticMethods); +} + //========================================================================================== // Returns TRUE if it's valid to request data from the current position inline BOOL MethodTable::MethodIterator::IsValid() const diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index f14d3e2c0202ca..36b5d83fe28732 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -1180,17 +1180,17 @@ MethodTableBuilder::bmtInterfaceEntry::CreateSlotTable( CONSISTENCY_CHECK(m_pImplTable == NULL); - SLOT_INDEX cSlots = (SLOT_INDEX)GetInterfaceType()->GetMethodTable()->GetNumVirtuals(); + MethodTable *interfaceMT = GetInterfaceType()->GetMethodTable(); + SLOT_INDEX cSlots = interfaceMT->GetNumVirtuals(); + if (interfaceMT->HasVirtualStaticMethods()) + { + cSlots += interfaceMT->GetNumNonVirtualSlots(); + } bmtInterfaceSlotImpl * pST = new (pStackingAllocator) bmtInterfaceSlotImpl[cSlots]; MethodTable::MethodIterator it(GetInterfaceType()->GetMethodTable()); - for (; it.IsValid(); it.Next()) + for (; m_cImplTable < cSlots; it.Next()) { - if (!it.IsVirtual()) - { - break; - } - bmtRTMethod * pCurMethod = new (pStackingAllocator) bmtRTMethod(GetInterfaceType(), it.GetDeclMethodDesc()); @@ -2945,7 +2945,15 @@ MethodTableBuilder::EnumerateClassMethods() } if(IsMdStatic(dwMemberAttrs)) { - BuildMethodTableThrowException(BFA_VIRTUAL_STATIC_METHOD); + if (fIsClassInterface) + { + bmtProp->fHasVirtualStaticMethods = TRUE; + } + else + { + // Static virtual methods are only allowed to exist in interfaces + BuildMethodTableThrowException(BFA_VIRTUAL_STATIC_METHOD); + } } if(strMethodName && (0==strcmp(strMethodName, COR_CTOR_METHOD_NAME))) { @@ -5021,14 +5029,14 @@ MethodTableBuilder::ValidateMethods() if (it.IsMethodImpl()) { - if (!IsMdVirtual(it.Attrs())) + if (!IsMdVirtual(it.Attrs()) && !IsMdStatic(it.Attrs())) { // Non-virtual methods cannot participate in a methodImpl pair. BuildMethodTableThrowException(IDS_CLASSLOAD_MI_MUSTBEVIRTUAL, it.Token()); } } // Virtual static methods are not allowed. - if (IsMdStatic(it.Attrs()) && IsMdVirtual(it.Attrs())) + if (IsMdStatic(it.Attrs()) && IsMdVirtual(it.Attrs()) && !IsInterface()) { BuildMethodTableThrowException(IDS_CLASSLOAD_STATICVIRTUAL, it.Token()); } @@ -5297,8 +5305,8 @@ MethodTableBuilder::PlaceVirtualMethods() DeclaredMethodIterator it(*this); while (it.Next()) { - if (!IsMdVirtual(it.Attrs())) - { // Only processing declared virtual methods + if (!IsMdVirtual(it.Attrs()) || IsMdStatic(it.Attrs())) + { // Only processing declared virtual instance methods continue; } @@ -5613,13 +5621,11 @@ MethodTableBuilder::ProcessMethodImpls() DeclaredMethodIterator it(*this); while (it.Next()) { - // Non-virtual methods cannot be classified as methodImpl - we should have thrown an - // error before reaching this point. - CONSISTENCY_CHECK(!(!IsMdVirtual(it.Attrs()) && it.IsMethodImpl())); - - if (!IsMdVirtual(it.Attrs())) - { // Only virtual methods can participate in methodImpls - continue; + if (!IsMdVirtual(it.Attrs()) && it.IsMethodImpl()) + { + // Non-virtual methods can only be classified as methodImpl when implementing + // static virtual methods. + CONSISTENCY_CHECK(IsMdStatic(it.Attrs())); } // If this method serves as the BODY of a MethodImpl specification, then @@ -6314,24 +6320,28 @@ MethodTableBuilder::PlaceMethodImpls() } else { - // Do not use pDecl->IsInterface here as that asks the method table and the MT may not yet be set up. - if (pCurDeclMethod->GetOwningType()->IsInterface()) - { - // Throws - PlaceInterfaceDeclarationOnClass( - pCurDeclMethod, - pCurImplMethod); - } - else + // Don't place static virtual method overrides in the vtable + if (!IsMdStatic(pCurDeclMethod->GetDeclAttrs())) { - // Throws - PlaceParentDeclarationOnClass( - pCurDeclMethod, - pCurImplMethod, - slots, - replaced, - &slotIndex, - dwMaxSlotSize); // Increments count + // Do not use pDecl->IsInterface here as that asks the method table and the MT may not yet be set up. + if (pCurDeclMethod->GetOwningType()->IsInterface()) + { + // Throws + PlaceInterfaceDeclarationOnClass( + pCurDeclMethod, + pCurImplMethod); + } + else + { + // Throws + PlaceParentDeclarationOnClass( + pCurDeclMethod, + pCurImplMethod, + slots, + replaced, + &slotIndex, + dwMaxSlotSize); // Increments count + } } } } @@ -9783,7 +9793,8 @@ MethodTable * MethodTableBuilder::AllocateNewMT( LoaderAllocator *pAllocator, BOOL isInterface, BOOL fDynamicStatics, - BOOL fHasGenericsStaticsInfo + BOOL fHasGenericsStaticsInfo, + BOOL fHasVirtualStaticMethods #ifdef FEATURE_COMINTEROP , BOOL fHasDynamicInterfaceMap #endif @@ -9926,6 +9937,10 @@ MethodTable * MethodTableBuilder::AllocateNewMT( // initialize the total number of slots pMT->SetNumVirtuals(static_cast(dwVirtuals)); + if (fHasVirtualStaticMethods) + { + pMT->SetHasVirtualStaticMethods(); + } pMT->SetParentMethodTable(pMTParent); @@ -10103,6 +10118,7 @@ MethodTableBuilder::SetupMethodTable2( IsInterface(), bmtProp->fDynamicStatics, bmtProp->fGenericsStatics, + bmtProp->fHasVirtualStaticMethods, #ifdef FEATURE_COMINTEROP fHasDynamicInterfaceMap, #endif diff --git a/src/coreclr/vm/methodtablebuilder.h b/src/coreclr/vm/methodtablebuilder.h index 346ec5012606a2..c44568a2f96c3c 100644 --- a/src/coreclr/vm/methodtablebuilder.h +++ b/src/coreclr/vm/methodtablebuilder.h @@ -1325,6 +1325,7 @@ class MethodTableBuilder bool fIsEnum; bool fNoSanityChecks; bool fSparse; // Set to true if a sparse interface is being used. + bool fHasVirtualStaticMethods; // Set to true if the interface type declares virtual static methods. // Com Interop, ComWrapper classes extend from ComObject bool fIsComObjectType; // whether this class is an instance of ComObject class @@ -1496,7 +1497,7 @@ class MethodTableBuilder AddNonVirtualMethod(bmtMDMethod * pMethod) { INDEBUG(SealVirtualSlotSection()); - CONSISTENCY_CHECK(!IsMdVirtual(pMethod->GetDeclAttrs())); + CONSISTENCY_CHECK(!IsMdVirtual(pMethod->GetDeclAttrs()) || IsMdStatic(pMethod->GetDeclAttrs())); pMethod->SetSlotIndex(pSlotTable->GetSlotCount()); if (!pSlotTable->AddMethodSlot(bmtMethodSlot(pMethod, pMethod))) return false; @@ -2984,7 +2985,8 @@ class MethodTableBuilder LoaderAllocator *pAllocator, BOOL isIFace, BOOL fDynamicStatics, - BOOL fHasGenericsStaticsInfo + BOOL fHasGenericsStaticsInfo, + BOOL fHasVirtualStaticMethods #ifdef FEATURE_COMINTEROP , BOOL bHasDynamicInterfaceMap #endif From 1d9c2ac9885625a95d917d4562708a54872f3173 Mon Sep 17 00:00:00 2001 From: Tomas Date: Wed, 21 Apr 2021 00:17:59 +0200 Subject: [PATCH 4/5] Address initial David's PR feedback --- src/coreclr/vm/methodtable.cpp | 1 + src/coreclr/vm/methodtablebuilder.cpp | 99 ++++++++++++++------------- 2 files changed, 52 insertions(+), 48 deletions(-) diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 164c833d5810db..6e9d8288d8f444 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -9176,6 +9176,7 @@ MethodDesc * MethodTable::ResolveVirtualStaticMethod(MethodDesc* pInterfaceMD) { // TODO + COMPlusThrow(kTypeLoadException, E_NOTIMPL); } //========================================================================================== diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 36b5d83fe28732..862c94dc87901c 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -5030,7 +5030,9 @@ MethodTableBuilder::ValidateMethods() if (it.IsMethodImpl()) { if (!IsMdVirtual(it.Attrs()) && !IsMdStatic(it.Attrs())) - { // Non-virtual methods cannot participate in a methodImpl pair. + { + // Non-virtual methods may only participate in a methodImpl pair when + // they are static and they implement a virtual static interface method. BuildMethodTableThrowException(IDS_CLASSLOAD_MI_MUSTBEVIRTUAL, it.Token()); } } @@ -6269,59 +6271,60 @@ MethodTableBuilder::PlaceMethodImpls() // Get the declaration part of the method impl. It will either be a token // (declaration is on this type) or a method desc. bmtMethodHandle hDeclMethod = bmtMethodImpl->GetDeclarationMethod(iEntry); - if(hDeclMethod.IsMDMethod()) - { - // The declaration is on the type being built - bmtMDMethod * pCurDeclMethod = hDeclMethod.AsMDMethod(); - - mdToken mdef = pCurDeclMethod->GetMethodSignature().GetToken(); - if (bmtMethodImpl->IsBody(mdef)) - { // A method declared on this class cannot be both a decl and an impl - BuildMethodTableThrowException(IDS_CLASSLOAD_MI_MULTIPLEOVERRIDES, mdef); - } - if (IsInterface()) - { - // Throws - PlaceInterfaceDeclarationOnInterface( - hDeclMethod, - pCurImplMethod, - slots, // Adds override to the slot and replaced arrays. - replaced, - &slotIndex, - dwMaxSlotSize); // Increments count - } - else - { - // Throws - PlaceLocalDeclarationOnClass( - pCurDeclMethod, - pCurImplMethod, - slots, // Adds override to the slot and replaced arrays. - replaced, - &slotIndex, - dwMaxSlotSize); // Increments count - } - } - else + // Don't place static virtual method overrides in the vtable + if (!IsMdStatic(hDeclMethod.GetDeclAttrs())) { - bmtRTMethod * pCurDeclMethod = hDeclMethod.AsRTMethod(); - - if (IsInterface()) + if(hDeclMethod.IsMDMethod()) { - // Throws - PlaceInterfaceDeclarationOnInterface( - hDeclMethod, - pCurImplMethod, - slots, // Adds override to the slot and replaced arrays. - replaced, - &slotIndex, - dwMaxSlotSize); // Increments count + // The declaration is on the type being built + bmtMDMethod * pCurDeclMethod = hDeclMethod.AsMDMethod(); + + mdToken mdef = pCurDeclMethod->GetMethodSignature().GetToken(); + if (bmtMethodImpl->IsBody(mdef)) + { // A method declared on this class cannot be both a decl and an impl + BuildMethodTableThrowException(IDS_CLASSLOAD_MI_MULTIPLEOVERRIDES, mdef); + } + + if (IsInterface()) + { + // Throws + PlaceInterfaceDeclarationOnInterface( + hDeclMethod, + pCurImplMethod, + slots, // Adds override to the slot and replaced arrays. + replaced, + &slotIndex, + dwMaxSlotSize); // Increments count + } + else + { + // Throws + PlaceLocalDeclarationOnClass( + pCurDeclMethod, + pCurImplMethod, + slots, // Adds override to the slot and replaced arrays. + replaced, + &slotIndex, + dwMaxSlotSize); // Increments count + } } else { - // Don't place static virtual method overrides in the vtable - if (!IsMdStatic(pCurDeclMethod->GetDeclAttrs())) + bmtRTMethod * pCurDeclMethod = hDeclMethod.AsRTMethod(); + + if (IsInterface()) + { + // Throws + PlaceInterfaceDeclarationOnInterface( + hDeclMethod, + pCurImplMethod, + slots, // Adds override to the slot and replaced arrays. + replaced, + &slotIndex, + dwMaxSlotSize); // Increments count + } + else { // Do not use pDecl->IsInterface here as that asks the method table and the MT may not yet be set up. if (pCurDeclMethod->GetOwningType()->IsInterface()) From 0de5c12926a2e4c15ecbb8d48a20fbbf301ae628 Mon Sep 17 00:00:00 2001 From: Tomas Date: Wed, 21 Apr 2021 00:47:11 +0200 Subject: [PATCH 5/5] Revert changes to CreateSlotTable; short-circuit MethodImpl validation --- src/coreclr/vm/methodtablebuilder.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 862c94dc87901c..6231a4e074ab47 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -1180,17 +1180,17 @@ MethodTableBuilder::bmtInterfaceEntry::CreateSlotTable( CONSISTENCY_CHECK(m_pImplTable == NULL); - MethodTable *interfaceMT = GetInterfaceType()->GetMethodTable(); - SLOT_INDEX cSlots = interfaceMT->GetNumVirtuals(); - if (interfaceMT->HasVirtualStaticMethods()) - { - cSlots += interfaceMT->GetNumNonVirtualSlots(); - } + SLOT_INDEX cSlots = (SLOT_INDEX)GetInterfaceType()->GetMethodTable()->GetNumVirtuals(); bmtInterfaceSlotImpl * pST = new (pStackingAllocator) bmtInterfaceSlotImpl[cSlots]; MethodTable::MethodIterator it(GetInterfaceType()->GetMethodTable()); - for (; m_cImplTable < cSlots; it.Next()) + for (; it.IsValid(); it.Next()) { + if (!it.IsVirtual()) + { + break; + } + bmtRTMethod * pCurMethod = new (pStackingAllocator) bmtRTMethod(GetInterfaceType(), it.GetDeclMethodDesc()); @@ -5628,6 +5628,7 @@ MethodTableBuilder::ProcessMethodImpls() // Non-virtual methods can only be classified as methodImpl when implementing // static virtual methods. CONSISTENCY_CHECK(IsMdStatic(it.Attrs())); + continue; } // If this method serves as the BODY of a MethodImpl specification, then