Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix constrained call corner cases #22464

Merged
merged 5 commits into from
Feb 18, 2019

Conversation

MichalStrehovsky
Copy link
Member

Fixes #22423. I'm still unclear on how JitStress manages to get itself into that situation, but I was able to write a repro that triggers the !pMDAfterConstraintResolution->IsInterface() assert using regular IL, so we need to handle that either way. The repro for that is constrained3.il.

While figuring out the repro, I wrote a bunch of other test code and found another bug (constrained2), where we would box in a situations that doesn't require boxing (canonically ambiguous situation where there's a suitable default interface implementation and a valuetype implementation of the constrained method that does not requires boxing once we no longer deal with __Canon).

Fixes #22423. I'm still unclear on how JitStress manages to get itself into that situation, but I was able to write a repro that triggers the `!pMDAfterConstraintResolution->IsInterface()` assert using regular IL, so we need to handle that either way. The repro for that is constrained3.il.

While figuring out the repro, I wrote a bunch of other test code and found another bug (constrained2), where we would box in a situations that doesn't require boxing (canonically ambiguous situation where there's a suitable default interface implementation and a valuetype implementation of the constrained method that does not requires boxing once we no longer deal with __Canon).
@MichalStrehovsky
Copy link
Member Author

Cc @sergiy-k

@MichalStrehovsky
Copy link
Member Author

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test
@dotnet-bot test Ubuntu arm Cross Checked Innerloop Build and Test
@dotnet-bot test Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test

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.

Seems like good work, the change in the jitinterface seems just right. I'd like some more understanding of the change in src/vm/methodtable.cpp though.

@@ -9850,7 +9850,7 @@ MethodTable::TryResolveConstraintMethodApprox(
pMD = pCanonMT->GetMethodDescForInterfaceMethod(thPotentialInterfaceType, pGenInterfaceMD, FALSE /* throwOnConflict */);

// See code:#TryResolveConstraintMethodApprox_DoNotReturnParentMethod
if ((pMD != NULL) && !pMD->GetMethodTable()->IsValueType())
if ((pMD != NULL) && !pMD->GetMethodTable()->IsValueType() && !pMD->IsInterface())
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the method that we resolve to hear is a generic, instantiated over canon. Does this still work here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added test coverage for the generic methods case, but I'm not sure I understood the concern - we strip the method instantiation before entering this code path, so the particular instantiation of the generic method shouldn't matter.

Copy link
Member

Choose a reason for hiding this comment

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

I meant type generic. You know, the notorious case that looks like

interface ITest<A>
{
 string M() { return "default implementation"; }
}


interface IDerivedTest<A> : ITest<A>
{
    override ITest<A>.M() { return "derived default implementation"; }
}

struct Test<X, Y> : ITest<X> : IDerivedTest<Y>
{
}

...
string SomeGenericMethod<W, Z> where W : ITest<Z>
{
    return W.M();
}

...
SomeGenericMethod<Test<object,string>, object>() -> should yield "default implementation"
and 
SomeGenericMethod<Test<object,object>, object>() -> should yield "derived default implementation"

Now that I think about it though, I think you've discussed already disabling this weird situation in any case, so it shouldn't matter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this case is going to throw because it requires a "boxing stub". constrained2.il has a test for a similar situation (we're in a situation where the dispatch is ambiguous at compile time between an implementation provided by the value type and the default implementation), but at runtime we can call the implementation on the value type (there's also a try/catch for the "boxing stub" case that works both if the runtime does the right thing, or when it throws).

@MichalStrehovsky
Copy link
Member Author

@dotnet-bot test CentOS7.1 x64 Checked Innerloop Build and Test
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test
@dotnet-bot test Windows_NT x64 Checked Innerloop Build and Test
@dotnet-bot test Windows_NT x64 Release CoreFX Tests

@MichalStrehovsky MichalStrehovsky merged commit 8824991 into dotnet:master Feb 18, 2019
@MichalStrehovsky MichalStrehovsky deleted the fix22423 branch February 18, 2019 16:20
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Fixes dotnet/coreclr#22423. I'm still unclear on how JitStress manages to get itself into that situation, but I was able to write a repro that triggers the `!pMDAfterConstraintResolution->IsInterface()` assert using regular IL, so we need to handle that either way. The repro for that is constrained3.il.

While figuring out the repro, I wrote a bunch of other test code and found another bug (constrained2), where we would box in a situations that doesn't require boxing (canonically ambiguous situation where there's a suitable default interface implementation and a valuetype implementation of the constrained method that does not requires boxing once we no longer deal with __Canon).

Commit migrated from dotnet/coreclr@8824991
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants