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 virtual static dispatch for variant interfaces when using default implementations #88639

Merged
merged 5 commits into from
Jul 11, 2023

Conversation

davidwrighton
Copy link
Member

@davidwrighton davidwrighton commented Jul 11, 2023

Fixes #78621

@@ -8911,7 +8912,7 @@ MethodTable::ResolveVirtualStaticMethod(
// Search for match on a per-level in the type hierarchy
for (MethodTable* pMT = this; pMT != nullptr; pMT = pMT->GetParentMethodTable())
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this parent type descent also be in a 2-pass loop over allowVariance = FALSE, TRUE when allowVariantMatches is set i.o.w. shouldn't it also be in the below loop used for default interface resolution?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, after getting my head around the tests I take it back, the variant lookup should take place before descending into parent.

@@ -8955,7 +8956,7 @@ MethodTable::ResolveVirtualStaticMethod(
{
// Variant or equivalent matching interface found
// Attempt to resolve on variance matched interface
pMD = pMT->TryResolveVirtualStaticMethodOnThisType(pItfInMap, pInterfaceMD, verifyImplemented, level);
pMD = pMT->TryResolveVirtualStaticMethodOnThisType(pItfInMap, pInterfaceMD, verifyImplemented, /*allowVariance*/ FALSE, level);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we be setting allowVariance to TRUE (or perhaps to allowVariantMatches) here? Otherwise I don't see where we are supposed to be performing the variant resolution in this method; can you please explain to me what am I missing?

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, if it's the case that in practice all variant resolution is only limited to the default interface implementation case (which is probably correct), why do we need this additional pass over the interface map in this place?

@trylek trylek closed this Jul 11, 2023
@trylek trylek reopened this Jul 11, 2023
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 David! I had a bit of trouble getting my head around the exact code flow in SVM resolution but I now think it is actually correct.

@lambdageek
Copy link
Member

@davidwrighton I didn't see the new test run with Mono - I think there might be some issue with the pipeline triggers when only a test changed. Could you touch some file in src/mono/mono? (maybe src/mono/mono/metadata/class-setup-vtable.c)

@davidwrighton davidwrighton merged commit 57f691a into dotnet:main Jul 11, 2023
113 of 117 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2023
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.

Variant static interface dispatch crashes the runtime
3 participants