Rebuild assemblies with non-embedded PDBs by RikkiGibson · Pull Request #52272 · dotnet/roslyn

RikkiGibson · 2021-03-31T00:26:53Z

~~Tagging @jaredpar. I have the implementation side working fine, but there's an issue with CompilationDiff that I can't track down. See #52272 (comment)~~ Resolved.

This adds a file system abstraction sufficient to hook all file open events inside `CommonCompiler`. This is necessary to fully test the rebuild scenarios that we need to add

The determinism / round trip tests in emit were depending on a bug in our test framework. The metadata writer only emits references if there is a name associated with the reference. Before my PR all of our in memory references lacked image names and hence they were omitted from the PDB. Once I fixed that they started showing up and needed to be accounted for in the logic.

Co-authored-by: Youssef Victor <youssefvictor00@gmail.com>

src/Tools/BuildValidator/CompilationDiff.cs

src/Compilers/Core/Rebuild/CSharpCompilationFactory.cs

src/Compilers/Core/Rebuild/CompilationFactory.cs

src/Compilers/Core/RebuildTest/Microsoft.CodeAnalysis.Rebuild.UnitTests.csproj

src/Compilers/Core/RebuildTest/RoundTripUtil.cs

src/Tools/BuildValidator/CompilationDiff.cs

cston · 2021-04-06T05:55:02Z

src/Compilers/Core/RebuildTest/RebuildCommandLineTests.cs


            return list;

            IEnumerable<CommandInfo> Permutate(CommandInfo commandInfo)


IEnumerable [](start = 12, length = 11)

Consider declaring these local functions (other than Add) as static?

cston · 2021-04-06T06:06:45Z

src/Compilers/Core/RebuildTest/RoundTripUtil.cs

+            if (rebuildPdbStream is null)
+            {
+                rebuildPdbReader = rebuildReader.GetEmbeddedPdbMetadataReader() ?? throw new InvalidOperationException();
+                rebuildPdbBytes = new ReadOnlySpan<byte>(rebuildPdbReader.MetadataPointer, rebuildPdbReader.MetadataLength).ToArray().ToImmutableArray();


ToArray() [](start = 124, length = 9)

Minor: Is ToArray() needed?

Unfortunately, I haven't been able to find API on ReadOnlySpan which allows creating an ImmutableArray without copying (save for creating a builder and copying the items over manually, I guess, which we could do)

https://stackoverflow.com/questions/66912187/how-should-i-create-an-immutablearray-from-a-span-readonlyspan?noredirect=1#comment118278058_66912187

Punting on this since it's test code.

cston · 2021-04-06T06:09:48Z

src/Compilers/Core/RebuildTest/RoundTripUtil.cs

+                            Assert.NotNull(originalPdbReader);
+
+                            var pdbSpan = new ReadOnlySpan<byte>(originalPdbReader!.MetadataPointer, originalPdbReader.MetadataLength);
+                            return new EmitInfo(originalBytes, originalReader, pdbSpan.ToArray().ToImmutableArray(), originalPdbReader);


ToArray() [](start = 87, length = 9)

Is ToArray() needed?

cston · 2021-04-06T06:14:42Z

src/Compilers/Core/RebuildTest/RoundTripUtil.cs

-            Assert.Equal(originalBytes.ToArray(), rebuildStream.ToArray());
+
+            var rebuildBytes = rebuildStream.ToImmutable();
+            var rebuildReader = new PEReader(rebuildBytes);


var rebuildReader [](start = 12, length = 17)

Should we use using var ... = new PEReader(bytes); here and in cases below?

cston · 2021-04-06T06:27:31Z

src/Tools/BuildValidator/Program.cs

-            try
+            // Find the embedded pdb
+            var originalBinaryStream = File.OpenRead(assemblyInfo.FilePath);
+            var originalPeReader = new PEReader(originalBinaryStream);


Why remove the using from these two declarations?

I believe I removed this because I was tracking down #52272 (comment). At first I was concerned about reintroducing this because it looks like the PEReader can escape the method via CompilationDiff, but now I see that there is no field of type PEReader on CompilationDiff, so I think we're in the clear with that.

It feels like we might want to make a pass over data types which contain disposable things to determine if the containing type should be made disposable, and to check if things are actually getting disposed when they need to.

jaredpar · 2021-04-06T12:51:02Z

src/Compilers/Core/RebuildTest/RoundTripUtil.cs

+
+

Suggested change

Nit: double blank line

jaredpar · 2021-04-06T12:52:23Z

src/Compilers/Core/RebuildTest/RoundTripUtil.cs

+                var originalMdv = getMdv(originalEmit.PdbReader);
+                var rebuildMdv = getMdv(rebuildEmit.PdbReader);
+
+                Assert.True(false, $@"


I think we should be passing this to the xUnit output infrastructure. That is usually presented with full information where as Assert failures will often clip information and make it hard to read.

I will punt this to #52327 if that's ok. I agree that the whole output should probably go through the test output helper. But it might make sense to put the diff in the assert failure message.

That's fine

jaredpar · 2021-04-06T12:52:31Z

src/Compilers/Core/RebuildTest/RoundTripUtil.cs

+
+                // https://github.com/dotnet/roslyn/issues/52327
+                // this is not all that useful without manual copy/pasting. Can we diff this during the test and show the differences?
+                Assert.True(false, $@"


I think we should be passing this to the xUnit output infrastructure. That is usually presented with full information where as Assert failures will often clip information and make it hard to read.

cston · 2021-04-06T17:52:57Z

src/Tools/BuildValidator/Program.cs

        {
-            MetadataReaderProvider? pdbReaderProvider = null;
+            // Find the embedded pdb
+            using var originalPeReader = new PEReader(File.OpenRead(assemblyInfo.FilePath));


File.OpenRead(assemblyInfo.FilePath) [](start = 54, length = 36)

Does PEReader.Dispose() dispose of the Stream?

https://docs.microsoft.com/en-us/dotnet/api/system.reflection.portableexecutable.pereader.-ctor?view=net-5.0#System_Reflection_PortableExecutable_PEReader__ctor_System_IO_Stream_

Ownership of the stream is transferred to the PEReader upon successful validation of constructor arguments. It will be disposed by the PEReader and the caller must not manipulate it.

jaredpar and others added 23 commits March 28, 2021 12:42

Add file system abstraction

f7c867a

This adds a file system abstraction sufficient to hook all file open events inside `CommonCompiler`. This is necessary to fully test the rebuild scenarios that we need to add

Generated code has access to ref name

4477efb

Tests for round tripping through CommonCompiler

3eb6324

Fix debug plus round trip

ba7d53d

Fix a build error

c8c72b1

Respond to PR feedback

e7ec9a7

Fix test failures

fe94cea

PR feedback

14b43fb

Fix namespace in Rebuild library

ac63eef

PR feedback

33085bd

fix

f9492b6

Apply suggestions from code review

e05debe

Co-authored-by: Youssef Victor <youssefvictor00@gmail.com>

fix IVT

01bec52

Merge branch 'opt' of github.com:jaredpar/roslyn into rebuild-ns

1f69d94

Test failure

0dfd7a1

Fix usings

44e957d

one doesn't just rename a namespace and push

7ec863d

Fixups

7759390

PR feedback

c299526

Merge branch 'opt' of github.com:jaredpar/roslyn into rebuild-ns

86efe49

Rebuild assemblies with non-embedded PDBs

14c3ea2

Merge branch 'main' of github.com:dotnet/roslyn into rebuild-pdb

f1a8204

ghost added the Area-Compilers label Mar 31, 2021

RikkiGibson added 5 commits March 31, 2021 09:57

Merge branch 'main' of github.com:dotnet/roslyn into rebuild-ns

811f2a1

Merge branch 'rebuild-ns' into rebuild-pdb

6107f5f

cleanup

7dfead4

WIP: handle non-embedded PDBs in CompilationDiff

2f1d915

Merge branch 'main' of github.com:dotnet/roslyn into rebuild-pdb

fc6b039

RikkiGibson commented Apr 1, 2021

View reviewed changes

src/Tools/BuildValidator/CompilationDiff.cs Outdated Show resolved Hide resolved

RikkiGibson marked this pull request as ready for review April 1, 2021 01:27

RikkiGibson requested review from a team as code owners April 1, 2021 01:27

jaredpar requested changes Apr 1, 2021

View reviewed changes

RikkiGibson added 6 commits April 1, 2021 13:00

Address some feedback

00e4339

cleanup

4dff5fe

fix build

4a9df68

Cleanup

ee40d91

Check portable PDBs in VerifyRoundTrip tests

d25584f

Remove dead TODO

da9d482

cston reviewed Apr 6, 2021

View reviewed changes

jaredpar approved these changes Apr 6, 2021

View reviewed changes

RikkiGibson added 2 commits April 6, 2021 10:05

Merge branch 'main' of github.com:dotnet/roslyn into rebuild-pdb

2ef4630

Address feedback

a13881d

cston reviewed Apr 6, 2021

View reviewed changes

cston approved these changes Apr 6, 2021

View reviewed changes

RikkiGibson enabled auto-merge (squash) April 6, 2021 23:03

RikkiGibson merged commit 1431a93 into dotnet:main Apr 6, 2021

ghost added this to the Next milestone Apr 6, 2021

RikkiGibson deleted the rebuild-pdb branch April 6, 2021 23:30

dibarbet modified the milestones: Next, 16.10.P3 Apr 26, 2021


		return list;

		IEnumerable<CommandInfo> Permutate(CommandInfo commandInfo)

Conversation

RikkiGibson commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RikkiGibson commented Mar 31, 2021 •

edited

Loading