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

Add reflection introspection support for function pointers #71516

Closed
wants to merge 10 commits into from

Conversation

steveharter
Copy link
Member

@steveharter steveharter commented Jul 1, 2022

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

Design\implementation notes:

  • Per the API issue, function pointers are supported thrown new members on System.Type class through additional members including bool IsFunctionPointer {get}.
  • Type.ToString() returns a friendly representation of the function pointer but does not include calling conventions.
  • Type.Name returns *()
  • Type.FullName and Type.AssemblyQualifiedName return null. In V8 we can support a non-null version based on feedback; that would require a type name grammar update including custom modifiers as well as support for Type.GetType(), Assembly.GetType() and any friend APIs that would need to parse that string and construct a function pointer.
    • Note that these two properties are already nullable, and do return null in open generic parameters cases as well.
  • Type.IsPointer returns false as to prevent another breaking change and because function pointers are a different type in the CLR and in ECMA 335.
  • Native calling conventions are exposed via GetFunctionPointerCallingConventions() which returns the corresponding System.Runtime.CompilerServices.CallConv* types. This makes consuming the APIs easier and less error-prone that exposing both the raw "CallKind" enum along with the newer mod opts on the return parameter, since there can be more than one way to encode a calling convention by using those.
  • No invocation support or Emit support was added. These are a V8 candidate feature based on feedback.
  • Mono and Native support will come later.
  • Tests are shared between runtime and MetadataLoadContext.

Todo:

  • API doc
  • Misc other tests

@steveharter steveharter added this to the 7.0.0 milestone Jul 1, 2022
@steveharter steveharter self-assigned this Jul 1, 2022
@dotnet-issue-labeler
Copy link

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.

@ghost
Copy link

ghost commented Jul 1, 2022

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

Issue Details

Detecting Mono errors etc

Author: steveharter
Assignees: steveharter
Labels:

area-System.Reflection

Milestone: 7.0.0

src/coreclr/vm/typekey.h Outdated Show resolved Hide resolved
src/coreclr/vm/typedesc.h Outdated Show resolved Hide resolved
@steveharter steveharter force-pushed the FcnPtr branch 3 times, most recently from 2528f1f to 66f942e Compare July 6, 2022 19:25
@steveharter steveharter marked this pull request as ready for review July 6, 2022 21:55
@steveharter steveharter added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jul 6, 2022
@ghost 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 Jul 6, 2022
@ghost
Copy link

ghost commented Jul 6, 2022

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

When you commit this breaking change:

  1. 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.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

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

@steveharter
Copy link
Member Author

steveharter commented Jul 6, 2022

@MichalStrehovsky thoughts on arm64 NativeAOT CI failure?

Looks like this path:



case TypeFlags.FunctionPointer:
case TypeFlags.ByRef:
// Pointers and byrefs are not boxable
return false;

  System.Runtime.Tests -> /__w/1/s/artifacts/bin/System.Runtime.Tests/Release/net7.0-unix/System.Runtime.Tests.dll
  Optimizing assemblies for size may change the behavior of the app. Be sure to test after publishing. See: https://aka.ms/dotnet-illink
  Generating compatible native code. To optimize for size or speed, visit https://aka.ms/OptimizeNativeAOT
EXEC : error : Failed to load type 'Int32 ()*[]' from assembly '?' [/__w/1/s/src/libraries/System.Runtime/tests/System.Runtime.Tests.csproj]
##[error]EXEC(0,0): error : (NETCORE_ENGINEERING_TELEMETRY=Build) Failed to load type 'Int32 ()*[]' from assembly '?'
  Internal.TypeSystem.TypeSystemException+TypeLoadException: Failed to load type 'Int32 ()*[]' from assembly '?'
     at Internal.TypeSystem.ThrowHelper.ThrowTypeLoadException(ExceptionStringID id, String typeName, String assemblyName) in /_/src/coreclr/tools/Common/TypeSystem/Common/ThrowHelper.cs:line 17
     at ILCompiler.DependencyAnalysis.EETypeNode..ctor(NodeFactory factory, TypeDesc type) in /_/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs:line 81
     at ILCompiler.DependencyAnalysis.NodeFactory.CreateConstructedTypeNode(TypeDesc type) in /_/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs:line 531
     at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
     at ILCompiler.DependencyAnalysis.ReflectableFieldNode.GetStaticDependencies(NodeFactory factory) in /_/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectableFieldNode.cs:line 89
     at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.GetStaticDependenciesImpl(DependencyNodeCore`1 node) in /_/src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/DependencyAnalyzer.cs:line 181
     at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.GetStaticDependencies(DependencyNodeCore`1 node) in /_/src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/DependencyAnalyzer.cs:line 221
     at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.ProcessMarkStack() in /_/src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/DependencyAnalyzer.cs:line 256
     at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.ComputeMarkedNodes() in /_/src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/DependencyAnalyzer.cs:line 307
     at ILCompiler.ILScanner.ILCompiler.IILScanner.Scan() in /_/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs:line 140
     at ILCompiler.Program.Run(String[] args) in /_/src/coreclr/tools/aot/ILCompiler/Program.cs:line 833
     at ILCompiler.Program.Main(String[] args) in /_/src/coreclr/tools/aot/ILCompiler/Program.cs:line 1090
/__w/1/s/artifacts/bin/coreclr/Linux.arm64.Release/build/Microsoft.NETCore.Native.targets(271,5): error MSB3073: The command ""/__w/1/s/artifacts/bin/coreclr/Linux.arm64.Release/x64/ilc/ilc" @"/__w/1/s/artifacts/obj/System.Runtime.Tests/Release/net7.0-unix/native/System.Runtime.Tests.ilc.rsp"" exited with code 1. [/__w/1/s/src/libraries/System.Runtime/tests/System.Runtime.Tests.csproj]
##[error]artifacts/bin/coreclr/Linux.arm64.Release/build/Microsoft.NETCore.Native.targets(271,5): error MSB3073: (NETCORE_ENGINEERING_TELEMETRY=Build) The command ""/__w/1/s/artifacts/bin/coreclr/Linux.arm64.Release/x64/ilc/ilc" @"/__w/1/s/artifacts/obj/System.Runtime.Tests/Release/net7.0-unix/native/System.Runtime.Tests.ilc.rsp"" exited with code 1.

Build FAILED.

EXEC : error : Failed to load type 'Int32 ()*[]' from assembly '?' [/__w/1/s/src/libraries/System.Runtime/tests/System.Runtime.Tests.csproj]
/__w/1/s/artifacts/bin/coreclr/Linux.arm64.Release/build/Microsoft.NETCore.Native.targets(271,5): error MSB3073: The command ""/__w/1/s/artifacts/bin/coreclr/Linux.arm64.Release/x64/ilc/ilc" @"/__w/1/s/artifacts/obj/System.Runtime.Tests/Release/net7.0-unix/native/System.Runtime.Tests.ilc.rsp"" exited with code 1. [/__w/1/s/src/libraries/System.Runtime/tests/System.Runtime.Tests.csproj]
    0 Warning(s)
    2 Error(s)

@MichalStrehovsky
Copy link
Member

We're not supposed to get to the ConstructedEEType path from ReflectableFieldNode for function pointers. There's a very explicit check for that case here.

// Runtime reflection stack needs to obtain the type handle of the field
// (but there's no type handles for function pointers)
if (!_field.FieldType.IsFunctionPointer)
dependencies.Add(factory.MaximallyConstructableType(_field.FieldType.NormalizeInstantiation()), "Type of the field");

Similar thing was reported in #71063 but I never got a repro I could look at. Happy we now have a repro. I'll take a look and report back with a fix.

@steveharter
Copy link
Member Author

I wanted to try this out locally to see how it behaves. It's running into a contract violation in the modifiers.il test (that I wanted to use for my experimentation). Pasting here just in case it isn't known - we don't run this test in the CI because it's Pri-1.

For now, run in release build (not checked or debug). I'm working on local changes that resolve the contract issues.

@steveharter steveharter changed the title [WIP]Add reflection introspection support for function pointers Add reflection introspection support for function pointers Jul 8, 2022
}

// Normalize the calling conventions.
if (!foundCallingConvention)
Copy link
Member

Choose a reason for hiding this comment

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

This should be the other way around. sigCallingConvention should be checked first and the modopts should be read only for SignatureCallingConvention.Unmanaged.

That's how it is done in the runtime and Roslyn:

https://github.com/dotnet/roslyn/blob/ca8398b08b142baab1eaa5d4d89eeea61c415565/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/SymbolKey/SymbolKey.FunctionPointerTypeSymbolKey.cs#L18-L21

case MethodSignatureFlags.UnmanagedCallingConvention:
if (TryGetUnmanagedCallingConventionFromModOpt(signature, out CorInfoCallConvExtension callConvMaybe, out suppressGCTransition))
{
return callConvMaybe;
}
else
{
return (CorInfoCallConvExtension)PlatformDefaultUnmanagedCallingConvention();
}

Copy link
Member Author

Choose a reason for hiding this comment

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

In addition to normalizing the value from GetFunctionPointerCallingConventions, this also "normalizes" the value from GetOptionalCustomModifiers(), meaning for example:

  • If sigCallingConvention is StdCall (and thus no StdCall mod opt) then we add the corresponding Stdcall modopt.
  • If sigCallingConvention is Unmanaged and there is a modopt for StdCall, then we leave as-is.

Copy link
Member

Choose a reason for hiding this comment

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

This normalization can still be done even when sigCallingConvention is checked first.

@@ -2953,13 +2955,36 @@ TypeHandle ClassLoader::CreateTypeHandleForTypeKey(TypeKey* pKey, AllocMemTracke
else if (pKey->GetKind() == ELEMENT_TYPE_FNPTR)
{
Module *pLoaderModule = ComputeLoaderModule(pKey);
Copy link
Member

Choose a reason for hiding this comment

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

ComputeLoaderModule needs to be modified to take the custommods into account to handle the case where they are defined in unloaded assembly.

Also, we need to call EnsureInstantiation or something like that to capture the dependencies.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think 'll hold off on this until we decide if we want to go with a restricted approach such as option (2) or (3) above since it wouldn't be needed assuming all calling conventions are in the runtime (e.g. System.Private.CoreLib) we don't have to worry about it being unloaded.

@jkotas
Copy link
Member

jkotas commented Jul 11, 2022

I have been thinking about this over the weekend:

  • The modopts and calling convention details are very similar in nature. We have prior art with managed C++ where modopts affect parameter calling convention (IsCopyConstructed modopt). We should either include both or none. It disqualifies option 2.
  • As @MichalStrehovsky pointed out, we have a more general problem with modopts. It affects managed C++ only currently. Chances are that it is going to affect C# too in future and we may need to introduce the SignatureType concept.
  • The calling convention details should be really only needed for tooling scenarios. If we have to add the SignatureType concept in future, it can be added to MetadataLoadContext only via extension methods, without complicating the runtime.
  • When one is not sure, rule of thumb is to lean towards simplicity.

So my vote would be to go with option (3). If we get requests to expose full modop details or the full calling conventions details, we would add the SignatureType concept, ideally to MetadataLoadContext only.

@steveharter
Copy link
Member Author

steveharter commented Jul 11, 2022

I have been thinking about this over the weekend:
...
So my vote would be to go with option (3). If we get requests to expose full modop details or the full calling conventions details, we would add the SignatureType concept, ideally to MetadataLoadContext only.

@jkotas thanks for this and @MichalStrehovsky thanks for bringing up the concerns with modifiers. We don't want to introduce something that causes issues elsewhere including managed C++.

I'll likely close this PR and re-schedule for V8 since we are locking down for V7 features.

@MichalStrehovsky
Copy link
Member

The reason why I was leaning towards 2 is that the problem with calling convention modopts is self-inflicted - because we wanted to prevent a structural breaking change to the file format, we decided to stash this additional metadata wherever possible. If we allowed ourselves to change the format, this would likely not be a custom modifier because it has a different role.

Going with option 3 will have a slight negative consequence that castability will work differently between calling conventions that can be represented the "old way" (in flags) vs calling conventions that are represented the "new way".

object arr1 = new delegate* unmanaged<int, int>[1];

// Throws
var arr2 = (delegate* unmanaged[Cdecl]<int, int>[])arr1;

// Works
var arr3 = (delegate* unmanaged[MemberFunction]<int, int>[])arr1;

But I can get behind option 3 too. It's how it works right now. It's a bit of a hole.

@steveharter
Copy link
Member Author

The reason why I was leaning towards 2 is that the problem with calling convention modopts is self-inflicted

For option 2 I assume we'd have to cherry-pick all of the mod opts we want to include. I thought it would be as simple as the CallConv* ones but Jan also mentioned IsCopyConstructed.

@jkotas
Copy link
Member

jkotas commented Jul 12, 2022

I thought it would be as simple as the CallConv* ones but Jan also mentioned IsCopyConstructed.

IsCopyConstructed is applied to individual arguments (you cannot do that in C# today).

@steveharter
Copy link
Member Author

Going with option 3 will have a slight negative consequence that castability will work differently between calling conventions that can be represented the "old way" (in flags) vs calling conventions that are represented the "new way".

Note that there is option 3a which proposes just managed vs. unmanaged and not the "CallConv" byte so it wouldn't have this inconsistency.

I suppose we could add a new option 3b would be to just not use the "CallConv" byte at all.

Either 3a or 3b should work in the runtime per discussion with @jkotas since the runtime doesn't really care about calling conventions except when invoking via calli and in that case it uses the signature (from the provided signature token) and not a function pointer type.

However, I now wonder if option 2 has an advantage for future APIs, such as for a new IL Emit calli overload where it could take an IntPtr (actual pointer) + System.Type (as a function pointer type) instead of IntPtr+ReturnType+ParamTypes+CallingConvention.

Note I did update the proposed API to include new "Signature" APIs that would allow all modifiers to be obtained that would be useful\compatible with either option 2 or 3*.

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Jul 13, 2022

Not including the calling convention sounds like a good option to me too. It feels cleaner than including only some of the calling conventions.

It also eliminates potential class of question marks - delegate* unmanaged<void> and delegate* unmanaged[Stdcall]<void> mean the same thing on Windows (because the default unmanaged calling convention on Windows is Stdcall) - are they separate types and if yes are they castable?

@jkotas
Copy link
Member

jkotas commented Jul 13, 2022

Note I did update the #69273 to include new "Signature" APIs that would allow all modifiers to be obtained that would be useful\compatible with either option 2 or 3*.

The Signature APIs need to be able to deconstruct the types. For example, if you have an array of const pointers, you need to be able to lookup the const modopt on the array element. The const modopt won't be on the array itself.

I think the needs to be something like:

namespace System
{
    public abstract class Type
    {
        // Throws InvalidOperationException if IsFunctionPointer is false.
+       public virtual Type GetFunctionPointerReturnType();

        // Throws InvalidOperationException if IsFunctionPointer is false.
+       public virtual Type[] GetFunctionPointerParameterTypes();

        // Note that IsPointer returns 'false' for function pointers.
        public virtual bool IsPointer { get; }

+       public virtual bool IsFunctionPointer { get; }
+       public virtual bool IsUnmanagedFunctionPointer { get; }

        public virtual Type[] GetOptionalCustomModifiers();

+       // GetRequiredCustomModifiers, GetOptionalCustomModifiers and GetCallingConventions work on signature types only.
+       // These APIs will throw InvalidOperationException on regular runtime types.
+       public virtual Type[] GetRequiredCustomModifiers();
+       public virtual Type[] GetOptionalCustomModifiers();

+        // This returns a subset of GetFunctionPointerReturnType().GetOptionalCustomModifiers() that start with the CallConv prefix
+        public virtual Type[] GetCallingConventions();
    }
}

// Extensions in the MetadataLoadContext assembly only; we could add directly to the appropriate types instead
// if we also want the runtime to implement these as well (pending feedback).

+   public static class MetadataLoadContextExtensions
+   {
+       public static Type GetSignatureType(this FieldInfo fieldInfo);
+       public static Type GetSignatureType(this PropertyInfo propertyInfo);

         // For MethodInfo, ContructorInfo and PropertyInfo with indexers
+       public static Type GetSignatureType(this ParameterInfo parameterInfo); 
+   }
}

@steveharter
Copy link
Member Author

Closing; we need to update the API and design based on feedback.

@steveharter
Copy link
Member Author

See design at dotnet/designs#282

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet new-api-needs-documentation
Projects
None yet
5 participants