[cdac] Synthesize a valid ECMA-335 image for read/write metadata instead of providing a separate metadata reader

[jkoritzinsky]

Instead of cDAC having its own metadata reader API, instead synthesize a valid ECMA metadata image from the separate target spans.

I looked into the alternative of having MetadataReader support an API that takes ReadOnlySequence<byte>, but it looked like the cost would be quite high with the existing API surface and the expected allocation and performance requirements of MetadataReader. This looked significantly cheaper in comparison.

[dotnet-policy-service]

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

[jkotas]

What's the perf impact on cDac instance initialization? How much copying does this do in a typical case? Ideally, there would be no data copying for regular on-disk assemblies - I am not able to tell whether it is the case by just looking at the code.

Otherwise, I like the idea.

[jkoritzinsky]

This doesn't do any more copying than the previous implementation.

The standard case reads once into an array (like the previous case) and MetadataReaderProvider pins the provided ImmutableArray (which we get though ImmutableCollectionsMarshal).

In the case where we assemble a metadata image, we may end up allocating a little more due to how BlobBuilder allocates blobs, but we still only read one copy of the target spans into memory.

[lambdageek]

Two thoughts:

1. Should this be a contract, rather than a utility. Not every runtime has readily available Ecma metadata to extract (for example nativeaot, or mono in the case of reflection emit images)
2. How does this handle CorElementType.Internal? See David's GetMethodDescName PR where it had to do some special work for Internal

[jkotas]

How does this handle CorElementType.Internal?

This cannot show up in ECMA-335 metadata blob. It can only show up in signatures of items that are not represented in ECMA-335 metadata: DynamicMethodDescs, generic dictionary signatures, etc.

[jkoritzinsky]

Should this be a contract, rather than a utility. Not every runtime has readily available Ecma metadata to extract (for example nativeaot, or mono in the case of reflection emit images)

This utility builds upon the existing Loader contract, which already has a mechanism to represent "no metadata available". We could consolidate this into the Loader contract and change it as follows if you'd prefer:

- TargetPointer GetMetadataAddress(ModuleHandle handle, out ulong size);
- AvailableMetadataType GetAvailableMetadataType(ModuleHandle handle);
- TargetPointer GetReadWriteSavedMetadataAddress(ModuleHandle handle, out ulong size);
- TargetEcmaMetadata GetReadWriteMetadata(ModuleHandle handle);
+ System.Reflection.Metadata.MetadataReader? GetMetadata(ModuleHandle handle); // Provide the implementation of the Metadata utility and all of the above methods.

I don't know what our policy is on what .NET types we want to use in our contracts. Are we comfortable using SRM types?

Additionally, we could separate this out into a separate "EcmaMetadata" contract. Then support for retrieving any ECMA metadata could be indicated by support of the contract (ie NativeAOT wouldn't support the contract), and conditional support indicated by returning null.

[lambdageek]

Additionally, we could separate this out into a separate "EcmaMetadata" contract. Then support for retrieving any ECMA metadata could be indicated by support of the contract (ie NativeAOT wouldn't support the contract), and conditional support indicated by returning null.

I think this is what I had in mind, yes

[davidwrighton]

I actually like the idea of pushing this into a contract. For something like NativeAOT, we'll never have the metadata in the actual memory space of the application, but the equivalent would be to work like how .NET Native works, where the metadata is embedded in the symbols data of the binary, and I have this vague idea that we could abstract that difference by having the contract read the symbol data.

[jkoritzinsky]

I'll split the contract then as well.

To avoid having to immediately version away a majority of the Loader contract APIs, I'll try to get this into .NET 9. If this PR doesn't get in, I can make a smaller change (that uses the hand-written ECMA reader but implements the new contract surface) for .NET 9.

Sound good to everyone?

[jkoritzinsky]

/azp run runtime, runtime-dev-innerloop

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[jkoritzinsky]

I've fixed the merge conflicts here. Can I get a review pass?

[davidwrighton]

LGTM

[elinor-fung]

Sorry, didn't think our changes would conflict - it was just Loader.md.