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.

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

Jump to bottom

Implement reabstraction in CoreCLR for Static Virtual Methods #88711

Merged
merged 10 commits into from
Jul 14, 2023
Merged

Implement reabstraction in CoreCLR for Static Virtual Methods #88711

merged 10 commits into from
Jul 14, 2023

Conversation

davidwrighton
Copy link
Member

  • Add a new helper for the re-abstraction case when the JIT detects an issue
  • In the late bound case, instead of producing an error directly, produce an IL stub and have it make the virtual stub dispatch call itself, this will fall back to the JIT implementation for re-abstraction
  • Tweak the late bound case to also actively detect the AmbiguousMatchException case as well and use the same helper there as well.

Fixes #71414

@ghost ghost assigned davidwrighton Jul 12, 2023
@MichalStrehovsky
Copy link
Member

The issue to block the test on for NativeAOT is #72589. It didn't make sense to implement if CoreCLR proper doesn't implement it.

@build-analysis build-analysis bot mentioned this pull request Jul 12, 2023
Closed
@davidwrighton davidwrighton mentioned this pull request Jul 12, 2023
Open
@build-analysis build-analysis bot mentioned this pull request Jul 12, 2023
Closed
trylek
trylek reviewed Jul 12, 2023
View reviewed changes
src/coreclr/vm/ilstubcache.cpp
@@ -159,6 +159,7 @@ namespace
case DynamicMethodDesc::StubWrapperDelegate: return "IL_STUB_WrapperDelegate_Invoke";
case DynamicMethodDesc::StubTailCallStoreArgs: return "IL_STUB_StoreTailCallArgs";
case DynamicMethodDesc::StubTailCallCallTarget: return "IL_STUB_CallTailCallTarget";
case DynamicMethodDesc::StubVirtualStaticMethodDispatch: return "IL_STUB_bVirtualStaticMethodDispatch";
Copy link
Member

Choose a reason for hiding this comment

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

Nit - is the lowercase 'b' after IL_STUB_ intentional or just a typo?

trylek
trylek approved these changes Jul 12, 2023
View reviewed changes
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.

Looks great to me, thanks David!

@build-analysis build-analysis bot mentioned this pull request Jul 13, 2023
Closed
@davidwrighton
Copy link
Member Author

Only failing issue not flagged by BuildAnalysis as known is #88870 which simply isn't being detected correctly by the BuildAnalysis stuff.

@davidwrighton davidwrighton merged commit 52a3995 into dotnet:main Jul 14, 2023
106 of 117 checks passed
This was referenced Jul 14, 2023
Closed
Closed
@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.
Reviewers

@trylek trylek trylek approved these changes

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

Assignees

@davidwrighton davidwrighton

Labels
area-TypeSystem-coreclr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Static virtual reabstraction doesn't seem to work
3 participants
@davidwrighton @MichalStrehovsky @trylek