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 .mvid section to PE and remove dependency on MetadataReader #19133

Merged
merged 9 commits into from
May 2, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/compilers/CSharp/CommandLine.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
| ---- | ---- |
| **OUTPUT FILES** |
| `/out:`*file* | Specify output file name (default: base name of file with main class or first file)
| `/refout:`*file* | Specify the reference assembly's output file name
| `/target:exe` | Build a console executable (default) (Short form: `/t:exe`)
| `/target:winexe` | Build a Windows executable (Short form: `/t:winexe` )
| `/target:library` | Build a library (Short form: `/t:library`)
Expand Down Expand Up @@ -32,6 +33,7 @@
| `/debug`:{`full`|`pdbonly`|`portable`} | Specify debugging type (`full` is default, and enables attaching a debugger to a running program. `portable` is a cross-platform format)
| `/optimize`{`+`|`-`} | Enable optimizations (Short form: `/o`)
| `/deterministic` | Produce a deterministic assembly (including module version GUID and timestamp)
| `/refonly | Produce a reference assembly, instead of a full assembly, as the primary output
| **ERRORS AND WARNINGS**
| `/warnaserror`{`+`|`-`} | Report all warnings as errors
| `/warnaserror`{`+`|`-`}`:`*warn list* | Report specific warnings as errors
Expand Down
2 changes: 2 additions & 0 deletions docs/compilers/Visual Basic/CommandLine.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
| ---- | ---- |
| **OUTPUT FILE**
| `/out:`*file* | Specifies the output file name.
| `/refout:`*file* | Specify the reference assembly's output file name
| `/target:exe` | Create a console application (default). (Short form: `/t`)
| `/target:winexe` | Create a Windows application.
| `/target:library` | Create a library assembly.
Expand Down Expand Up @@ -34,6 +35,7 @@
| `/debug:portable` | Emit debugging information in the portable format.
| `/debug:pdbonly` | Emit PDB file only.
| `/deterministic` | Produce a deterministic assembly (including module version GUID and timestamp)
| `/refonly | Produce a reference assembly, instead of a full assembly, as the primary output
| **ERRORS AND WARNINGS**
| `/nowarn` | Disable all warnings.
| `/nowarn:`*number_list* | Disable a list of individual warnings.
Expand Down
10 changes: 6 additions & 4 deletions docs/features/refout.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,28 +32,31 @@ Two mutually exclusive command-line parameters will be added to `csc.exe` and `v
- `/refout`
- `/refonly`

The `/refout` parameter specifies a file path where the ref assembly should be output. This translates to `metadataPeStream` in the `Emit` API (see details below).
The `/refout` parameter specifies a file path where the ref assembly should be output. This translates to `metadataPeStream` in the `Emit` API (see details below). The filename for the ref assembly should generally match that of the primary assembly, but it can be in a different folder.

The `/refonly` parameter is a flag that indicates that a ref assembly should be output instead of an implementation assembly.
The `/refonly` parameter is not allowed together with the `/refout` parameter, as it doesn't make sense to have both the primary and secondary outputs be ref assemblies. Also, the `/refonly` parameter silently disables outputting PDBs, as ref assemblies cannot be executed.
The `/refonly` parameter translates to `EmitMetadataOnly` being `true`, and `IncludePrivateMembers` being `false` in the `Emit` API (see details below).
Neither `/refonly` nor `/refout` are permitted with `/target:module` or `/addmodule` options.

When the compiler produces documentation, the contents produced will match the APIs that go into the primary output. In other words, the documentation will be filtered down when using the `/refonly` parameter.

The compilation from the command-line will either produce both assemblies (implementation and ref) or neither. There is no "partial success" scenario.


### CscTask/CoreCompile
The `CoreCompile` target will support a new output, called `IntermediateRefAssembly`, which parallels the existing `IntermediateAssembly`.
The `Csc` task will support a new output, called `OutputRefAssembly`, which parallels the existing `OutputAssembly`.
Both of those basically map to the `/refout` command-line parameter.

An additional task, called `CopyRefAssembly`, will be provided along with the existing `Csc` task. It takes a `SourcePath` and a `DestinationPath` and generally copies the file from the source over to the destination. But if it can determine that the contents of those two files match, then the destination file is left untouched.
An additional task, called `CopyRefAssembly`, will be provided along with the existing `Csc` task. It takes a `SourcePath` and a `DestinationPath` and generally copies the file from the source over to the destination. But if it can determine that the contents of those two files match (by comparing their MVIDs, see details below), then the destination file is left untouched.

### CodeAnalysis APIs
It is already possible to produce metadata-only assemblies by using `EmitOptions.EmitMetadataOnly`, which is used in IDE scenarios with cross-language dependencies.
The compiler will be updated to honour the `EmitOptions.IncludePrivateMembers` flag as well. When combined with `EmitMetadataOnly` or a `metadataPeStream` in `Emit`, a ref assembly will be produced.
The diagnostic check for emitting methods lacking a body (`void M();`) will be moved from declaration diagnostics to regular diagnostics, so that code will successfully emit with `EmitMetadataOnly`.
The diagnostic check for emitting methods lacking a body (`void M();`) will be filtered from declaration diagnostics, so that code will successfully emit with `EmitMetadataOnly`.
Later on, the `EmitOptions.TolerateErrors` flag will allow emitting error types as well.
`Emit` is also modified to produce a new PE section called ".mvid" containing a copy of the MVID, when producing ref assemblies. This makes it easy for `CopyRefAssembly` to extract and compare MVIDs from ref assemblies.

Going back to the 4 driving scenarios:
1. For a regular compilation, `EmitMetadataOnly` is left to `false` and no `metadataPeStream` is passed into `Emit`.
Expand All @@ -70,7 +73,6 @@ As mentioned above, there may be further refinements after C# 7.1:
- should explicit method implementations be included in ref assemblies?
- Non-public attributes on public APIs (emit attribute based on accessibility rule)
- ref assemblies and NoPia
- `/refout` and `/addmodule`, should we disallow this combination?

## Related issues
- Produce ref assemblies from command-line and msbuild (https://github.com/dotnet/roslyn/issues/2184)
Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Test/Emit/CSharpCompilerEmitTest.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Debug|AnyCPU'" />
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Release|AnyCPU'" />
<ItemGroup>
<Compile Include="..\..\..\Core\MSBuildTask\MvidReader.cs">
<Link>Emit\MvidReader.cs</Link>
</Compile>
<Compile Include="Attributes\AttributeTests.cs" />
<Compile Include="Attributes\AttributeTests_Assembly.cs" />
<Compile Include="Attributes\AttributeTests_CallerInfoAttributes.cs" />
Expand Down
46 changes: 45 additions & 1 deletion src/Compilers/CSharp/Test/Emit/Emit/CompilationEmitTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ public void Main()
Diagnostic(ErrorCode.ERR_AssgLvalueExpected, "x"));
}


[Fact]
public void CompilationEmitWithQuotedMainType()
{
Expand Down Expand Up @@ -262,9 +261,11 @@ internal static void Main()

VerifyEntryPoint(output, expectZero: false);
VerifyMethods(output, new[] { "void C.Main()", "C..ctor()" });
VerifyMvid(output, hasMvidSection: false);

VerifyEntryPoint(metadataOutput, expectZero: true);
VerifyMethods(metadataOutput, new[] { "C..ctor()" });
VerifyMvid(metadataOutput, hasMvidSection: true);
}

void VerifyEntryPoint(MemoryStream stream, bool expectZero)
Expand All @@ -275,6 +276,33 @@ void VerifyEntryPoint(MemoryStream stream, bool expectZero)
}
}

/// <summary>
/// Extract the MVID using two different methods (PEReader and MvidReader) and compare them.
/// We only expect an .mvid section in ref assemblies.
/// </summary>
private void VerifyMvid(MemoryStream stream, bool hasMvidSection)
{
stream.Position = 0;
using (var reader = new PEReader(stream))
{
var metadataReader = reader.GetMetadataReader();
Guid mvidFromModuleDefinition = metadataReader.GetGuid(metadataReader.GetModuleDefinition().Mvid);

stream.Position = 0;
var mvidFromMvidReader = BuildTasks.MvidReader.ReadAssemblyMvidOrEmpty(stream);

Assert.NotEqual(Guid.Empty, mvidFromModuleDefinition);
if (hasMvidSection)
{
Assert.Equal(mvidFromModuleDefinition, mvidFromMvidReader);
Copy link
Member

@cston cston May 1, 2017

Choose a reason for hiding this comment

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

Are we checking the mvids are non-default? #Resolved

}
else
{
Assert.Equal(Guid.Empty, mvidFromMvidReader);
}
}
}

[Fact]
public void EmitRefAssembly_PrivatePropertySetter()
{
Expand All @@ -296,6 +324,8 @@ public class C
VerifyMethods(output, new[] { "System.Int32 C.<PrivateSetter>k__BackingField", "System.Int32 C.PrivateSetter.get", "void C.PrivateSetter.set",
"C..ctor()", "System.Int32 C.PrivateSetter { get; private set; }" });
VerifyMethods(metadataOutput, new[] { "System.Int32 C.PrivateSetter.get", "C..ctor()", "System.Int32 C.PrivateSetter { get; }" });
VerifyMvid(output, hasMvidSection: false);
VerifyMvid(metadataOutput, hasMvidSection: true);
}
}

Expand Down Expand Up @@ -559,6 +589,20 @@ private void CompareAssemblies(string sourceTemplate, string change1, string cha
{
AssertEx.NotEqual(image1, image2, message: $"Expecting difference for includePrivateMembers={includePrivateMembers} case, but they matched.");
}

var mvid1 = BuildTasks.MvidReader.ReadAssemblyMvidOrEmpty(new MemoryStream(image1.DangerousGetUnderlyingArray()));
var mvid2 = BuildTasks.MvidReader.ReadAssemblyMvidOrEmpty(new MemoryStream(image2.DangerousGetUnderlyingArray()));

if (!includePrivateMembers)
{
Assert.NotEqual(Guid.Empty, mvid1);
Assert.Equal(expectMatch, mvid1 == mvid2);
}
else
{
Assert.Equal(Guid.Empty, mvid1);
Assert.Equal(Guid.Empty, mvid2);
}
}

[Fact]
Expand Down
25 changes: 23 additions & 2 deletions src/Compilers/CSharp/Test/Emit/Emit/DeterministicTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,19 @@ namespace Microsoft.CodeAnalysis.CSharp.UnitTests.Emit
public class DeterministicTests : EmitMetadataTestBase
{
private Guid CompiledGuid(string source, string assemblyName, bool debug)
{
return CompiledGuid(source, assemblyName, options: debug ? TestOptions.DebugExe : TestOptions.ReleaseExe);
}

private Guid CompiledGuid(string source, string assemblyName, CSharpCompilationOptions options, EmitOptions emitOptions = null)
{
var compilation = CreateCompilation(source,
assemblyName: assemblyName,
references: new[] { MscorlibRef },
options: (debug ? TestOptions.DebugExe : TestOptions.ReleaseExe).WithDeterministic(true));
options: options.WithDeterministic(true));

Guid result = default(Guid);
base.CompileAndVerify(compilation, validator: a =>
base.CompileAndVerify(compilation, emitOptions: emitOptions, validator: a =>
{
var module = a.Modules[0];
result = module.GetModuleVersionIdOrThrow();
Expand Down Expand Up @@ -104,6 +109,22 @@ public static void Main(string[] args) {}
Assert.NotEqual(mvid3, mvid7);
}

[Fact]
public void RefAssembly()
{
var source =
@"class Program
{
public static void Main(string[] args) {}
CHANGE
}";
var emitRefAssembly = EmitOptions.Default.WithEmitMetadataOnly(true).WithIncludePrivateMembers(false);

var mvid1 = CompiledGuid(source.Replace("CHANGE", ""), "X1", TestOptions.DebugDll, emitRefAssembly);
var mvid2 = CompiledGuid(source.Replace("CHANGE", "private void M() { }"), "X1", TestOptions.DebugDll, emitRefAssembly);
Assert.Equal(mvid1, mvid2);
}

const string CompareAllBytesEmitted_Source = @"
using System;
using System.Linq;
Expand Down
6 changes: 1 addition & 5 deletions src/Compilers/Core/MSBuildTask/CopyRefAssembly.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@

using System;
using System.IO;
using System.Reflection.Metadata;
using System.Reflection.PortableExecutable;
using Microsoft.Build.Framework;
using Microsoft.Build.Utilities;

Expand Down Expand Up @@ -85,10 +83,8 @@ private bool Copy()
private Guid ExtractMvid(string path)
{
using (FileStream source = File.OpenRead(path))
using (var reader = new PEReader(source))
{
var metadataReader = reader.GetMetadataReader();
return metadataReader.GetGuid(metadataReader.GetModuleDefinition().Mvid);
return MvidReader.ReadAssemblyMvidOrEmpty(source);
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/Core/MSBuildTask/MSBuildTask.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
</Compile>
<Compile Include="AssemblyResolution.cs" />
<Compile Include="CanonicalError.cs" />
<Compile Include="MvidReader.cs" />
<Compile Include="CopyRefAssembly.cs" />
<Compile Include="ValidateBootstrap.cs" />
<Compile Include="CommandLineBuilderExtension.cs" />
Expand Down
119 changes: 119 additions & 0 deletions src/Compilers/Core/MSBuildTask/MvidReader.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Diagnostics;
using System.IO;

namespace Microsoft.CodeAnalysis.BuildTasks
{
public static class MvidReader
{
public static Guid ReadAssemblyMvidOrEmpty(Stream stream)
{
return ReadAssemblyMvidOrEmpty(new BinaryReader(stream));
}

private static Guid ReadAssemblyMvidOrEmpty(BinaryReader reader)
{
Guid empty = Guid.Empty;

// DOS Header (64), DOS Stub (64), PE Signature (4), COFF Header (20), PE Header (224), 1x Section Header (40)
if (reader.BaseStream.Length < 64 + 64 + 4 + 20 + 224 + 40)
{
return empty;
}

// DOS Header: PE (2)
if (reader.ReadUInt16() != 0x5a4d)
{
return empty;
}

// DOS Header: Start (58)
Skip(58, reader);

// DOS Header: Address of PE Signature
MoveTo(reader.ReadUInt32(), reader);

// PE Signature ('P' 'E' null null)
if (reader.ReadUInt32() != 0x00004550)
Copy link
Contributor

@AlekseyTs AlekseyTs May 2, 2017

Choose a reason for hiding this comment

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

if (reader.ReadUInt32() != 0x00004550) [](start = 12, length = 38)

This looks like an attempt to read at an unknown location, which can throw if we are near the end of the stream. #Closed

Copy link
Member Author

@jcouv jcouv May 2, 2017

Choose a reason for hiding this comment

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

Already checked above. #Resolved

{
return empty;
}

// COFF Header: Machine (2)
Skip(2, reader);

// COFF Header: NumberOfSections (2)
ushort sections = reader.ReadUInt16();

// COFF Header: TimeDateStamp (4), PointerToSymbolTable (4), NumberOfSymbols (4)
Skip(12, reader);

// COFF Header: OptionalHeaderSize (2)
ushort optionalHeaderSize = reader.ReadUInt16();

// COFF Header: Characteristics (2)
Skip(2, reader);

// Optional header
Skip(optionalHeaderSize, reader);

// Section headers
return FindMvidInSections(sections, reader);
}

private static Guid FindMvidInSections(ushort count, BinaryReader reader)
Copy link
Member

@cston cston May 1, 2017

Choose a reason for hiding this comment

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

count is not used. Should we check there is at least one section? #Resolved

Copy link
Member Author

@jcouv jcouv May 1, 2017

Choose a reason for hiding this comment

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

Good point, thanks #Resolved

{
// .mvid section must be first, if it's there
// Section: Name (8)
byte[] name = reader.ReadBytes(8);
if (name.Length == 8 && name[0] == '.' &&
name[1] == 'm' && name[2] == 'v' && name[3] == 'i' && name[4] == 'd' && name[5] == '\0')
{
// Section: VirtualSize (4)
uint virtualSize = reader.ReadUInt32();

// Section: VirtualAddress (4), SizeOfRawData (4)
Copy link
Member Author

@jcouv jcouv Apr 30, 2017

Choose a reason for hiding this comment

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

@tmat One thing I noticed is that SizeOfRawData is 500. I wonder if there is a way to reduce that overhead by tweaking FileAlignment? After all, this section only contains 16 bytes. #Resolved

Copy link
Member

@tmat tmat May 1, 2017

Choose a reason for hiding this comment

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

Sadly, the PE spec requires the value of FileAlignment to be

... a power of 2 between 512 and 64K, inclusive. #Resolved

Skip(8, reader);

// Section: PointerToRawData (4)
uint pointerToRawData = reader.ReadUInt32();

// The .mvid section only stores a Guid
if (virtualSize != 16)
{
Debug.Assert(false);
return Guid.Empty;
}

if (MoveTo(pointerToRawData, reader))
{
Copy link
Member

@tmat tmat May 1, 2017

Choose a reason for hiding this comment

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

Nit: no need for else since the if-statement returns #Resolved

byte[] guidBytes = new byte[16];
if (reader.BaseStream.Read(guidBytes, 0, 16) == 16)
{
return new Guid(guidBytes);
}
Copy link
Member

Choose a reason for hiding this comment

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

break;?

}
}

return Guid.Empty;
}

private static void Skip(int bytes, BinaryReader reader)
{
reader.BaseStream.Seek(bytes, SeekOrigin.Current);
}

private static bool MoveTo(uint position, BinaryReader reader)
{
if (reader.BaseStream.Length < position)
{
return false;
}

reader.BaseStream.Seek(position, SeekOrigin.Begin);
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

return true; [](start = 12, length = 12)

We probably shouldn't assume that Seek succeeds. Consider return reader.BaseStream.Seek(position, SeekOrigin.Begin) == position; instead.

}
Copy link
Member

Choose a reason for hiding this comment

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

These methods can be private.

}
}
1 change: 0 additions & 1 deletion src/Compilers/Core/MSBuildTask/project.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
"System.IO.Pipes": "4.3.0",
"System.Linq": "4.3.0",
"System.Reflection": "4.3.0",
"System.Reflection.Metadata": "1.4.2",
"System.Security.AccessControl": "4.3.0",
"System.Security.Cryptography.Algorithms": "4.3.0",
"System.Security.Principal.Windows": "4.3.0",
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/Core/Portable/CodeAnalysis.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@
<Compile Include="Emit\AsyncMoveNextBodyDebugInfo.cs" />
<Compile Include="Emit\IteratorMoveNextBodyDebugInfo.cs" />
<Compile Include="Emit\StateMachineMoveNextDebugInfo.cs" />
<Compile Include="PEWriter\ExtendedPEBuilder.cs" />
<Compile Include="Serialization\IObjectWritable.cs" />
<Compile Include="Serialization\ObjectBinder.cs" />
<Compile Include="Serialization\ObjectBinderSnapshot.cs" />
Expand Down
Loading