Fix remaining failing test issues in current testbed (#13)

- Fix crashing issue where wrong instantiation parameter is passed
  - Instead use the CORINFO_CALL_CODE_POINTER infrastructure to simply use an exact function pointer
  - Additionally, my previous attempt to fix a different bug added the concept of allowing instantiation parameters to be required when using resolving the static virtual at compile time. In order to reliably use the CORINFO_CALL_CODE_POINTER infrastructure, that logic was deleted and removed.
- Fix issue where the type resolved via ResolveVirtualStaticMethod was on a slighly different interface type
  - Fix by passing the interface type pointer to ResolveVirtualStaticMethod
  - And by performing exact interface type checks in TryResolveVirtualStaticMethodOnThisType
- Finally there was 1 remaining bug in the test generator for the GenericContextTest where the correct string to test for was not generated
  - Fix by deleting a hack used earlier to get closer to passing
- Drive by feature to verify that the MethodBody on a MethodImpl follows the rules around being a MethodDef and being implemented by the type that defines the MethodImpl.

Author: davidwrighton

src/coreclr/vm/genericdict.cpp

@@ -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);
 

src/coreclr/vm/jitinterface.cpp

@@ -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
@@ -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())
         {

src/coreclr/vm/methodtable.cpp

@@ -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;
+                }
+            }
         }
     }
 
@@ -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();
@@ -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++)
     {
@@ -9237,8 +9267,7 @@ MethodTable::TryResolveVirtualStaticMethodOnThisType(MethodDesc* pInterfaceMD, B
             tkParent,
             &sigTypeContext)
             .GetMethodTable();
-        if (pInterfaceMT != pInterfaceMD->GetMethodTable() &&
-            pInterfaceMT->GetCanonicalMethodTable() != pInterfaceMD->GetMethodTable())
+        if (pInterfaceMT != pInterfaceType)
         {
             continue;
         }
@@ -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,
@@ -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
         {
@@ -9292,7 +9335,6 @@ MethodDesc *
 MethodTable::TryResolveConstraintMethodApprox(
     TypeHandle   thInterfaceType,
     MethodDesc * pInterfaceMD,
-    BOOL         allowInstParam,
     BOOL *       pfForceUseRuntimeLookup)   // = NULL
 {
     CONTRACTL {
@@ -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;

src/coreclr/vm/methodtable.h

@@ -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.
     //
@@ -2297,7 +2297,6 @@ class MethodTable
     MethodDesc * TryResolveConstraintMethodApprox(
         TypeHandle   ownerType,
         MethodDesc * pMD,
-        BOOL         fExactResolution = FALSE,
         BOOL *       pfForceUseRuntimeLookup = NULL);
 
     //-------------------------------------------------------------------
@@ -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);

src/tests/Loader/classloader/StaticVirtualMethods/GenericContext/Generator/Program.cs

@@ -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)
                 {