Enable embedding sources to Windows PDBs #21391

tmat · 2017-08-08T23:06:17Z

Finishes implementation of #12625 for Windows PDBs.

The feature was only completed for Portable PDBs due to bugs in Microsoft.DiaSymReader.Native that are now fixed.

The first couple of commits change the PDB validation to treat order of data as significant. Previously we used AssertXml helper that ignores order.

Resolves #13707.

tmat · 2017-08-08T23:07:52Z

build/Targets/Packages.props

-    <MicrosoftDiaSymReaderPortablePdbVersion>1.3.0</MicrosoftDiaSymReaderPortablePdbVersion>
+    <MicrosoftDiaSymReaderVersion>1.2.0-beta1-62008-01</MicrosoftDiaSymReaderVersion>
+    <MicrosoftDiaSymReaderConverterXmlVersion>1.1.0-beta1-62008-01</MicrosoftDiaSymReaderConverterXmlVersion>
+    <MicrosoftDiaSymReaderNativeVersion>1.7.0-private-25604</MicrosoftDiaSymReaderNativeVersion>


I'll replace this with official bits once they are available.

Turns out VCTools are not being inserted into VS 15.5 yet. There won't be any official builds until next week. As long as we don't insert the private bits into VS (#21420 removes DSRN from insertion) it is ok to use them.

Is this the version of the native PDB writer that supports full determinism?

Yes, that change is in the branch I built from.

Can you pull in the powershell change from this PR?

https://github.com/dotnet/roslyn/pull/20527/files

That should pass now and keep us clean.

Actually, let me do that in a separate PR. Determinism is not really related to this change.

Gotcha. Don't worry about a second PR then. I'll just rebase mine against these changes once they're merged.

OK. Have you seen my email to YongKang about PDBs not being deterministic when writing twice within the same process? Might affect compiler server.

Yeah I saw that. It didn't seem to affect our tests for some reason. Maybe just got lucky. But definitely need to get that fixed before we declare it "done".

tmat · 2017-08-08T23:20:34Z

@richlander FYI

AssertXml doesn't validate ordering.

tmat · 2017-08-09T16:53:30Z

@jaredpar The unit test run failed in RunTests.Cache.ContentUtil:

Roslyn.Services.VisualBasic.UnitTests.dll - running
Command line Roslyn.Services.VisualBasic.UnitTests.dll: C:\Users\dotnet-bot\.nuget\packages\xunit.runner.console\2.2.0-beta4-build3444\tools\xunit.console.x86.exe "D:\j\workspace\windows_debug---09a07fba\Binaries\Debug\UnitTests\VisualBasicServicesTest\Roslyn.Services.VisualBasic.UnitTests.dll"  -xml "D:\j\workspace\windows_debug---09a07fba\Binaries\Debug\UnitTests\VisualBasicServicesTest\xUnitResults\Roslyn.Services.VisualBasic.UnitTests.dll.xml" -noshadow -verbose
Source data for $Roslyn.Services.VisualBasic.UnitTests.dll: {
  "MachineName": "win2016-f153b4",
  "EnlistmentRoot": "D:\\j\\workspace\\windows_debug---09a07fba",
  "AssemblyName": "Roslyn.Services.VisualBasic.UnitTests.dll",
  "Source": null,
  "IsJenkins": true,
  "CommitSha": "b6756c36862a548bb18f10aef3a2788c5ef1a096",
  "MergeCommitSha": "a21dd66b36a59502d3adf1dffd2fdcf6f8218511",
  "Repository": "dotnet/roslyn",
  "IsPullRequest": true,
  "PullRequestId": 21391,
  "PullRequestUserName": "tmat"
}
Error At least one object must implement IComparable.: Unable to calculate content file for D:\j\workspace\windows_debug---09a07fba\Binaries\Debug\UnitTests\CSharpVisualStudioTest\Roslyn.VisualStudio.CSharp.UnitTests.dll
At least one object must implement IComparable.
   at System.Collections.Comparer.Compare(Object a, Object b)
   at System.Linq.EnumerableSorter`2.CompareKeys(Int32 index1, Int32 index2)
   at System.Linq.EnumerableSorter`1.QuickSort(Int32[] map, Int32 left, Int32 right)
   at System.Linq.EnumerableSorter`1.Sort(TElement[] elements, Int32 count)
   at System.Linq.OrderedEnumerable`1.<GetEnumerator>d__1.MoveNext()
   at RunTests.Cache.ContentUtil.GetReferenceSectionCore(String unitTestAssemblyPath) in q:\roslyn\src\Tools\Source\RunTests\Cache\ContentUtil.cs:line 174
   at RunTests.Cache.ContentUtil.AppendReferences(StringBuilder builder, String unitTestAssemblyPath) in q:\roslyn\src\Tools\Source\RunTests\Cache\ContentUtil.cs:line 79
   at RunTests.Cache.ContentUtil.BuildTestResultContent(AssemblyInfo assemblyInfo) in q:\roslyn\src\Tools\Source\RunTests\Cache\ContentUtil.cs:line 63
   at RunTests.Cache.ContentUtil.GetTestResultContentFile(AssemblyInfo assemblyInfo) in q:\roslyn\src\Tools\Source\RunTests\Cache\ContentUtil.cs:line 43
   at RunTests.Cache.CachingTestExecutor.<RunTestAsync>d__7.MoveNext() in q:\roslyn\src\Tools\Source\RunTests\Cache\CachingTestExecutor.cs:line 36

https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_debug_unit32_prtest/8785/artifact/Binaries/Debug/runtests.log

tmat · 2017-08-09T16:55:22Z

test windows_debug_unit32_prtest please

jaredpar · 2017-08-09T17:29:15Z

That's bizzare. It's not failing with that anywhere else and I don't see anything in yourchange that would cause that

nguerrera · 2017-08-09T18:20:52Z

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

+        /// </summary>
+        /// <remarks>
+        /// This is done after serializing method debug info to ensure that we embed all requested
+        /// text even if there are no correspodning sequence points.


Typo: corresponding. (This was me, sorry.)

tannergooding · 2017-08-09T18:24:07Z

@tmat, will this require any coordination when inserted? @brettfo for possible FYI.

tmat · 2017-08-09T20:05:21Z

@tannergooding No insertion coordination is needed.

cston · 2017-08-10T03:33:54Z

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

+                    break;
+                case "/debug:full":
+                    ValidateEmbeddedSources_Windows(expectedEmbeddedMap, dir);
+                    break;


Is "/debug:full" tested?

cston · 2017-08-10T03:37:34Z

src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb

+                Next
+            Finally
+                symReader?.Dispose()
+            End Try
        End Sub


Can these helper methods be shared between C# and VB, perhaps in PdbTestUtilities?

cston · 2017-08-10T03:50:34Z

LGTM

tmat · 2017-08-11T00:26:11Z

@dotnet/roslyn-compiler Can I have a second compiler sign off please?

…throw-statement-expression * dotnet/features/ioperation: (41 commits) Update CSharpReplaceMethodWithPropertyService.cs Add VB side of fix. Remove unneeded function. Improve trivia preservation when converting methods into a property. VB side of do-not-simplify-nameof if it changes semantics. Do not simplify to an alias in a nameof if it changes the value of hte nameof. Include System.Runtime.Serialization.Primitives and System.Security.Cryptography.Csp in PortableFacades CoreXT package. (dotnet#21438) Address one more refactoring feedback Address PR feedback Fix possible race conditions in TestExtensionErrorHandler Fix expected test results to properly consider trivia Improve messages when tests fail due to expected text Default to considering trivia during testing Remove RemoveUnneededReferences from LamdaRewriter (dotnet#21367) Enable embedding sources to Windows PDBs (dotnet#21391) re-enabled assert we have disabled Do not insert Microsoft.DiaSymReader.Native (dotnet#21420) Recommend 'case' keyword after a pattern-case-clause. Fix NamedArgumentInParameterOrderWithDefaultValue test for new IOperation output. Resolving merge conflict ...

…-literal-text * dotnet/features/ioperation: Update CSharpReplaceMethodWithPropertyService.cs Add VB side of fix. Remove unneeded function. Improve trivia preservation when converting methods into a property. VB side of do-not-simplify-nameof if it changes semantics. Do not simplify to an alias in a nameof if it changes the value of hte nameof. Include System.Runtime.Serialization.Primitives and System.Security.Cryptography.Csp in PortableFacades CoreXT package. (dotnet#21438) Address one more refactoring feedback Address PR feedback Fix possible race conditions in TestExtensionErrorHandler Fix expected test results to properly consider trivia Improve messages when tests fail due to expected text Default to considering trivia during testing Remove RemoveUnneededReferences from LamdaRewriter (dotnet#21367) Enable embedding sources to Windows PDBs (dotnet#21391) re-enabled assert we have disabled Do not insert Microsoft.DiaSymReader.Native (dotnet#21420) Resolving merge conflict Don't pick a project arbitrarily when navigating to symbols

…tatement-refactor * dotnet/features/ioperation: Update CSharpReplaceMethodWithPropertyService.cs Add VB side of fix. Remove unneeded function. Improve trivia preservation when converting methods into a property. VB side of do-not-simplify-nameof if it changes semantics. Do not simplify to an alias in a nameof if it changes the value of hte nameof. Include System.Runtime.Serialization.Primitives and System.Security.Cryptography.Csp in PortableFacades CoreXT package. (dotnet#21438) Address one more refactoring feedback Address PR feedback Fix possible race conditions in TestExtensionErrorHandler Fix expected test results to properly consider trivia Improve messages when tests fail due to expected text Default to considering trivia during testing Remove RemoveUnneededReferences from LamdaRewriter (dotnet#21367) Enable embedding sources to Windows PDBs (dotnet#21391) re-enabled assert we have disabled Do not insert Microsoft.DiaSymReader.Native (dotnet#21420) Resolving merge conflict Don't pick a project arbitrarily when navigating to symbols

…oalescingexpression-refactor * dotnet/features/ioperation: Update CSharpReplaceMethodWithPropertyService.cs Add VB side of fix. Remove unneeded function. Improve trivia preservation when converting methods into a property. VB side of do-not-simplify-nameof if it changes semantics. Do not simplify to an alias in a nameof if it changes the value of hte nameof. Include System.Runtime.Serialization.Primitives and System.Security.Cryptography.Csp in PortableFacades CoreXT package. (dotnet#21438) Address one more refactoring feedback Address PR feedback Fix possible race conditions in TestExtensionErrorHandler Fix expected test results to properly consider trivia Improve messages when tests fail due to expected text Default to considering trivia during testing Remove RemoveUnneededReferences from LamdaRewriter (dotnet#21367) Enable embedding sources to Windows PDBs (dotnet#21391) re-enabled assert we have disabled Do not insert Microsoft.DiaSymReader.Native (dotnet#21420) Resolving merge conflict Don't pick a project arbitrarily when navigating to symbols

dnfclas added the cla-already-signed label Aug 8, 2017

tmat requested review from nguerrera and a team August 8, 2017 23:06

tmat added this to the 15.5 milestone Aug 8, 2017

tmat added the Area-Compilers label Aug 8, 2017

tmat commented Aug 8, 2017

View reviewed changes

tmat force-pushed the EmbeddedSourcesWindowsPdb branch from 9f3d1e2 to 1fdb83e Compare August 8, 2017 23:24

nguerrera and others added 4 commits August 8, 2017 18:14

Compare lines instead of using AssertXml when validating PDBs.

c9dc666

AssertXml doesn't validate ordering.

Reorder expected PDB XML nodes to match actual order

1f8565f

Update to the latest Microsoft.DiaSymReader.* packages.

7c59af2

Enable embedding sources to Windows PDBs

b6756c3

tmat force-pushed the EmbeddedSourcesWindowsPdb branch from 1fdb83e to b6756c3 Compare August 9, 2017 01:15

nguerrera approved these changes Aug 9, 2017

View reviewed changes

cston reviewed Aug 10, 2017

View reviewed changes

jaredpar approved these changes Aug 11, 2017

View reviewed changes

tmat merged commit 1c1fbc6 into dotnet:master Aug 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable embedding sources to Windows PDBs #21391

Enable embedding sources to Windows PDBs #21391

tmat commented Aug 8, 2017 •

edited

Loading

tmat Aug 8, 2017

tmat Aug 10, 2017

jaredpar Aug 10, 2017

tmat Aug 10, 2017

jaredpar Aug 10, 2017

tmat Aug 10, 2017

tmat Aug 10, 2017

jaredpar Aug 10, 2017

tmat Aug 10, 2017

jaredpar Aug 10, 2017

tmat commented Aug 8, 2017

tmat commented Aug 9, 2017 •

edited

Loading

tmat commented Aug 9, 2017

jaredpar commented Aug 9, 2017

nguerrera Aug 9, 2017

tannergooding commented Aug 9, 2017

tmat commented Aug 9, 2017

cston Aug 10, 2017

cston Aug 10, 2017

cston commented Aug 10, 2017

tmat commented Aug 11, 2017

Enable embedding sources to Windows PDBs #21391

Enable embedding sources to Windows PDBs #21391

Conversation

tmat commented Aug 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat commented Aug 8, 2017

tmat commented Aug 9, 2017 • edited Loading

tmat commented Aug 9, 2017

jaredpar commented Aug 9, 2017

Choose a reason for hiding this comment

tannergooding commented Aug 9, 2017

tmat commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented Aug 10, 2017

tmat commented Aug 11, 2017

tmat commented Aug 8, 2017 •

edited

Loading

tmat commented Aug 9, 2017 •

edited

Loading