KEMBAR78
Fix virtual static dispatch for variant interfaces when using default implementations by davidwrighton · Pull Request #88639 · dotnet/runtime · GitHub
Skip to content

Conversation

@davidwrighton
Copy link
Member

@davidwrighton davidwrighton commented Jul 11, 2023

Fixes #78621

if (!canonicalEquivalentFound)
{
// 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.

// 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
@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