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.

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

[mono] Support function pointer reflection introspection API #89712

Merged
merged 5 commits into from
Aug 11, 2023

Conversation

ivanpovazan
Copy link
Member

@ivanpovazan ivanpovazan commented Jul 31, 2023

This PR implements support for function pointers reflection introspection API with Mono based on the design doc.

Done

  • Added support for IsFunctionPointer
  • Added support for IsUnmanagedFunctionPointer
  • Added support for ToString
  • Added support for GetFunctionPointerParameterTypes
  • Added support for GetFunctionPointerReturnType
  • Added support for GetRequiredCustomModifiers
  • Added support for GetOptionalCustomModifiers
  • Added support for GetFunctionPointerCallingConventions
  • Implement ModifiedType.Mono
    • Added support for GetModifiedParameterType
    • Added support for GetModifiedFieldType
    • Added support for GetModifiedPropertyType
  • Enabled previously disabled function pointer tests
  • Added additional tests to src/libraries/Common/tests/System/ModifiedTypeTests.cs to verify fetching custom modifiers attached to generic arguments exposed via ModifiedType on a generic field and property (this is an addition to existing generic parameter tests

Not done

What is missing in this PR is proper support for function pointer type equality and type compatibility which was reported as a separate issue: #90308 and will be handled separately.

The remaining tests cases verifying this missing functionality have been updated with the new tracking issue.


Fixes #71095

@ivanpovazan ivanpovazan added the NO-REVIEW Experimental/testing PR, do NOT review it label Jul 31, 2023
@ivanpovazan ivanpovazan added this to the 8.0.0 milestone Jul 31, 2023
@ivanpovazan ivanpovazan self-assigned this Jul 31, 2023
@ivanpovazan
Copy link
Member Author

@vargaz, I have a question regarding fptr namespace and name handling.

The tests expect that:

Assert.Equal(string.Empty, t.Name);
Assert.Null(t.Namespace);

Should we keep internally the following values:

result->name_space = "System";
result->name = "MonoFNPtrFakeClass";

and only special case ves_icall_RuntimeType_GetName and ves_icall_RuntimeType_GetNamespace for function pointers?

I am asking this because changing result->name_space = NULL; would break in different places as we assume name_space is not NULL.

@vargaz
Copy link
Contributor

vargaz commented Jul 31, 2023

Special casing GetName/GetNamespace would probably cause less problems than setting the namespace to NULL.

@@ -1,3 +1,4 @@
<linker>
<assembly fullname="TestLoadAssembly" />
<assembly fullname="TestILAssembly" />
Copy link
Member Author

@ivanpovazan ivanpovazan Aug 9, 2023

Choose a reason for hiding this comment

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

Not sure if this is the best place to root the assembly in question, but it needs to be rooted to prevent failures in EnableAggressiveTrimming setups for FPTR testing.

PS The assembly is used in several places to test FPTR support.

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

LGTM, although I'd like TypeSignature to contain more of the logic about figuring out which signature source is being used so that less of it leaked out into the rest of ModifiedType

@ivanpovazan ivanpovazan marked this pull request as ready for review August 10, 2023 13:14
@ivanpovazan ivanpovazan changed the title WIP: [mono] Support function pointer reflection introspection API [mono] Support function pointer reflection introspection API Aug 10, 2023
@ivanpovazan ivanpovazan removed the NO-REVIEW Experimental/testing PR, do NOT review it label Aug 10, 2023
…stILAssembly for aggressive trimming scenarios
@ivanpovazan
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan
Copy link
Member Author

The CI failures are unrelated.

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@@ -5,16 +5,151 @@ namespace System.Reflection
{
internal partial class ModifiedType
{
/// <summary>
Copy link
Member

Choose a reason for hiding this comment

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

👍 This whole file looks much clearer now, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Yes and glad the template worked for mono.

@@ -42,5 +42,20 @@
IL_0000: nop
IL_0001: ret
}

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding these tests.

@steveharter
Copy link
Member

I assume mono_class_create_fnptr() is still used along with an internal type of "MonoFNPtrFakeClass"?

Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

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

Thanks - the Mono integration looks fairly clean.

@ivanpovazan
Copy link
Member Author

I assume mono_class_create_fnptr() is still used along with an internal type of "MonoFNPtrFakeClass"?

Yes, that is correct.

@ivanpovazan ivanpovazan merged commit 5d30e6d into dotnet:main Aug 11, 2023
183 of 212 checks passed
@ivanpovazan ivanpovazan deleted the fptr-mono-reflection branch August 12, 2023 12:34
@ghost ghost locked as resolved and limited conversation to collaborators Sep 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Function Pointer support to Mono Reflection
4 participants