Fix failure when crossgen (*.ni.pdbs) and portable pdbs exists in the debug directory #104
Conversation
… debug directory Start withe the last debug directory entry instead of the first. Also bumped the System.Reflection.Metadata version to 1.5.0 to work properly when running under msbuild on desktop. Otherwise it doesn't work for the DotNet-SymStore repo uploader build task.
| @@ -83,7 +83,7 @@ public static ImmutableArray<string> GetImportStrings(ISymUnmanagedReader reader | |||
|
|
|||
| public static bool TryReadPdbId(PEReader peReader, out BlobContentId id, out int age) | |||
| { | |||
| var codeViewEntry = peReader.ReadDebugDirectory().FirstOrDefault(entry => entry.Type == DebugDirectoryEntryType.CodeView); | |||
| var codeViewEntry = peReader.ReadDebugDirectory().LastOrDefault(entry => entry.Type == DebugDirectoryEntryType.CodeView); | |||
There was a problem hiding this comment.
Mike for completeness there is another place you should update from FirstOrDefault to LastOrDefault
There is similar logic in GetPdbPathFromCodeViewEntry in the Pdb2Pdb.cs file (used when infering the PDB file name from the DLL).
|
Fixed it. Thanks.
|
|
Note that for others who may not be familiar with the semantics here. The DebugDirectory is a list of PDBs that are associated with the DLL (it is how you find the PDB on the symbol server). Most DLLs have only one entry, but .NET Native images have two (one for the IL code, one for the native code). All debugger to date always choose the LAST entry (which is the IL entry). This change is simply making the converter follow the same conventions as every other tool (if you only care about the IL PDB, choose the last entry, which will be the IL PDB). |
|
Some feedback:
|
|
I’m not sure how to disallow multiple portable CodeView records. Crossgen adds the *.ni.pdb entry at the first one and just copies the rest. The compiler (I think) would be the only one that would/could add multiple.
Do you mean validate and fail in the converter code if there is more than one portable pdb entry?
|
Yes. The converter is also a verifier (think of PEVerify for PDBs). This doesn't need to be "fatal" error, we can go on and take the last entry, but it should report diagnostic that something's wrong with the PDB. |
|
>I think we should disallow multiple portable CodeView records. Is there a way to distinguish record for native PDB vs. record for managed PDB in the native image? If we can I'd also disallow multiple managed PDB records (or perhaps even multiple native PDB records).
There really isn’t a deterministic way to distinguish between native PDB and managed PDB debug entries other than the “.ni.” in the name. We can only verify that there is only one portable pdb entry using the MinorVersion, but TryReadPdbId doesn’t look for portable pdb entries specifically and is used for converting in both directions so it can’t look for it.
I really don’t have a lot of time to do all the things you want. I’ll try to come up with a test and document this in the corefx repo.
|
That's fine. We don't need to block this PR on them, other then adding the test. Let's file issues for them and address them later. |
|
One more question about uploading the result package(s). When I create the packages with -pack they have a version of 1.1.0-dev (i.e. Microsoft.DiaSymReader.Converter.1.1.0-dev.nupkg) and the latest on the build tools feed is “1.1.0-beta1-62225-01". What version should the new package be?
|
|
@mikem8361 You should not upload any nuget packages. They are produced and published by our Microbuild build definition. |
|
👍 |
|
Thanks! |
|
@tmat can you merge this PR? I don't have write access. |
|
New official build will be published shortly on myget |
|
Microsoft.DiaSymReader.Converter.1.1.0-beta1-62407-01.nupkg |
|
Filed #105 |
Start with the last debug directory entry instead of the first.
Also bumped the System.Reflection.Metadata version to 1.5.0 to work properly when running under msbuild on desktop. Otherwise it doesn't work for the DotNet-SymStore repo uploader build task.