-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Initial PDB / PerfMap support in Crossgen2 + System.Private.CoreLib switchover to use Crossgen2 #47019
Conversation
/cc @dotnet/crossgen-contrib |
src/coreclr/tools/aot/ILCompiler.ReadyToRun/ObjectWriter/MapFileBuilder.cs
Outdated
Show resolved
Hide resolved
@@ -205,6 +226,20 @@ public void SaveCsv(string nodeStatsCsvFileName, string mapCsvFileName) | |||
} | |||
} | |||
|
|||
public void SavePdb(string pdbPath, string dllFileName) |
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.
It feels weird having MapFileBuilder
be the thing that writes PDBs and Perf maps since its purpose is to write a text summary file at the end of compilation. The map file builder's data structures at this point are a nice model of node to file location which makes it handy for these purposes. Maybe we could split the concepts into a class containing mapping details (shareable by map, perfmap, pdb generation)?
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 in 9th commit - I have split the logic into an "output file info collector" (OutputInfoBuilder) and the actual emitters using it - MapFileBuilder and SymbolFileBuilder, thanks for the suggestion.
Appears there are still some lingering issues on x86? |
I have moved the PDB writer code to a new assembly ILCompiler.Diagnostics so that it can be reused by Crossgen2. I have also added an initial trivial implementation of the PerfMap writer. Thanks Tomas
Based on Simon's suggestion I moved the PDB / PerfMap-specific code to a new file SymbolFileBuilder as I concur that its squatting in the MapFileBuilder was somewhat hacky. Both classes use a new helper class ObjectInfoBuilder that collects the information common to both - I was reluctant to make it a base class of the *FileBuilder's as we'd have to collect all the elements twice. Thanks Tomas
I have addressed initial Manish's and Simon's PR feedback and the change finally passes PR testing. I have launched an outerloop run and I believe the change is ready to be merged in. Please take another look when you have a chance and let me know what additional comments need addressing before this goes in. |
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 looks awesome to me 😎 Thanks Tomas!
(*) Move PDB writer support out of R2RDump to a new assembly ILCompiler.Diagnostics;
(*) Add initial PerfMap writer support parallel to PDB emitter;
(*) Add Crossgen2 support for emitting PDB / PerfMap using the ILCompiler.Diagnostics assembly;
(*) Add support for PDB generation to R2RTest;
(*) Add support for optional PDB generation to the src\tests\build.cmd script (PDB is only supported on Windows).
Thanks
Tomas