-
Notifications
You must be signed in to change notification settings - Fork 745
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
@@ -979,7 +979,11 @@ internal bool GetPhysicalFileFromServer(string serverPath, string pdbIndexPath, | |||
{ | |||
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.
@@ -1245,6 +1337,12 @@ private string GetFileFromServer(string urlForServer, string fileIndexPath, stri | |||
m_log.WriteLine("FindSymbolFilePath: Searching Symbol Server {0}.", urlForServer); | |||
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
GetFileFromServerWithMsfzSupport
method that handles MSFZ cache checking and file management - Updated
FindSymbolFilePath
to use the new method for symbol servers - Downloads now use
<filename>.new
files and MSFZ files are moved directly to msfz0 directory - Removed
MoveMsfzFileToSubdirectory
method 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>
@@ -1270,6 +1365,24 @@ private string GetFileFromServer(string urlForServer, string fileIndexPath, stri | |||
return null; | |||
} | |||
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:
|
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
GetPhysicalFileFromServer
to includeAccept: application/msfz0
header in all HTTP requests to symbol servers2. MSFZ File Detection
IsMsfzFile
method that detects files starting with "Microsoft MSFZ Container" (UTF-8 encoded)3. MSFZ Cache Management
MoveMsfzFileToSubdirectory
method to move MSFZ files to a separate "msfz0" subdirectory within the normal cache path4. Enhanced Cache Search Logic
GetFileFromServer
to check both the normal cache location and the "msfz0" subdirectory5. Content Type Support
application/msfz0
in addition toapplication/octet-stream
Workflow
Accept: application/msfz0
headermsfz0/
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.