-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement SourceLink for Windows PDBs #18539
Conversation
@ctaggart FYI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent.
@tmat Nice! I was wondering if ya'll were going to pursue this. Any ideas how to detect support when this ships? Currently I look for DebugType of portable or embedded. Should I look for the msbuild version, roslyn version, or something else? <CompileDependsOn Condition="'$(SourceLinkCreate)' == 'true' and ($(DebugType) == 'portable' or $(DebugType) == 'embedded')">SourceLinkCreate;$(CompileDependsOn)</CompileDependsOn> |
@ctaggart I don't think you need to check for anything. If one uses some version of SourceLink that doesn't have that check they should also use a compiler that supports it. If not an error will be reported by the compiler. So just mentioning it in SourceLink docs should be sufficient imo. |
4d5dc67
to
b50fbcd
Compare
Requires changes in #18803. |
@dotnet/roslyn-compiler Please review. Still need to update the dependencies to pass tests, but the code is ready for review (and tests pass on my box). |
@@ -31,6 +31,8 @@ | |||
|
|||
using static Roslyn.Test.Utilities.SharedResourceHelpers; | |||
using static Microsoft.CodeAnalysis.CommonDiagnosticAnalyzers; | |||
using Roslyn.Test.PdbUtilities; | |||
using Microsoft.DiaSymReader; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort usings #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. #Resolved
var stream = new TestStream(canSeek: false, backingStream: new MemoryStream(sourceArray)); | ||
stream.ReadByte(); | ||
Assert.Equal(new byte[] { 2, 3, 4 }, stream.ReadAllBytes()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test verifying behavior when stream is at the end. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. #Closed
@@ -23,7 +23,7 @@ public static class SymReaderFactory | |||
[DllImport("Microsoft.DiaSymReader.Native.amd64.dll", EntryPoint = "CreateSymReader")] | |||
private extern static void CreateSymReader64(ref Guid id, [MarshalAs(UnmanagedType.IUnknown)]out object symReader); | |||
|
|||
private static ISymUnmanagedReader3 CreateNativeSymReader(Stream pdbStream, object metadataImporter) | |||
private static ISymUnmanagedReader5 CreateNativeSymReader(Stream pdbStream, object metadataImporter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the past there were cases where the compiler was installed without the specific version of native diasymreader that we needed. That meant it could be older. Is that the case anymore? If so do we have to worry about cases where ISymUnmanagedReader5 is not avialable? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compiler doesn't use ISymUnmanagedReader, only ISymUnmanagedWriter. This helper only loads SymReader from DSRN where it always is the latest version. #ByDesign
fixed (byte* bufferPtr = buffer) | ||
{ | ||
byte b = 0; | ||
symWriter8.SetSourceLinkData((bufferPtr != null) ? bufferPtr : &b, buffer.Length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what case is bufferPtr going to be null? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When buffer is empty. #Closed
return null; | ||
} | ||
|
||
var buffer = new StringBuilder(256); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 256? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arbitrary max path. I'll remove the commit from this PR - I accidentally added it to this branch while investigating why tests fail on Jenkins and not on my box. #Resolved
@@ -118,5 +118,33 @@ public void PrematureEndOfStream() | |||
var expected = new byte[] { 1, 2, 3, 4, 0, 0 }; | |||
Assert.Equal(expected, destArray); | |||
} | |||
|
|||
[Fact] | |||
public void ReadAllBytes_Seakable() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: Seekable
#Resolved
stream.ReadByte(); | ||
stream.ReadByte(); | ||
Assert.Equal(new byte[0], stream.ReadAllBytes()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider testing ReadAllBytes()
when TestStream.Length > backingStream.Length
(the Array.Resize
case). #Resolved
LGTM |
"; | ||
var sourceLinkBlob = Encoding.UTF8.GetBytes(@" | ||
{ | ||
""documents"": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: open brace on new line #WontFix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not C# ;) #WontFix
|
||
var pdbStream = new MemoryStream(); | ||
c.EmitToArray(EmitOptions.Default.WithDebugInformationFormat(format), pdbStream: pdbStream, sourceLinkStream: new MemoryStream(sourceLinkBlob)); | ||
pdbStream.Position = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this position reset necessary? Maybe GetSourceLinkData
should handle the reset, if it doesn't already. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
} | ||
"; | ||
var sourceLinkStream = new TestStream(canRead: true, readFunc: (_, __, ___) => { throw new Exception("Error!"); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully, discards will be supported in lambdas soon ;-) #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -1193,12 +1193,9 @@ public new CSharpCommandLineArguments Parse(IEnumerable<string> args, string bas | |||
keyFileSetting = ParseGenericPathToFile(keyFileSetting, diagnostics, baseDirectory); | |||
} | |||
|
|||
if (sourceLink != null) | |||
if (sourceLink != null && !emitPdb) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the VB command-line parser have a similar change? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll also want to update the command-line documentation file: https://github.com/dotnet/roslyn/blob/master/docs/compilers/CSharp/CommandLine.md
(and its VB counter-part if applicable)
Thanks
@jcouv Updated documentation of the command line args for both VB and C#. Please check if it looks good. |
@@ -1,6 +1,6 @@ | |||
{ | |||
"dependencies": { | |||
"Microsoft.DiaSymReader.Native": "1.5.0" | |||
"Microsoft.DiaSymReader.Native": "1.6.0-beta2-25219" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming that we will not ship dev15.3 with a reference to beta package. Can you file a follow-up issue to fix this reference once a release package is available?
It's probably also good to think about timeline to make this happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We produce RTM packages once the final VS RTM build is available. I don't think a work item is needed as this is a common step taken during Roslyn nuget package publishing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should probably clarify. The flow for this package is documented here: https://github.com/dotnet/roslyn-debug/tree/master/src/Microsoft.DiaSymReader.Native. To get RTM binaries we need to wait until VS RTM build is produced by the lab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Sounds like this step is already covered by standard release process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@tmat looks like we have two sign offs but unfortunately this ended up conflicting with my SDK change. Can we rebase this and get it merged soon? Bar for compiler layer starts going up next week. Want to get this in while it still meets the bar. |
@jaredpar Waiting for a fixed build of DSRN. Should be available on Wed. |
d2d3a92
to
6fa0bc7
Compare
test windows_release_vs-integration_prtest please |
Is there a tentative milestone where this will ship? 15.[something]? |
@kzu this will ship in 15.3 |
Should it be in the current preview then? or will it make it to preview 2? |
Enables /sourcelink for Windows PDBs. Previously we only supported it for Portable PDBs.