Skip to content

Fix remaining failing test issues in current testbed #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 1, 2021
Merged

Fix remaining failing test issues in current testbed #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/coreclr/vm/genericdict.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1382,11 +1382,11 @@ Dictionary::PopulateEntry(
}
_ASSERTE(!constraintType.IsNull());

MethodDesc *pResolvedMD = constraintType.GetMethodTable()->TryResolveConstraintMethodApprox(ownerType, pMethod, !pMethod->IsStatic());
MethodDesc *pResolvedMD = constraintType.GetMethodTable()->TryResolveConstraintMethodApprox(ownerType, pMethod);

// All such calls should be resolvable. If not then for now just throw an error.
_ASSERTE(pResolvedMD);
INDEBUG(if (!pResolvedMD) constraintType.GetMethodTable()->TryResolveConstraintMethodApprox(ownerType, pMethod, !pMethod->IsStatic());)
INDEBUG(if (!pResolvedMD) constraintType.GetMethodTable()->TryResolveConstraintMethodApprox(ownerType, pMethod);)
if (!pResolvedMD)
COMPlusThrowHR(COR_E_BADIMAGEFORMAT);

Expand Down
24 changes: 23 additions & 1 deletion src/coreclr/vm/jitinterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5183,7 +5183,6 @@ void CEEInfo::getCallInfo(
MethodDesc * directMethod = constrainedType.GetMethodTable()->TryResolveConstraintMethodApprox(
exactType,
pMD,
!!(flags & CORINFO_CALLINFO_ALLOWINSTPARAM),
&fForceUseRuntimeLookup);
if (directMethod
#ifdef FEATURE_DEFAULT_INTERFACES
Expand Down Expand Up @@ -5355,6 +5354,29 @@ void CEEInfo::getCallInfo(

bool allowInstParam = (flags & CORINFO_CALLINFO_ALLOWINSTPARAM);

// If the target method is resolved via constrained static virtual dispatch
// And it requires an instParam, we do not have the generic dictionary infrastructure
// to load the correct generic context arg via EmbedGenericHandle.
// Instead, force the call to go down the CORINFO_CALL_CODE_POINTER code path
// which should have somewhat inferior performance. This should only actually happen in the case
// of shared generic code calling a shared generic implementation method, which should be rare.
//
// An alternative design would be to add a new generic dictionary entry kind to hold the MethodDesc
// of the constrained target instead, and use that in some circumstances; however, implementation of
// that design requires refactoring variuos parts of the JIT interface as well as
// TryResolveConstraintMethodApprox. In particular we would need to be abled to embed a constrained lookup
// via EmbedGenericHandle, as well as decide in TryResolveConstraintMethodApprox if the call can be made
// via a single use of CORINFO_CALL_CODE_POINTER, or would be better done with a CORINFO_CALL + embedded
// constrained generic handle, or if there is a case where we would want to use both a CORINFO_CALL and
// embedded constrained generic handle. Given the current expected high performance use case of this feature
// which is generic numerics which will always resolve to exact valuetypes, it is not expected that
// the complexity involved would be worth the risk. Other scenarios are not expected to be as performance
// sensitive.
if (IsMdStatic(dwTargetMethodAttrs) && constrainedType != NULL && pResult->exactContextNeedsRuntimeLookup)
{
allowInstParam = FALSE;
}

// Create instantiating stub if necesary
if (!allowInstParam && pTargetMD->RequiresInstArg())
{
Expand Down
66 changes: 55 additions & 11 deletions src/coreclr/vm/methodtable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9173,14 +9173,42 @@ MethodDesc *MethodTable::GetDefaultConstructor(BOOL forceBoxedEntryPoint /* = FA
//==========================================================================================
// Finds the (non-unboxing) MethodDesc that implements the interface virtual static method pInterfaceMD.
MethodDesc *
MethodTable::ResolveVirtualStaticMethod(MethodDesc* pInterfaceMD, BOOL allowInstParam, BOOL allowNullResult)
MethodTable::ResolveVirtualStaticMethod(MethodTable* pInterfaceType, MethodDesc* pInterfaceMD, BOOL allowNullResult)
{
for (MethodTable* pMT = this; pMT != nullptr; pMT = pMT->GetParentMethodTable())
if (!pInterfaceMD->IsSharedByGenericMethodInstantiations() && !pInterfaceType->IsSharedByGenericInstantiations())
{
MethodDesc* pMD = pMT->TryResolveVirtualStaticMethodOnThisType(pInterfaceMD, allowInstParam);
if (pMD != nullptr)
// Check that there is no implementation of the interface on this type which is the canonical interface for a shared generic. If so, that indicates that
// we cannot exactly compute a target method result, as even if there is an exact match in the type hierarchy
// it isn't guaranteed that we will always find the right result, as we may find a match on a base type when we should find the match
// on a more derived type.

MethodTable *pInterfaceTypeCanonical = pInterfaceType->GetCanonicalMethodTable();
bool canonicalEquivalentFound = false;
if (pInterfaceType != pInterfaceTypeCanonical)
{
return pMD;
InterfaceMapIterator it = IterateInterfaceMap();
while (it.Next())
{
if (pInterfaceTypeCanonical == it.GetInterface())
{
canonicalEquivalentFound = true;
break;
return NULL;
}
}
}

if (!canonicalEquivalentFound)
{
// 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);
if (pMD != nullptr)
{
return pMD;
}
}
}
}

Expand All @@ -9194,7 +9222,7 @@ MethodTable::ResolveVirtualStaticMethod(MethodDesc* pInterfaceMD, BOOL allowInst
// Try to locate the appropriate MethodImpl matching a given interface static virtual method.
// Returns nullptr on failure.
MethodDesc*
MethodTable::TryResolveVirtualStaticMethodOnThisType(MethodDesc* pInterfaceMD, BOOL allowInstParam)
MethodTable::TryResolveVirtualStaticMethodOnThisType(MethodTable* pInterfaceType, MethodDesc* pInterfaceMD)
{
HRESULT hr = S_OK;
IMDInternalImport* pMDInternalImport = GetMDImport();
Expand All @@ -9210,6 +9238,8 @@ MethodTable::TryResolveVirtualStaticMethodOnThisType(MethodDesc* pInterfaceMD, B
// This gets the count out of the metadata interface.
uint32_t dwNumberMethodImpls = hEnumMethodImpl.EnumMethodImplGetCount();

// TODO: support type-equivalent interface type matches and variant interface scenarios

// Iterate through each MethodImpl declared on this class
for (uint32_t i = 0; i < dwNumberMethodImpls; i++)
{
Expand Down Expand Up @@ -9237,8 +9267,7 @@ MethodTable::TryResolveVirtualStaticMethodOnThisType(MethodDesc* pInterfaceMD, B
tkParent,
&sigTypeContext)
.GetMethodTable();
if (pInterfaceMT != pInterfaceMD->GetMethodTable() &&
pInterfaceMT->GetCanonicalMethodTable() != pInterfaceMD->GetMethodTable())
if (pInterfaceMT != pInterfaceType)
{
continue;
}
Expand All @@ -9256,6 +9285,13 @@ MethodTable::TryResolveVirtualStaticMethodOnThisType(MethodDesc* pInterfaceMD, B
{
continue;
}

// Spec requires that all body token for MethodImpls that refer to static virtual implementation methods must be MethodDef tokens.
if (TypeFromToken(methodBody) != mdtMethodDef)
{
COMPlusThrow(kTypeLoadException, E_FAIL);
}

MethodDesc *pMethodImpl = MemberLoader::GetMethodDescFromMemberDefOrRefOrSpec(
GetModule(),
methodBody,
Expand All @@ -9267,9 +9303,16 @@ MethodTable::TryResolveVirtualStaticMethodOnThisType(MethodDesc* pInterfaceMD, B
COMPlusThrow(kTypeLoadException, E_FAIL);
}

// Spec requires that all body token for MethodImpls that refer to static virtual implementation methods must to methods
// defined on the same type that defines the MethodImpl
if (!HasSameTypeDefAs(pMethodImpl->GetMethodTable()))
{
COMPlusThrow(kTypeLoadException, E_FAIL);
}

if (pInterfaceMD->HasMethodInstantiation() || pMethodImpl->HasMethodInstantiation() || HasInstantiation())
{
return pMethodImpl->FindOrCreateAssociatedMethodDesc(pMethodImpl, this, FALSE, pInterfaceMD->GetMethodInstantiation(), allowInstParam);
return pMethodImpl->FindOrCreateAssociatedMethodDesc(pMethodImpl, this, FALSE, pInterfaceMD->GetMethodInstantiation(), /* allowInstParam */ FALSE);
}
else
{
Expand All @@ -9292,7 +9335,6 @@ MethodDesc *
MethodTable::TryResolveConstraintMethodApprox(
TypeHandle thInterfaceType,
MethodDesc * pInterfaceMD,
BOOL allowInstParam,
BOOL * pfForceUseRuntimeLookup) // = NULL
{
CONTRACTL {
Expand All @@ -9302,7 +9344,9 @@ MethodTable::TryResolveConstraintMethodApprox(

if (pInterfaceMD->IsStatic())
{
MethodDesc *result = ResolveVirtualStaticMethod(pInterfaceMD, allowInstParam, pfForceUseRuntimeLookup != NULL);
_ASSERTE(!thInterfaceType.IsTypeDesc());
_ASSERTE(thInterfaceType.IsInterface());
MethodDesc *result = ResolveVirtualStaticMethod(thInterfaceType.GetMethodTable(), pInterfaceMD, pfForceUseRuntimeLookup != NULL);
if (result == NULL)
{
*pfForceUseRuntimeLookup = TRUE;
Expand Down
5 changes: 2 additions & 3 deletions src/coreclr/vm/methodtable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2280,7 +2280,7 @@ class MethodTable


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

// Try a partial resolve of the constraint call, up to generic code sharing.
//
Expand All @@ -2297,7 +2297,6 @@ class MethodTable
MethodDesc * TryResolveConstraintMethodApprox(
TypeHandle ownerType,
MethodDesc * pMD,
BOOL fExactResolution = FALSE,
BOOL * pfForceUseRuntimeLookup = NULL);

//-------------------------------------------------------------------
Expand Down Expand Up @@ -2398,7 +2397,7 @@ class MethodTable

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

public:
static MethodDesc *MapMethodDeclToMethodImpl(MethodDesc *pMDDecl);
Expand Down
1 change: 0 additions & 1 deletion src/tests/Loader/classloader/StaticVirtualMethods/GenericContext/Generator/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,6 @@ static void Main(string[] args)

string expectedString = constrainedTypePrefix + AppendSuffixToConstrainedType(scenario, GetConstrainedTypeName(scenario.ConstrainedTypeDefinition, withImplPrefix: false), out _) + "'" + interfaceTypeSansImplPrefix + "." + interfaceMethodRoot + "'" + interfaceMethodInstantiation;
expectedString = expectedString.Replace(ImplPrefix, "");
expectedString = expectedString.Replace("<class GenericClass`1<!!0>>", "<class GenericClass`1<!0>>");

if (scenario.CallerScenario == CallerMethodScenario.NonGeneric)
{
Expand Down
Loading