From ec9b2d4e482e6ca87374730a97a298ad00999c24 Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Mon, 1 May 2017 15:17:46 -0700 Subject: [PATCH] Address PR feedback from Chuck and Aleksey --- docs/features/refout.md | 5 +- .../Test/Emit/Emit/CompilationEmitTests.cs | 19 ++- src/Compilers/Core/MSBuildTask/MvidReader.cs | 108 ++++++++---------- 3 files changed, 69 insertions(+), 63 deletions(-) diff --git a/docs/features/refout.md b/docs/features/refout.md index 0da4d809079a1..5dd6c37d5c6f6 100644 --- a/docs/features/refout.md +++ b/docs/features/refout.md @@ -37,11 +37,13 @@ The `/refout` parameter specifies a file path where the ref assembly should be o 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`. @@ -68,10 +70,9 @@ As mentioned above, there may be further refinements after C# 7.1: - produce ref assemblies even when there are errors outside method bodies (emitting error types when `EmitOptions.TolerateErrors` is set) ## Open questions -- should explicit method implementations be included in ref assemblies? (answer: no. The interfaces that are declared as implemented are what matter to consuming compilations) +- 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? (answer: yes. This will not be supported in C# 7.1) ## Related issues - Produce ref assemblies from command-line and msbuild (https://github.com/dotnet/roslyn/issues/2184) diff --git a/src/Compilers/CSharp/Test/Emit/Emit/CompilationEmitTests.cs b/src/Compilers/CSharp/Test/Emit/Emit/CompilationEmitTests.cs index d2fae1c7f34d6..1c7da18965727 100644 --- a/src/Compilers/CSharp/Test/Emit/Emit/CompilationEmitTests.cs +++ b/src/Compilers/CSharp/Test/Emit/Emit/CompilationEmitTests.cs @@ -282,23 +282,22 @@ void VerifyEntryPoint(MemoryStream stream, bool expectZero) /// private void VerifyMvid(MemoryStream stream, bool hasMvidSection) { - Guid mvidFromModuleDefinition; stream.Position = 0; using (var reader = new PEReader(stream)) { var metadataReader = reader.GetMetadataReader(); - mvidFromModuleDefinition = metadataReader.GetGuid(metadataReader.GetModuleDefinition().Mvid); + 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); } else { - Assert.NotEqual(Guid.Empty, mvidFromModuleDefinition); Assert.Equal(Guid.Empty, mvidFromMvidReader); } } @@ -590,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] diff --git a/src/Compilers/Core/MSBuildTask/MvidReader.cs b/src/Compilers/Core/MSBuildTask/MvidReader.cs index f075346ffd5eb..7ceb3b43aedc7 100755 --- a/src/Compilers/Core/MSBuildTask/MvidReader.cs +++ b/src/Compilers/Core/MSBuildTask/MvidReader.cs @@ -3,125 +3,117 @@ using System; using System.Diagnostics; using System.IO; -using System.Text; namespace Microsoft.CodeAnalysis.BuildTasks { - public sealed class MvidReader : BinaryReader + public static class MvidReader { - public MvidReader(Stream stream) : base(stream) - { - } - public static Guid ReadAssemblyMvidOrEmpty(Stream stream) { - try - { - var mvidReader = new MvidReader(stream); - return mvidReader.TryFindMvid(); - } - catch (EndOfStreamException) - { - } - - return Guid.Empty; + return ReadAssemblyMvidOrEmpty(new BinaryReader(stream)); } - public Guid TryFindMvid() + 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) - if (BaseStream.Length < 64 + 64 + 4 + 20 + 224) + // 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 (ReadUInt16() != 0x5a4d) + if (reader.ReadUInt16() != 0x5a4d) { return empty; } // DOS Header: Start (58) - Skip(58); + Skip(58, reader); // DOS Header: Address of PE Signature - MoveTo(ReadUInt32()); + MoveTo(reader.ReadUInt32(), reader); // PE Signature ('P' 'E' null null) - if (ReadUInt32() != 0x00004550) + if (reader.ReadUInt32() != 0x00004550) { return empty; } // COFF Header: Machine (2) - Skip(2); + Skip(2, reader); // COFF Header: NumberOfSections (2) - ushort sections = ReadUInt16(); + ushort sections = reader.ReadUInt16(); // COFF Header: TimeDateStamp (4), PointerToSymbolTable (4), NumberOfSymbols (4) - Skip(12); + Skip(12, reader); // COFF Header: OptionalHeaderSize (2) - ushort optionalHeaderSize = ReadUInt16(); + ushort optionalHeaderSize = reader.ReadUInt16(); // COFF Header: Characteristics (2) - Skip(2); + Skip(2, reader); // Optional header - Skip(optionalHeaderSize); + Skip(optionalHeaderSize, reader); // Section headers - return FindMvidInSections(sections); + return FindMvidInSections(sections, reader); } - private Guid FindMvidInSections(ushort count) + private static Guid FindMvidInSections(ushort count, BinaryReader reader) { - for (int i = 0; i < count; i++) + // .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: Name (8) - byte[] name = 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 = ReadUInt32(); + // Section: VirtualSize (4) + uint virtualSize = reader.ReadUInt32(); - // Section: VirtualAddress (4), SizeOfRawData (4) - Skip(8); + // Section: VirtualAddress (4), SizeOfRawData (4) + Skip(8, reader); - // Section: PointerToRawData (4) - uint pointerToRawData = ReadUInt32(); + // Section: PointerToRawData (4) + uint pointerToRawData = reader.ReadUInt32(); - // The .mvid section only stores a Guid - Debug.Assert(virtualSize == 16); + // The .mvid section only stores a Guid + if (virtualSize != 16) + { + Debug.Assert(false); + return Guid.Empty; + } - BaseStream.Position = pointerToRawData; + if (MoveTo(pointerToRawData, reader)) + { byte[] guidBytes = new byte[16]; - BaseStream.Read(guidBytes, 0, 16); - - return new Guid(guidBytes); + if (reader.BaseStream.Read(guidBytes, 0, 16) == 16) + { + return new Guid(guidBytes); + } } - - // Section: VirtualSize (4), VirtualAddress (4), SizeOfRawData (4), - // PointerToRawData (4), PointerToRelocations (4), PointerToLineNumbers (4), - // NumberOfRelocations (2), NumberOfLineNumbers (2), Characteristics (4) - Skip(4 + 4 + 4 + 4 + 4 + 4 + 2 + 2 + 4); } return Guid.Empty; } - public void Skip(int bytes) + private static void Skip(int bytes, BinaryReader reader) { - BaseStream.Seek(bytes, SeekOrigin.Current); + reader.BaseStream.Seek(bytes, SeekOrigin.Current); } - public void MoveTo(uint position) + private static bool MoveTo(uint position, BinaryReader reader) { - BaseStream.Seek(position, SeekOrigin.Begin); + if (reader.BaseStream.Length < position) + { + return false; + } + + reader.BaseStream.Seek(position, SeekOrigin.Begin); + return true; } } }