Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect devirtualization in shared generic context #51982

Closed
MichalStrehovsky opened this issue Apr 28, 2021 · 4 comments · Fixed by #53255
Closed

Incorrect devirtualization in shared generic context #51982

MichalStrehovsky opened this issue Apr 28, 2021 · 4 comments · Fixed by #53255
Assignees
Milestone

Comments

@MichalStrehovsky
Copy link
Member

using Internal;
using System;
using System.Runtime.CompilerServices;

Console.WriteLine(Foo<Atom1>.Call());
Console.WriteLine(Foo<Atom2>.Call());


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>();
        return ((IFooable<T>)f).DoFoo(default) + ((IFooable<Atom2>)f).DoFoo(null);
    }
}

class Atom1 { }
class Atom2 { }

Compile the above with csc /noconfig /nostdlib Program.cs /r:System.Private.CoreLib.dll and run with corerun.

Output:

FooBase
FooFoo

Now add /O to the csc command line.

Output:

FooFoo
FooFoo
@MichalStrehovsky MichalStrehovsky added this to the 6.0.0 milestone Apr 28, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 28, 2021
@MichalStrehovsky
Copy link
Member Author

Note this hits problem in both the VM and in crossgen2. The repro steps are without R2R, but there's a similar problem when compiled with crossgen2.

@mangod9 mangod9 removed the untriaged New issue has not been triaged by the area owner label Apr 29, 2021
@mangod9
Copy link
Member

mangod9 commented Apr 29, 2021

Assigning to @trylek since he might be potentially working in the related typesystem code.

@AndyAyersMS
Copy link
Member

For the runtime, behavior changes when the jit is optimizing:

set complus_jitprintdevirtualizedmethods=*
set complus_tieredcompilation=

D:\bugs\r51918> c:\repos\runtime2\artifacts\tests\coreclr\Windows.x64.Checked\Tests\Core_Root\corerun.exe ex.exe
Devirtualized interface call to IFooable`1[__Canon]:DoFoo; now direct call to Foo`1[__Canon]:IFooable<T>.DoFoo [exact]

FooBase
FooFoo

D:\bugs\r51918>set complus_tieredcompilation=0

D:\bugs\r51918> c:\repos\runtime2\artifacts\tests\coreclr\Windows.x64.Checked\Tests\Core_Root\corerun.exe ex.exe
Devirtualized interface call to IFooable`1[__Canon]:DoFoo; now direct call to Foo`1[__Canon]:IFooable<T>.DoFoo [exact]
Devirtualized interface call to IFooable`1[__Canon]:DoFoo; now direct call to Foo`1[__Canon]:IFooable<T>.DoFoo [exact]
...

FooFoo
FooFoo

I think the issue is here in resolveVirtualMethod where we discard part of the owner type:

// For generic interface methods we must have context to
// safely devirtualize.
if (info->context != nullptr)
{
TypeHandle OwnerClsHnd = GetTypeFromContext(info->context);
MethodTable* pOwnerMT = OwnerClsHnd.GetMethodTable();
// If the derived class is a shared class, make sure the
// owner class is too.
if (pObjMT->IsSharedByGenericInstantiations())
{
pOwnerMT = pOwnerMT->GetCanonicalMethodTable();
}
pDevirtMD = pObjMT->GetMethodDescForInterfaceMethod(TypeHandle(pOwnerMT), pBaseMD, FALSE /* throwOnConflict */);
}

Seems like instead, if we're presented with an exact owner type but shared interface type, we need to project the interface type to the corresponding exact interface type, instead of projecting the owner type to a shared type?

Not sure if this is doable...

@davidwrighton davidwrighton self-assigned this May 24, 2021
@davidwrighton
Copy link
Member

@AndyAyersMS, its absolutely do-able, but what I think is a more reasonable approach, is that a devirtualize to IFooable<__Canon> on a type which only implements 1 possible instantiation should perform the devirtualization, but when there are 2 possible candidate target types, to skip devirtualization. Multiple implementations of the same interface on a type are rare and generally not worth writing the extremely fragile logic that can make it work. I'm looking at that now.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 25, 2021
davidwrighton added a commit that referenced this issue May 26, 2021
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
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants