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

Metadata writer refactoring (4) #9699

Closed
wants to merge 1 commit into from
Closed

Conversation

tmat
Copy link
Member

@tmat tmat commented Mar 11, 2016

[Equivalent to https://github.com//pull/9459, with all commits squashed into a single one for easier review.]

Refactors PeWriter into components independent of Roslyn, so that it can later be moved to System.Reflection.Metadata in CoreFX.

The goal is to provide PE/COFF builder and ECMA-335 metadata builder that can be used broadly to write managed and native PE files. The goal is not to immediately provide the full support for writing all existing sections of the PE/COFF format but rather to enable composing PE/COFF files from section blobs and to provide builders for sections commonly appearing in managed PE files.

Includes implementation of a set of types, blob encoders that can be used for serialization of various blob structures found in the ECMA-335 metadata (especially signatures). The introduced abstraction provides type-system enforced structure to the blobs with minimal runtime overhead.

More work will follow to polish the public surface of the builders and serializers.

Tracked by work item: #6904

@tmat
Copy link
Member Author

tmat commented Mar 11, 2016

@dotnet/roslyn-compiler Please review. Feel free to skip all files in src/Compilers/Core/Portable/System directory. They will be moved to CoreFX repo (System.Reflection.Metadata) and reviewed there. You can only focus on code that remains in Roslyn. The code in src/Compilers/Core/Portable/System directory will become public API of the SRM. This API is not finished yet, there will be more changes following this one. For example, argument checking, exception messages, the exact shape of the API, etc.

The change was tested thoroughly to avoid functional and perf regressions. Both CoreFX and Roslyn repos were built with a compiler containing these changes and the results were bit-for-bit equal to the corresponding binaries built by a compiler without this change (in deterministic mode).


if (signature == 0 || signature == 1)
{
nameFlags = (System.Reflection.AssemblyNameFlags)value;
nameFlags = (AssemblyFlags)(AssemblyNameFlags)value;
Copy link
Member

Choose a reason for hiding this comment

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

I don't see an explicit conversion operator for these types -- is there a reason why the two conversions are necessary instead of one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Value is object, so the first cast is unbox, which needs to match the type it was boxed with, and the second is cast between enums.

@tmat tmat force-pushed the MetadataWriter-3S branch from 5679441 to 49d09f8 Compare March 17, 2016 20:40
@tmat
Copy link
Member Author

tmat commented Mar 21, 2016

@dotnet/roslyn-compiler Please review. Mac/Linux failures look like an issue in CoreCLR. Investigating.

@tmat
Copy link
Member Author

tmat commented Mar 21, 2016

The failures on CoreCLR are caused by missing HashAlgorithm.TransformBlock. The fix is to move to netstandard1.3.

@tmat
Copy link
Member Author

tmat commented Mar 30, 2016

@dotnet/roslyn-compiler Ping.


if (configureToExecuteInAppContainer)
{
result |= DllCharacteristics.AppContainer;
Copy link
Member

Choose a reason for hiding this comment

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

Is WindowsRuntime referred to as AppContainer in some other context? How did you choose this name?

@AlekseyTs
Copy link
Contributor

LGTM to the degree GitHub was able to show the diff.

@tmat
Copy link
Member Author

tmat commented Apr 1, 2016

retest vsi please

1 similar comment
@tmat
Copy link
Member Author

tmat commented Apr 1, 2016

retest vsi please

@tmat tmat closed this Apr 1, 2016
@tmat tmat reopened this Apr 1, 2016
@tmat tmat closed this Apr 1, 2016
@tmat tmat reopened this Apr 1, 2016
@jaredpar
Copy link
Member

jaredpar commented Apr 1, 2016

test determinism please

@tmat tmat closed this Apr 1, 2016
@tmat tmat reopened this Apr 1, 2016
@tmat tmat force-pushed the MetadataWriter-3S branch from bf8dc22 to e12fb57 Compare April 25, 2016 00:03
@tmat tmat closed this Apr 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants