-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
vtable setup fix for generic default interface methods in mono runtime #64102
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
81 changes: 81 additions & 0 deletions
81
src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github61244.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System; | ||
|
||
// In GH issue 61244 the mono runtime aborted when inflating the default | ||
// interface method because the context used was from the base interface. | ||
|
||
// The OneArgBaseInterface portion of this test handles the original bug | ||
// where the base interface has less generic arguments than the derived | ||
// interface and the runtime aborts. | ||
|
||
// The SecondInterface portion of this test handles an additional scenario | ||
// where the number of generic arguments are the same in the base and | ||
// derived interface contexts, but the order is changed (or different.) | ||
// When this occurs the generic info is incorrect for the inflated method. | ||
|
||
class Program | ||
{ | ||
static int Main(string[] args) | ||
{ | ||
return new TestClass().DoTest(); | ||
} | ||
} | ||
|
||
public interface OneArgBaseInterface<T1> | ||
{ | ||
int SomeFunc1(T1 someParam1, Type someParam1Type); | ||
} | ||
|
||
public interface TwoArgBaseInterface<T1, T2> | ||
{ | ||
int SomeFunc1(T1 someParam1, T2 someParam2, Type someParam1Type, Type someParam2Type); | ||
} | ||
|
||
public interface SecondInterface<TParam2Type, TParam1Type> : OneArgBaseInterface<TParam1Type>, TwoArgBaseInterface<TParam1Type, TParam2Type> | ||
{ | ||
int OneArgBaseInterface<TParam1Type>.SomeFunc1(TParam1Type someParam1, Type someParam1Type) | ||
{ | ||
if (typeof(TParam1Type) != someParam1Type) | ||
{ | ||
Console.WriteLine("Failed => 101"); | ||
return 101; | ||
} | ||
|
||
return 100; | ||
} | ||
|
||
int TwoArgBaseInterface<TParam1Type, TParam2Type>.SomeFunc1(TParam1Type someParam1, TParam2Type someParam2, Type someParam1Type, Type someParam2Type) | ||
{ | ||
if (typeof(TParam1Type) != someParam1Type) | ||
{ | ||
Console.WriteLine("Failed => 102"); | ||
return 102; | ||
} | ||
|
||
if (typeof(TParam2Type) != someParam2Type) | ||
{ | ||
Console.WriteLine("Failed => 103"); | ||
return 103; | ||
} | ||
|
||
return 100; | ||
} | ||
} | ||
|
||
public class TestClass : SecondInterface<int, string> | ||
{ | ||
public int DoTest () | ||
{ | ||
int ret = (this as OneArgBaseInterface<string>).SomeFunc1("test string", typeof(string)); | ||
if (ret != 100) | ||
return ret; | ||
|
||
ret = (this as TwoArgBaseInterface<string, int>).SomeFunc1("test string", 0, typeof(string), typeof(int)); | ||
if (ret != 100) | ||
return ret; | ||
|
||
Console.WriteLine("Passed => 100"); | ||
return 100; | ||
} | ||
} |
9 changes: 9 additions & 0 deletions
9
src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github61244.csproj
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<Project Sdk="Microsoft.NET.Sdk"> | ||
<PropertyGroup> | ||
<AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||
<OutputType>Exe</OutputType> | ||
</PropertyGroup> | ||
<ItemGroup> | ||
<Compile Include="$(MSBuildProjectName).cs" /> | ||
</ItemGroup> | ||
</Project> |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not certain that we need the
decl->is_inflated
case at all. I can not think of a scenario where this is needed. If we can find the testcase I would like to add that to the tests in this PR as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did change the code locally to assert if we hit the
decl->is_inflated
case andoverride
changed. I then ran all the tests and I do think this this assert was hit and I got the same results. Depending on how well dims are covered with our test code, this code block may really be unnecessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll do a separate PR for net7 with
we'll backport the current version to mono/mono and net6