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

Virt call unsafe accessor tests #89220

Merged

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Jul 19, 2023

Makes CoreCLR align with the current NativeAOT implementation and not examine the type hierarchy. See #89212 (comment)

/cc @lambdageek @fanyang-mono

@ghost
Copy link

ghost commented Jul 19, 2023

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes NativeAOT issues described at #89212 (comment)

/cc @lambdageek @fanyang-mono

Author: AaronRobinsonMSFT
Assignees: -
Labels:

area-System.Runtime.CompilerServices

Milestone: 8.0.0

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Jul 20, 2023

Another possible fix could be to use IntroducedMethodIterator on the CoreCLR side

Yep. This is purely an academic exercise at this point. The entire point of UnsafeAccessor is to enable access of a private member on a type. The hierarchy lookup has a trivial around and this effort is requiring a fix to the NativeAOT type system to respect the very notion of OOP - which seems like a huge oversight in its design. A managed type system that has no way to handle that concept seems broken in my opinion. I will defer to IntroducedMethodIterator and make CoreCLR and NativeAOT work the same.

@fanyang-mono and @lambdageek Please don't try to enable this on Mono. We will no longer walk the type hierarchy.

@lambdageek
Copy link
Member

@fanyang-mono and @lambdageek Please don't try to enable this on Mono. We will no longer walk the type hierarchy.

I'm fine with the more restricted lookup in the interest of shipping something workable in .NET 8. But I think in the future we should make UnsafeAccessor and MemberRef IL lookup work the same way.

That means CoreCLR would need to walk the MethodDef metadata table of the target type - and parent classes - not the MethodTable::MethodIterator which is confusingly named. It is not a method iterator. It's a MethodTable slot iterator. CoreCLR slots are weird because they mix non-virtual methods from a single class together with virtual methods from the entire ancestor hierarchy of that class.

@AaronRobinsonMSFT
Copy link
Member Author

@MichalStrehovsky and @lambdageek I've updated CoreCLR to not look through the type hierarchy.

Copy link
Member

@MichalStrehovsky MichalStrehovsky 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!

@AaronRobinsonMSFT
Copy link
Member Author

Known issue #86532

@MichalStrehovsky
Copy link
Member

The hierarchy lookup has a trivial around and this effort is requiring a fix to the NativeAOT type system to respect the very notion of OOP - which seems like a huge oversight in its design.

Btw, we do have a way to iterate virtual slots in the managed type system. We just don't have a way to iterate virtual slots intermingled with non-virtual methods introduced on the current type. That one is just a CoreCLR implementation quirk. I don't think such iterator is necessary to implement OOP.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 20, 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.

4 participants