Conversation
…, ILGeneratorImpl.EmitCalli(Type)
|
Tagging subscribers to this area: @dotnet/area-system-reflection |
| public abstract void EmitCalli(OpCode opcode, CallingConvention unmanagedCallConv, Type? returnType, Type[]? parameterTypes); | ||
|
|
||
| /// <summary> | ||
| /// Puts a <see cref="OpCodes.Calli"/> instruction onto the Microsoft intermediate language (MSIL) stream, |
There was a problem hiding this comment.
I kept this description largely the same as the existing overloads. I think it might make more sense to update it though, since as far as I'm concerned the term MSIL was dropped in favor of CIL.
src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureType.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR implements new function pointer APIs as proposed in issue #75348, enabling better support for function pointer types in IL generation and reflection scenarios.
Changes:
- Adds
Type.MakeFunctionPointerSignatureTypeandType.MakeModifiedSignatureTypestatic factory methods for creating signature types - Implements
ILGenerator.EmitCalli(Type functionPointerType)overload in ILGeneratorImpl for modern function pointer calling conventions - Adds new internal SignatureFunctionPointerType and SignatureModifiedType classes to represent these signature types
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| System.Runtime.cs | Adds public API surface for new function pointer and modified signature type factory methods |
| Type.cs | Implements MakeFunctionPointerSignatureType and MakeModifiedSignatureType factory methods with XML documentation |
| SignatureFunctionPointerType.cs | New internal class representing function pointer signature types with support for calling conventions |
| SignatureModifiedType.cs | New internal class representing types with required/optional custom modifiers |
| SignatureType.cs | Changes UnderlyingSystemType from sealed to virtual to allow SignatureModifiedType to override it |
| ILGenerator.cs | Adds new EmitCalli overload accepting function pointer Type with XML documentation |
| ILGeneratorImpl.cs | Implements the new EmitCalli overload with stack tracking and signature token generation |
| SignatureHelper.cs | Makes WriteSignatureForFunctionPointerType internal and adds logic to unwrap SignatureTypes |
| ModuleBuilderImpl.cs | Adds GetFunctionPointerSignatureToken method to generate standalone signatures for calli instructions |
| DynamicILGenerator.cs | Adds stub TODO implementation for EmitCalli - not yet functional |
| System.Reflection.Emit.ILGeneration.cs | Adds public API surface for new EmitCalli overload |
| Strings.resx | Adds error message for invalid function pointer type arguments |
| System.Private.CoreLib.Shared.projitems | Adds new SignatureFunctionPointerType and SignatureModifiedType to project, plus unrelated whitespace fix |
| SignatureTypes.cs | Adds tests for the new MakeFunctionPointerSignatureType and MakeModifiedSignatureType methods |
| AssemblySaveILGeneratorTests.cs | Adds end-to-end test for EmitCalli with function pointer types |
| Utilities.cs | Adds ClassWithFunctionPointer test helper class |
Comments suppressed due to low confidence (1)
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/SignatureHelper.cs:6
- This using directive appears to be unused. There are no references to System.IO types in this file. Consider removing it to keep the code clean.
using System.Reflection.Metadata;
src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureFunctionPointerType.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureModifiedType.cs
Outdated
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicILGenerator.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureFunctionPointerType.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureModifiedType.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureType.cs
Show resolved
Hide resolved
…ointerSignatureType
src/libraries/System.Reflection.Emit.ILGeneration/tests/ILGenerator/Emit4Tests.cs
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Reflection/SignatureTypes.cs
Show resolved
Hide resolved
|
Is the remaining test failure on Mono a bug in the Mono runtime? I haven't looked that deep into it, but this logic in return GUINT_TO_INT8 (mono_method_signature_has_ext_callconv (m_type_data_get_method_unchecked (type), MONO_EXT_CALLCONV_SUPPRESS_GC_TRANSITION) ? MONO_CALL_UNMANAGED_MD : m_type_data_get_method_unchecked (type)->call_convention);It seems to special case For example, in this signature: delegate* unmanaged[Swift]<delegate* unmanaged[Stdcall, MemberFunction]<short, bool>, string>it simply strips off the |
|
Since said code is subject to change as per #90308 I would rather not touch it for now. I have disabled the offending test on Mono. |
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs
Outdated
Show resolved
Hide resolved
🤖 Copilot Code Review — PR #123819Holistic AssessmentMotivation: This PR implements API Proposal #75348, which is Approach: The PR takes a reasonable split approach: a static factory ( Summary: Detailed Findings
|
src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureType.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureModifiedType.cs
Show resolved
Hide resolved
|
I added @stephentoub Regarding your AI-generated code review, all the problems it detected are deliberate and match the behavior of other reflection APIs like |
src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/SignatureHelper.cs
Outdated
Show resolved
Hide resolved
| if (parameterTypes[i] is not RuntimeType rtType) | ||
| throw new ArgumentException(SR.Argument_MustBeRuntimeType, nameof(parameterTypes)); |
There was a problem hiding this comment.
Do we really want to disallow e.g. signature types here?
typeof(void).MakeFunctionPointerType([Type.MakeGenericMethodParameter(0)], false);A sample handling of signature variables is in MakeGenericType a couple lines below.
There was a problem hiding this comment.
Would it not be confusing if MakeFunctionPointerType could also return signature types, when there is already MakeFunctionPointerSignatureType that is meant for that?
There was a problem hiding this comment.
Would it not be confusing if
MakeFunctionPointerTypecould also return signature types, when there is alreadyMakeFunctionPointerSignatureTypethat is meant for that?
To me the confusing API is MakeFunctionPointerSignatureType. All the other MakeXXX APIs have no problem returning signature types if they receive signature types as inputs:
Console.WriteLine(Type.MakeGenericMethodParameter(0).MakeArrayType().GetType());
Console.WriteLine(Type.MakeGenericMethodParameter(0).MakePointerType().GetType());
Console.WriteLine(Type.MakeGenericMethodParameter(0).MakeByRefType().GetType());
Console.WriteLine(typeof(List<>).MakeGenericType(Type.MakeGenericMethodParameter(0)).GetType());Prints:
System.Reflection.SignatureArrayType
System.Reflection.SignaturePointerType
System.Reflection.SignatureByRefType
System.Reflection.SignatureConstructedGenericType
Maybe instead of MakeFunctionPointerSignatureType, we actually need a MakeModifiedType API that can construct modified types that can then be passed to the existing MakeXXX APIs.
There was a problem hiding this comment.
I originally envisioned MakeModifiedSignatureType and MakeFunctionPointerSignatureType (then without the Signature prefix) as simple convenience methods for ILGenerator.EmitCalli. These methods being static was purely a style choice, but it led @jkotas to update the proposal to distinguish between signature types and runtime types. Jan, maybe you can give your opinion on this?
Maybe instead of
MakeFunctionPointerSignatureType, we actually need aMakeModifiedTypeAPI that can construct modified types that can then be passed to the existingMakeXXXAPIs.
Is that feasible? Was it not decided in #69273 that custom modifiers are not part of type identity?
There was a problem hiding this comment.
Is that feasible? Was it not decided in #69273 that custom modifiers are not part of type identity?
Yes, modifiers are not part of the runtime type identity, so a type with modifiers will not be a RuntimeType (we do not have existing factory APIs to make modified types right now). However, the existing MakeXXX APIs do handle signature types, which is why MakeGenericType will work just fine when passed a signature type.
The closest thing to the MakeFunctionPointerSignatureType is the pre-existing MakeGenericSignatureType API. The reason why this API was added is described in #27152 - it's purely for people who need to call MakeGenericType on a type that is not a RuntimeType (if the generic definition was a runtime type, one could just call MakeGenericType and get the same result).
MakeFunctionPointerSignatureType as it is in this PR is kind of a macro - it will create a modified type as needed and then a function pointer from it. But I think it should be possible to construct a signature (non-runtime) function pointer type using the MakeFunctionPointerType API too, for parity with the existing MakeXXX APIs..
(And I do wonder a bit whether we should also have a non-macro version of MakeFunctionPointerSignatureType)
There was a problem hiding this comment.
- It creates an illegal state where the calling conventions are present as optional modifiers on the return type, but not part of
GetFunctionPointerCallingConventions
This depends on where does the function pointer signature type pull the data from. In this PR, the calling conventions are stored "on the side", in the function pointer type itself. The return parameter can also be a modified type. We currently don't allow the return parameter to have callconv modifiers (or... we do, but we don't report them as the calling convention of the function pointer type).
An interesting exercise would be what happens when there are more non-callconv modifiers on the return type. C++/CLI generates a modifier for const. But we don't even have to go to C++/CLI, C# also does this sometimes:
unsafe class Program
{
// Generated as:
// .field private method unmanaged cdecl int32& modreq([System.Runtime]System.Runtime.InteropServices.InAttribute) modopt([System.Runtime]System.Runtime.CompilerServices.CallConvSuppressGCTransition) *() _field
delegate* unmanaged[SuppressGCTransition]<ref readonly int> _field;
}Are we able to currently Ref.Emit this? If so, are we only able to emit this because of luck? (I.e. if a different language compiler emits the modifier in a different order - callconv first, then the other non-callconv modifier, would we still be able to emit this?). The order of modifiers matters - swapping the order means the signatures are no longer equal - memberrefs will not resolve, overrides will no longer override.
For the ordering reason I'm also a bit suspicious of the MakeModifiedSignatureType(Type type, Type[]? requiredCustomModifiers, Type[]? optionalCustomModifiers) API, but that one can be worked around (I would have preferred MakeModifiedType(Type modifier, bool required) mentioned in #20377 - but this can be emulated with the API added here.
There was a problem hiding this comment.
delegate* unmanaged[SuppressGCTransition]<ref readonly int> _field;Are we able to currently Ref.Emit this?
With the APIs in this PR, yes. Although the generated signature you showed is incorrect (there should not be a cdecl there - ILDASM displays this incorrectly, ILSpy does it right).
If so, are we only able to emit this because of luck?
The modifier encoding is defined by Reflection.Emit, not by any compiler. It is deterministic, so there is no luck involved. If a third-party .NET compiler decided to encode the modifiers differently from the established standard, that would not only break interop with Ref.Emit, but also with existing .NET assemblies. Reflection.Emit follows the standard set by Roslyn, that seems fine to me.
There was a problem hiding this comment.
Also, MakeModifiedSignatureType was never meant for specifying calling conventions. It was only intended to be used together with MakeFunctionPointerSignatureType to enable function pointers with ref readonly, in or out parameters.
There was a problem hiding this comment.
The modifier encoding is defined by Reflection.Emit, not by any compiler. It is deterministic, so there is no luck involved. If a third-party .NET compiler decided to encode the modifiers differently from the established standard, that would not only break interop with Ref.Emit, but also with existing .NET assemblies. Reflection.Emit follows the standard set by Roslyn, that seems fine to me.
The calling conventions are communicated in-band with the other modifiers on the return type. The .NET runtime doesn't require the calling convention ones to be first, or last, or form an uninterrupted run.
If we don't want the new API on Ref.Emit to be able to generate combinations that are valid and can be generated with other writers, that's fine I guess, but it should be a conscious decision. Inability to generate valid signatures will be a Ref.Emit limitation.
The order doesn't matter for calli. It will matter if we want to Ref.Emit an override for a method that has a parameter with a function pointer with an unrepresentable order of modifiers, or generate a MemberRef to such method/field.
There was a problem hiding this comment.
The .NET runtime doesn't require the calling convention ones to be first, or last, or form an uninterrupted run.
I worded that a bit imprecisely, the only rule Reflection.Emit follows is that all modreqs are written before all modopts. You can generate any signature you want if you pass in a TypeDelegator or similar, or even just a type retrieved from MakeModifiedSignatureType.
MakeFunctionPointerSignatureType is the API that "enforces" Roslyn encoding rules by default, but you do not have to use it.
There was a problem hiding this comment.
Do we need to override MakeFunctionPointerType here? To support Type.MakeGenericMethodParameter(0).MakeFunctionPointerType(...).
Might want to make a pass over all overrides of MakeGenericType in this repo to see if we need to also override the new API.
|
|
||
| for (int i = 0; i < parameterTypes.Length; i++) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(parameterTypes[i], nameof(parameterTypes)); |
There was a problem hiding this comment.
Do we want to allow void as parameter type?
There was a problem hiding this comment.
(Or uninstantiated generic type definitions.)
There was a problem hiding this comment.
Probably not, I'll add checks for those.
| { | ||
| Debug.Assert(type != null); | ||
|
|
||
| if (type.IsFunctionPointer) |
There was a problem hiding this comment.
I do not see why we are special casing function pointer here. Should it be handled by AddOneArgTypeHelper instead?
If there is a non-obvious reason for this special case, comment would be nice.
There was a problem hiding this comment.
I moved it to AddOneArgTypeHelper.
| if (t.ContainsGenericParameters) | ||
| throw new ArgumentException(SR.Argument_GenericsInvalid, nameof(requiredCustomModifiers)); | ||
|
|
||
| AddElementType((CorElementType)0x22); // ELEMENT_TYPE_CMOD_INTERNAL |
There was a problem hiding this comment.
Is there a reason why we cannot use the ELEMENT_TYPE_CMOD_OPT/ELEMENT_TYPE_CMOD_REQD encoding that's used for arguments in AddDynamicArgument? It looks odd that argument types and return types use different encoding.
| } | ||
| } | ||
| else if (clsArgument.IsFunctionPointer && scope != null) | ||
| { |
There was a problem hiding this comment.
Should we throw NotSupportedException for scope == null? It is better to throw than produce bad binary silently.
I assume that it can be hit with (non-persistent) Reflection.Emit. Is that right? We may want to add a test that validates it is throwing (and not crashing or producing bad output).
This implements #75348.
The APIs from the proposal are all implemented now, but there are still some rough edges and design considerations needing to be discussed.