Use reflection metadata to represent methods/fields in native layout #113413
Conversation
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR removes obsolete native layout metadata support and updates metadata emission for reflection to use a more compact format without custom modifiers. Key changes include replacing string‐based method/field identifiers with handle‑based equivalents, updating equality/hash code computations, and adjusting various helper APIs throughout the TypeLoader and CoreLib codebases.
Reviewed Changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| RuntimeMethodHandleInfo.cs | Replaces Name and RuntimeSignature properties with MetadataReader and MethodHandle, and updates equality/hash code logic. |
| TypeLoaderEnvironment.LdTokenResultLookup.cs | Adjusts field handle retrievals and key comparisons to use new handle types instead of string names. |
| TypeLoaderEnvironment.Metadata.cs, GVMResolution.cs, ConstructedGenericMethodsLookup.cs | Updates method signature handling to use handle conversions and GetName() methods instead of raw string tokens. |
| Other files (NativeLayoutInfoLoadContext.cs, GenericDictionaryCell.cs, etc.) | Propagate handle‑based metadata lookups and update logging & assertions accordingly. |
| @@ -30,24 +38,17 @@ public override bool Equals(object? compare) | |||
| if (other == null) | |||
| return false; | |||
|
|
|||
| if (Name != other.Name) | |||
| if (GetName() != other.GetName()) | |||
There was a problem hiding this comment.
Repeated calls to GetName() in the equality check may incur unnecessary performance overhead if this method is computationally expensive. Consider caching the result of GetName() in a local variable for comparison.
Copilot uses AI. Check for mistakes.
src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeBuilder.cs
Show resolved
Hide resolved
I assume reflection can return MethodInfos for these. Is that right? Can we filter these out, so that reflection won't return them at all? Would it be an option to not attach them to the owning type in the metadata, so that they are unreachable via regular metadata traversal? |
...eaot/System.Private.CoreLib/src/Internal/Runtime/CompilerServices/RuntimeMethodHandleInfo.cs
Outdated
Show resolved
Hide resolved
src/coreclr/nativeaot/System.Private.CoreLib/src/System/RuntimeFieldHandle.cs
Outdated
Show resolved
Hide resolved
src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeBuilder.cs
Show resolved
Hide resolved
Yep, it can, this case should be no different from not having the We've not made an effort to hide "things we structurally need in the metadata but weren't seen as reflected on by the user" in the past. We could do more if we wanted to, but will it make a difference? There should be no case where someone can get to this method without seeing a trimming warning. The "I found the method, but parameter names are missing" case already happens with ILLink today. It doesn't go a try stripping custom attributes because the underlying runtime could have some that are meaningful to it, but that's the only reason why we don't drop the entire |
This is a good point. If this behaves just like it behaves with IL linker, then it sounds fine me. I am really only worried about the failure modes that this can trigger. We do not want to end up with hard to diagnose crash deep in the reflection implementation when somebody ignores trim warnings, manages to get one of these partial MethodInfos and calls a method on it. |
From the implementation perspective, this returns the same ParameterInfo class that we use when a return parameter doesn't have metadata attached. I've added a test to cover all the interesting scenarios I could think of. |
.../System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeLoaderEnvironment.Metadata.cs
Outdated
Show resolved
Hide resolved
| @@ -47,7 +47,7 @@ public static MethodInfo GetDelegateMethodInfo(Delegate del) | |||
| callTryGetMethod = false; | |||
| methodHandle = QMethodDefinition.FromObjectAndInt(resolver->Reader, resolver->Handle); | |||
|
|
|||
| if (!TypeLoaderEnvironment.Instance.TryGetRuntimeMethodHandleComponents(resolver->GVMMethodHandle, out _, out _, out genericMethodTypeArgumentHandles)) | |||
| if (!TypeLoaderEnvironment.Instance.TryGetRuntimeMethodHandleComponents(resolver->GVMMethodHandle, out _, out QMethodDefinition dummy, out genericMethodTypeArgumentHandles)) | |||
There was a problem hiding this comment.
Number of these TryGet... methods do not perform any lookups and cannot possibly return false anymore. We may want to change them to Get... methods that just return the value.
(This can be follow up cleanup.)
|
/ba-g canceled build in an unrelated leg |
Native layout is a parallel form of metadata that was introduced in .NET Native mostly because in the structuring of that compiler, the decisions about reflection metadata and decisions about native layout happened in different components written in different languages (reflection metadata in C# during IL2IL transforms, native layout in C++ in NUTC).
Native layout has its own representation of methods and fields. There is a bunch of code to read and reconcile those two formats at runtime. This then introduces problems such as in #91381 where updating one format necessitates updating the other one.
Instead of adding more code to native layout generation/reading/reconciliation to account for custom modifiers, I'm just deleting ~2000 lines of code responsible for native layout metadata from the product instead.
One thing I needed to adjust was that native layout metadata is more compact than reflection metadata - so I added a new way of emitting reflection metadata for methods where the generated method won't have any custom attributes or parameter names. This hobbled method is not reachable with trim safe code. If someone gets a hold of it, more things will be broken than just the ability to invoke it.
Draft because I want to run all the testing on this and then write a test so that I can resolve #91381 too.
Cc @dotnet/ilc-contrib