Skip to content

[mini] Don't add unbox tramopline on generic DIM calls #58521

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.

Sign up for GitHub

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

Merged
lambdageek merged 2 commits into from
Sep 3, 2021
Merged

[mini] Don't add unbox tramopline on generic DIM calls #58521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
439b106
[mini] Don't add unbox tramopline on generic DIM calls
lambdageek Sep 1, 2021
e9bd9d4
Add regression test
lambdageek Sep 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/mono/mono/mini/mini-trampolines.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,11 +632,13 @@ common_call_trampoline (host_mgreg_t *regs, guint8 *code, MonoMethod *m, MonoVTa
return NULL;

if (generic_virtual || variant_iface) {
if (m_class_is_valuetype (vt->klass)) /*FIXME is this required variant iface?*/
if (m_class_is_valuetype (vt->klass) && !mini_method_is_default_method (m)) /*FIXME is this required variant iface?*/
need_unbox_tramp = TRUE;
} else if (orig_vtable_slot) {
if (m_class_is_valuetype (m->klass))
if (m_class_is_valuetype (m->klass)) {
g_assert (!mini_method_is_default_method (m));
Copy link
Member

@fanyang-mono fanyang-mono Sep 2, 2021

Choose a reason for hiding this comment

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

Why don't you add the same condition as above?

Copy link
Member Author

@lambdageek lambdageek Sep 2, 2021

Choose a reason for hiding this comment

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

Good question.
I couldn't tell if we could ever end up in this branch with a valuetype klass and a default interface method.

I thought it would be better to add an assertion (because if it does ever happen the failure will be right at this spot) instead of skipping the need_unbox_tramp. If we erroneous add or don't add an unbox trampoline, we will get a crash later when we actually call the underlying managed method and it uses this in some way. That will be a crash in some unrelated managed code and it won't be obvious how to trace it back to this trampoline. With the assert if anyone comes up with a way to violate it the assumptions it will break in the place where we need to fix it.

Copy link
Member

@fanyang-mono fanyang-mono Sep 2, 2021

Choose a reason for hiding this comment

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

That's right. The assertion would definitely make debugging easier. But my confusion is that in the future, when someone hit this assertion, I feel the solution would be to skip adding unbox trampoline. Or you think more work would be needed on top of that?

Copy link
Member Author

@lambdageek lambdageek Sep 2, 2021

Choose a reason for hiding this comment

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

That's right. The assertion would definitely make debugging easier. But my confusion is that in the future, when someone hit this assertion, I feel the solution would be to skip adding unbox trampoline. Or you think more work would be needed on top of that?

I don't know. I can't tell from the code how we end up in this branch with a DIM. I'd need to see an example and I couldn't make one.

And also I can't tell if in that case the right thing would be to add an unbox, or to not add one. To be honest I was hoping one of our existing tests would break on the assert, and then I'd know that things were fine as is.

But nothing broke. So I still don't know whether that case can happen. And what we should do if it does happen.

Copy link
Member

@fanyang-mono fanyang-mono Sep 3, 2021

Choose a reason for hiding this comment

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

make sense!

need_unbox_tramp = TRUE;
}
}

addr = mini_add_method_trampoline (m, compiled_method, need_rgctx_tramp, need_unbox_tramp);
Expand Down
47 changes: 47 additions & 0 deletions src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github58394.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Runtime.CompilerServices;

namespace GenericDimValuetypeBug
{
class Program
{
static int Main()
{
if (RunOne() != 17)
return 1;
if (RunTwo() != 23)
return 2;
return 100;
}

[MethodImpl(MethodImplOptions.NoInlining)]
public static int RunOne()
{
return (new Foo() { x = 17 } as IFoo).NoCrash();
}

[MethodImpl(MethodImplOptions.NoInlining)]
public static int RunTwo()
{
return (new Foo() { x = 23 } as IFoo).Crash<int>();
}
}

interface IFoo
{
int Crash<T>() => Bla();

int NoCrash() => Bla();

int Bla();
}

struct Foo: IFoo
{
public int x;
public int Bla() => x;
}
}
11 changes: 11 additions & 0 deletions src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github58394.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<OutputType>Exe</OutputType>
<CLRTestKind>BuildAndRun</CLRTestKind>
<CLRTestPriority>0</CLRTestPriority>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>