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

[Mono] UnsafeAccessor: Support ambiguous method match #89217

Merged
merged 10 commits into from
Jul 25, 2023

Conversation

fanyang-mono
Copy link
Member

@fanyang-mono fanyang-mono commented Jul 19, 2023

Fixes #89098

This PR adjusted Mono's method lookup behavior for UnsafeAccessor to match with CoreCLR. It conducted two rounds of lookup.

  • Relaxed match. Look for methods but ignore custom modifiers. If there are less than or equal to one match, return. If there are more than one match, enter exact match.
  • Exact match. Look for methods matching exactly. If found, return the exact match. If not found, throw AmbiguousMatchException.

Special notes for function pointer calling convention:
If there is a single calling convention defined (i.e., Cdecl or Stdcall) then these values are not defined as custom modifiers, but instead encoded in the ECMA bits for the function pointer. They become custom modifiers, when there are more than one calling conventions defined. For example,

public unsafe class C {
    public void M(delegate* unmanaged[Cdecl]<void> p) {}
    public void N(delegate* unmanaged[Cdecl, SuppressGCTransition]<void> p) {}
}

The corresponding IL code for methods N and M is

    .method public hidebysig 
        instance void M (
            method unmanaged cdecl void *() p
        ) cil managed 
    {
        // Method begins at RVA 0x2069
        // Code size 2 (0x2)
        .maxstack 8

        IL_0000: nop
        IL_0001: ret
    } // end of method C::M

    .method public hidebysig 
        instance void N (
            method unmanaged void modopt([System.Runtime]System.Runtime.CompilerServices.CallConvSuppressGCTransition) modopt([System.Runtime]System.Runtime.CompilerServices.CallConvCdecl) *() p
        ) cil managed 
    {
        // Method begins at RVA 0x2069
        // Code size 2 (0x2)
        .maxstack 8

        IL_0000: nop
        IL_0001: ret
    } // end of method C::N

src/mono/mono/metadata/metadata.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/metadata.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/loader.c Outdated Show resolved Hide resolved
@fanyang-mono fanyang-mono marked this pull request as ready for review July 24, 2023 21:49
@fanyang-mono fanyang-mono changed the title [Mono][WIP] UnsafeAccessor: Support ambiguous method match [Mono] UnsafeAccessor: Support ambiguous method match Jul 24, 2023
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

LGTM.

My one suggestion is to move all the unsafe accessor stuff to unsafe-accessor.c because now it doesn't depend on the loader's lookup methods.

(It would be nice to make them share, but I think this lookup stuff is kind of fragile, and if we're going to be clarifying how UnsafeAccessor method lookup is supposed to work in .net 9, we'll have to tweak it probably, anyway. So separate file is better, I think)

src/mono/mono/metadata/loader.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/loader.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/metadata.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/metadata.c Show resolved Hide resolved
src/mono/mono/utils/mono-error.c Show resolved Hide resolved
@fanyang-mono
Copy link
Member Author

Test failures are unrelated and have been reported.

@fanyang-mono fanyang-mono merged commit 2b477ae into dotnet:main Jul 25, 2023
149 of 153 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 24, 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.

[mono] Support UnsafeAccessorAttribute method calls with custom modifiers
3 participants