Include source files w/o method bodies in the PDB documents #39136

ivanbasov · 2019-10-08T22:03:30Z

Currently the compiler does not list source files in the debug documents in the PDB that are part of the compilation but do not have any method body. These documents are only added in order to support sequence points.

jaredpar · 2019-10-08T23:33:05Z

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

@@ -104,6 +104,7 @@ internal static class PeWriter
                }

                nativePdbWriterOpt.WriteRemainingEmbeddedDocuments(mdWriter.Module.DebugDocumentsBuilder.EmbeddedDocuments);
+                nativePdbWriterOpt.WriteRemainingDebugDocuments(mdWriter.Module.DebugDocumentsBuilder.GetAllDebugDocuments());


Nit: this boxes the ImmutableArray<T> into IEnumerable<T>. Given both these APIs are new consider changing the type of one to avoid the box. #Resolved

tmat · 2019-10-09T23:22:28Z

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

@@ -768,6 +768,21 @@ public void AddRemainingEmbeddedDocuments(IEnumerable<DebugSourceDocument> docum
            }
        }

+        /// <summary>
+        /// Add document entries for all debug documents that does not yet have an entry.


does [](start = 62, length = 4)

typo: do #Resolved

ivanbasov · 2019-10-09T23:23:22Z

@tmat PDB tests have been fixed.
BreakMode_RudeEdits_DocumentWithoutSequencePoints is still broken. Either the test should be corrected, or maybe CommitedSolution.cs should be updated as well. #Resolved

tmat · 2019-10-09T23:24:03Z

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

+            {
+                GetOrAddDocument(document, _documentIndex);
+            }
+        }


Why do we need two methods? They seem to be doing the same thing. #Resolved

tmat · 2019-10-09T23:26:06Z

        set { Debug.Assert(value != null); _embeddedDocuments = value; }

This Assert is always true. #Resolved

Refers to: src/Compilers/Core/Portable/Emit/DebugDocumentsBuilder.cs:40 in d26e2ef. [](commit_id = d26e2ef, deletion_comment = False)

tmat · 2019-10-09T23:28:27Z

src/Compilers/Core/Portable/Emit/DebugDocumentsBuilder.cs

@@ -47,6 +47,9 @@ internal void AddDebugDocument(Cci.DebugSourceDocument document)
            _debugDocuments.Add(document.Location, document);
        }

+        internal ImmutableArray<Cci.DebugSourceDocument> GetAllDebugDocuments()
+            => _debugDocuments.Values.ToImmutableArray();


_debugDocuments.Values.ToImmutableArray(); [](start = 15, length = 42)

This will result in non-deterministic PDB. We need to order the documents somehow. #Resolved

We could also optimize this to avoid allocations and extra work. Return IReadOnlyDictionary from this method. Enumerate the dictionary in AddRemainingDebugDocuments to filter out documents that are already indexed. Sort the remaining and add them.

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

Sounds good. I just think that filtering is not necessary because we add only item not yet added.

In reply to: 333278515 [](ancestors = 333278515,333275492)

The filtering is an optimization, so that we don't need to sort all the documents upfront just to realize we only need to add a few. #Resolved

Let me try

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

Thank you, @tmat! Updated.

In reply to: 333303897 [](ancestors = 333303897,333303087)

tmat · 2019-10-09T23:36:26Z

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

@@ -104,6 +104,7 @@ internal static class PeWriter
                }

                nativePdbWriterOpt.WriteRemainingEmbeddedDocuments(mdWriter.Module.DebugDocumentsBuilder.EmbeddedDocuments);


WriteRemainingEmbeddedDocuments [](start = 35, length = 31)

I don't think this is necessary anymore - embedded sources are added to the set of all debug documents on the document builder in CreateDebugDocuments. If we make sure we add an entry for each document then it follows that all embedded documents will also be added.

Also, once we remove this call we can remove EmbeddedDocuments array from DebugDocumentsBuilder. #Resolved

tmat

🕐

tmat · 2019-10-10T19:34:01Z

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

            {
-                Debug.Assert(document.GetSourceInfo().EmbeddedTextBlob != null);
-                GetOrAddDocument(document, _documentIndex);
+                GetOrAddDocument(kvp.Value, _documentIndex);


GetOrAddDocument [](start = 16, length = 16)

Nit: we could factor out AddDocument method - here we don't need to check again that the document is in the index. #Resolved

tmat · 2019-10-10T19:36:45Z

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

@@ -11037,5 +11039,76 @@ public void InvalidCharacterInPdbPath()
                    Diagnostic(ErrorCode.FTL_InvalidInputFileName).WithArguments("test\\?.pdb").WithLocation(1, 1));
            }
        }
+
+        [ConditionalFact(typeof(WindowsDesktopOnly), Reason = ConditionalSkipReason.NativePdbRequiresDesktop)]


[ConditionalFact(typeof(WindowsDesktopOnly), Reason = ConditionalSkipReason.NativePdbRequiresDesktop)] [](start = 8, length = 102)

Is there a reason to limit the test to Win desktop? This should work on all platforms. #Resolved

@tmat , neighbor tests apply these restrictions. And in the previous iteration, those tests failed in Linux and Mac only: https://dev.azure.com/dnceng/public/_build/results?buildId=383581&view=ms.vss-test-web.build-test-results-tab #Resolved

The C# ones did not fail. The VB ones failed because VB PDB tests don't normalize line endings yet (WithWindowsLineBreaks). #Resolved

tmat · 2019-10-10T19:36:56Z

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

+");
+        }
+
+        [ConditionalFact(typeof(WindowsDesktopOnly), Reason = ConditionalSkipReason.NativePdbRequiresDesktop)]


[ConditionalFact(typeof(WindowsDesktopOnly), Reason = ConditionalSkipReason.NativePdbRequiresDesktop)] [](start = 8, length = 102)

The same here. #Resolved

tmat

tmat · 2019-10-11T03:41:28Z

BreakMode_RudeEdits_DocumentWithoutSequencePoints is still broken.

Please mark that test as skipped for now and file an issue to follow up.

jasonmalinowski · 2019-10-14T19:34:44Z

src/EditorFeatures/Test/EditAndContinue/EditAndContinueWorkspaceServiceTests.cs

@@ -972,7 +972,7 @@ public async Task BreakMode_RudeEdits_DocumentOutOfSync()
            }, _telemetryLog);
        }

-        [Fact]
+        [Fact(Skip = "https://github.com/dotnet/roslyn/issues/39271")]


Is this just because the test was implicitly assuming the old behavior and we need some new way to write the test? (The filed bug doesn't make it clear if we understand the issue here.)

The test was introduced recently #38905 with a workaround for the particular issue. The current PR is a step toward a regular solution but it requires more changes in CommitedSolution.cs started at #38905.

jcouv · 2019-10-14T23:12:10Z

src/Compilers/Core/Portable/NativePdbWriter/PdbWriter.cs

-            foreach (var document in embeddedDocuments)
+            foreach (var kvp in documents
+                .Where(kvp => !_documentIndex.ContainsKey(kvp.Value))
+                .OrderBy(kvp => kvp.Key))


OrderBy [](start = 17, length = 7)

I didn't understand why we need ordering. There are other methods that invoke AddDocumentIndex and don't seem to do so in any particular order (namely GetDocumentIndex).

This is to resolve #39136 (comment)
I think there is some kind of ordering in documents for ones having sequence points likely governed by sequence points ordering. And we want to have some consistence for the order of documents without sequence points. Let them be ordered by file names. @tmat may want to comment more.

Any time we produce output based on dictionary enumeration we need to order the results in order to achieve determinism.

Makes sense. Thanks

jcouv

LGTM Thanks (iteration 9)

Include source files w/o method bodies in the PDB documents

d26e2ef

ivanbasov added the Area-Compilers label Oct 8, 2019

ivanbasov requested review from tmat, jcouv and a team October 8, 2019 22:03

jaredpar reviewed Oct 8, 2019

View reviewed changes

ivanbasov added 2 commits October 9, 2019 14:15

code review feedback

68bb295

updated PDB tests

a4cef18

tmat reviewed Oct 9, 2019

View reviewed changes

tmat requested changes Oct 9, 2019

View reviewed changes

ivanbasov added 3 commits October 9, 2019 19:08

code review feedback

7b98102

code review feedback

a200fc8

skipping windows tests in mac and linux

25ee210

tmat reviewed Oct 10, 2019

View reviewed changes

ivanbasov added 2 commits October 10, 2019 13:31

minor refactoring

c08e2a2

revert skipping C# tests in linus and mac

eaab9f3

tmat approved these changes Oct 11, 2019

View reviewed changes

ivanbasov mentioned this pull request Oct 14, 2019

EnC does not show diagnostic when editing files without method bodies #39271

Closed

disabling skipped test

28c8b82

ivanbasov requested a review from a team as a code owner October 14, 2019 18:44

jasonmalinowski reviewed Oct 14, 2019

View reviewed changes

ivanbasov merged commit fce41a7 into dotnet:master Oct 14, 2019

ivanbasov deleted the PDB branch October 14, 2019 20:17

jcouv self-assigned this Oct 14, 2019

jcouv added this to the 16.4.P3 milestone Oct 14, 2019

jcouv reviewed Oct 14, 2019

View reviewed changes

tmat mentioned this pull request Oct 15, 2019

Rely on PDB document table for identification of design-time-only source files #39295

Merged

shiftkey mentioned this pull request Feb 2, 2020

Documents node in embedded PDB different in .NET Core 3 shiftkey/dotnetcore-sourcelink-test-bug#1

Open

rainersigwald mentioned this pull request Feb 3, 2020

Write generated AssemblyAttributes file to intermediate output path dotnet/msbuild#5101

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include source files w/o method bodies in the PDB documents #39136

Include source files w/o method bodies in the PDB documents #39136

ivanbasov commented Oct 8, 2019

jaredpar Oct 8, 2019 •

edited by ivanbasov

Loading

tmat Oct 9, 2019 •

edited by ivanbasov

Loading

ivanbasov commented Oct 9, 2019 •

edited

Loading

tmat Oct 9, 2019 •

edited by ivanbasov

Loading

tmat commented Oct 9, 2019 •

edited by ivanbasov

Loading

tmat Oct 9, 2019 •

edited by ivanbasov

Loading

tmat Oct 9, 2019

ivanbasov Oct 10, 2019

tmat Oct 10, 2019 •

edited by ivanbasov

Loading

ivanbasov Oct 10, 2019

ivanbasov Oct 10, 2019

tmat Oct 9, 2019 •

edited by ivanbasov

Loading

tmat left a comment

tmat Oct 10, 2019 •

edited by ivanbasov

Loading

tmat Oct 10, 2019 •

edited by ivanbasov

Loading

ivanbasov Oct 10, 2019 •

edited

Loading

tmat Oct 10, 2019 •

edited by ivanbasov

Loading

tmat Oct 10, 2019 •

edited by ivanbasov

Loading

tmat left a comment

tmat commented Oct 11, 2019

jasonmalinowski Oct 14, 2019

ivanbasov Oct 14, 2019

jcouv Oct 14, 2019

ivanbasov Oct 14, 2019

tmat Oct 14, 2019

jcouv Oct 15, 2019

jcouv left a comment

		@@ -104,6 +104,7 @@ internal static class PeWriter
		}

		nativePdbWriterOpt.WriteRemainingEmbeddedDocuments(mdWriter.Module.DebugDocumentsBuilder.EmbeddedDocuments);

Include source files w/o method bodies in the PDB documents #39136

Include source files w/o method bodies in the PDB documents #39136

Conversation

ivanbasov commented Oct 8, 2019

jaredpar Oct 8, 2019 • edited by ivanbasov Loading

Choose a reason for hiding this comment

tmat Oct 9, 2019 • edited by ivanbasov Loading

Choose a reason for hiding this comment

ivanbasov commented Oct 9, 2019 • edited Loading

tmat Oct 9, 2019 • edited by ivanbasov Loading

Choose a reason for hiding this comment

tmat commented Oct 9, 2019 • edited by ivanbasov Loading

tmat Oct 9, 2019 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Oct 10, 2019 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Oct 9, 2019 • edited by ivanbasov Loading

Choose a reason for hiding this comment

tmat left a comment

Choose a reason for hiding this comment

tmat Oct 10, 2019 • edited by ivanbasov Loading

Choose a reason for hiding this comment

tmat Oct 10, 2019 • edited by ivanbasov Loading

Choose a reason for hiding this comment

ivanbasov Oct 10, 2019 • edited Loading

Choose a reason for hiding this comment

tmat Oct 10, 2019 • edited by ivanbasov Loading

Choose a reason for hiding this comment

tmat Oct 10, 2019 • edited by ivanbasov Loading

Choose a reason for hiding this comment

tmat left a comment

Choose a reason for hiding this comment

tmat commented Oct 11, 2019

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

jcouv left a comment

Choose a reason for hiding this comment

jaredpar Oct 8, 2019 •

edited by ivanbasov

Loading

tmat Oct 9, 2019 •

edited by ivanbasov

Loading

ivanbasov commented Oct 9, 2019 •

edited

Loading

tmat Oct 9, 2019 •

edited by ivanbasov

Loading

tmat commented Oct 9, 2019 •

edited by ivanbasov

Loading

tmat Oct 9, 2019 •

edited by ivanbasov

Loading

tmat Oct 10, 2019 •

edited by ivanbasov

Loading

tmat Oct 9, 2019 •

edited by ivanbasov

Loading

tmat Oct 10, 2019 •

edited by ivanbasov

Loading

tmat Oct 10, 2019 •

edited by ivanbasov

Loading

ivanbasov Oct 10, 2019 •

edited

Loading

tmat Oct 10, 2019 •

edited by ivanbasov

Loading

tmat Oct 10, 2019 •

edited by ivanbasov

Loading