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

Fix devirtualization in crossgen2 #146

Merged
merged 2 commits into from
Nov 21, 2019
Merged

Fix devirtualization in crossgen2 #146

merged 2 commits into from
Nov 21, 2019

Conversation

janvorli
Copy link
Member

In some casesi that are only expressible in IL, devirtualization is
choosing a wrong virtual method. This causes a failure of the
JIT\Regression\JitBlue\GitHub_19222 test.

The test uses the following class hierarchy:

class A
{
    public virtual void f() {}
}

class B : A
{
    public override void f() {}
}

class C : B
{
    public new void f() {}
}

class D : C
{
    public new void f() {}
}

Then there is a local of type C to which we assign a reference to D.
After that the following virtual call is made:

callvirt instance int32 modopt([mscorlib]System.Runtime.CompilerServices.IsLong) A::f()

Devirtualization will choose C.f to call instead of the D.f.

This change disables devirtualization in such cases. Old crossgen does
the same thing.

In some casesi that are only expressible in IL, devirtualization is
choosing a wrong virtual method. This causes a failure of the
JIT\Regression\JitBlue\GitHub_19222 test.

The test uses the following class hierarchy:
class A
{
    public virtual void f() {}
}

class B : A
{
    public override void f() {}
}

class C : B
{
    public new void f() {}
}

class D : C
{
    public new void f() {}
}

Then there is a local of type C to which we assign a reference to D.
After that the following virtual call is made:

callvirt instance int32 modopt([mscorlib]System.Runtime.CompilerServices.IsLong) A::f()

Devirtualization will choose C.f to call instead of the D.f.

This change disables devirtualization in such cases. Old crossgen does
the same thing.
@janvorli janvorli added this to the 5.0 milestone Nov 20, 2019
@janvorli janvorli self-assigned this Nov 20, 2019
Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for fixing this!

Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix to use the slot defining method check instead of IsNewSlot and such. The technique used is incorrect if the impl method is a non-newslot method that overrides a newslot .override of the original decl method.

@janvorli
Copy link
Member Author

@davidwrighton I've fixed it based on your feedback, can you please take a look again?

Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@janvorli janvorli merged commit 5e5a0d4 into dotnet:master Nov 21, 2019
@janvorli janvorli deleted the fix-crossgen2-devirt branch November 21, 2019 21:44
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants