Skip to content
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

WIP: fix PerfView to look up Crossgen2 PDBs next to the DLLs #1973

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

trylek
Copy link
Member

@trylek trylek commented Jan 12, 2024

Based on David Wrighton's suggestion I have investigated why PerfView doesn't seem to find the .ni.pdb files next do the appropriate dlls in manual build mode. According to my reading of the code this logic is simply not implemented in HandleNetCorePdbs, this simple change fixes that and lets me locally see that PerfView is now capable of finding the .ni.pdb file.

The purpose of the PR is to discuss whether this is the logic we want for finding Crossgen2 PDBs or what is the right thing to do here. I vaguely recall that PE executables have a way of storing the "original PDB path" referring to the path where the PDB was put at the time of building the executable but AFAIK Crossgen2 currently doesn't support emitting this information.

If the "original PDB path" was the right way to go, we'd need to make a Crossgen2 fix that would store the name of the .ni.pdb file somewhere in the file plus the appropriate PerfView fix for looking up the record but I'm not sure if I'm the right person to do that as we agreed with Manish last summer that for .NET 9 I should switch over from Crossgen2 ownership to other work assignments.

Thanks

Tomas

/cc @brianrob @davidwrighton @mangod9
(David said I should also tag Ashok Kamath from the VS to comment on this but I cannot find him on GitHub, I'll ping him about this PR on our VS/.NET sync thread.)

Based on David Wrighton's suggestion I have investigated why
PerfView doesn't seem to find the .ni.pdb files next do the
appropriate dlls in manual build mode. According to my reading
of the code this logic is simply not implemented in
HandleNetCorePdbs, this simple change fixes that and lets me
locally see that PerfView is now capable of finding the .ni.pdb
file.

The purpose of the PR is to discuss whether this is the logic
we want for finding Crossgen2 PDBs or what is the right thing
to do here. I vaguely recall that PE executables have a way of
storing the "original PDB path" referring to the path where the
PDB was put at the time of building the executable but AFAIK
Crossgen2 currently doesn't support emitting this information
right now.

If the "original PDB path" was the right way to go, we'd need
to make a Crossgen2 fix that would store the name of the .ni.pdb
file somewhere in the file and I'm not sure if I'm the right
person to do that as we agreed with Manish last summer that for
.NET 9 I should switch over from Crossgen2 ownership to other
work assignments.

Thanks

Tomas
@pharring
Copy link
Member

pharring commented Jan 12, 2024

Funnily enough, I was looking at this myself just yesterday.

In the PE header for a crossgen'd binary, you'll find two "cv" entries in the debug directories. The first one is for the .ni.pdb. The second is for the original PDB. (At least, that's my observation. I don't know if it's guaranteed).

In a perf trace, the Loader/ModuleDCStop event has both PDB signatures (the native PDB and the managed PDB).

@kamathac
Copy link

My understanding of PDB look up is this - A PDB matching the name, GUID and Age mentioned in the image header is looked up along the symbol search path. Matching based just on name can be incorrect/unsafe.

The problems with crossgen2 + Perfview:

  • The GUID of the PDB generated and the GUID mentioned in the crossgened image header do not match. I think this issue is a critical issue, is confirmed and there is a separate workitem to address that in crossgen?
  • Secondly, since the crossgened binary has 2 PDBs mentioned, consumers have to more discriminating in terms of what they look for. Perfview will have to look up specifically the one with ".ni.pdb", find its GUID/Age and then perform the lookup. If perfview isnt doing this, that'll need to happen.

If a tool wants to show symbols AND also source code information, it will need to read both the NI PDB (for symbol to address mapping) and original PDB (symbol to source code mapping).

@trylek
Copy link
Member Author

trylek commented Jan 12, 2024

@kamathac - I was under the impression that Crossgen2 should already store the GUID matching the .ni.pdb in the executable, in fact this investigation started after VS people noticed they no longer match due to the bug I fixed in

dotnet/runtime#96518

It may be the case that my PerfView fix in this WIP PR is incomplete in this respect, I saw some logic for GUID matching but I'm not 100% sure it gets triggered on this particular code path.

@brianrob
Copy link
Member

@trylek thanks for submitting this. This would be a great addition to PerfView. A few thoughts:

  1. Agreed that your change does not check that the PDB signature against the expected one in the binary. This is something that does need to be added.
  2. I think it's reasonable to move the call to HandleNetCorePdbs from where it currently is called to here: https://github.com/microsoft/perfview/blob/main/src/TraceEvent/Symbols/SymbolReader.cs#L121C95-L121C95. Line 121 checks to see if we should create NGEN PDBs, but this really doesn't fall into that bucket - instead we're just checking to see if there is a local PDB that matches. This logic should run regardless of if NGEN PDB generation is enabled.
  3. I'd also be happy to have your change remove the code to call crossgen to generate the PDB. This isn't something that we've needed to do for several releases.

Let me know if you have any questions - happy to help here.

@kamathac
Copy link

To clarify, you are saying dotnet/runtime#96518 will fix the mismatched PDB, correct? That fix should go to .net 8 and looks like it is going to.

About the second issue mentioned above, Perfview does seem to have knowledge about potentially two PDBs in the header and the convention that NI PDB will be the first one, so it specifically looks for the first one (https://github.com/microsoft/perfview/blob/de7d273ec68215319bc56c677d2761ca74110b2c/src/TraceEvent/Symbols/SymbolReader.cs#L96C23-L96C50). After dotnet/runtime#96518 is fixed, have we confirmed if there indeed is a problem with symbol resolution of crossgened images in perfview?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants