Fix for issue #80350 tracking runtime deficiencies in the presence of static virtual method reabstraction (#80987)

This change fixes the issue #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. Based on discussion with the
Mono team I have had to temporarily disable the svm_diamondshape
test on Mono before the correct fix is found.

Thanks

Tomas

Author: trylek

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,11 @@ VOID MethodTableBuilder::TestMethodImpl(
     {
         BuildMethodTableThrowException(IDS_CLASSLOAD_MI_NONVIRTUAL_DECL);
     }
-    if ((IsMdVirtual(dwImplAttrs) && IsMdStatic(dwImplAttrs)) || (!IsMdVirtual(dwImplAttrs) && !IsMdStatic(dwImplAttrs)))
+    if (!!IsMdVirtual(dwImplAttrs) != !!IsMdAbstract(dwImplAttrs) && IsMdStatic(dwImplAttrs))
+    {
+        BuildMethodTableThrowException(IDS_CLASSLOAD_MI_MUSTBEVIRTUAL);
+    }
+    if (!IsMdVirtual(dwImplAttrs) && !IsMdStatic(dwImplAttrs))
     {
         BuildMethodTableThrowException(IDS_CLASSLOAD_MI_MUSTBEVIRTUAL);
     }
@@ -5636,7 +5648,10 @@ MethodTableBuilder::ProcessMethodImpls()
     DeclaredMethodIterator it(*this);
     while (it.Next())
     {
-        if (!IsMdVirtual(it.Attrs()) && it.IsMethodImpl() && bmtProp->fNoSanityChecks)
+        bool isVirtualStaticOverride = it.IsMethodImpl() && IsMdStatic(it.Attrs()) &&
+            !!IsMdVirtual(it.Attrs()) == !!IsMdAbstract(it.Attrs());
+
+        if (isVirtualStaticOverride && bmtProp->fNoSanityChecks)
         {
             // Non-virtual methods can only be classified as methodImpl when implementing
             // static virtual methods.
@@ -5819,7 +5834,7 @@ MethodTableBuilder::ProcessMethodImpls()
                                 }
 
                                 // 3. Find the matching method.
-                                declMethod = FindDeclMethodOnInterfaceEntry(pItfEntry, declSig, !IsMdVirtual(it.Attrs())); // Search for statics when the impl is non-virtual
+                                declMethod = FindDeclMethodOnInterfaceEntry(pItfEntry, declSig, isVirtualStaticOverride); // Search for statics when the impl is non-virtual
                             }
                             else
                             {
@@ -5854,7 +5869,7 @@ MethodTableBuilder::ProcessMethodImpls()
                         BuildMethodTableThrowException(IDS_CLASSLOAD_MI_MUSTBEVIRTUAL, it.Token());
                     }
 
-                    if (!IsMdVirtual(it.Attrs()) && it.IsMethodImpl() && IsMdStatic(it.Attrs()))
+                    if (isVirtualStaticOverride)
                     {
                         // Non-virtual methods can only be classified as methodImpl when implementing
                         // static virtual methods.

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)

src/tests/Loader/classloader/StaticVirtualMethods/RegressionTests/GitHub_80350.cs

@@ -0,0 +1,40 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+
+// This regression test tracks the issue where implementation of a static virtual method
+// on a derived type is not found when there is a re-abstraction of the same method
+// higher in inheritance hierarchy.
+
+class Test1 : I2
+{
+
+    static int Main()
+    {
+        string result = Test<Test1>();
+        const string expectedResult = "Test1.M1";
+        Console.WriteLine("Expected {0}, found {1}: {2}", expectedResult, result, expectedResult == result ? "match" : "mismatch");
+        return result == expectedResult ? 100 : 101;
+    }
+
+    static string Test<i1>() where i1 : I1
+    {
+        return i1.M1();
+    }
+
+    static string I1.M1()
+    {
+        return "Test1.M1";
+    }
+}
+
+public interface I1
+{
+    static abstract string M1();
+}
+
+public interface I2 : I1
+{
+    static abstract string I1.M1();
+}

src/tests/Loader/classloader/StaticVirtualMethods/RegressionTests/GitHub_80350.csproj

@@ -0,0 +1,9 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <OutputType>Exe</OutputType>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>

src/tests/issues.targets

@@ -1525,6 +1525,9 @@
         <ExcludeList Include="$(XunitTestBinBase)/Loader/classloader/MethodImpl/CovariantReturns/UnitTest/CompatibleWithTest/**">
             <Issue>Doesn't pass after LLVM AOT compilation.</Issue>
         </ExcludeList>
+        <ExcludeList Include="$(XunitTestBinBase)/Loader/classloader/StaticVirtualMethods/DiamondShape/**">
+            <Issue>https://github.com/dotnet/runtime/issues/80350</Issue>
+        </ExcludeList>
         <ExcludeList Include="$(XunitTestBinBase)/Loader/classloader/StaticVirtualMethods/InterfaceVariance/**">
             <Issue>Static virtual methods are not yet implemented in the Mono runtime.</Issue>
         </ExcludeList>