Implements support for Embedded Portable PDB #10199
Conversation
| /// | ||
| /// Data spans the remainder of the blob. | ||
| /// </remarks> | ||
| EmbeddedPortablePdb = 17, |
There was a problem hiding this comment.
Need to check if the value 17 is available.
There was a problem hiding this comment.
Also fix the comment, it's obsolete.
|
@noahfalk @gregg-miskelly @caslan This is the implementation of the reader and writer. Once this is in, Roslyn will call the new APIs to emit the embedded PDBs. |
|
@marek-safar @jbevain Heads up. |
|
|
||
| > Note: Including both entries enables a tool that does not recognize Embedded Portable PDB entry to locate debug infomration as long as it is also available in a file specified in CodeView entry. Such file can be created by extracting the embedded Portable PDB image to a separate file. | ||
|
|
||
| The Major version indicates the version of the Portable PDB format itself. The Minor version indicates the version of the Embedded Portable PDB data format. |
There was a problem hiding this comment.
Shouldn't the portable PDB have a self-contained version header and wouldn't need supplementary versioning information to parse it? The debugger won't have this supplementary versioning information when the Portable PDB is a loose file and presumably everything still works equally well.
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm curious why the MajorVersion would need to replicate the pdb's version?
There was a problem hiding this comment.
Doesn't need to, but it doesn't harm to put the version there. Mostly for consistency with the CodeView Portable PDB entry. The minor benefit is that the reader can see if they support the version without decompressing the data.
There was a problem hiding this comment.
I'm a little confused. The CodeView debug data entry spec'ed above for PortablePDB defines a MajorVersion=0x0100. That text didn't suggest to me that the version will change to track the version of the PortablePDB being pointed at. Is it intended to do so?
I'm not opposed to the plan, just trying to make the pieces fit : )
There was a problem hiding this comment.
Good catch - it should be stated there. It is the intention that the MajorVersion is the Portable PDB format version.
| | Offset | Size | Field | Description | | ||
| |:-------|:---------------|:-----------------|-------------------------------------------------------| | ||
| | 0 | 4 | UncompressedSize | The size of decompressed Portable PDB image | | ||
| | 4 | SizeOfData - 4 | PortablePdbImage | Portable PDB image compressed using Deflate algorithm | |
There was a problem hiding this comment.
Does the Deflate algorithm have some sort of magic number at the start of it? Since I don't know of any good central repository for the IMAGE_DEBUG_TYPE_* constants, it would be good if we had a magic number that we could check to verify this really is an embedded portable PDB and not some custom format that we don't understand.
There was a problem hiding this comment.
Good point. Deflate doesn't seem to have magic number. I'll add 4B with some magic.
|
@tmat Would you mind also updating Microsoft.DiaSymReader.PortablePdb's implementation of GetReaderFromStream to accept the bytes from the debug directory? That would save us a day or two's work to hookup our native code to some new managed code to perform the deflate compression. |
|
Otherwise LGTM. |
|
@gregg-miskelly Sure. I'll take a look into that. |
| @@ -29,10 +29,30 @@ The associated .pdb file may not exist at the path indicated by Path field. If i | |||
|
|
|||
| If the containing PE/COFF file is deterministic the Guid field above and DateTimeStamp field of the directory entry are calculated deterministically based solely on the content of the associated .pdb file. Otherwise the value of Guid is random and the value of DateTimeStamp indicates the time and date that the debug data was created. | |||
|
|
|||
| *Version Major=0x0100, Minor=0x504d* of the data format has the same structure as above. The Age shall be 1. The format of the associated .pdb file is Portable PDB. Together 16B of the Guid concatenated with 4B of the TimeDateStamp field of the entry form a PDB ID that should be used to match the PE/COFF image with the associated PDB (instead of Guid and Age). Matching PDB ID is stored in the #Pdb stream of the .pdb file. | |||
| *Version Major=any, Minor=0x504d* of the data format has the same structure as above. The Age shall be 1. The format of the associated .pdb file is Portable PDB. The Major version specified in the entry indicates the version of the Portable PDB format. Together 16B of the Guid concatenated with 4B of the TimeDateStamp field of the entry form a PDB ID that should be used to match the PE/COFF image with the associated PDB (instead of Guid and Age). Matching PDB ID is stored in the #Pdb stream of the .pdb file. | |||
There was a problem hiding this comment.
This doesn't specify how the version of the portable pdb is encoded. I'm assuming 0x0100 was correct for v1.0 and e.g. v1.2 would be 0x0102, correct? I think this should be spelled out because reading this and seeing major=0x0100 would make me think PPDB version=256.
|
👍 (with only minor comments) |
Introduce a new type of Debug Directory Entry which embeds a compressed Portable PDB blob.
See e1dd448#diff-f595b39db5266b534bf46339a9f285efR40 for details.