Add reflection introspection support for function pointers #81006

steveharter · 2023-01-22T23:41:01Z

Function pointers are now supported via reflection introspection in both the runtime (CoreClr) and MetadataLoadContext. This closes a reflection introspection functionality gap that has existed since the beginning of .NET but has become more prevalent since .NET 5.0 with the C# support for function pointers. It is now possible to fully enumerate function pointers parameters and the return value to find all of the referenced types as well as inspect calling conventions.

Fixes #69273
Fixes #43791

See the Design for details and previous discussions.

Mono and Native support will come later. The managed code and tests are designed to be shared. Tests are also shared between runtime and MetadataLoadContext.

ghost · 2023-01-22T23:41:15Z

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

[WIP]

Author:	steveharter
Assignees:	steveharter
Labels:	`area-System.Reflection`
Milestone:	8.0.0

dotnet-issue-labeler · 2023-01-22T23:41:16Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

src/libraries/System.Private.CoreLib/src/System/Reflection/ModifiedFunctionPointerType.cs

src/coreclr/vm/typekey.h

src/libraries/System.Private.CoreLib/src/System/Reflection/ModifiedType.cs

src/coreclr/System.Private.CoreLib/src/System/Reflection/ModifiedType.CoreCLR.cs

src/libraries/Common/tests/System/ModifiedTypeTests.cs

ghost · 2023-01-24T20:12:47Z

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

buyaa-n

Left a few NITs, overall LGTM thanks!

src/coreclr/utilcode/sigparser.cpp

src/libraries/Common/tests/System/FunctionPointerTests.cs

src/libraries/Common/tests/System/ModifiedTypeTests.cs

src/coreclr/vm/runtimehandles.cpp

src/libraries/System.Private.CoreLib/src/System/Reflection/ModifiedType.cs

...tem.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Types/RoModifiedType.cs

jkotas · 2023-02-15T13:09:17Z

src/libraries/System.Private.CoreLib/src/System/Reflection/ModifiedType.cs

+            return GetCustomModifiers(required: false);
+        }
+
+        // Modified types do not support Equals other than when references are equal.


I am wondering whether it would be better to throw from Equals and GetHashCode instead.

I do not think that this Equals implementation can be ever used for anything useful, and it is not what one would expect. Throwing would make it obvious.

I was wondering if there as a precedence for this, and found there is:
https://github.com/dotnet/runtime/blob/main/src/coreclr/nativeaot/System.Private.CoreLib/src/System/TypedReference.cs#L73
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HeaderDescriptor.cs#L56
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/HashCode.cs#L503
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/ReadOnlySpan.cs#L190
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Span.cs#L196

So throwing is the safest option at this point; we can of course implement the full Equals if necessary, but I just don't see the value in doing that now.

It turns out MetadataLoadContext does use Equals\GetHashCode for caching; determining options.

Does it mean that the cache is broken or ineffective in the current version of the PR?

Ineffective; changing code to not cache for modified types and ensuring test coverage.

jkotas · 2023-02-15T13:11:58Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-02-15T13:12:29Z

Azure Pipelines successfully started running 1 pipeline(s).

steveharter · 2023-02-15T19:48:57Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-02-15T19:49:26Z

Azure Pipelines successfully started running 1 pipeline(s).

steveharter · 2023-02-15T21:58:24Z

OSX errors due to #82240

Mono source gen errors due to #81249

Below are various runtime-extra-platform CI errors that appear unrelated:

RestApiException`1: The response contained an invalid status code 500 Internal Server Error
Body: {"Message":"An error occured.","ActivityId":"7faed39a15b124fd31a066b17c04b2c8"}
   at Microsoft.DotNet.Helix.Client.Storage.OnNewFailed(Request req, Response res) in /_/src/Microsoft.DotNet.Helix/Client/CSharp/generated-code/Storage.cs:line 202
   at Microsoft.DotNet.Helix.Client.Storage.NewAsync(ContainerCreationRequest body, CancellationToken cancellationToken) in /_/src/Microsoft.DotNet.Helix/Client/CSharp/generated-code/Storage.cs:line 164
   at Microsoft.DotNet.Helix.Client.ApiBlobHelper.GetContainerAsync(String requestedName, String targetQueue, CancellationToken cancellationToken) in /_/src/Microsoft.DotNet.Helix/JobSender/StorageHelpers/ApiBlobHelper.cs:line 25
   at Microsoft.DotNet.Helix.Client.JobDefinition.SendAsync(Action`1 log, CancellationToken cancellationToken) in /_/src/Microsoft.DotNet.Helix/JobSender/JobDefinition.cs:line 174
   at Microsoft.DotNet.Helix.Sdk.SendHelixJob.ExecuteCore(CancellationToken cancellationToken) in /_/src/Microsoft.DotNet.Helix/Sdk/SendHelixJob.cs:line 267
   at Microsoft.DotNet.Helix.Sdk.HelixTask.Execute() in /_/src/Microsoft.DotNet.Helix/Sdk/HelixTask.cs:line 58

Issue #80976:

Metadata file '/Users/runner/work/1/s/artifacts/bin/StartupHookForFunctionalTest/Release/net8.0/iossimulator-x64/StartupHookForFunctionalTest.dll' could not be found

Issue #82141:

    MonoTests.System.Drawing.GraphicsTest.Rotate [FAIL]
      Microsoft.DotNet.XUnitExtensions.SkipTestException : Precision on float numbers
      Stack Trace:
        /_/src/libraries/System.Drawing.Common/tests/mono/System.Drawing/GraphicsTests.cs(2385,0): at MonoTests.System.Drawing.GraphicsTest.Rotate()
           at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
        /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs(59,0): at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
    MonoTests.System.Drawing.GraphicsTest.VisibleClipBound_BigClip [FAIL]
      Microsoft.DotNet.XUnitExtensions.SkipTestException : Precision on float numbers
      Stack Trace:
        /_/src/libraries/System.Drawing.Common/tests/mono/System.Drawing/GraphicsTests.cs(2338,0): at MonoTests.System.Drawing.GraphicsTest.VisibleClipBound_BigClip()
           at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
        /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs(59,0): at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
    MonoTests.System.Drawing.GraphicsTest.VisibleClipBound [FAIL]
      Microsoft.DotNet.XUnitExtensions.SkipTestException : Precision on float numbers
      Stack Trace:
        /_/src/libraries/System.Drawing.Common/tests/mono/System.Drawing/GraphicsTests.cs(2302,0): at MonoTests.System.Drawing.GraphicsTest.VisibleClipBound()
           at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
        /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs(59,0): at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

Issue #82022; NativeAOT failures in Caching and EventSource; this PR did not change NativeAOT)

----- start Wed 02/15/2023 22:51:58.16 ===============  To repro directly: ===================================================== 
pushd C:\h\w\AB95098E\w\AEBC094D\e\
Microsoft.Extensions.Logging.EventSource.Tests.exe -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing -xml testResults.xml 
popd
===========================================================================================================

C:\h\w\AB95098E\w\AEBC094D\e>Microsoft.Extensions.Logging.EventSource.Tests.exe -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing -xml testResults.xml  
Running assembly:Microsoft.Extensions.Logging.EventSource.Tests, Version=8.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60
----- end Wed 02/15/2023 22:52:03.43 ----- exit code -2147483645 ----------------------------------------------------------
2023-02-15T22:52:05.777Z	INFO   	run.py	run(48)	main	Beginning reading of test results.
2023-02-15T22:52:05.777Z	INFO   	run.py	__init__(42)	read_results	Searching 'C:\h\w\AB95098E\w\AEBC094D\e' for test results files
2023-02-15T22:52:05.777Z	INFO   	run.py	__init__(42)	read_results	Searching 'C:\h\w\AB95098E\w\AEBC094D\uploads' for test results files
2023-02-15T22:52:05.793Z	WARNING	run.py	__init__(55)	read_results	No results file found in any of the following formats: xunit, junit, trx
2023-02-15T22:52:05.793Z	INFO   	run.py	packing_test_reporter(30)	report_results	Packing 0 test reports to 'C:\h\w\AB95098E\w\AEBC094D\e\__test_report.json'
2023-02-15T22:52:05.793Z	INFO   	run.py	packing_test_reporter(33)	report_results	Packed 1543 bytes
ERROR: The process "corerun.exe" not found.
read file: C:\h\w\AB95098E\p\debug-dump-template.md
writing output file: C:\h\w\AB95098E\w\AEBC094D\uploads\how-to-debug-dump.md
done writing debug dump information
['Microsoft.Extensions.Logging.EventSource.Tests' END OF WORK ITEM LOG: Command exited with -2147483645]

jkotas

LGTM!

steveharter · 2023-03-16T17:41:55Z

Breaking change doc: dotnet/docs#34623

After dotnet#81006, the calling convention is no longer part of the type system identity of a function pointer type - it serves more like a modopt as far as the type system is concerned. The type system only cares whether the pointer is managed or not. Adjust the managed type system accordingly: * If we're reading/representing a standalone method signature, read it as usual. Calling convention is available in flags/modopts. * If we're reading/representing a function pointer type, collapse the calling convention information into the managed/unmanaged bit only.

Add reflection introspection support for function pointers

173b84d

steveharter added the area-System.Reflection label Jan 22, 2023

steveharter added this to the 8.0.0 milestone Jan 22, 2023

steveharter self-assigned this Jan 22, 2023

dotnet-issue-labeler bot added the new-api-needs-documentation label Jan 22, 2023

build-analysis bot mentioned this pull request Jan 23, 2023

Tracking issue for CI build timeouts #76454

Closed

Fix compiler issue; test MLC to NetFramework

e546ee2

jkotas reviewed Jan 23, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Reflection/ModifiedFunctionPointerType.cs Outdated Show resolved Hide resolved

jkotas reviewed Jan 23, 2023

View reviewed changes

src/coreclr/vm/typekey.h Outdated Show resolved Hide resolved

steveharter added 3 commits January 23, 2023 11:45

Fix compile issue; prep for review

36e07d6

Compile issue with MLC under certain conditions

e755c1c

compile issue cont'd; lazy load mods on root

b3b64ff

jkotas reviewed Jan 23, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Reflection/ModifiedType.cs Outdated Show resolved Hide resolved

compile issues cont'd; prepare for review

3aef789

steveharter marked this pull request as ready for review January 24, 2023 01:01

steveharter requested review from marek-safar and MichalStrehovsky as code owners January 24, 2023 01:01

steveharter requested review from buyaa-n, lambdageek and AaronRobinsonMSFT January 24, 2023 01:02

jkotas reviewed Jan 24, 2023

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/Reflection/ModifiedType.CoreCLR.cs Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Jan 24, 2023

tracing/eventpipe/eventsourceerror/eventsourceerror/eventsourceerror failure #80666

Closed

runfoapp bot mentioned this pull request Jan 24, 2023

Infra improvements for Helix #68176

Closed

Remove Node back reference; fix edge case test failure

f76c75d

steveharter commented Jan 24, 2023

View reviewed changes

src/libraries/Common/tests/System/ModifiedTypeTests.cs Outdated Show resolved Hide resolved

steveharter added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jan 24, 2023

ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jan 24, 2023

buyaa-n approved these changes Feb 15, 2023

View reviewed changes

src/coreclr/utilcode/sigparser.cpp Show resolved Hide resolved

src/libraries/Common/tests/System/FunctionPointerTests.cs Outdated Show resolved Hide resolved

src/libraries/Common/tests/System/ModifiedTypeTests.cs Show resolved Hide resolved