Produce ref assemblies from command-line and msbuild #17558

jcouv · 2017-03-05T05:36:06Z

Relates to #2184

There are two command-line parameters (/refout: and /refonly) which map to CommonCommandLineArguments.OutputRefFileName and EmitOptions.EmitMetadataOnly.

If /refonly is specified, the ref assembly will be output as the primary output (to the /out path) and no /refout path is allowed. (This maintins the previous behavior for the EmitOptions.EmitMetadataOnly flag)
Also, if /refonly is specific, the no PDB will be generated (despite any command-line parameters to the contrary).

If /refout is specified, then the assembly is output as the primary output (to /out path) as usual, and the ref assembly is output to /refout path.

Background:

The compiler already supports emitting "metadata-only" assemblies. This is used by the IDE for cross-language dependencies. Those include private members, but do not include any method bodies.

We will be introducing a second concept, which is "reference assemblies" (also called skeleton assemblies). Those are a further stripped down versions metadata-only assemblies (with some caveats, they won't include private types or members, for instance). They will be used for build scenarios. Producing ref assemblies will be driven by the /refonly and /refout parameters.
The guiding principle of ref assemblies is that they should have the minimum amount of stuff to preserve the compile-time behavior of consumers.

But, as a starting point (to unblock work on msbuild), we are using metadata-only assemblies as a close approximation of ref assemblies. That gives us many of the benefits of ref assemblies at much lower cost. As we narrow down what is included in ref assemblies (see #17612), the two concepts will diverge. When they do, we will likely need specific emit or compilation options (TBD) to indicate that we're producing ref assemblies.

Filed #17612 for refining what changes and diagnostics should affect ref assemblies.

jcouv · 2017-03-09T18:12:35Z

src/Test/Utilities/Portable/Metadata/MetadataReaderUtils.cs

                default:
                    return handle.Kind.ToString();
            }
        }
+
+        // PROTOTYPE it would be better to shared the implementation with PdbToXml


I will not check this in as-is. I'll try to factor this common code. #Resolved

jcouv · 2017-03-09T18:15:22Z

src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs

+                        if (!CompileAndEmitRefAssembly(compilation, emitOptions.WithEmitMetadataOnly(true),
+                            finalRefPeFilePath, touchedFilesLogger, diagnosticBag, cancellationToken))
+                        {
+                            return Failed;


Maybe we should still write touched files (see below) even if some part this failed? What do you think? #Closed

jcouv · 2017-03-09T18:18:02Z

@dotnet/roslyn-compiler Please take a look at your convenience.
If possible, let's focus on tests, validation and scenarios first.
I'll port the remaining tests to VB once we have agreement on C# side. #Resolved

AlekseyTs · 2017-03-09T22:21:23Z

It looks like there are some legitimate test failures. #Closed

jcouv · 2017-03-09T22:58:53Z

Sorry about that. Fixed now. #Resolved

AlekseyTs · 2017-03-09T23:33:22Z

src/Compilers/Core/Portable/CommandLine/CommonCommandLineArguments.cs

@@ -86,6 +86,11 @@ public abstract class CommandLineArguments
        public string OutputFileName { get; internal set; }

        /// <summary>
+        /// Path of the output ref assembly or null if not specified.
+        /// </summary>
+        public string OutputRefFilePath { get; internal set; }


OutputRefFilePath [](start = 22, length = 17)

We are not adding RefOnly in this PR? #Closed

RefOnly is in EmitOptions. #Resolved

We're using EmitOptions.EmitMetadataOnly. #Resolved

AlekseyTs · 2017-03-09T23:37:57Z

src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs

+                    if (finalRefPeFilePath != null)
+                    {
+                        if (!CompileAndEmitRefAssembly(compilation, emitOptions.WithEmitMetadataOnly(true),
+                            finalRefPeFilePath, touchedFilesLogger, diagnosticBag, cancellationToken))


finalRefPeFilePath, touchedFilesLogger, diagnosticBag, cancellationToken)) [](start = 28, length = 74)

Consider shifting this line to the right. #Closed

jcouv · 2017-03-21T17:30:28Z

FYI @AlekseyTs we had a test that was trying to generate netmodules with metadata only. Per our discussion, this is not useful and it is explicitly disallowed now (both from the command-line and the API). Updated the test to avoid that case.

tmat · 2017-03-22T00:51:46Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

@@ -2008,6 +2036,7 @@ internal void EnsureAnonymousTypeTemplates(CancellationToken cancellationToken)
            IMethodSymbol debugEntryPoint = null,
            Stream sourceLinkStream = null,
            IEnumerable<EmbeddedText> embeddedTexts = null,
+            Stream metadataPeStream = null,


metadataPeStream [](start = 19, length = 16)

'e' should be upper case: metadataPEStream

I'd suggest a different name though. Something like contractAssemblyStream, or referenceAssemblyStream.

I'll capitalize.
As for the name, ref/contract assemblies are only one kind of metadata-only assemblies. That's why I used this name.

On second thought, I don't understand. Why would we capitalize PE, but not PDB?

PE is two letters, PDB is three. See .NET Design Guidelines

What other kinds of metadata-only assemblies are there? Perhaps metadataOnlyAssemblyStream?

tmat · 2017-03-22T00:52:58Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

@@ -2331,6 +2387,7 @@ internal void EnsureAnonymousTypeTemplates(CancellationToken cancellationToken)
            DiagnosticBag metadataDiagnostics = null;
            DiagnosticBag pdbBag = null;
            Stream peStream = null;
+            Stream refPeStream = null;


refPeStream [](start = 19, length = 11)

referenceAssemblyStream

tmat · 2017-03-22T00:53:09Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

@@ -2318,6 +2373,7 @@ internal void EnsureAnonymousTypeTemplates(CancellationToken cancellationToken)
        internal bool SerializeToPeStream(
            CommonPEModuleBuilder moduleBeingBuilt,
            EmitStreamProvider peStreamProvider,
+            EmitStreamProvider metadataPeStreamProvider,


metadataPeStreamProvider [](start = 31, length = 24)

refrenceAssemblyStreamProvider

tmat · 2017-03-22T00:54:05Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

+                }
+            }
+
+            if (metadataPeStream != null && options?.EmitMetadataOnly == true)


if [](start = 12, length = 2)

We should also disallow Embedded PDBs.

(in that case pdbStream will be null since the PDB is emitted to peStream)

In reply to: 107311874 [](ancestors = 107311874)

tmat · 2017-03-22T01:02:31Z

src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs

            // If so, use the RVA of the already serialized one.
            // Note that we don't need to rewrite the fake tokens in the body before looking it up.

            // Don't do small body method caching during deterministic builds until this issue is fixed
            // https://github.com/dotnet/roslyn/issues/7595
            int bodyOffset;
-            if (!_deterministic && isSmallBody && _smallMethodBodies.TryGetValue(methodBody.IL, out bodyOffset))
+            if (!_deterministic && isSmallBody && _smallMethodBodies.TryGetValue(il, out bodyOffset))


_deterministic [](start = 17, length = 14)

This issue doesn't apply to "throw null" bodies, so perhaps we should actually reuse these bodies in reference assemblies.

How confident are you that's the case? Aleksey wasn't sure we understood the root cause of this problem, in which case it is safer to apply the same workaround.

100%. The problem with determinism and body haring is related to string literals. There are no string literals in the "throw null" body.

tmat · 2017-03-22T01:04:41Z

src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs


        // A map of method body before token translation to RVA. Used for deduplication of small bodies.
        private readonly Dictionary<ImmutableArray<byte>, int> _smallMethodBodies;

+        private static readonly ImmutableArray<byte> ThrowNullIL = ImmutableArray.Create((byte)ILOpCode.Ldnull, (byte)ILOpCode.Throw);


ThrowNullIL [](start = 53, length = 11)

Instead of hardcoding just the method IL, i'd suggest to hardocde the entire encoded method body byte sequence. It's gonna be just a single byte or two more and we can avoid some extra code in SerializeMethodBody.

tmat · 2017-03-22T01:05:36Z

src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs

+                (methodBody.LocalsAreZeroed ? MethodBodyAttributes.InitLocals : MethodBodyAttributes.None),
+                ref mvidStringHandle,
+                ref mvidStringFixup);
+        }


We can avoid all this clutter if we hardcode encoded "throw null" body instead of just the IL.

tmat · 2017-03-22T01:08:00Z

src/Compilers/Core/Portable/PEWriter/PeWriter.cs

+                {
+                    return false;
+                }
+            }


Wouldn't it be better to move this to the caller that potentially produces uses both outputs? I think that would be just a single caller.

I'm not sure I see the advantage. There is a single caller, but why have two methods with same name spread across two classes? Especially since this one is a thin wrapper on top of the existing one.

tmat · 2017-03-22T01:10:02Z

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs

                PdbPath = pdbPath,
-                EmitPdb = emitPdb,
+                EmitPdb = emitPdb && !metadataOnly,


emitPdb && !metadataOnly [](start = 26, length = 24)

Code above checks the value of emitPdb. So if we are gonna change it here the check above won't take the metadataOnly flag into account.

That's correct and that's by design. The idea is that you can just add /refonly on a command-line and it will work, without having to remove the parameters related to emitting PDBs. For instance, you may have source links and we won't complain about it (one of the checks above).
Does that seem ok?

OK. I'd then add a comment.

tmat · 2017-03-22T01:17:15Z

src/Test/Utilities/Portable/Metadata/MetadataReaderUtils.cs

+        /// Asserts that all the method bodies in the PE are empty or `throw null`, if expectEmptyOrThrowNull is true.
+        /// Asserts that none of the methods bodies are empty or `throw null`, if expectEmptyOrThrowNull is false.
+        /// </summary>
+        internal static void VerifyMethodBodies(ImmutableArray<byte> peImage, bool expectEmptyOrThrowNull)


VerifyMethodBodies [](start = 29, length = 18)

This is an odd helper. Perhaps change to VerifyMethodBodies(peImage, Action<byte[]> ilValidator)

tmat · 2017-03-22T01:19:21Z

src/Test/Utilities/Portable/Metadata/MetadataReaderUtils.cs

+
+            public string GetTypeFromDefinition(MetadataReader reader, TypeDefinitionHandle handle, byte rawTypeKind)
+            {
+                var typeDef = reader.GetTypeDefinition(handle);


var typeDef = reader.GetTypeDefinition(handle) [](start = 16, length = 46)

Doesn't capture nested types.

tmat · 2017-03-22T01:19:27Z

src/Test/Utilities/Portable/Metadata/MetadataReaderUtils.cs

+
+            public string GetTypeFromReference(MetadataReader reader, TypeReferenceHandle handle, byte rawTypeKind)
+            {
+                var typeRef = reader.GetTypeReference(handle);


var typeRef = reader.GetTypeReference(handle); [](start = 16, length = 46)

Doesn't capture nested types.

You can steal a helper from
https://github.com/dotnet/symreader-converter/blob/master/src/Microsoft.DiaSymReader.Converter/PdbConverterPortableToWindows.MetadataModel.cs#L172

In reply to: 107314698 [](ancestors = 107314698)

tmat · 2017-03-22T01:22:48Z

src/Test/Utilities/Portable/Metadata/MetadataReaderUtils.cs

+            }
+
+            public string GetSZArrayType(string elementType)
+            {


Perhaps make all these methods expression-bodied.

jasonmalinowski

One comment that might need addressing in the build tasks. That part is very subtle and mistakes there can mean problems for the IDE, so definitely think through it.

jasonmalinowski · 2017-03-23T00:38:30Z

src/Compilers/Core/MSBuildTask/ManagedCompiler.cs

@@ -705,6 +718,8 @@ protected internal virtual void AddResponseFileCommands(CommandLineBuilderExtens
            commandLine.AppendPlusOrMinusSwitch("/optimize", _store, nameof(Optimize));
            commandLine.AppendSwitchIfNotNull("/pathmap:", PathMap);
            commandLine.AppendSwitchIfNotNull("/out:", OutputAssembly);
+            commandLine.AppendSwitchIfNotNull("/refout:", OutputRefAssembly);
+            commandLine.AppendWhenTrue("/refonly", _store, nameof(RefOnly));


Do you intend this to be user-settable in projects, for example if I have some project with manual type definitions so I can make a reference assembly that way? If so, then you should consider moving this to AddResponseFileCommandsForSwitchesSinceInitialReleaseThatAreNeededByTheHost.

jasonmalinowski · 2017-03-23T00:41:02Z

src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs

+                var readStream = OpenFile(readFilesPath, consoleOutput, mode: FileMode.OpenOrCreate);
+                var writtenStream = OpenFile(writtenFilesPath, consoleOutput, mode: FileMode.OpenOrCreate);
+
+                if (readStream == null || writtenStream == null)


So if one is created and the other isn't we leak the stream?

jcouv · 2017-03-23T17:45:48Z

src/Compilers/Core/Portable/PublicAPI.Unshipped.txt

 Microsoft.CodeAnalysis.CommandLineArguments.SourceLink.get -> string
 Microsoft.CodeAnalysis.Compilation.CreateAnonymousTypeSymbol(System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.ITypeSymbol> memberTypes, System.Collections.Immutable.ImmutableArray<string> memberNames, System.Collections.Immutable.ImmutableArray<bool> memberIsReadOnly = default(System.Collections.Immutable.ImmutableArray<bool>), System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.Location> memberLocations = default(System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.Location>)) -> Microsoft.CodeAnalysis.INamedTypeSymbol
 Microsoft.CodeAnalysis.Compilation.CreateErrorNamespaceSymbol(Microsoft.CodeAnalysis.INamespaceSymbol container, string name) -> Microsoft.CodeAnalysis.INamespaceSymbol
 Microsoft.CodeAnalysis.Compilation.CreateTupleTypeSymbol(Microsoft.CodeAnalysis.INamedTypeSymbol underlyingType, System.Collections.Immutable.ImmutableArray<string> elementNames = default(System.Collections.Immutable.ImmutableArray<string>), System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.Location> elementLocations = default(System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.Location>)) -> Microsoft.CodeAnalysis.INamedTypeSymbol
 Microsoft.CodeAnalysis.Compilation.CreateTupleTypeSymbol(System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.ITypeSymbol> elementTypes, System.Collections.Immutable.ImmutableArray<string> elementNames = default(System.Collections.Immutable.ImmutableArray<string>), System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.Location> elementLocations = default(System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.Location>)) -> Microsoft.CodeAnalysis.INamedTypeSymbol
-Microsoft.CodeAnalysis.Compilation.Emit(System.IO.Stream peStream, System.IO.Stream pdbStream = null, System.IO.Stream xmlDocumentationStream = null, System.IO.Stream win32Resources = null, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.ResourceDescription> manifestResources = null, Microsoft.CodeAnalysis.Emit.EmitOptions options = null, Microsoft.CodeAnalysis.IMethodSymbol debugEntryPoint = null, System.IO.Stream sourceLinkStream = null, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.EmbeddedText> embeddedTexts = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> Microsoft.CodeAnalysis.Emit.EmitResult
+Microsoft.CodeAnalysis.Compilation.Emit(System.IO.Stream peStream, System.IO.Stream pdbStream = null, System.IO.Stream xmlDocumentationStream = null, System.IO.Stream win32Resources = null, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.ResourceDescription> manifestResources = null, Microsoft.CodeAnalysis.Emit.EmitOptions options = null, Microsoft.CodeAnalysis.IMethodSymbol debugEntryPoint = null, System.IO.Stream sourceLinkStream = null, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.EmbeddedText> embeddedTexts = null, System.IO.Stream metadataPeStream = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> Microsoft.CodeAnalysis.Emit.EmitResult


Forgot to ping @jaredpar and @gafter on public API change in this PR.

jcouv added the Area-Compilers label Mar 5, 2017

jcouv self-assigned this Mar 5, 2017

dnfclas added the cla-already-signed label Mar 5, 2017

jcouv force-pushed the msbuild-refdll branch 7 times, most recently from 6a721ff to 349610e Compare March 7, 2017 00:42

gafter added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 7, 2017

jcouv force-pushed the msbuild-refdll branch from e8eac0d to 470b770 Compare March 7, 2017 07:19

jcouv mentioned this pull request Mar 7, 2017

[Work items] Refining ref assemblies #17612

Closed

34 tasks

jcouv force-pushed the msbuild-refdll branch 4 times, most recently from f8a0f4a to 4529afb Compare March 9, 2017 17:58

refonly and refout on command-line and msbuild

04a5c0d

jcouv force-pushed the msbuild-refdll branch from 4529afb to 04a5c0d Compare March 9, 2017 18:11

jcouv commented Mar 9, 2017

View reviewed changes

jcouv changed the title ~~[Not ready for review] Produce ref assemblies from command-line and msbuild~~ Produce ref assemblies from command-line and msbuild Mar 9, 2017

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 9, 2017

jcouv commented Mar 9, 2017

View reviewed changes

jcouv changed the base branch from master to features/refout March 9, 2017 18:28

Getting ParseOut test passing again

49a84f5

AlekseyTs reviewed Mar 9, 2017

View reviewed changes

jcouv closed this Mar 20, 2017

jcouv reopened this Mar 20, 2017

dnfclas added the cla-already-signed label Mar 20, 2017

Can no longer produce netmodule with metadata only

ef4ee02

jcouv merged commit 235acb3 into dotnet:features/refout Mar 21, 2017

jcouv deleted the msbuild-refdll branch March 21, 2017 18:55

tmat reviewed Mar 22, 2017

View reviewed changes

jasonmalinowski reviewed Mar 23, 2017

View reviewed changes

jcouv mentioned this pull request Mar 23, 2017

Ref assemblies: Address more feedback on #17558 #18091

Merged

jcouv added a commit to jcouv/roslyn that referenced this pull request Mar 23, 2017

Address feedback from Tomas on dotnet#17558

b360fd0

jcouv commented Mar 23, 2017

View reviewed changes

jcouv added a commit that referenced this pull request Mar 25, 2017

Ref assemblies: Address more feedback on #17558 (#18091)

20aa0c2

gafter mentioned this pull request Apr 11, 2017

Test plan for "ref assemblies" (7.1) #18612

Closed

54 tasks

rainersigwald mentioned this pull request Apr 17, 2017

Support reference assemblies in build flows dotnet/msbuild#1986

Closed

12 tasks

0x53A mentioned this pull request May 17, 2017

Produce ref assemblies from command-line and msbuild dotnet/fsharp#3066

Closed

Produce ref assemblies from command-line and msbuild #17558

Produce ref assemblies from command-line and msbuild #17558

Conversation

jcouv commented Mar 5, 2017 • edited Loading

Background:

jcouv Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv commented Mar 9, 2017 • edited Loading

AlekseyTs commented Mar 9, 2017 • edited Loading

jcouv commented Mar 9, 2017 • edited Loading

AlekseyTs Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv commented Mar 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Mar 5, 2017 •

edited

Loading

jcouv Mar 9, 2017 •

edited

Loading

jcouv Mar 9, 2017 •

edited

Loading

jcouv commented Mar 9, 2017 •

edited

Loading

AlekseyTs commented Mar 9, 2017 •

edited

Loading

jcouv commented Mar 9, 2017 •

edited

Loading

AlekseyTs Mar 9, 2017 •

edited

Loading

jcouv Mar 9, 2017 •

edited

Loading

jcouv Mar 9, 2017 •

edited

Loading

AlekseyTs Mar 9, 2017 •

edited

Loading

tmat Mar 22, 2017 •

edited

Loading

tmat Mar 22, 2017 •

edited

Loading

tmat Mar 22, 2017 •

edited

Loading