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 handling of static virtual method implementation checking #54710

Conversation

davidwrighton
Copy link
Member

  • It turns out that GetMethodDescFromMemberDefOrRefOrSpec and FindOrCreateAssociatedMethodDesc are not safe to use on a MemberRef whent the associated MethodTable is not fully loaded.
  • Instead only use that feature when working with a MethodDef or a fully loaded type, and when working with a not fully loaded type, use MemberLoader::FindMethod instead.
  • When running the resolution algorithm for doing constraint validation, it also is not necessary to fully resolve to the exact correct MethodDesc, which as that process uses FindOrCreateAssociatedMethodDesc needs to be avoided.
  • The above was not evident as in many cases, the validation algorithm did not run as it was misplaced and located directly before the call to SetIsFullyLoaded. That code path is only followed if the type is able to fully load without circular dependencies. (Test case CuriouslyRecurringGenericWithUnimplementedMethod added to cover that scenario)
  • In addition, while investigating these issues, I realized we were lacking checks that the constraints on the impl and decl method were not checked at during type load, but that work was instead deferred to dispatch time. Along with the constraint check there was also a set of accessibility checks that had been missed that are common to all MethodImpl handling. Fix by adding tweaking the logic to share most of that code.

- It turns out that GetMethodDescFromMemberDefOrRefOrSpec and FindOrCreateAssociatedMethodDesc are not safe to use on a MemberRef whent  the associated MethodTable is not fully loaded.
- Instead only use that feature when working with a MethodDef or a fully loaded type, and when working with a not fully loaded type, use MemberLoader::FindMethod instead.
- When running the resolution algorithm for doing constraint validation, it also is not necessary to fully resolve to the exact correct MethodDesc, which as that process uses FindOrCreateAssociatedMethodDesc needs to be avoided.
- The above was not evident as in many cases, the validation algorithm did not run as it was misplaced and located directly before the call to SetIsFullyLoaded. That code path is only followed if the type is able to fully load without circular dependencies. (Test case CuriouslyRecurringGenericWithUnimplementedMethod added to cover that scenario)
- In addition, while investigating these issues, I realized we were lacking checks that the constraints on the impl and decl method were not checked at during type load, but that work was instead deferred to dispatch time. Along with the constraint check there was also a set of accessibility checks that had been missed that are common to all MethodImpl handling. Fix by adding tweaking the logic to share most of that code.
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 good, thanks for fixing this and for expanding our code coverage!

@davidwrighton davidwrighton merged commit 333c4e7 into dotnet:main Jun 25, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 25, 2021
@davidwrighton davidwrighton deleted the fix_static_virtual_method_classloadlevel_issues branch April 13, 2023 18:48
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.

2 participants