Skip to content

Commit

Permalink
Treat signature calling convention bits like we treat
Browse files Browse the repository at this point in the history 
custom modifiers during method lookup.
  • Loading branch information
@AaronRobinsonMSFT
AaronRobinsonMSFT committed Jun 13, 2023
1 parent c2be6fc commit 63f975c
Show file tree
Hide file tree
Showing 4 changed files with 100 additions and 86 deletions.
9 changes: 7 additions & 2 deletions src/coreclr/tools/Common/TypeSystem/Common/MethodDesc.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
130 changes: 59 additions & 71 deletions src/coreclr/tools/Common/TypeSystem/IL/UnsafeAccessors.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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[]

This comment has been minimized.

Sign in to view

Copy link
@IDisposable

IDisposable Jun 13, 2023

Contributor

Can this be a static const instead of allocated every time?
{
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)
{
Expand Down Expand Up @@ -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)
Expand Down
9 changes: 8 additions & 1 deletion src/coreclr/vm/siginfo.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
38 changes: 26 additions & 12 deletions src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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()
Expand All @@ -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));

Expand All @@ -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.
Expand Down

0 comments on commit 63f975c

Please sign in to comment.