Implement Support for portable pdbs #999
Conversation
I don't know the differences between full and coreclr pdbs, if full pdbs aren't usable outside of windows, why would we want the coreclr compiler to support emitting them even on windows? I guess it would help the debugging story on windows, maybe you can give few hints on what are the implication / benefits of supporting full pdb in this case. I'm just thinking to start out, supporting the compiler's target type of pdb should be good enough first target, but I'm probably very oblivious of the myriad of considerations against this first stance. Please give us more insight on matter of the choice! |
src/absil/ilwrite.fs
Outdated
| // From ECMA for UserStrings: | ||
| // This final byte holds the value 1 if and only if any UTF16 character within the string has any bit set in its top byte, or its low byte is any of the following: | ||
| // 0x01–0x08, 0x0E–0x1F, 0x27, 0x2D, | ||
| // 0x01�0x08, 0x0E�0x1F, 0x27, 0x2D, |
There was a problem hiding this comment.
probably not intended
|
Made few stylistics comments over a short review that can be discarded if not relevant. Though, it seems there is code going to be duplicated to support both full pdb and portable pdb, I know this comes with a risk while making changes to either of supported pdb formats, but on the other hand, extending the support for both formats would will be easier if the top level structure is more apparent and parts of implementation that needs to stay identical are properly factorized. |
|
CC @tmat |
| let count s c = s |> Seq.filter(fun ch -> if c = ch then true else false) |> Seq.length | ||
|
|
||
| let s1, s2 = '/', '\\' | ||
| let separator = if (count name s1) >= (count name s2) then s1 else s2 |
There was a problem hiding this comment.
let separator = System.IO.Path.DirectorySeparatorChar
| docs.[i].File | ||
|
|
||
| let getDocumentHandle d = | ||
| if docs.Length = 0 || d < 0 || d > docs.Length then |
There was a problem hiding this comment.
if d < 0 || d >= docs.Length then or remove the if entirely.
|
|
||
| <!-- Produce portable pdbs --> | ||
| <PropertyGroup Condition="'$(Configuration)'!='Proto'"> | ||
| <OtherFlags>$(OtherFlags) --debug:portable</OtherFlags> |
There was a problem hiding this comment.
I'm wondering if it should be possible to produce multiple different debug formats in a single compilation. It seems that would be very useful in build-server situations for library packages. Have you considered this? Thanks
There was a problem hiding this comment.
It is not possible. PE file can only reference a single PDB. Once implemented our symbol server publishing tool will however be able to publish both Windows and Portable PDB into the symbol store so that consumer (debuggers) can fetch the most appropriate format for their platform. To publish both PDBs you'd compile your project with Windows PDB (on Windows machine). The symbol publishing tool will have an option to translate Windows PDB to Portable PDB and upload both.
src/absil/il.fs
Outdated
| [ILAttribElem.Int32( | ||
| (* See System.Diagnostics.DebuggableAttribute.DebuggingModes *) | ||
| (if jitTracking then 1 else 0) ||| | ||
| (0) ||| |
There was a problem hiding this comment.
Just remove this line :) However I have to ask: would it reduce risk to leave this in, just in case some tool (e.g. Mono?) pays attention to this?
There was a problem hiding this comment.
Doh!!!! I couldn't see the wood for the trees.
|
This looks great! My main comment is this one: #999 Thanks |
Implement Support for portable pdbs
7 participants
This PR implements support for portable pdbs.
I am still trying to decide how to support full pdbs on the coreclr compiler, originally I thought that we would desktop PDBs from the coreclr compiler on Linux and osx, even though the pdbs would only be useful on the windows desktop. However the format for desktop pdbs is pretty complex and not well documented. And so I may move to a model where the coreclr compiler only supports producing full pdbs on the windows desktop.
From CLR v2.0 the CLR ignores jittracking field of DebuggableAttribute. there is a bug on CORECLR when this attribute is set to true, internals visible doesn't work. The CoreClr bug fix will be to also ignore it, so, I have also removed the compiler control over this setting.
There is some additional clean up with namespaces and related PDB functions.
I value your thoughts on this.
The PR also includes a number of minor changes, to enable the coreclr build to work on a machine with VS2015 and VS v.Next.
@Microsoft/fsharp-compiler
Kevin