Skip to content

Commit

Permalink
Fix devirtualization in shared generic context
Browse files Browse the repository at this point in the history
In circumstances where the JIT doesn't provide exact enough details about the impl type and interface method to identify exactly which method should be called

- In particular, when the impl class implements multiple copies of the target interface, and they are canonically compatible with the interface method that is to be called
- Simply disable devirtualization for these scenarios

Fixes #51982
  • Loading branch information
davidwrighton authored May 26, 2021
1 parent 1883505 commit 5c875e4
Show file tree
Hide file tree
Showing 4 changed files with 108 additions and 0 deletions.
22 changes: 22 additions & 0 deletions src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,28 @@ protected virtual MethodDesc ResolveVirtualMethod(MethodDesc declMethod, DefType

if (declMethod.OwningType.IsInterface)
{
if (declMethod.OwningType.IsCanonicalSubtype(CanonicalFormKind.Any) || implType.IsCanonicalSubtype(CanonicalFormKind.Any))
{
DefType[] implTypeRuntimeInterfaces = implType.RuntimeInterfaces;
int canonicallyMatchingInterfacesFound = 0;
DefType canonicalInterfaceType = (DefType)declMethod.OwningType.ConvertToCanonForm(CanonicalFormKind.Specific);
for (int i = 0; i < implTypeRuntimeInterfaces.Length; i++)
{
DefType runtimeInterface = implTypeRuntimeInterfaces[i];
if (canonicalInterfaceType.HasSameTypeDefinition(runtimeInterface) &&
runtimeInterface.ConvertToCanonForm(CanonicalFormKind.Specific) == canonicalInterfaceType)
{
canonicallyMatchingInterfacesFound++;
if (canonicallyMatchingInterfacesFound > 1)
{
// We cannot resolve the interface as we don't know with exact enough detail which interface
// of multiple possible interfaces is being called.
return null;
}
}
}
}

impl = implType.ResolveInterfaceMethodTarget(declMethod);
if (impl != null)
{
Expand Down
17 changes: 17 additions & 0 deletions src/coreclr/vm/jitinterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8964,6 +8964,23 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info)
if (pObjMT->IsSharedByGenericInstantiations())
{
pOwnerMT = pOwnerMT->GetCanonicalMethodTable();

// Check to see if the derived class implements multiple variants of a matching interface.
// If so, we cannot predict exactly which implementation is in use here.
MethodTable::InterfaceMapIterator it = pObjMT->IterateInterfaceMap();
int canonicallyMatchingInterfacesFound = 0;
while (it.Next())
{
if (it.GetInterface()->GetCanonicalMethodTable() == pOwnerMT)
{
canonicallyMatchingInterfacesFound++;
if (canonicallyMatchingInterfacesFound > 1)
{
// Multiple canonically identical interfaces found when attempting to devirtualize an inexact interface dispatch
return false;
}
}
}
}

pDevirtMD = pObjMT->GetMethodDescForInterfaceMethod(TypeHandle(pOwnerMT), pBaseMD, FALSE /* throwOnConflict */);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
using System;
using System.Runtime.CompilerServices;


public class MultipleCanonicallyCompatibleImplementations
{
static int Main()
{
string atom1Call = Foo<Atom1>.Call();
string atom2Call = Foo<Atom2>.Call();
Console.WriteLine($"Atom1Call `{atom1Call}`");
Console.WriteLine($"Atom2Call `{atom2Call}`");

if (atom1Call != "FooBaseFooBaseFoo")
{
Console.WriteLine("Atom1Call should be FooBaseFooBaseFoo");
return 1;
}
if (atom2Call != "FooFooFooBaseFoo")
{
Console.WriteLine("Atom2Call should be FooFooFooBaseFoo");
return 2;
}

return 100;
}
}

interface IFooable<T>
{
public string DoFoo(T x);
}

class Base : IFooable<Atom2>
{
string IFooable<Atom2>.DoFoo(Atom2 x) => "Base";
}

sealed class Foo<T> : Base, IFooable<T>
{
string IFooable<T>.DoFoo(T x) => "Foo";

[MethodImpl(MethodImplOptions.NoInlining)]
public static string Call()
{
var f = new Foo<T>();
var fA1 = new Foo<Atom1>();
var fA2 = new Foo<Atom2>();
return ((IFooable<T>)f).DoFoo(default) + ((IFooable<Atom2>)f).DoFoo(null)
+ ((IFooable<Atom1>)fA1).DoFoo(default) + ((IFooable<Atom2>)fA1).DoFoo(null)
+ ((IFooable<Atom2>)fA2).DoFoo(default);
}
}

class Atom1 { }
class Atom2 { }
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<CLRTestPriority>1</CLRTestPriority>
</PropertyGroup>
<PropertyGroup>
<DebugType>PdbOnly</DebugType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="MultipleCanonicallyCompatibleImplementations.cs" />
</ItemGroup>
</Project>

0 comments on commit 5c875e4

Please sign in to comment.