Address David Wrighton's PR feedback

* Added stricter check to delegate constructor * Disabled MethodBodyOnUnrelatedType test in Crossgen2 mode * Reverted change to virtual method resolver Thanks Tomas
dotnet · Aug 8, 2023 · 1374c32 · 1374c32
1 parent 26dd5cf
commit 1374c32
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 21 deletions.
diff --git a/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs
@@ -1003,28 +1003,11 @@ private static MethodDesc TryResolveVirtualStaticMethodOnThisType(MetadataType c
                 if (methodImpl.Decl == interfaceMethodDefinition)
                 {
                     MethodDesc resolvedMethodImpl = methodImpl.Body;
-                    TypeDesc bodyOwningType = resolvedMethodImpl.OwningType;
-                    TypeDesc checkOwningType = constrainedType;
-                    bool isValidMethodImpl = (bodyOwningType == checkOwningType);
-                    if (!isValidMethodImpl)
-                    {
-                        while ((checkOwningType = checkOwningType.BaseType) != null)
-                        {
-                            if (bodyOwningType == checkOwningType)
-                            {
-                                isValidMethodImpl = true;
-                                break;
-                            }
-                        }
-                    }
-                    if (isValidMethodImpl)
+                    if (interfaceMethod != interfaceMethodDefinition)
                     {
-                        if (interfaceMethod != interfaceMethodDefinition)
-                        {
-                            resolvedMethodImpl = resolvedMethodImpl.MakeInstantiatedMethod(interfaceMethod.Instantiation);
-                        }
-                        return resolvedMethodImpl;
+                        resolvedMethodImpl = resolvedMethodImpl.MakeInstantiatedMethod(interfaceMethod.Instantiation);
                     }
+                    return resolvedMethodImpl;
                 }
             }
 

diff --git a/...aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DelegateCtorSignature.cs b/...aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DelegateCtorSignature.cs
@@ -42,12 +42,20 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)
             {
                 SignatureContext innerContext = builder.EmitFixup(factory, ReadyToRunFixupKind.DelegateCtor, _methodToken.Token.Module, factory.SignatureContext);
 
+                bool needsInstantiatingStub = _targetMethod.Method.HasInstantiation;
+                if (_targetMethod.Method.IsVirtual && _targetMethod.Method.Signature.IsStatic)
+                {
+                    // For static virtual methods, we always require an instantiating stub as the method may resolve to a canonical representation
+                    // at runtime without us being able to detect that at compile time.
+                    needsInstantiatingStub |= (_targetMethod.Method.OwningType.HasInstantiation || _methodToken.ConstrainedType != null);
+                }
+
                 builder.EmitMethodSignature(
                     _methodToken,
                     enforceDefEncoding: false,
                     enforceOwningType: false,
                     innerContext,
-                    isInstantiatingStub: _targetMethod.Method.HasInstantiation || _targetMethod.Method.OwningType.HasInstantiation || _methodToken.ConstrainedType != null);
+                    isInstantiatingStub: needsInstantiatingStub);
 
                 builder.EmitTypeSignature(_delegateType, innerContext);
             }

diff --git a/...oader/classloader/StaticVirtualMethods/NegativeTestCases/MethodBodyOnUnrelatedType.ilproj b/...oader/classloader/StaticVirtualMethods/NegativeTestCases/MethodBodyOnUnrelatedType.ilproj
@@ -1,6 +1,9 @@
 <Project Sdk="Microsoft.NET.Sdk.IL">
   <PropertyGroup>
     <OutputType>Exe</OutputType>
+
+    <!-- Crossgen2 currently doesn't support this negative check - that should be fine as runtime behavior is undefined in the presence of invalid IL. -->
+    <CrossGenTest>false</CrossGenTest>
   </PropertyGroup>
   <PropertyGroup>
     <DebugType>Full</DebugType>