-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Support references to unmanaged function pointers in Reflection.Emit #121128
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for emitting references to unmanaged function pointer types in Reflection.Emit, addressing issue #120909. Previously, attempting to reference fields or methods with unmanaged function pointer signatures would fail.
Key Changes:
- Modified signature encoding to properly handle function pointer types by using
UnderlyingSystemTypefor non-function-pointer types - Updated member reference handling to retrieve modified types for function pointer fields and method parameters/returns
- Expanded test coverage to include both fields and methods with various function pointer signatures
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| AssemblySaveTypeBuilderTests.cs | Expanded tests from just fields to include methods with function pointer signatures; uncommented previously broken test cases |
| SignatureHelper.cs | Added logic to use UnderlyingSystemType for non-function-pointer types when writing signatures |
| ModuleBuilderImpl.cs | Enhanced member reference handling to use modified types (GetModifiedFieldType, GetModifiedParameterType) for function pointers |
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/SignatureHelper.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/SignatureHelper.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs
Show resolved
Hide resolved
|
Tagging subscribers to this area: @dotnet/area-system-reflection-emit |
...raries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveTypeBuilderTests.cs
Show resolved
Hide resolved
I have tried a small standalone test and I do not see a problem with this. Also, I have checked the runtime implementation and it does take assembly forwarding into account, so modopts referencing System.Runtime and System.Private.Corelib are expected to match. I think it is something else. |
|
Wrt Mono failures: It is ok to disable this test on Mono |
The problem is that the order of modopts is getting swapped. The defining assembly has CallConvSuppressGCTransition first: The reference has the modopts in opposite order - CallConvCdecl is first: The order must match. |
|
I am currently looking into it more thoroughly, of course using the modified type only when the type itself is a function pointer was too conservative of a condition. I switched to using modified types all the time, but I stumbled on some strange behavior regarding generics: using System;
using System.Reflection;
// Generic type left open on purpose
FieldInfo field = typeof(Container<>).GetField("Field");
Console.WriteLine(field.FieldType.GetFunctionPointerReturnType().IsGenericParameter);
Console.WriteLine(field.GetModifiedFieldType().GetFunctionPointerReturnType().IsGenericParameter);
// Output:
// True
// False
public unsafe class Container<T>
{
public static delegate*<T> Field;
public static delegate* unmanaged[Cdecl]<T, U> Method<U>() => null;
}To me this makes no sense, or am I overseeing something? The type being modified should not interfere with |
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/SignatureHelper.cs
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/SignatureHelper.cs
Outdated
Show resolved
Hide resolved
Looks like a bug in the ModifiedType implementation |
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs
Outdated
Show resolved
Hide resolved
|
Works now for all cases covered by tests. None of the Reflection.Emit APIs implement I imagine there might be some edge cases, such as when you use a type parameter of a generic |
What would it take to implement these APIs on these types? Special casing the Reflection.Emit APIs by type name does not look good. |
For the new implementation ( As for the types defined in |
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/FieldBuilderImpl.cs
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/SignatureHelper.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/SignatureHelper.cs
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/SignatureHelper.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs
Outdated
Show resolved
Hide resolved
|
There are failing tests on Mono: Could you please either fix if it is something simple, or disable these tests on Mono (you can use existing issue tracks RefEmit failures on Mono - |
I have disabled this test for now. I might look over it in the future, but right now I don't have an environment where I can work on this or test it effectively on Mono. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @AaronRobinsonMSFT Could you please review this as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
![]()
|
/ba-g Unrelated wasm failures on FireFox. |
This fixes #120909.
There is one small issue with the tests when the calling conventions are encoded as modopts: The
PersistedAssemblyBuilderfrom the test specifiesSystem.Private.CoreLibas its core assembly, which prevents the runtime from matching the signatures with the ones defined in C# (where the modifier types are fromSystem.Runtime). This is seemingly not an issue with parameter types, but only with modifiers. @jkotas or anyone else, do you have an idea how to fix this? If the AssemblyBuilder used the same core assembly as the C# program, it should work. For now, I have commented out this specific field reference.