Remove legacy behavior around non-virtual interface calls (dotnet/cor…

…eclr#23032) * Throw BadImageFormat for direct calls to abstract methods * Remove legacy behavior around non-virtual interface calls * Try fixing failing tests The test we inherited from the default interface method prototype branch is doing exactly the thing it shouldn't do (rely on the bad behavior) for unexplained reasons. Commit migrated from dotnet/coreclr@502de2a
gugavaro · Mar 7, 2019 · 1d84d6b · 1d84d6b
1 parent bdcd6b8
commit 1d84d6b
Show file tree

Hide file tree

Showing 4 changed files with 165 additions and 11 deletions.
diff --git a/src/coreclr/src/vm/jitinterface.cpp b/src/coreclr/src/vm/jitinterface.cpp
@@ -5295,6 +5295,7 @@ void CEEInfo::getCallInfo(
     //
 
     MethodDesc * pTargetMD = pMDAfterConstraintResolution;
+    DWORD dwTargetMethodAttrs = pTargetMD->GetAttrs();
 
     if (pTargetMD->HasMethodInstantiation())
     {
@@ -5352,12 +5353,6 @@ void CEEInfo::getCallInfo(
         directCall = true;
     }
     else
-    // Backwards compat: calls to abstract interface methods are treated as callvirt
-    if (pTargetMD->GetMethodTable()->IsInterface() && pTargetMD->IsAbstract())
-    {
-        directCall = false;
-    }
-    else
     if (!(flags & CORINFO_CALLINFO_CALLVIRT) || fResolvedConstraint)
     {
         directCall = true;
@@ -5398,12 +5393,11 @@ void CEEInfo::getCallInfo(
         if (pTargetMD->GetMethodTable()->IsInterface())
         {
             // Handle interface methods specially because the Sealed bit has no meaning on interfaces.
-            devirt = !IsMdVirtual(pTargetMD->GetAttrs());
+            devirt = !IsMdVirtual(dwTargetMethodAttrs);
         }
         else
         {
-            DWORD dwMethodAttrs = pTargetMD->GetAttrs();
-            devirt = !IsMdVirtual(dwMethodAttrs) || IsMdFinal(dwMethodAttrs) || pTargetMD->GetMethodTable()->IsSealed();
+            devirt = !IsMdVirtual(dwTargetMethodAttrs) || IsMdFinal(dwTargetMethodAttrs) || pTargetMD->GetMethodTable()->IsSealed();
         }
 
         if (devirt)
@@ -5415,6 +5409,14 @@ void CEEInfo::getCallInfo(
 
     if (directCall)
     {
+        // Direct calls to abstract methods are not allowed
+        if (IsMdAbstract(dwTargetMethodAttrs) &&
+            // Compensate for always treating delegates as direct calls above
+            !(((flags & CORINFO_CALLINFO_LDFTN) && (flags & CORINFO_CALLINFO_CALLVIRT) && !resolvedCallVirt)))
+        {
+            COMPlusThrowHR(COR_E_BADIMAGEFORMAT, BFA_BAD_IL);
+        }
+
         bool allowInstParam = (flags & CORINFO_CALLINFO_ALLOWINSTPARAM);
 
         // Create instantiating stub if necesary

diff --git a/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/valuetypes/valuetypes.il b/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/valuetypes/valuetypes.il
@@ -58,15 +58,15 @@
              int32 V_1)
     IL_0000:  nop
     IL_0001:  ldarg.0
-    IL_0002:  call       instance int32 IValue::GetValue()
+    IL_0002:  callvirt   instance int32 IValue::GetValue()
     IL_0007:  stloc.0
     IL_0008:  ldloc.0
     IL_0009:  ldarg.1
     IL_000a:  add
     IL_000b:  stloc.0
     IL_000c:  ldarg.0
     IL_000d:  ldloc.0
-    IL_000e:  call       instance void IValue::SetValue(int32)
+    IL_000e:  callvirt   instance void IValue::SetValue(int32)
     IL_0013:  nop
     IL_0014:  ldloc.0
     IL_0015:  stloc.1

diff --git a/src/coreclr/tests/src/Regressions/coreclr/22407/abstractcalls.il b/src/coreclr/tests/src/Regressions/coreclr/22407/abstractcalls.il
@@ -0,0 +1,117 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+//
+
+.assembly extern mscorlib { }
+
+.assembly abstractcalls { }
+
+.class interface public abstract auto ansi I1
+{
+  .method public hidebysig newslot virtual abstract
+          instance int32 Add(int32 x) cil managed
+  {
+  }
+}
+
+.class public abstract auto ansi C1
+  extends [mscorlib]System.Object
+  implements I1
+{
+  .method public hidebysig specialname rtspecialname 
+          instance void .ctor() cil managed
+  {
+    ldarg.0
+    call instance void [mscorlib]System.Object::.ctor()
+    ret
+  }
+
+  .method public hidebysig newslot virtual abstract
+          instance int32 Remove(int32 x) cil managed
+  {
+  }
+}
+
+.class public auto ansi C2
+  extends [mscorlib]System.Object
+  implements I1
+{
+  .method public hidebysig specialname rtspecialname 
+          instance void .ctor() cil managed
+  {
+    ldarg.0
+    call instance void C1::.ctor()
+    ret
+  }
+
+  .method public hidebysig virtual
+          instance int32 Remove(int32 x) cil managed
+  {
+    ldc.i4.0
+    ret
+  }
+
+  .method public hidebysig newslot virtual
+          instance int32 Add(int32 x) cil managed
+  {
+    ldc.i4.5
+    ret
+  }
+}
+
+.method private hidebysig static void CallClass() cil managed
+{
+  newobj instance void C2::.ctor()
+  ldc.i4.0
+  call instance int32 C1::Remove(int32)
+  pop
+  ret
+}
+
+.method private hidebysig static void CallInterface() cil managed
+{
+  newobj instance void C2::.ctor()
+  ldc.i4.0
+  call instance int32 I1::Add(int32)
+  pop
+  ret
+}
+
+.method private hidebysig static int32 Main() cil managed
+{
+  .entrypoint
+
+  .try
+  {
+    call void CallClass()
+    leave Fail
+  }
+  catch [System.Runtime]System.BadImageFormatException
+  {
+    pop
+    leave AbstractClassOK
+  }
+
+AbstractClassOK:
+
+  .try
+  {
+    call void CallInterface()
+    leave Fail
+  }
+  catch [System.Runtime]System.BadImageFormatException
+  {
+    pop
+    leave AbstractInterfaceOK
+  }
+
+AbstractInterfaceOK:
+
+  ldc.i4 100
+  ret
+
+Fail:
+  ldc.i4.m1
+  ret
+}
diff --git a/src/coreclr/tests/src/Regressions/coreclr/22407/abstractcalls.ilproj b/src/coreclr/tests/src/Regressions/coreclr/22407/abstractcalls.ilproj
@@ -0,0 +1,35 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <AssemblyName>$(MSBuildProjectName)</AssemblyName>
+    <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <SchemaVersion>2.0</SchemaVersion>
+    <ProjectGuid>{85DFC527-4DB1-595E-A7D7-E94EE1F8140D}</ProjectGuid>
+    <FileAlignment>512</FileAlignment>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <NuGetPackageImportStamp>7a9bfb7d</NuGetPackageImportStamp>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <ReferenceLocalMscorlib>true</ReferenceLocalMscorlib>
+    <OutputType>Exe</OutputType>
+    <CLRTestKind>BuildAndRun</CLRTestKind>
+    <CLRTestPriority>0</CLRTestPriority>
+  </PropertyGroup>
+
+  <ItemGroup>
+    <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
+      <Visible>False</Visible>
+    </CodeAnalysisDependentAssemblyPaths>
+  </ItemGroup>
+
+  <ItemGroup>
+    <Compile Include="abstractcalls.il" />
+  </ItemGroup>
+
+
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+</Project>