From 3663bad75c7e642862739de9432c349ed7c8f90a Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Mon, 12 Jun 2023 15:53:09 -0700 Subject: [PATCH] Tests for calling convention bits on NativeAOT. --- .../Common/TypeSystem/IL/UnsafeAccessors.cs | 112 +++++++++++------- .../UnsafeAccessors/UnsafeAccessorsTests.cs | 19 ++- 2 files changed, 87 insertions(+), 44 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/IL/UnsafeAccessors.cs b/src/coreclr/tools/Common/TypeSystem/IL/UnsafeAccessors.cs index 50fb4e70895f3..dc3e1322b19fe 100644 --- a/src/coreclr/tools/Common/TypeSystem/IL/UnsafeAccessors.cs +++ b/src/coreclr/tools/Common/TypeSystem/IL/UnsafeAccessors.cs @@ -227,51 +227,13 @@ private static bool DoesMethodMatchUnsafeAccessorDeclaration(ref GenerationConte // If we are, do it first. if (!ignoreCustomModifiers) { - // One signature has custom modifiers the other doesn't, no match. - if (declSig.HasEmbeddedSignatureData != maybeSig.HasEmbeddedSignatureData) + // Compare custom modifiers on the signatures. + var declCustomMod = declSig.GetEmbeddedSignatureData(EmbeddedSignatureDataKind.RequiredCustomModifier, EmbeddedSignatureDataKind.OptionalCustomModifier) ?? Array.Empty(); + var maybeCustomMod = maybeSig.GetEmbeddedSignatureData(EmbeddedSignatureDataKind.RequiredCustomModifier, EmbeddedSignatureDataKind.OptionalCustomModifier) ?? Array.Empty(); + if (!CompareEmbeddedData(context.Kind, declCustomMod, maybeCustomMod)) { return false; } - - // Get all custom modifiers on the signatures. - var declData = declSig.GetEmbeddedSignatureData(EmbeddedSignatureDataKind.RequiredCustomModifier, EmbeddedSignatureDataKind.OptionalCustomModifier); - var maybeData = maybeSig.GetEmbeddedSignatureData(EmbeddedSignatureDataKind.RequiredCustomModifier, EmbeddedSignatureDataKind.OptionalCustomModifier); - 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) - { - // Decrement the second to last index by one to - // account for the difference in declarations. - string[] tmp = declIndex.Split('.'); - int toUpdate = tmp.Length < 2 ? 0 : tmp.Length - 2; - int idx = int.Parse(tmp[toUpdate], CultureInfo.InvariantCulture); - idx--; - tmp[toUpdate] = idx.ToString(); - declIndex = string.Join(".", tmp); - } - - if (declIndex != md.index) - { - return false; - } - } } // Validate calling convention. @@ -281,6 +243,14 @@ private static bool DoesMethodMatchUnsafeAccessorDeclaration(ref GenerationConte return false; } + // Compare unmanaged callconv on the signatures. + var declUnmanaged = declSig.GetEmbeddedSignatureData(EmbeddedSignatureDataKind.UnmanagedCallConv) ?? Array.Empty(); + var maybeUnmanaged = maybeSig.GetEmbeddedSignatureData(EmbeddedSignatureDataKind.UnmanagedCallConv) ?? Array.Empty(); + if (!CompareEmbeddedData(context.Kind, declUnmanaged, maybeUnmanaged)) + { + return false; + } + // Validate argument count and return type if (context.Kind == UnsafeAccessorKind.Constructor) { @@ -330,6 +300,64 @@ 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) diff --git a/src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs b/src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs index 7dfd885333fd0..d5c849e9aba22 100644 --- a/src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs +++ b/src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs @@ -24,6 +24,7 @@ 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); private static string _F = PrivateStatic; private string _f; @@ -43,11 +44,15 @@ private void _mvv() {} private string Prop { get; init; } // Used to validate ambiguity is handled via custom modifiers. - private string _Ambiguous(delegate* unmanaged[Cdecl, MemberFunction] fptr) { return nameof(CallConvCdecl); } - private string _Ambiguous(delegate* unmanaged[Stdcall, MemberFunction] fptr) { return nameof(CallConvStdcall); } + private string _Ambiguous(delegate* unmanaged[Cdecl, MemberFunction] fptr) => nameof(CallConvCdecl); + private string _Ambiguous(delegate* unmanaged[Stdcall, MemberFunction] fptr) => nameof(CallConvStdcall); // Used to validate pointer values. private static string _Pointer(void* ptr) => "void*"; + + // Used to validate the embedded callconv bits (non-unmanaged bit) in + // ECMA-335 signatures for methods. + private static string _CallConvBits(delegate* unmanaged[Cdecl] fptr) => nameof(CallConvCdecl); } [StructLayout(LayoutKind.Sequential)] @@ -347,6 +352,10 @@ public static void Verify_InvalidTargetUnsafeAccessor() isNativeAot ? null : UserDataClass.MethodPointerName, () => CallPointerMethod(null, null)); + AssertExtensions.ThrowsMissingMemberException( + isNativeAot ? null : UserDataClass.MethodCallConvBitsName, + () => CallCallConvBitsMethod(null, null)); + Assert.Throws( () => CallAmbiguousMethod(CallPrivateConstructorClass(), null)); @@ -366,6 +375,12 @@ public static void Verify_InvalidTargetUnsafeAccessor() [UnsafeAccessor(UnsafeAccessorKind.StaticMethod, Name=UserDataClass.MethodPointerName)] extern static string CallPointerMethod(UserDataClass d, delegate* unmanaged[Stdcall] 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] 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.