Treat signature calling convention bits like we treat

custom modifiers during method lookup.

Author: AaronRobinsonMSFT

src/coreclr/tools/Common/TypeSystem/Common/MethodDesc.cs

@@ -184,12 +184,17 @@ public bool EmbeddedSignatureMismatchPermitted
             }
         }
 
-        public EmbeddedSignatureData[] GetEmbeddedSignatureData(params EmbeddedSignatureDataKind[] kinds)
+        public EmbeddedSignatureData[] GetEmbeddedSignatureData()
+        {
+            return GetEmbeddedSignatureData(default);
+        }
+
+        public EmbeddedSignatureData[] GetEmbeddedSignatureData(ReadOnlySpan<EmbeddedSignatureDataKind> kinds)
         {
             if ((_embeddedSignatureData == null) || (_embeddedSignatureData.Length == 0))
                 return null;
 
-            if (kinds.Length == 0)
+            if (kinds.IsEmpty)
                 return (EmbeddedSignatureData[])_embeddedSignatureData.Clone();
 
             List<EmbeddedSignatureData> ret = new();

src/coreclr/tools/Common/TypeSystem/IL/UnsafeAccessors.cs

@@ -227,30 +227,76 @@ private static bool DoesMethodMatchUnsafeAccessorDeclaration(ref GenerationConte
             // If we are, do it first.
             if (!ignoreCustomModifiers)
             {
-                // Compare custom modifiers on the signatures.
-                var declCustomMod = declSig.GetEmbeddedSignatureData(EmbeddedSignatureDataKind.RequiredCustomModifier, EmbeddedSignatureDataKind.OptionalCustomModifier) ?? Array.Empty<EmbeddedSignatureData>();
-                var maybeCustomMod = maybeSig.GetEmbeddedSignatureData(EmbeddedSignatureDataKind.RequiredCustomModifier, EmbeddedSignatureDataKind.OptionalCustomModifier) ?? Array.Empty<EmbeddedSignatureData>();
-                if (!CompareEmbeddedData(context.Kind, declCustomMod, maybeCustomMod))
+                // Compare any unmanaged callconv and custom modifiers on the signatures.
+                // We treat unmanaged calling conventions at the same level of precedance
+                // as custom modifiers, eventhough they are normally bits in a signature.
+                ReadOnlySpan<EmbeddedSignatureDataKind> kinds = new EmbeddedSignatureDataKind[]
+                {
+                    EmbeddedSignatureDataKind.UnmanagedCallConv,
+                    EmbeddedSignatureDataKind.RequiredCustomModifier,
+                    EmbeddedSignatureDataKind.OptionalCustomModifier
+                };
+
+                var declData = declSig.GetEmbeddedSignatureData(kinds) ?? Array.Empty<EmbeddedSignatureData>();
+                var maybeData = maybeSig.GetEmbeddedSignatureData(kinds) ?? Array.Empty<EmbeddedSignatureData>();
+                if (declData.Length != maybeData.Length)
                 {
                     return false;
                 }
+
+                // Validate the custom modifiers match precisely.
+                for (int i = 0; i < declData.Length; ++i)
+                {
+                    EmbeddedSignatureData dd = declData[i];
+                    EmbeddedSignatureData md = maybeData[i];
+                    if (dd.kind != md.kind || dd.type != md.type)
+                    {
+                        return false;
+                    }
+
+                    // The indices on non-constructor declarations require
+                    // some slight modification since there is always an extra
+                    // argument in the declaration compared to the target.
+                    string declIndex = dd.index;
+                    if (context.Kind != UnsafeAccessorKind.Constructor)
+                    {
+                        string unmanagedCallConvMaybe = string.Empty;
+
+                        // Check for and drop the unmanaged calling convention
+                        // value suffix to add it back after updating below.
+                        if (declIndex.Contains('|'))
+                        {
+                            Debug.Assert(dd.kind == EmbeddedSignatureDataKind.UnmanagedCallConv);
+                            var tmp = declIndex.Split('|');
+                            Debug.Assert(tmp.Length == 2);
+                            declIndex = tmp[0];
+                            unmanagedCallConvMaybe = "|" + tmp[1];
+                        }
+
+                        // Decrement the second to last index by one to
+                        // account for the difference in declarations.
+                        string[] lvls = declIndex.Split('.');
+                        int toUpdate = lvls.Length < 2 ? 0 : lvls.Length - 2;
+                        int idx = int.Parse(lvls[toUpdate], CultureInfo.InvariantCulture);
+                        idx--;
+                        lvls[toUpdate] = idx.ToString();
+                        declIndex = string.Join(".", lvls) + unmanagedCallConvMaybe;
+                    }
+
+                    if (declIndex != md.index)
+                    {
+                        return false;
+                    }
+                }
             }
 
-            // Validate calling convention.
+            // Validate calling convention of declaration.
             if ((declSig.Flags & MethodSignatureFlags.UnmanagedCallingConventionMask)
                 != (maybeSig.Flags & MethodSignatureFlags.UnmanagedCallingConventionMask))
             {
                 return false;
             }
 
-            // Compare unmanaged callconv on the signatures.
-            var declUnmanaged = declSig.GetEmbeddedSignatureData(EmbeddedSignatureDataKind.UnmanagedCallConv) ?? Array.Empty<EmbeddedSignatureData>();
-            var maybeUnmanaged = maybeSig.GetEmbeddedSignatureData(EmbeddedSignatureDataKind.UnmanagedCallConv) ?? Array.Empty<EmbeddedSignatureData>();
-            if (!CompareEmbeddedData(context.Kind, declUnmanaged, maybeUnmanaged))
-            {
-                return false;
-            }
-
             // Validate argument count and return type
             if (context.Kind == UnsafeAccessorKind.Constructor)
             {
@@ -300,64 +346,6 @@ private static bool DoesMethodMatchUnsafeAccessorDeclaration(ref GenerationConte
             }
 
             return true;
-
-            static bool CompareEmbeddedData(
-                UnsafeAccessorKind kind,
-                EmbeddedSignatureData[] declData,
-                EmbeddedSignatureData[] maybeData)
-            {
-                if (declData.Length != maybeData.Length)
-                {
-                    return false;
-                }
-
-                // Validate the custom modifiers match precisely.
-                for (int i = 0; i < declData.Length; ++i)
-                {
-                    EmbeddedSignatureData dd = declData[i];
-                    EmbeddedSignatureData md = maybeData[i];
-                    if (dd.kind != md.kind || dd.type != md.type)
-                    {
-                        return false;
-                    }
-
-                    // The indices on non-constructor declarations require
-                    // some slight modification since there is always an extra
-                    // argument in the declaration compared to the target.
-                    string declIndex = dd.index;
-                    if (kind != UnsafeAccessorKind.Constructor)
-                    {
-                        string unmanagedCallConvMaybe = string.Empty;
-
-                        // Check for and drop the unmanaged calling convention
-                        // value suffix to add it back after updating below.
-                        if (declIndex.Contains('|'))
-                        {
-                            Debug.Assert(dd.kind == EmbeddedSignatureDataKind.UnmanagedCallConv);
-                            var tmp = declIndex.Split('|');
-                            Debug.Assert(tmp.Length == 2);
-                            declIndex = tmp[0];
-                            unmanagedCallConvMaybe = "|" + tmp[1];
-                        }
-
-                        // Decrement the second to last index by one to
-                        // account for the difference in declarations.
-                        string[] lvls = declIndex.Split('.');
-                        int toUpdate = lvls.Length < 2 ? 0 : lvls.Length - 2;
-                        int idx = int.Parse(lvls[toUpdate], CultureInfo.InvariantCulture);
-                        idx--;
-                        lvls[toUpdate] = idx.ToString();
-                        declIndex = string.Join(".", lvls) + unmanagedCallConvMaybe;
-                    }
-
-                    if (declIndex != md.index)
-                    {
-                        return false;
-                    }
-                }
-
-                return true;
-            }
         }
 
         private static bool TrySetTargetMethod(ref GenerationContext context, string name, out bool isAmbiguous, bool ignoreCustomModifiers = true)

src/coreclr/vm/siginfo.cpp

@@ -3927,7 +3927,14 @@ MetaSig::CompareElementType(
             IfFailThrow(CorSigUncompressElementType_EndPtr(pSig1, pEndSig1, &callingConvention1));
             CorElementType callingConvention2 = ELEMENT_TYPE_MAX; // initialize to illegal
             IfFailThrow(CorSigUncompressElementType_EndPtr(pSig2, pEndSig2, &callingConvention2));
-            if (callingConvention1 != callingConvention2)
+
+            // Calling conventions are generally treated as custom modifiers.
+            // When callers request calling conventions to be ignored, we also ignore
+            // calling conventions and this is okay. It is okay because calling conventions,
+            // when more than one is defined (for example, SuppressGCTransition), all
+            // become encoded as custom modifiers.
+            if (!state->IgnoreCustomModifiers
+                && callingConvention1 != callingConvention2)
             {
                 return FALSE;
             }

src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs

@@ -24,7 +24,8 @@ class UserDataClass
         public const string MethodVoidName = nameof(_mvv);
         public const string MethodNameAmbiguous = nameof(_Ambiguous);
         public const string MethodPointerName = nameof(_Pointer);
-        public const string MethodCallConvBitsName = nameof(_CallConvBits);
+        public const string MethodCdeclCallConvBitName = nameof(_CdeclCallConvBit);
+        public const string MethodStdcallCallConvBitName = nameof(_StdcallCallConvBit);
 
         private static string _F = PrivateStatic;
         private string _f;
@@ -52,7 +53,8 @@ private void _mvv() {}
 
         // Used to validate the embedded callconv bits (non-unmanaged bit) in
         // ECMA-335 signatures for methods.
-        private static string _CallConvBits(delegate* unmanaged[Cdecl]<void> fptr) => nameof(CallConvCdecl);
+        private string _CdeclCallConvBit(delegate* unmanaged[Cdecl]<void> fptr) => nameof(CallConvCdecl);
+        private string _StdcallCallConvBit(delegate* unmanaged[Stdcall]<void> fptr) => nameof(CallConvStdcall);
     }
 
     [StructLayout(LayoutKind.Sequential)]
@@ -326,6 +328,28 @@ public static void Verify_PreciseMatchCustomModifier()
         extern static string CallPrivateMethod(UserDataClass d, delegate* unmanaged[Stdcall, MemberFunction]<void> fptr);
     }
 
+    [Fact]
+    [ActiveIssue("https://github.com/dotnet/runtime/issues/86040", TestRuntimes.Mono)]
+    public static void Verify_CallConvBitsAreTreatedAsCustomModifiersAndIgnored()
+    {
+        Console.WriteLine($"Running {nameof(Verify_CallConvBitsAreTreatedAsCustomModifiersAndIgnored)}");
+
+        var ud = CallPrivateConstructorClass();
+        Assert.Equal(nameof(CallConvCdecl), CallCdeclMethod(ud, null));
+        Assert.Equal(nameof(CallConvStdcall), CallStdcallMethod(ud, null));
+
+        // The names of the declarations don't match the calling conventions in the function
+        // pointer signature, this is by design for this test. The intent here is to validate that
+        // calling conventions, when encoded in the ECMA-335 bits, are ignored on the first pass
+        // in the same way custom modifiers are ignored.
+        [UnsafeAccessor(UnsafeAccessorKind.Method, Name=UserDataClass.MethodCdeclCallConvBitName)]
+        extern static string CallCdeclMethod(UserDataClass d, delegate* unmanaged[Stdcall]<void> fptr);
+
+        // See comment above regarding naming.
+        [UnsafeAccessor(UnsafeAccessorKind.Method, Name=UserDataClass.MethodStdcallCallConvBitName)]
+        extern static string CallStdcallMethod(UserDataClass d, delegate* unmanaged[Cdecl]<void> fptr);
+    }
+
     [Fact]
     [ActiveIssue("https://github.com/dotnet/runtime/issues/86040", TestRuntimes.Mono)]
     public static void Verify_InvalidTargetUnsafeAccessor()
@@ -352,10 +376,6 @@ public static void Verify_InvalidTargetUnsafeAccessor()
             isNativeAot ? null : UserDataClass.MethodPointerName,
             () => CallPointerMethod(null, null));
 
-        AssertExtensions.ThrowsMissingMemberException<MissingMethodException>(
-            isNativeAot ? null : UserDataClass.MethodCallConvBitsName,
-            () => CallCallConvBitsMethod(null, null));
-
         Assert.Throws<AmbiguousMatchException>(
             () => CallAmbiguousMethod(CallPrivateConstructorClass(), null));
 
@@ -375,12 +395,6 @@ public static void Verify_InvalidTargetUnsafeAccessor()
         [UnsafeAccessor(UnsafeAccessorKind.StaticMethod, Name=UserDataClass.MethodPointerName)]
         extern static string CallPointerMethod(UserDataClass d, delegate* unmanaged[Stdcall]<void> fptr);
 
-        // This is used to validate the ECMA-335 calling convention bits are checked
-        // before checking the custom modifiers that encode the calling conventions
-        // when there is more than one or one option doesn't have an existing bit.
-        [UnsafeAccessor(UnsafeAccessorKind.Method, Name=UserDataClass.MethodCallConvBitsName)]
-        extern static string CallCallConvBitsMethod(UserDataClass d, delegate* unmanaged[Cdecl, SuppressGCTransition]<void> fptr);
-
         // This is an ambiguous match since there are two methods each with two custom modifiers.
         // Therefore the default "ignore custom modifiers" logic fails. The fallback is for a
         // precise match and that also fails because the custom modifiers don't match precisely.