-
Notifications
You must be signed in to change notification settings - Fork 752
Implement MSFZ symbols format support in SymbolReader #2244
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
Conversation
Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
| m_log.WriteLine("FindSymbolFilePath: In task, sending HTTP request {0}", fullUri); | ||
|
|
||
| var responseTask = HttpClient.GetAsync(fullUri, HttpCompletionOption.ResponseHeadersRead); | ||
| // Create request with MSFZ support |
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.
Let's modify this comment to be "Tell the symbol server that we support MSFZ symbols."
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.
Updated the comment to "Tell the symbol server that we support MSFZ symbols" in commit d95b319.
|
|
||
| using (var stream = File.OpenRead(filePath)) | ||
| { | ||
| if (stream.Length < headerBytes.Length) |
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.
From a stylistic perspective, we use {} around if blocks. Please update the style throughout the PR.
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.
Added braces around all single-line if blocks in the MSFZ implementation in commit d95b319.
| if (stream.Length < headerBytes.Length) | ||
| return false; | ||
|
|
||
| var buffer = new byte[headerBytes.Length]; |
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.
Let's avoid this allocation by using stackalloc.
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.
Replaced the byte array allocation with stackalloc to avoid heap allocation in commit d95b319.
| } | ||
| finally | ||
| { | ||
| if (File.Exists(testFile)) |
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.
Please also make sure that nonMsfzFile is properly deleted.
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.
Moved nonMsfzFile declaration outside try block and ensured it's properly deleted in the finally block in commit d95b319.
|
|
||
| // The file should have been moved to msfz0 subdirectory since it's an MSFZ file | ||
| var msfzPath = Path.Combine(Path.GetDirectoryName(targetPath), "msfz0", Path.GetFileName(targetPath)); | ||
| Assert.True(File.Exists(msfzPath), "MSFZ file should be moved to msfz0 subdirectory"); |
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.
The test is failing on line 487. Please take a look.
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 the core issue causing all tests to fail (PrepareTestData working directory problem) in commit 5ad163a. The specific test was calling the wrong method (GetPhysicalFileFromServer instead of GetFileFromServer). Debugging the HTTP intercepting handler issue now.
…s, fix test cleanup Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
…mpletely Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
…TTP test Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
| [Fact] | ||
| public void NativeCppPdbHasValidSourceInfo() | ||
| { | ||
| PrepareTestData(); |
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.
Please undo the change to move PrepareTestData into each test.
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.
Reverted the PrepareTestData changes back to the constructor in commit e46ee06.
| if (GetPhysicalFileFromServer(urlForServer, fileIndexPath, targetPath, onlyBinaryContent)) | ||
| { | ||
| // Check if the downloaded file is an MSFZ file and move it if so | ||
| if (IsMsfzFile(targetPath)) |
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.
Let's pull the MSFZ-specific paths from GetFileFromServer. Not all callers of GetFileFromServer care about MSFZ. Instead, let's move the MSFZ-specific work into FindSymbolFilePath. In addition, when downloading the file, let's save it to <filename>.new and then either rename it or move it to the msfz0 directory and rename it. The goal here is that if for some reason we can't delete or properly move an msfz0 file, it doesn't continue to live outside of an msfz0 directory where tools that can't handle msfz files might find it.
You may also need to update the tests given this change.
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.
Refactored the MSFZ implementation as requested in commit e46ee06:
- Removed MSFZ-specific logic from
GetFileFromServer - Created
GetFileFromServerWithMsfzSupportmethod that handles MSFZ cache checking and file management - Updated
FindSymbolFilePathto use the new method for symbol servers - Downloads now use
<filename>.newfiles and MSFZ files are moved directly to msfz0 directory - Removed
MoveMsfzFileToSubdirectorymethod since logic is now integrated
MSFZ files now never exist outside msfz0 directories as requested. Working on updating tests to match the new architecture.
…h, use .new files Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
| /// and properly places MSFZ files in msfz0 subdirectory. | ||
| /// </summary> | ||
| private string GetFileFromServerWithMsfzSupport(string urlForServer, string fileIndexPath, string targetPath) | ||
| { |
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.
I was thinking that moving the msfz changes out of GetFileFromServer would be simple, but it is not. Let's go ahead and merge them back into GetFileFromServer so that we don't have to have two paths. Be sure to update the tests.
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.
Merged MSFZ logic back into GetFileFromServer as requested in commit 5a8b0d7. Removed the separate GetFileFromServerWithMsfzSupport method and integrated all MSFZ functionality directly into the main GetFileFromServer method. Updated tests to reflect the unified approach.
| } | ||
|
|
||
| // Read and compare byte by byte to avoid allocation | ||
| for (int i = 0; i < headerBytes.Length; i++) |
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.
Let's do this in one read operation of sizeof(headerBytes).
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.
Updated IsMsfzFile to read the header in one operation using stream.Read() instead of byte-by-byte reading in commit 5a8b0d7.
| var responseTask = HttpClient.GetAsync(fullUri, HttpCompletionOption.ResponseHeadersRead); | ||
| // Tell the symbol server that we support MSFZ symbols | ||
| var request = new HttpRequestMessage(HttpMethod.Get, fullUri); | ||
| request.Headers.Add("Accept", "application/msfz0"); |
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.
When a symbol file is downloaded, there is a callout that can be registered. PerfView registers this callout and implements it in PerfView.App.CacheInLocalSymDir. You will also need to update App.cs line 960 to include the msfz0 subdirectory change if the file is an msfz format file. You can do a quick check on pdbPath to see if the last subdirectory right before the actual file is "msfz0".
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.
Added MSFZ subdirectory support to PerfView App.cs CacheInLocalSymDir method in commit 5a8b0d7. The method now detects if a pdbPath is from an msfz0 subdirectory and creates the corresponding msfz0 subdirectory in the local cache.
…ver, optimize header reading, add PerfView App.cs support Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
| File.Delete(compressedFilePath); | ||
|
|
||
| // Check if the decompressed file is an MSFZ file and move it to the appropriate location | ||
| if (IsMsfzFile(targetPath)) |
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.
When decompressing a compressed file (line 1359), rather than extracting to targetPath, you should extract to the temporary path and then do the same logic as above (if it's an msfz file, put it in mfsz0, otherwise put it in the targetPath).
Please use the FileUtilities methods when possible.
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 the compressed file extraction logic as requested in commit 91b1de5. Now when decompressing files, they extract to a temporary path (targetPath + ".expanding") first, then check if the file is MSFZ format and move it to the appropriate location (msfz0 subdirectory for MSFZ files, normal target path for regular PDB files). This ensures MSFZ files never exist outside msfz0 directories. Also using FileUtilities.ForceDelete and FileUtilities.ForceMove methods as requested.
…ic correctly Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
|
Current Status:
|
|
Copilot Post-Mortem:
|
leculver
left a comment
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.
LGTM
This PR implements support for the MSFZ symbols format in the SymbolReader class as requested in the issue.
Changes Made
1. HTTP Accept Header Support
GetPhysicalFileFromServerto includeAccept: application/msfz0header in all HTTP requests to symbol servers2. MSFZ File Detection
IsMsfzFilemethod that detects files starting with "Microsoft MSFZ Container" (UTF-8 encoded)3. MSFZ Cache Management
MoveMsfzFileToSubdirectorymethod to move MSFZ files to a separate "msfz0" subdirectory within the normal cache path4. Enhanced Cache Search Logic
GetFileFromServerto check both the normal cache location and the "msfz0" subdirectory5. Content Type Support
application/msfz0in addition toapplication/octet-stream6. Update MSDIA140
Workflow
Accept: application/msfz0headermsfz0/subdirectorymsfz0/first, then fall back to normal locationTesting
Example
Fixes #2243.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.