Skip to content

Commit

Permalink
Partial fix for the negative virtual static method tests (#17)
Browse files Browse the repository at this point in the history 
* Partial fix for the negative virtual static methods

1) UnimplementedAbstractMethodOnConcreteClass - I have added logic
to verify that all SVMs are implemented once a class is fully loaded.
That required me to propagate a new flag around limiting the class
load level for methods, otherwise we were hitting stack overflows
in many of the tests. I have recycled a pre-existing HRESULT ID which
is probably not what we want, I just didn't want to churn the codebase
before hearing from you what is the right way to proceed here.

2) UnimplementedAbstractMethodOnAbstractClass - the one remaining
failing case is 'ConstraintCheckShouldFail' - I guess that corresponds
to what you alluded to in mentioning that we're not properly checking
class constraints yet.

3) MultipleMethodImplRecordsSameMethodDecl - I added an extra parameter
to the SVM resolution logic that traverses all the MethodImpls and
throws when hitting a duplicate.

The remaining failures involve the following negative tests:

InterfaceVariance (pending my offline feedback)
VarianceSafety (pending my offline feedback)
UnimplementedAbstractMethodOnAbstractClass / ConstraintCheckShouldFail

Thanks

Tomas

* Address David's PR feedback

* Add virtual static check to SVM resolution logic
  • Loading branch information
@trylek
trylek authored May 12, 2021
1 parent 946b6c4 commit 5a8a973
Show file tree
Hide file tree
Showing 8 changed files with 102 additions and 20 deletions.
1 change: 1 addition & 0 deletions src/coreclr/dlls/mscorrc/mscorrc.rc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,7 @@ BEGIN
IDS_CLASSLOAD_MI_MISSING_SIG_BODY "Signature for the body in a method implementation cannot be found. Type: '%1'. Assembly: '%2'."
IDS_CLASSLOAD_MI_MISSING_SIG_DECL "Signature for the declaration in a method implementation cannot be found. Type: '%1'. Assembly: '%2'."
IDS_CLASSLOAD_MI_BADRETURNTYPE "Return type in method '%1' on type '%2' from assembly '%3' is not compatible with base type method '%4'."
IDS_CLASSLOAD_STATICVIRTUAL_NOTIMPL "Virtual static method '%3' is not implemented on type '%1' from assembly '%2'."

IDS_CLASSLOAD_EQUIVALENTSTRUCTMETHODS "Could not load the structure '%1' from assembly '%2'. The structure is marked as eligible for type equivalence, but it has a method."
IDS_CLASSLOAD_EQUIVALENTSTRUCTFIELDS "Could not load the structure '%1' from assembly '%2'. The structure is marked as eligible for type equivalence, but it has a static or non-public field."
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/dlls/mscorrc/resource.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@
#define IDS_CLASSLOAD_MI_MISSING_SIG_BODY 0x17a6
#define IDS_CLASSLOAD_MI_MISSING_SIG_DECL 0x17a7
#define IDS_CLASSLOAD_MI_BADRETURNTYPE 0x17a8
#define IDS_CLASSLOAD_STATICVIRTUAL_NOTIMPL 0x17a9

#define IDS_CLASSLOAD_TOOMANYGENERICARGS 0x17ab
#define IDS_ERROR 0x17b0
Expand Down
5 changes: 4 additions & 1 deletion src/coreclr/vm/genmeth.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1180,7 +1180,10 @@ MethodDesc::FindOrCreateAssociatedMethodDesc(MethodDesc* pDefMD,
pExactMT->GetCanonicalMethodTable(),
FALSE,
Instantiation(repInst, methodInst.GetNumArgs()),
TRUE);
/* allowInstParam */ TRUE,
/* forceRemotableMethod */ FALSE,
/* allowCreate */ TRUE,
/* level */ level);

_ASSERTE(pWrappedMD->IsSharedByGenericInstantiations());
_ASSERTE(!methodInst.IsEmpty() || !pWrappedMD->IsSharedByGenericMethodInstantiations());
Expand Down
15 changes: 10 additions & 5 deletions src/coreclr/vm/memberload.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,8 @@ FieldDesc * MemberLoader::GetFieldDescFromMemberRefAndType(Module * pModule,
//
MethodDesc* MemberLoader::GetMethodDescFromMethodDef(Module *pModule,
mdToken MethodDef,
BOOL strictMetadataChecks)
BOOL strictMetadataChecks,
ClassLoadLevel owningTypeLoadLevel)
{
CONTRACTL
{
Expand Down Expand Up @@ -623,7 +624,7 @@ MethodDesc* MemberLoader::GetMethodDescFromMethodDef(Module *pModule,
}
}

pMD->CheckRestore();
pMD->CheckRestore(owningTypeLoadLevel);

#if 0
// <TODO> Generics: enable this check after the findMethod call in the Zapper which passes
Expand Down Expand Up @@ -713,7 +714,8 @@ MemberLoader::GetMethodDescFromMemberDefOrRefOrSpec(
BOOL strictMetadataChecks,
// Normally true - the zapper is one exception. Throw an exception if no generic method args
// given for a generic method, otherwise return the 'generic' instantiation
BOOL allowInstParam)
BOOL allowInstParam,
ClassLoadLevel owningTypeLoadLevel)
{
CONTRACTL
{
Expand All @@ -738,7 +740,7 @@ MemberLoader::GetMethodDescFromMemberDefOrRefOrSpec(
switch (TypeFromToken(MemberRef))
{
case mdtMethodDef:
pMD = GetMethodDescFromMethodDef(pModule, MemberRef, strictMetadataChecks);
pMD = GetMethodDescFromMethodDef(pModule, MemberRef, strictMetadataChecks, owningTypeLoadLevel);
th = pMD->GetMethodTable();
break;

Expand Down Expand Up @@ -770,7 +772,10 @@ MemberLoader::GetMethodDescFromMemberDefOrRefOrSpec(
th.GetMethodTable(),
FALSE /* don't get unboxing entry point */,
strictMetadataChecks ? Instantiation() : pMD->LoadMethodInstantiation(),
allowInstParam);
allowInstParam,
/* forceRemotableMethod */ FALSE,
/* allowCreate */ TRUE,
/* level */ owningTypeLoadLevel);
} // MemberLoader::GetMethodDescFromMemberDefOrRefOrSpec

//---------------------------------------------------------------------------------------
Expand Down
6 changes: 4 additions & 2 deletions src/coreclr/vm/memberload.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ class MemberLoader
mdToken MemberRefOrDefOrSpec,
const SigTypeContext *pTypeContext, // Context for type parameters in any parent TypeSpec and in the instantiation in a MethodSpec
BOOL strictMetadataChecks, // Normally true - the zapper is one exception. Throw an exception if no generic method args given for a generic method, otherwise return the 'generic' instantiation
BOOL allowInstParam);
BOOL allowInstParam,
ClassLoadLevel owningTypeLoadLevel = CLASS_LOADED);

static FieldDesc* GetFieldDescFromMemberDefOrRef(Module *pModule,
mdMemberRef MemberDefOrRef,
Expand All @@ -92,7 +93,8 @@ class MemberLoader

static MethodDesc* GetMethodDescFromMethodDef(Module *pModule,
mdToken MethodDef,
BOOL strictMetadataChecks);
BOOL strictMetadataChecks,
ClassLoadLevel owningTypeLoadLevel = CLASS_LOADED);

static FieldDesc* GetFieldDescFromFieldDef(Module *pModule,
mdToken FieldDef,
Expand Down
83 changes: 73 additions & 10 deletions src/coreclr/vm/methodtable.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5659,6 +5659,22 @@ void MethodTable::DoFullyLoad(Generics::RecursionGraph * const pVisited, const
}
else
{
// Validate implementation of virtual static methods on all implemented interfaces unless:
// 1) The type resides in the system module (System.Private.CoreLib); we own this module and ensure
// its consistency by other means not requiring runtime checks;
// 2) There are no virtual static methods defined on any of the interfaces implemented by this type;
// 3) The method is abstract in which case it's allowed to leave some virtual static methods unimplemented
// akin to equivalent behavior of virtual instance method overriding in abstract classes;
// 4) The type is a shared generic in which case we generally don't have enough information to perform
// the validation.
if (!GetModule()->IsSystem() &&
HasVirtualStaticMethods() &&
!IsAbstract() &&
!IsSharedByGenericInstantiations())
{
VerifyThatAllVirtualStaticMethodsAreImplemented();
}

// Finally, mark this method table as fully loaded
SetIsFullyLoaded();
}
Expand Down Expand Up @@ -9173,7 +9189,7 @@ MethodDesc *MethodTable::GetDefaultConstructor(BOOL forceBoxedEntryPoint /* = FA
//==========================================================================================
// Finds the (non-unboxing) MethodDesc that implements the interface virtual static method pInterfaceMD.
MethodDesc *
MethodTable::ResolveVirtualStaticMethod(MethodTable* pInterfaceType, MethodDesc* pInterfaceMD, BOOL allowNullResult)
MethodTable::ResolveVirtualStaticMethod(MethodTable* pInterfaceType, MethodDesc* pInterfaceMD, BOOL allowNullResult, BOOL checkDuplicates)
{
if (!pInterfaceMD->IsSharedByGenericMethodInstantiations() && !pInterfaceType->IsSharedByGenericInstantiations())
{
Expand Down Expand Up @@ -9203,7 +9219,7 @@ MethodTable::ResolveVirtualStaticMethod(MethodTable* pInterfaceType, MethodDesc*
// Search for match on a per-level in the type hierarchy
for (MethodTable* pMT = this; pMT != nullptr; pMT = pMT->GetParentMethodTable())
{
MethodDesc* pMD = pMT->TryResolveVirtualStaticMethodOnThisType(pInterfaceType, pInterfaceMD);
MethodDesc* pMD = pMT->TryResolveVirtualStaticMethodOnThisType(pInterfaceType, pInterfaceMD, checkDuplicates);
if (pMD != nullptr)
{
return pMD;
Expand Down Expand Up @@ -9253,7 +9269,7 @@ MethodTable::ResolveVirtualStaticMethod(MethodTable* pInterfaceType, MethodDesc*
// Try to locate the appropriate MethodImpl matching a given interface static virtual method.
// Returns nullptr on failure.
MethodDesc*
MethodTable::TryResolveVirtualStaticMethodOnThisType(MethodTable* pInterfaceType, MethodDesc* pInterfaceMD)
MethodTable::TryResolveVirtualStaticMethodOnThisType(MethodTable* pInterfaceType, MethodDesc* pInterfaceMD, BOOL checkDuplicates)
{
HRESULT hr = S_OK;
IMDInternalImport* pMDInternalImport = GetMDImport();
Expand Down Expand Up @@ -9294,7 +9310,11 @@ MethodTable::TryResolveVirtualStaticMethodOnThisType(MethodTable* pInterfaceType
MethodTable *pInterfaceMT = ClassLoader::LoadTypeDefOrRefOrSpecThrowing(
GetModule(),
tkParent,
&sigTypeContext)
&sigTypeContext,
ClassLoader::ThrowIfNotFound,
ClassLoader::FailIfUninstDefOrRef,
ClassLoader::LoadTypes,
CLASS_LOAD_EXACTPARENTS)
.GetMethodTable();
if (pInterfaceMT != pInterfaceType)
{
Expand All @@ -9305,7 +9325,8 @@ MethodTable::TryResolveVirtualStaticMethodOnThisType(MethodTable* pInterfaceType
methodDecl,
&sigTypeContext,
/* strictMetadataChecks */ FALSE,
/* allowInstParam */ FALSE);
/* allowInstParam */ FALSE,
/* owningTypeLoadLevel */ CLASS_LOAD_EXACTPARENTS);
if (pMethodDecl == nullptr)
{
COMPlusThrow(kTypeLoadException, E_FAIL);
Expand All @@ -9326,7 +9347,8 @@ MethodTable::TryResolveVirtualStaticMethodOnThisType(MethodTable* pInterfaceType
methodBody,
&sigTypeContext,
/* strictMetadataChecks */ FALSE,
/* allowInstParam */ FALSE);
/* allowInstParam */ FALSE,
/* owningTypeLoadLevel */ CLASS_LOAD_EXACTPARENTS);
if (pMethodImpl == nullptr)
{
COMPlusThrow(kTypeLoadException, E_FAIL);
Expand All @@ -9341,15 +9363,56 @@ MethodTable::TryResolveVirtualStaticMethodOnThisType(MethodTable* pInterfaceType

if (pInterfaceMD->HasMethodInstantiation() || pMethodImpl->HasMethodInstantiation() || HasInstantiation())
{
return pMethodImpl->FindOrCreateAssociatedMethodDesc(pMethodImpl, this, FALSE, pInterfaceMD->GetMethodInstantiation(), /* allowInstParam */ FALSE);
pMethodImpl = pMethodImpl->FindOrCreateAssociatedMethodDesc(
pMethodImpl,
this,
FALSE,
pInterfaceMD->GetMethodInstantiation(),
/* allowInstParam */ FALSE,
/* forceRemotableMethod */ FALSE,
/* allowCreate */ TRUE,
/* level */ CLASS_LOAD_EXACTPARENTS);
}
else
if (pMethodImpl != nullptr)
{
return pMethodImpl;
if (!checkDuplicates)
{
return pMethodImpl;
}
if (pPrevMethodImpl != nullptr)
{
// Two MethodImpl records found for the same virtual static interface method
COMPlusThrow(kTypeLoadException, E_FAIL);
}
pPrevMethodImpl = pMethodImpl;
}
}

return nullptr;
return pPrevMethodImpl;
}

void
MethodTable::VerifyThatAllVirtualStaticMethodsAreImplemented()
{
InterfaceMapIterator it = IterateInterfaceMap();
while (it.Next())
{
MethodTable *pInterfaceMT = it.GetInterface();
if (pInterfaceMT->HasVirtualStaticMethods())
{
for (MethodIterator it(pInterfaceMT); it.IsValid(); it.Next())
{
MethodDesc *pMD = it.GetMethodDesc();
if (pMD->IsVirtual() &&
pMD->IsStatic() &&
!ResolveVirtualStaticMethod(pInterfaceMT, pMD, /* allowNullResult */ TRUE, /* checkDuplicates */ TRUE))
{
IMDInternalImport* pInternalImport = GetModule()->GetMDImport();
GetModule()->GetAssembly()->ThrowTypeLoadException(pInternalImport, GetCl(), pMD->GetName(), IDS_CLASSLOAD_STATICVIRTUAL_NOTIMPL);
}
}
}
}
}

//==========================================================================================
Expand Down
6 changes: 4 additions & 2 deletions src/coreclr/vm/methodtable.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1560,6 +1560,8 @@ class MethodTable
inline BOOL HasVirtualStaticMethods() const;
inline void SetHasVirtualStaticMethods();

void VerifyThatAllVirtualStaticMethodsAreImplemented();

inline WORD GetNumVirtuals()
{
LIMITED_METHOD_DAC_CONTRACT;
Expand Down Expand Up @@ -2280,7 +2282,7 @@ class MethodTable


// Resolve virtual static interface method pInterfaceMD on this type.
MethodDesc *ResolveVirtualStaticMethod(MethodTable* pInterfaceType, MethodDesc* pInterfaceMD, BOOL allowNullResult);
MethodDesc *ResolveVirtualStaticMethod(MethodTable* pInterfaceType, MethodDesc* pInterfaceMD, BOOL allowNullResult, BOOL checkDuplicates = FALSE);

// Try a partial resolve of the constraint call, up to generic code sharing.
//
Expand Down Expand Up @@ -2397,7 +2399,7 @@ class MethodTable

// Try to resolve a given static virtual method override on this type. Return nullptr
// when not found.
MethodDesc *TryResolveVirtualStaticMethodOnThisType(MethodTable* pInterfaceType, MethodDesc* pInterfaceMD);
MethodDesc *TryResolveVirtualStaticMethodOnThisType(MethodTable* pInterfaceType, MethodDesc* pInterfaceMD, BOOL checkDuplicates);

public:
static MethodDesc *MapMethodDeclToMethodImpl(MethodDesc *pMDDecl);
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/vm/methodtablebuilder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,11 @@ MethodTableBuilder::ExpandApproxInterface(
{
STANDARD_VM_CONTRACT;

if (pNewInterface->HasVirtualStaticMethods())
{
bmtProp->fHasVirtualStaticMethods = TRUE;
}

//#ExpandingInterfaces
// We expand the tree of inherited interfaces into a set by adding the
// current node BEFORE expanding the parents of the current node.
Expand Down

0 comments on commit 5a8a973

Please sign in to comment.