Order PDB document table to match compiler arguments #50615

RikkiGibson · 2021-01-19T21:55:46Z

@ryzngard are there tests for the compilation options embedded in the PDBs? or would it be practical to add any?

Have filed #50611 to follow up on fixing the document order in native PDBs.

tmat · 2021-01-19T22:02:30Z

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

@@ -847,6 +847,7 @@ private void EmbedCompilationOptions(CommonPEModuleBuilder module)
            WriteValue(CompilationOptionNames.CompilerVersion, compilerVersion);

            WriteValue(CompilationOptionNames.Language, module.CommonCompilation.Options.Language);
+            WriteValue(CompilationOptionNames.SourceFileCount, module.CommonCompilation.SyntaxTrees.Count().ToString());


Count() [](start = 100, length = 7)

Minor thing and not related to your change but just noticed that SyntaxTrees is IEnumerable but both C# and VB represent it as ImmutableArray. So we are boxing it unnecessarily. Can't change public API but could change and use CommonSyntaxTrees.

I think that's a good idea. Let's spin it off into its own issue.

RikkiGibson · 2021-01-19T22:09:12Z

Does this PR need to target 16.9?

edit: It looks like master currently targets 16.9-preview4, so we can keep this base branch.

tmat

ryzngard · 2021-01-19T23:30:34Z

are there tests for the compilation options embedded in the PDBs? or would it be practical to add any?

https://github.com/dotnet/roslyn/blob/master/src/Compilers/CSharp/Test/Emit/PDB/CSharpDeterministicBuildCompilationTests.cs

This could probably use an update to the name of the tests too fwiw.

RikkiGibson · 2021-01-20T01:00:28Z

Adding the "version" field was very straightforward so I decided to just do it in this PR.

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

jaredpar · 2021-01-20T17:55:12Z

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

+                    }
+                }
+
+


Double blink line

Suggested change

jaredpar · 2021-01-20T17:55:50Z

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

+                // Ensure document table lists files in command line order
+                var documentsBuilder = Module.DebugDocumentsBuilder;
+                var documents = documentsBuilder.DebugDocuments;
+                foreach (var tree in Module.CommonCompilation.SyntaxTrees)


At this point in the code does this include SyntaxTree that come from source generators?

Great question. I think the answer is yes because this is emit-phase work which we are only expected to do for the "final" compilation which includes source generator outputs.

It feels like it would be appropriate to have a test here.

Definitely need a test - it should also cover that all source generated files are embedded in the PDB.

I'm not yet certain how to formulate this test. See this simple source generators test:

roslyn/src/Compilers/CSharp/Test/Semantic/SourceGeneration/GeneratorDriverTests.cs

Lines 113 to 140 in a163778

[Fact]

public void Single_File_Is_Added()

{

var source = @"

class C { }

";

var generatorSource = @"

class GeneratedClass { }

";

var parseOptions = TestOptions.Regular;

Compilation compilation = CreateCompilation(source, options: TestOptions.DebugDll, parseOptions: parseOptions);

compilation.VerifyDiagnostics();

Assert.Single(compilation.SyntaxTrees);

SingleFileTestGenerator testGenerator = new SingleFileTestGenerator(generatorSource);

GeneratorDriver driver = CSharpGeneratorDriver.Create(new[] { testGenerator }, parseOptions: parseOptions);

driver.RunGeneratorsAndUpdateCompilation(compilation, out var outputCompilation, out _);

Assert.Equal(2, outputCompilation.SyntaxTrees.Count());

Assert.NotEqual(compilation, outputCompilation);

var generatedClass = outputCompilation.GlobalNamespace.GetTypeMembers("GeneratedClass").Single();

Assert.True(generatedClass.Locations.Single().IsInSource);

}

If I try to imitate the structure of the source generator tests, I will create an original compilation, feed it into the generator driver, then get a final compilation out and try to emit with it. But it seems obvious that the syntax trees for that compilation will include the generated sources, so the test won't really be demonstrating anything useful.

It feels like if we had a higher level test that straight up ran through CommonCompiler.Run we might be able to demonstrate something meaningful, but I'm not sure it's good to spend the time to come up with the automated test just yet.

I also imagine that in the near future we might write a test harness that allows us to just take any Compilation and attempt to "round-trip" it, by emitting its dll+pdb, gathering the information to recreate it, and then doing so on the spot. Test failure messages could be given by comparing output of ildasm or mdv or something like that on the before and after binaries.

The test could verify that the PDB contains the sources of the generated files. That is, the compilation that comes out of the driver is setup correctly.

jaredpar · 2021-01-20T17:56:21Z

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

@@ -835,6 +835,9 @@ private void EmbedSourceLink(Stream stream)
                value: _debugMetadataOpt.GetOrAddBlob(bytes));
        }

+        ///<summary>The version of the compilation options schema to be written to the PDB.</summary>
+        public const int Version = 1;


Suggested change

public const int Version = 1;

public const int CompilationOptionsSchemaVersion = 1;

jaredpar · 2021-01-20T17:58:49Z

src/Compilers/CSharp/Test/Emit/PDB/CheckSumTest.cs

-    <file id=""1"" name=""a\..\a.cs"" language=""C#"" checksumAlgorithm=""406ea660-64cf-4c82-b6f0-42d48172a799"" checksum=""AB-00-7F-1D-23-D5"" />
-    <file id=""2"" name=""C:\a\..\b.cs"" language=""C#"" checksumAlgorithm=""SHA1"" checksum=""36-39-3C-83-56-97-F2-F0-60-95-A4-A0-32-C6-32-C7-B2-4B-16-92"" />
+    <file id=""1"" name=""C:\a\..\b.cs"" language=""C#"" checksumAlgorithm=""SHA1"" checksum=""36-39-3C-83-56-97-F2-F0-60-95-A4-A0-32-C6-32-C7-B2-4B-16-92"" />
+    <file id=""2"" name=""a\..\a.cs"" language=""C#"" checksumAlgorithm=""406ea660-64cf-4c82-b6f0-42d48172a799"" checksum=""AB-00-7F-1D-23-D5"" />


Why is there no checksum algorithm here?

It seems like the checksum algorithm is present on this line. Am I misunderstanding something?

ugg, the double quoting fooled me.

jaredpar · 2021-01-22T23:45:49Z

@dotnet/roslyn-compiler can we get one more review here? Part of the fix here is for 16.9 important bugs

RikkiGibson · 2021-01-25T18:58:16Z

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

@@ -28,5 +29,6 @@ internal static class CompilationOptionNames
        public const string Nullable = "nullable";
        public const string Define = "define";
        public const string Strict = "strict";
+        public const string SourceFileCount = "source-file-count";


The purpose of this property is to let us order the "real" documents at the beginning of the table, then allow documents added for #line directives to come at positions after this count so that we can skip them when rebuilding.

cston · 2021-01-25T19:24:05Z

src/Compilers/CSharp/Test/Emit/PDB/CSharpDeterministicBuildCompilationTests.cs

-                    VerifyCompilationOptions(compilationOptions, originalCompilation, emitOptions, compilationOptionsReader, langVersion);
+                    if (debugDocumentsCount is not null)
+                    {
+                        Assert.Equal(debugDocumentsCount, pdbReader.Documents.Count);


Assert.Equal(debugDocumentsCount, pdbReader.Documents.Count); [](start = 24, length = 61)

Should we assert unconditionally? Assert.Equal(debugDocumentsCount ?? syntaxTrees.Length, pdbReader.Documents.Count);

We could if we ensure every test that doesn't pass 'debugDocumentsCount' passes a unique path of each of its documents, which isn't hard to do.

cston · 2021-01-25T19:47:15Z

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

+                foreach (var tree in Module.CommonCompilation.SyntaxTrees)
+                {
+                    string normalizedPath = documentsBuilder.NormalizeDebugDocumentPath(tree.FilePath, basePath: null);
+                    if (documents.TryGetValue(normalizedPath, out var doc) && !_documentIndex.ContainsKey(doc))


if (documents.TryGetDebugDocument(tree.FilePath, basePath: null) is { } doc && ...

Order PDB document table to match compiler arguments

e6d1189

RikkiGibson requested a review from a team as a code owner January 19, 2021 21:55

dotnet-issue-labeler bot added the Area-Compilers label Jan 19, 2021

tmat reviewed Jan 19, 2021

View reviewed changes

Remove TODO and add WorkItem to reference follow-up issue

b06cde1

tmat approved these changes Jan 19, 2021

View reviewed changes

RikkiGibson mentioned this pull request Jan 19, 2021

Compilation.CommonSyntaxTrees could use ImmutableArray return type #50617

Closed

RikkiGibson added 3 commits January 19, 2021 15:38

Verify source-file-count in tests

7ff865f

Add CompilationOptionsVersion and update tests

65e88d5

compilation-options-version -> version. Test debugDocumentsCount.

be34f79

jaredpar reviewed Jan 20, 2021

View reviewed changes

Address feedback

a2e5db4

jaredpar approved these changes Jan 22, 2021

View reviewed changes

RikkiGibson commented Jan 25, 2021

View reviewed changes

jaredpar approved these changes Jan 25, 2021

View reviewed changes

cston reviewed Jan 25, 2021

View reviewed changes

Always assert on the pdbReader.Documents.Count

316d75b

cston reviewed Jan 25, 2021

View reviewed changes

Simplify

fc412db

cston approved these changes Jan 25, 2021

View reviewed changes

RikkiGibson merged commit ab05661 into dotnet:master Jan 25, 2021

RikkiGibson deleted the pdb-document-order branch January 25, 2021 21:11

ghost added this to the Next milestone Jan 25, 2021

Cosifne modified the milestones: Next, 16.9 Jan 27, 2021

JoeRobich removed this from the 16.9 milestone Jan 27, 2021

JoeRobich added this to the 16.9.P4 milestone Jan 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Order PDB document table to match compiler arguments #50615

Order PDB document table to match compiler arguments #50615

RikkiGibson commented Jan 19, 2021 •

edited

tmat Jan 19, 2021

RikkiGibson Jan 19, 2021

RikkiGibson commented Jan 19, 2021 •

edited

tmat left a comment

ryzngard commented Jan 19, 2021

RikkiGibson commented Jan 20, 2021

jaredpar Jan 20, 2021

jaredpar Jan 20, 2021

RikkiGibson Jan 20, 2021

tmat Jan 20, 2021

RikkiGibson Jan 20, 2021 •

edited

tmat Jan 20, 2021

jaredpar Jan 20, 2021

jaredpar Jan 20, 2021

RikkiGibson Jan 20, 2021

jaredpar Jan 20, 2021

jaredpar commented Jan 22, 2021

RikkiGibson Jan 25, 2021

cston Jan 25, 2021 •

edited

RikkiGibson Jan 25, 2021

cston Jan 25, 2021

RikkiGibson Jan 25, 2021

	[Fact]
	public void Single_File_Is_Added()
	{
	var source = @"
	class C { }
	";

	var generatorSource = @"
	class GeneratedClass { }
	";

	var parseOptions = TestOptions.Regular;
	Compilation compilation = CreateCompilation(source, options: TestOptions.DebugDll, parseOptions: parseOptions);
	compilation.VerifyDiagnostics();

	Assert.Single(compilation.SyntaxTrees);

	SingleFileTestGenerator testGenerator = new SingleFileTestGenerator(generatorSource);

	GeneratorDriver driver = CSharpGeneratorDriver.Create(new[] { testGenerator }, parseOptions: parseOptions);
	driver.RunGeneratorsAndUpdateCompilation(compilation, out var outputCompilation, out _);

	Assert.Equal(2, outputCompilation.SyntaxTrees.Count());
	Assert.NotEqual(compilation, outputCompilation);

	var generatedClass = outputCompilation.GlobalNamespace.GetTypeMembers("GeneratedClass").Single();
	Assert.True(generatedClass.Locations.Single().IsInSource);
	}

	public const int Version = 1;
	public const int CompilationOptionsSchemaVersion = 1;

Order PDB document table to match compiler arguments #50615

Order PDB document table to match compiler arguments #50615

Conversation

RikkiGibson commented Jan 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Jan 19, 2021 • edited

tmat left a comment

Choose a reason for hiding this comment

ryzngard commented Jan 19, 2021

RikkiGibson commented Jan 20, 2021

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

RikkiGibson Jan 20, 2021 • edited

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

jaredpar commented Jan 22, 2021

Choose a reason for hiding this comment

cston Jan 25, 2021 • edited

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

RikkiGibson commented Jan 19, 2021 •

edited

RikkiGibson commented Jan 19, 2021 •

edited

RikkiGibson Jan 20, 2021 •

edited

cston Jan 25, 2021 •

edited