Improve runtime checks to understand the SVM + unit test fix

This change fixes the issue dotnet#80350 that uncovered several inconsistencies in the CoreCLR runtime w.r.t. static virtual methods. In particular, reabstraction (declaring an abstract explicit override of a static virtual method in a derived interface) turned out to be causing runtime issues. As part of the change I'm also fixing the reabstraction unit test svm_diamondshape as it turns out Roslyn emits the method flags in a slightly different manner than I originally expected in the hand-written IL test. Thanks Tomas
trylek · Feb 17, 2023 · 0130dd3 · 0130dd3
1 parent dec1445
commit 0130dd3
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 5 deletions.
diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp
@@ -2868,7 +2868,8 @@ MethodTableBuilder::EnumerateClassMethods()
         }
 
         // Check for the presence of virtual static methods
-        if (IsMdVirtual(dwMemberAttrs) && IsMdStatic(dwMemberAttrs))
+        bool isStaticVirtual = (IsMdVirtual(dwMemberAttrs) && IsMdStatic(dwMemberAttrs));
+        if (isStaticVirtual)
         {
             bmtProp->fHasVirtualStaticMethods = TRUE;
         }
@@ -3202,7 +3203,7 @@ MethodTableBuilder::EnumerateClassMethods()
             BuildMethodTableThrowException(BFA_GENERIC_METHODS_INST);
         }
 
-        // count how many overrides this method does All methods bodies are defined
+        // Check if the method is a MethodImpl body. All method bodies are defined
         // on this type so we can just compare the tok with the body token found
         // from the overrides.
         implType = METHOD_IMPL_NOT;
@@ -3215,6 +3216,13 @@ MethodTableBuilder::EnumerateClassMethods()
             }
         }
 
+        if (implType == METHOD_IMPL && isStaticVirtual && IsMdAbstract(dwMemberAttrs))
+        {
+            // Don't record reabstracted static virtual methods as they don't constitute
+            // actual method slots, they're just markers used by the static virtual lookup.
+            continue;
+        }
+
         // For delegates we don't allow any non-runtime implemented bodies
         // for any of the four special methods
         if (IsDelegate() && !IsMiRuntime(dwImplFlags))
@@ -4788,7 +4796,7 @@ VOID MethodTableBuilder::TestMethodImpl(
     {
         BuildMethodTableThrowException(IDS_CLASSLOAD_MI_NONVIRTUAL_DECL);
     }
-    if (!!IsMdVirtual(dwImplAttrs) != !!IsInterface() && IsMdStatic(dwImplAttrs))
+    if (!!IsMdVirtual(dwImplAttrs) != !!IsMdAbstract(dwImplAttrs) && IsMdStatic(dwImplAttrs))
     {
         BuildMethodTableThrowException(IDS_CLASSLOAD_MI_MUSTBEVIRTUAL);
     }
@@ -5640,7 +5648,8 @@ MethodTableBuilder::ProcessMethodImpls()
     DeclaredMethodIterator it(*this);
     while (it.Next())
     {
-        bool isVirtualStaticOverride = it.IsMethodImpl() && IsMdStatic(it.Attrs()) && !!IsMdVirtual(it.Attrs()) == !!IsInterface();
+        bool isVirtualStaticOverride = it.IsMethodImpl() && IsMdStatic(it.Attrs()) &&
+            !!IsMdVirtual(it.Attrs()) == !!IsMdAbstract(it.Attrs());
 
         if (isVirtualStaticOverride && bmtProp->fNoSanityChecks)
         {

diff --git a/src/tests/Loader/classloader/StaticVirtualMethods/DiamondShape/svm_diamondshape.il b/src/tests/Loader/classloader/StaticVirtualMethods/DiamondShape/svm_diamondshape.il
@@ -232,7 +232,7 @@
 .class interface private abstract auto ansi I4Reabstract
        implements I4
 {
-  .method private hidebysig abstract
+  .method private hidebysig virtual abstract
           static int32  Func(int32 a) cil managed
   {
     .override method int32 I1::Func(int32)