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 for issue #80350 tracking runtime deficiencies in the presence of static virtual method reabstraction #80987

Merged
merged 4 commits into from
Feb 17, 2023

Conversation

trylek
Copy link
Member

@trylek trylek commented Jan 22, 2023

This change fixes the issue

#80350

that uncovered several inconsistencies in the CoreCLR runtime
w.r.t. static virtual methods. In particular, reabstraction (declaring
an abstract explicit override of a static virtual method in a derived
interface) turned out to be causing runtime issues. As part of
the change I'm also fixing the reabstraction unit test
svm_diamondshape as it turns out Roslyn emits the method flags
in a slightly different manner than I originally expected in the
hand-written IL test.

Thanks

Tomas

P.S. This PR would be normally an obvious adept for nominating @davidwrighton as the reviewer. As David is on parental leave right now, I am designating a larger reviewer group to get more pairs of eyes to see the change and also ideally proliferate CoreCLR typesystem knowledge to improve our agility the next time David's not available.

@trylek trylek added this to the 8.0.0 milestone Jan 22, 2023
@ghost ghost assigned trylek Jan 22, 2023
@trylek
Copy link
Member Author

trylek commented Jan 22, 2023

/azp run coreclr-runtime outerloop

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@trylek
Copy link
Member Author

trylek commented Jan 22, 2023

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@trylek trylek closed this Jan 29, 2023
@trylek trylek reopened this Jan 29, 2023
@trylek
Copy link
Member Author

trylek commented Feb 5, 2023

@SamMonoRT - could you please advise someone who might be able to help me fix the Mono issues in this PR? I have updated the svm_diamondshape.il test by marking the static virtual method reabstraction as 'virtual' as I found out that's what Roslyn does and several Mono legs now fail in the test due to some VTable setup error. Thanks a lot!

@SamMonoRT
Copy link
Member

@SamMonoRT - could you please advise someone who might be able to help me fix the Mono issues in this PR? I have updated the svm_diamondshape.il test by marking the static virtual method reabstraction as 'virtual' as I found out that's what Roslyn does and several Mono legs now fail in the test due to some VTable setup error. Thanks a lot!

@BrzVlad will take a look at the failing Mono CI lanes later this week.

This change fixes the issue dotnet#80350 that uncovered several
inconsistencies in the CoreCLR runtime w.r.t. static virtual methods.
In particular, reabstraction (declaring an abstract explicit override
of a static virtual method in a derived interface) turned out to be
causing runtime issues. As part of the change I'm also fixing the
reabstraction unit test svm_diamondshape as it turns out Roslyn emits
the method flags in a slightly different manner than I originally
expected in the hand-written IL test.

Thanks

Tomas
Based on offline discussion with Vlad Brezae I have temporarily
disabled the svm_diamondshape test on Mono where it is causing
crashes in VTable construction that might take a bit to fix.

Thanks

Tomas
@trylek
Copy link
Member Author

trylek commented Feb 17, 2023

Based on offline discussion with @BrzVlad I have updated this PR by adding a temporary exclusion of the svm_diamondshape tests on Mono where they seem to be crashing VTable construction due to some subtle bug that may take a bit to fix. I plan to merge this in if the PR run finishes fine unless anyone has objections.

Thanks

Tomas

@trylek
Copy link
Member Author

trylek commented Feb 17, 2023

The only failure is tracked as #81901 and the fix is currently pending PR under #82309, merging in.

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.

3 participants