Fix WinMD handling in System.Reflection.MetadataLoadContext #58344

andrew-boyarshin · 2021-08-30T08:30:39Z

Add a test WinMD file to prevent regressions. Had to be compiled with .NET Framework ILASM, since .NET ILASM silently ignores /MDV switch, which is necessary to produce WinMD that reproduces the crash (MetadataVersion is used to determine the existence of virtual AssemblyRefs).

Add a test to prevent regressions

src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/Type/TypeTests.cs

andrew-boyarshin · 2021-08-30T09:23:04Z

runtime (Libraries Build Linux x64 Debug) failure appears to be #55449.

The MetadataLoadContext should work on Browser, but the test requires better MetadataAssemblyResolver implementation

andrew-boyarshin · 2021-09-26T05:31:32Z

Can this make it into 7.0.0 milestone? It's a pretty simple fix for a bug that makes it impossible to properly process any Windows Runtime WinMD file using System.Reflection.MetadataLoadContext.

src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/Type/TypeTests.cs

...flection.MetadataLoadContext/src/System/Reflection/TypeLoading/General/Ecma/MetadataTable.cs

steveharter · 2021-10-07T22:07:31Z

...flection.MetadataLoadContext/src/System/Reflection/TypeLoading/General/Ecma/MetadataTable.cs

 {
 Debug.Assert(!handle.IsNil);
 Debug.Assert(factory != null);

- int index = handle.GetToken().GetTokenRowNumber() - 1;


Is this necessary (was there an issue converting from token to row number) or just simplification?

Yeah, it's at least half of the fix. System.Reflection handles WinMDs fine, because it knows about virtual AssemblyRefs. System.Reflection.MetadataLoadContext didn't. I dropped the wrong/incomplete GetTokenRowNumber impl from System.Reflection.MetadataLoadContext, and used the correct System.Reflection one.

System.Reflection handles WinMDs fine, because it knows about virtual AssemblyRefs

I assume you mean System.Reflection.Metadata, not System.Reflection via the reader option MetadataReaderOptions.ApplyWindowsRuntimeProjections and the resulting MetadataKind.WindowsMetadata enum value.

steveharter · 2021-10-07T22:12:11Z

...adataLoadContext/src/System/Reflection/TypeLoading/Modules/Ecma/EcmaModule.MetadataTables.cs

 {
- return new MetadataTable<T, EcmaModule>(Reader.GetTableRowCount(tableIndex));
+ int rowCount = tableIndex switch


style\nit: for a simple two-case switch I think an if statement is more appropriate.

steveharter · 2021-10-08T18:21:42Z

src/libraries/System.Reflection.MetadataLoadContext/tests/src/TestUtils/TestData.cs

@@ -3074,5 +3074,63 @@ internal static class TestData
 "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" +
 "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" +
 "AAAAAAAAAAAAAAAA");
+


Was there is an attempt in tests to use an actual .winmd file, as in the original issue?

This is a reduced copy that still exhibits the issue. The only snag was having to use ILASM from .NET Framework instead of .NET one, which silently generated .winmd with wrong metadata version, which is a crucial thing for reproducing the issue. The original .winmd files (Windows SDK UnionMetadata one, and the ones from Windows installation folder) are too big to put in a test.

steveharter · 2021-10-08T18:24:43Z

@andrew-boyarshin can you explain a bit your scenario? I assume some UWP work given the original Windows.Foundation.winmd file specified in the issue. Thanks.

andrew-boyarshin · 2021-10-09T03:01:09Z

@steveharter I need to parse and generate some code for Windows SDK. Let's say, for something like SharpGen. To that end, I know I need to load WinMD and iterate through every type and method. Back in .NET Framework days, I know I needed reflection-only assembly loading. Now, the recommended replacement is System.Reflection.MetadataLoadContext, which doesn't work at all when you start querying information beyond Name or FullName (information, that requires information from referenced assemblies, like base types). All the tests operate only on .NET WinMDs, not the WinRT ones, so this bug was not noticed before.

System.Runtime still fails to resolve on Browser. WinMD parsing on Browser is questionable enough. Though, with better MetadataAssemblyResolver impl it should still work. Making this new unit test be Browser-aware is out-of-scope for this fix. The issue to be fixed is not related to assembly loading at all.

AaronRobinsonMSFT · 2021-10-16T01:19:14Z

I've been told the WinMD format has enough differences with ECMA-335 that support should be considered a non-goal. Not saying we shouldn't accept this but does this actually represent full support?

/cc @jkotas @jkoritzinsky

andrew-boyarshin · 2021-10-16T01:32:45Z

@AaronRobinsonMSFT well, this fix allowed me to parse the entire UnionMetadata.winmd, that is, the entire Windows Runtime API surface. Even if there are cases that aren't supported fully, personally, I am satisfied with the current (including this PR, of course) level of support.

jkotas · 2021-10-16T13:17:18Z

How would you explain (e.g. in documentation) what is and what is not supported after these fixes?

The most complex part of the WinRT interop support are type projections. This does not seem to be handling the type projections at all. I doubt that you can actually use this as .NET type system without the type projections.

We have intentionally removed the built-in support for WinRT interop in #35318 . The WinRT interop is being developed independently on .NET runtime in https://github.com/microsoft/CsWinRT/ .

andrew-boyarshin · 2021-10-16T13:36:32Z

@jkotas this is independent of WinRT interop support removed in 5.0. This is merely a library that leverages System.Reflection to load .NET-like PE assemblies and builds the type system for reflection. System.Reflection should already support everything needed for that. In fact, not using the helpers from System.Reflection was the cause of the issue fixed in this PR.
System.Reflection.MetadataLoadContext doesn't need type projections, since it's not an interop solution, but an assembly inspection library that provides building blocks for something like ildasm. ildasm doesn't need type projections, because it should display what is actually in the assembly. WinMD format was deliberately chosen to be a preexisting format to leverage the existing tooling. Unfortunately, a few extensions to .NET PE format were made, such as virtual AssemblyRefs, which is not a problem for System.Reflection, but before this PR, was a problem for System.Reflection.MetadataLoadContext. This is not a feature PR, but a bugfix PR to enable users of the old .NET Framework reflection-only assembly loading contexts to migrate to modern .NET. You can build an interop solution (do AOT codegen based on the type system provided by System.Reflection.MetadataLoadContext), but it's not something System.Reflection.MetadataLoadContext provides out-of-box, nor should it provide that, in my opinion.

jkotas · 2021-10-16T14:20:43Z

System.Reflection.MetadataLoadContext doesn't need type projections, since it's not an interop solution, but an assembly inspection library that provides building blocks for something like ildasm.

The abstraction to use for building ildasm is System.Reflection.Metadata. It is fine to depend on System.Reflection.Metadata for parsing .winmd files.

It is not possible to build ildasm using System.Reflection.MetadataLoadContext. It is too high level abstraction. It assumes semantics of .NET runtime type system that is different from semantics of WinRT type system without projections.

andrew-boyarshin · 2021-10-16T16:16:50Z

@jkotas fine. I have presented a patch that allowed me to parse an entire Windows Runtime API surface with a high-level API. It also reduces code complexity by reusing existing code and dropping generic parameter that is always known to be EcmaModule. I have provided a reduced WinMD file that previously exhibited erroneous behavior. Accepting this change allows using the recommended ReflectionOnlyLoad (which worked perfectly well for WinMDs) replacement in a case where it previously failed. Currently it appears that .NET 5.0 just thrown out a valid scenario, that worked before very well. It's not the first time in .NET history too.

By the way, I'd be interested to read about the differences between .NET type system and WinRT type system, without taking projecting IVector`1 to IList`1 etc into account, of course.

jkotas · 2021-10-18T01:04:43Z

I have presented a patch that allowed me to parse an entire Windows Runtime API surface with a high-level API.

The point of this discussion is whether we are comfortable with supporting this scenario going forward. WinRT is developed independently on .NET these days. System.Reflection.MetadataLoadContext was not designed for reading .winmd files. There are scenarios today where System.Reflection.MetadataLoadContext won't work well for reading .winmd files, and this set is likely to grow over time.

It also reduces code complexity by reusing existing code and dropping generic parameter

This simplification is definitely welcomed. Thank you!

I'd be interested to read about the differences between .NET type system and WinRT type system

I am not aware of comprehensive up-to-date document that lists all differences. I can give you one example: Names in assembly references are irrelevant in WinRT type system. The name in .winmd assembly references maps to correct file name some of the time. It is fragile to depend on it. The WinRT type system specifies algorithm for locating .winmd files. The runtime built-in WinRT interop support had methods like AppDomain.TypeResolve to support the WinRT specific algorithm for locating .winmd files. None of this support is present in System.Reflection.MetadataLoadContext.

Also, I was wrong that System.Reflection.MetadataLoadContext does not apply type projections. It does apply the type projections since MetadataReaderOptions.Default = MetadataReaderOptions.ApplyWindowsRuntimeProjections. However, this is the frozen snapshot of the type projections that matches the runtime built-in WinRT interop support. The set of WinRT type projections was extended since then, and so this won't work well for more recent WinRT types such like the ones that come with WinUI 3.

steveharter · 2021-11-01T23:31:51Z

Attempt at summary:

Workaround is to use MetadataReader and apply WinMD semantics appropriately.
WinRT runtime interop removed in Support WinRT APIs in .NET 5 #35318, although this PR is about the raw metadata format not runtime. WinRT interop is being developed independently on .NET runtime in https://github.com/microsoft/CsWinRT/. Also see also see see https://docs.microsoft.com/en-us/dotnet/core/compatibility/interop/5.0/built-in-support-for-winrt-removed
MetadataReader has some support for windows metadata via MetadataReaderOptions.ApplyWindowsRuntimeProjections but those for earlier point-in-time interop scenarios. MLC uses that option.
There are other cases where WinMD files won't work including basic Type names including base types (those are supposed to map to .WinMD files) and with additional to the more recent WinUI 3. Due to this, even with this PR, the resulting reflection types may not match to the expected WinMD reflection types without properly supporting type projections.

buyaa-n · 2021-11-30T21:33:13Z

@andrew-boyarshin with the above reasons mentioned in the @steveharter's Attempt at summary we are hesitant to accept this fix, as we did not receive any response from you for a while added no recent activity label so that the PR could get closed automatically if there is no activity within a week

buyaa-n · 2021-12-08T01:47:26Z

Closing, somehow the automation is not working for this PR

Fix WinMD handling in System.Reflection.MetadataLoadContext

d0806d6

Add a test to prevent regressions

dotnet-issue-labeler bot added the area-System.Reflection label Aug 30, 2021

ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 30, 2021

ghost added this to Needs triage in Triage POD for Meta, Reflection, etc Aug 30, 2021

am11 reviewed Aug 30, 2021

View reviewed changes

src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/Type/TypeTests.cs Outdated Show resolved Hide resolved

Apply review suggestion

114f43d

Skip the new test on Browser platform

66463ea

The MetadataLoadContext should work on Browser, but the test requires better MetadataAssemblyResolver implementation

This was referenced Aug 30, 2021

Can_Run_App_With_StatiHost installer test failure in CI #42101

Closed

Feed unreliability affecting CI #55449

Closed

joperezr moved this from Needs triage to Buyaa's triage backlog in Triage POD for Meta, Reflection, etc Aug 31, 2021

buyaa-n requested a review from steveharter September 2, 2021 00:05

buyaa-n moved this from Buyaa's triage backlog to Future in Triage POD for Meta, Reflection, etc Sep 2, 2021

buyaa-n reviewed Oct 7, 2021

View reviewed changes

src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/Type/TypeTests.cs Outdated Show resolved Hide resolved

steveharter reviewed Oct 7, 2021

View reviewed changes

steveharter reviewed Oct 8, 2021

View reviewed changes

andrew-boyarshin added 2 commits October 9, 2021 18:42

Address review feedback

ccb94df

andrew-boyarshin requested review from steveharter and buyaa-n October 9, 2021 16:46

runfoapp bot mentioned this pull request Oct 11, 2021

System.Runtime.Serialization.Xml.Tests failed #59926

Closed

buyaa-n added the needs more info label Nov 3, 2021

jeffhandley assigned buyaa-n Nov 15, 2021

joperezr added the needs-maintainer-attention label Nov 29, 2021

eiriktsarpalis removed the needs-maintainer-attention label Nov 30, 2021

buyaa-n added the no-recent-activity label Nov 30, 2021

buyaa-n added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs more info labels Dec 2, 2021

buyaa-n closed this Dec 8, 2021

Triage POD for Meta, Reflection, etc automation moved this from Future to Done Dec 8, 2021

ghost locked as resolved and limited conversation to collaborators Jan 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix WinMD handling in System.Reflection.MetadataLoadContext #58344

Fix WinMD handling in System.Reflection.MetadataLoadContext #58344

andrew-boyarshin commented Aug 30, 2021

andrew-boyarshin commented Aug 30, 2021

andrew-boyarshin commented Sep 26, 2021

steveharter Oct 7, 2021

andrew-boyarshin Oct 8, 2021

steveharter Nov 1, 2021

steveharter Oct 7, 2021

steveharter Oct 8, 2021

andrew-boyarshin Oct 9, 2021

steveharter commented Oct 8, 2021

andrew-boyarshin commented Oct 9, 2021

AaronRobinsonMSFT commented Oct 16, 2021

andrew-boyarshin commented Oct 16, 2021

jkotas commented Oct 16, 2021

andrew-boyarshin commented Oct 16, 2021

jkotas commented Oct 16, 2021

andrew-boyarshin commented Oct 16, 2021

jkotas commented Oct 18, 2021

steveharter commented Nov 1, 2021

buyaa-n commented Nov 30, 2021

buyaa-n commented Dec 8, 2021

Fix WinMD handling in System.Reflection.MetadataLoadContext #58344

Fix WinMD handling in System.Reflection.MetadataLoadContext #58344

Conversation

andrew-boyarshin commented Aug 30, 2021

andrew-boyarshin commented Aug 30, 2021

andrew-boyarshin commented Sep 26, 2021

steveharter Oct 7, 2021

Choose a reason for hiding this comment

andrew-boyarshin Oct 8, 2021

Choose a reason for hiding this comment

steveharter Nov 1, 2021

Choose a reason for hiding this comment

steveharter Oct 7, 2021

Choose a reason for hiding this comment

steveharter Oct 8, 2021

Choose a reason for hiding this comment

andrew-boyarshin Oct 9, 2021

Choose a reason for hiding this comment

steveharter commented Oct 8, 2021

andrew-boyarshin commented Oct 9, 2021

AaronRobinsonMSFT commented Oct 16, 2021

andrew-boyarshin commented Oct 16, 2021

jkotas commented Oct 16, 2021

andrew-boyarshin commented Oct 16, 2021

jkotas commented Oct 16, 2021

andrew-boyarshin commented Oct 16, 2021

jkotas commented Oct 18, 2021

steveharter commented Nov 1, 2021

buyaa-n commented Nov 30, 2021

buyaa-n commented Dec 8, 2021