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.

Sign up for GitHub

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

Adjust managed type system for new function pointer handling #84819

Merged
merged 2 commits into from
Apr 18, 2023
Merged

Adjust managed type system for new function pointer handling #84819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e1b1bcb
Adjust managed type system for new function pointer handling
MichalStrehovsky Apr 14, 2023
546b6a1
Make the encoding roundtrippable
MichalStrehovsky Apr 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/coreclr/tools/Common/TypeSystem/Common/MethodDesc.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ public enum EmbeddedSignatureDataKind
{
RequiredCustomModifier = 0,
OptionalCustomModifier = 1,
ArrayShape = 2
ArrayShape = 2,
UnmanagedCallConv = 3,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this as a EmbeddedSignatureDataKind instead of as a MethodSignatureFlags?

I think I might be missing something. It seems like this change applies to the nested arguments of a function pointer, rather than the function pointer itself. Is that right? Does that mean that this is only for function pointers of function pointers?

Copy link
Member Author

Choose a reason for hiding this comment

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

EmbeddedSignatureData is attached to method signatures, but we can decide which method signature it should attach to.

Consider: void Blah(delegate*unmanaged[Cdecl]<void> x);. When we're materializing the topmost parent method signature, we set a flag in the parser that it should generate embedded signature data. Embedded signature data is a sidecar that gets attached to the topmost method signature. It can express things in arbitrary type construction nesting. It's really weird. Back to the signature - we want to attach the modopt (and non-meaningful flags) to the topmost EmbeddedSignatureData. If we were to attach it to the nested signature, it would become part of the identity. But we don't want it part of the identity of the delegate*unmanaged[Cdecl]<void> type because it was decided that for the type system purposes, the only bit that matters is whether the method is unmanaged (and not the exact calling convention).

Copy link
Member

Choose a reason for hiding this comment

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

Ah it was avoiding putting it in the identity that I missed

}

public struct EmbeddedSignatureData
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/tools/Common/TypeSystem/Common/TypeSystemContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,10 @@ protected override FunctionPointerType CreateValueFromKey(MethodSignature key)

public FunctionPointerType GetFunctionPointerType(MethodSignature signature)
{
// The type system only distinguishes between unmanaged and managed signatures.
// The caller should have normalized the signature by modifying flags and stripping modopts.
Debug.Assert((signature.Flags & MethodSignatureFlags.UnmanagedCallingConventionMask) is 0 or MethodSignatureFlags.UnmanagedCallingConvention);
Debug.Assert(!signature.HasEmbeddedSignatureData);
return _functionPointerTypes.GetOrCreateValue(signature);
}

Expand Down
14 changes: 13 additions & 1 deletion src/coreclr/tools/Common/TypeSystem/Ecma/EcmaSignatureParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,19 @@ private MethodSignature ParseMethodSignatureImpl(bool skipEmbeddedSignatureData)
Debug.Assert((int)MethodSignatureFlags.CallingConventionVarargs == (int)SignatureCallingConvention.VarArgs);
Debug.Assert((int)MethodSignatureFlags.UnmanagedCallingConvention == (int)SignatureCallingConvention.Unmanaged);

flags = (MethodSignatureFlags)signatureCallConv;
// If skipEmbeddedSignatureData is true, we're building the signature for the purposes of building a type.
// We normalize unmanaged calling convention into a single value - "unmanaged".
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand why this is safe. You mean that, after the other change, it no longer matters if you have

delegate* unmanaged<int> p

vs.

delegate* unmanaged<[SomeModReq]int> p

We effectively erase custom modifiers for unmanaged function pointers?

Copy link
Member Author

Choose a reason for hiding this comment

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

Modifiers were already erased for runtime pointer types - that's what the existing skipEmbeddedSignatureData argument controls. It distinguishes between "are we parsing a standalone method signature" vs "are we parsing a TypeSpec that is a function pointer typespec". Custom modifiers never matter for type identity - modifiers only matter for signature resolution, but they don't contribute to identity.

We have some tests testing this like:

runtime/src/tests/reflection/Modifiers/modifiersdata.il

Lines 160 to 166 in ed0f1c5

.method public static hidebysig object TryBoxWithModifiedPointer()
{
.locals init (valuetype GenericStruct`1<int32 modreq (BarModifier)*[]>)
ldloc 0
box valuetype GenericStruct`1<int32 modreq (BarModifier)*[]>
ret
}

The logic I'm adding basically switches specific calling conventions into things that act like modifiers (previously they would be part of the type identity).

The added unit test is then testing two things:

  1. That we still consider signatures that differ by modopts to be different signatures
  2. The fact that asking for the type of the method parameter only distinguishes managed/unmanaged

if (skipEmbeddedSignatureData)
{
flags = MethodSignatureFlags.UnmanagedCallingConvention;

// But we still need to remember this signature is different, so add this to the EmbeddedSignatureData of the owner signature.
_embeddedSignatureDataList?.Add(new EmbeddedSignatureData { index = string.Join(".", _indexStack) + "|" + ((int)signatureCallConv).ToString(), kind = EmbeddedSignatureDataKind.UnmanagedCallConv, type = null });
}
else
{
flags = (MethodSignatureFlags)signatureCallConv;
}
}

if (!header.IsInstance)
Expand Down
24 changes: 24 additions & 0 deletions src/coreclr/tools/Common/TypeSystem/MetadataEmitter/TypeSystemMetadataEmitter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,27 @@ public void Push()
}
}

public void UpdateSignatureCallingConventionAtCurrentIndexStack(ref SignatureCallingConvention callConv)
{
if (!Complete)
{
if (_embeddedDataIndex < _embeddedData.Length)
{
if (_embeddedData[_embeddedDataIndex].kind == EmbeddedSignatureDataKind.UnmanagedCallConv)
{
string indexData = string.Join(".", _indexStack);

var unmanagedCallConvPossibility = _embeddedData[_embeddedDataIndex].index.Split('|');
if (unmanagedCallConvPossibility[0] == indexData)
{
callConv = (SignatureCallingConvention)int.Parse(unmanagedCallConvPossibility[1]);
_embeddedDataIndex++;
}
}
}
}
}

public void EmitArrayShapeAtCurrentIndexStack(BlobBuilder signatureBuilder, int rank)
{
var shapeEncoder = new ArrayShapeEncoder(signatureBuilder);
Expand Down Expand Up @@ -665,6 +686,9 @@ private void EncodeMethodSignature(BlobBuilder signatureBuilder, MethodSignature
break;
}

if (sigCallingConvention != SignatureCallingConvention.Default)
signatureDataEmitter.UpdateSignatureCallingConventionAtCurrentIndexStack(ref sigCallingConvention);

signatureEncoder.MethodSignature(sigCallingConvention, genericParameterCount, isInstanceMethod);
signatureBuilder.WriteCompressedInteger(sig.Length);
EncodeType(signatureBuilder, sig.ReturnType, signatureDataEmitter);
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/CoreTestAssembly.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
<AssemblyName>CoreTestAssembly</AssemblyName>
<GenerateAssemblyInfo>false</GenerateAssemblyInfo>
<IsCoreAssembly>true</IsCoreAssembly>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<SkipTestRun>true</SkipTestRun>
<TargetFramework>netstandard2.0</TargetFramework>
<!-- Don't add references to the netstandard platform since this is a core assembly -->
Expand Down
10 changes: 10 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/Platform.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ public struct RuntimeTypeHandle { }
public struct RuntimeMethodHandle { }
public struct RuntimeFieldHandle { }

public class Type
{
public static Type GetTypeFromHandle(RuntimeTypeHandle handle) => null;
}

public class Attribute { }
public class AttributeUsageAttribute : Attribute
{
Expand Down Expand Up @@ -159,9 +164,14 @@ public sealed class IsByRefLikeAttribute : Attribute
{
}

public class CallConvCdecl { }
public class CallConvStdcall { }
public class CallConvSuppressGCTransition { }

public static class RuntimeFeature
{
public const string ByRefFields = nameof(ByRefFields);
public const string UnmanagedSignatureCallingConvention = nameof(UnmanagedSignatureCallingConvention);
public const string VirtualStaticsInInterfaces = nameof(VirtualStaticsInInterfaces);
}

Expand Down
18 changes: 18 additions & 0 deletions ...coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/VirtualFunctionOverride.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,22 @@ class ClassWithFinalizer

}
}

unsafe class FunctionPointerOverloadBase
{
// Do not reorder these, the test assumes this order
public virtual Type Method(delegate* unmanaged[Cdecl]<void> p) => typeof(delegate* unmanaged[Cdecl]<void>);
public virtual Type Method(delegate* unmanaged[Stdcall]<void> p) => typeof(delegate* unmanaged[Stdcall]<void>);
public virtual Type Method(delegate* unmanaged[Stdcall, SuppressGCTransition]<void> p) => typeof(delegate* unmanaged[Stdcall, SuppressGCTransition]<void>);
public virtual Type Method(delegate*<void> p) => typeof(delegate*<void>);
}

unsafe class FunctionPointerOverloadDerived : FunctionPointerOverloadBase
{
// Do not reorder these, the test assumes this order
public override Type Method(delegate* unmanaged[Cdecl]<void> p) => null;
public override Type Method(delegate* unmanaged[Stdcall]<void> p) => null;
public override Type Method(delegate* unmanaged[Stdcall, SuppressGCTransition]<void> p) => null;
public override Type Method(delegate*<void> p) => null;
}
}
21 changes: 21 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/VirtualFunctionOverrideTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.Linq;
using Internal.TypeSystem;

Expand Down Expand Up @@ -268,5 +269,25 @@ public void TestExactMethodImplGenericDeclResolutionOnInterfaces()
Assert.Contains("!0,!1", md1.Name);
Assert.Contains("!1,!0", md2.Name);
}

[Fact]
public void TestFunctionPointerOverloads()
{
MetadataType baseClass = _testModule.GetType("VirtualFunctionOverride", "FunctionPointerOverloadBase");
MetadataType derivedClass = _testModule.GetType("VirtualFunctionOverride", "FunctionPointerOverloadDerived");

var resolvedMethods = new List<MethodDesc>();
foreach (MethodDesc baseMethod in baseClass.GetVirtualMethods())
resolvedMethods.Add(derivedClass.FindVirtualFunctionTargetMethodOnObjectType(baseMethod));

var expectedMethods = new List<MethodDesc>();
foreach (MethodDesc derivedMethod in derivedClass.GetVirtualMethods())
expectedMethods.Add(derivedMethod);

Assert.Equal(expectedMethods, resolvedMethods);

Assert.Equal(expectedMethods[0].Signature[0], expectedMethods[1].Signature[0]);
Assert.NotEqual(expectedMethods[0].Signature[0], expectedMethods[3].Signature[0]);
}
}
}