Elide version string extraction from dyld cache#5824
Elide version string extraction from dyld cache#5824hoyosjs wants to merge 1 commit intodotnet:mainfrom
Conversation
hoyosjs
force-pushed
the
juhoyosa/modules-perf
branch
from
46e8463 to
cc97193
Compare
…al and macho module creation
hoyosjs
force-pushed
the
juhoyosa/modules-perf
branch
from
cfda1bb to
eccc448
Compare
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce macOS module version-string retrieval overhead by detecting dyld shared cache Mach-O images and skipping the expensive version-string scan for those modules (where missing pages in dumps can trigger slow symbol-store download fallbacks).
Changes:
- Add detection of dyld shared cache Mach-O modules via the
MH_DYLIB_IN_CACHEheader flag. - Skip Mach-O version-string scanning for shared-cache modules in
ModuleService.GetVersionString. - Avoid creating the
MachOModuledownload wrapper for dyld shared cache modules.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Microsoft.Diagnostics.DebugServices.Implementation/ModuleService.cs | Skips Mach-O version-string scanning when MH_DYLIB_IN_CACHE is set. |
| src/Microsoft.Diagnostics.DebugServices.Implementation/MachOModule.cs | Filters out dyld shared cache modules during MachOModule service creation. |
Comments suppressed due to low confidence (1)
src/Microsoft.Diagnostics.DebugServices.Implementation/ModuleService.cs:413
- GetVersionString now treats MachO modules with MH_DYLIB_IN_CACHE set as an "unsupported module" and logs a TraceError. These are valid MachO modules; the intent is just to skip the version-string scan. Consider splitting the condition so dyld shared cache modules return null (or log at a lower level) without reporting an unsupported-platform error, to avoid misleading noise for most system libraries on macOS 11+.
// Skip version string scan for dyld shared cache modules. Since macOS 11 (Big Sur), system
// libraries are in the shared cache and their writable segment pages are often not in dumps.
// Scanning them triggers DownloadModuleFile fallbacks that time out.
if (machOFile is not null && (machOFile.Header.Flags & MH_DYLIB_IN_CACHE) == 0)
{
foreach (MachSegmentLoadCommand loadCommand in machOFile.Segments.Select((segment) => segment.LoadCommand))
{
if (loadCommand.Command == LoadCommandType.Segment64 &&
(loadCommand.InitProt & VmProtWrite) != 0 &&
loadCommand.SegName.ToString() != "__LINKEDIT")
{
ulong loadAddress = loadCommand.VMAddress + machOFile.PreferredVMBaseAddress;
long loadSize = (long)loadCommand.VMSize;
if (SearchVersionString(loadAddress, loadSize, out string productVersion))
{
return productVersion;
}
}
}
Trace.TraceInformation($"GetVersionString: not found in MachO file {module}");
}
else
{
Trace.TraceError($"GetVersionString: unsupported module {module} on platform {Target.OperatingSystem}");
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Skip modules that have no build id, are not macOS, or are part of the dyld shared cache. | ||
| // Since macOS 11 (Big Sur), system libraries are in the shared cache and cannot be downloaded | ||
| // as individual files from symbol servers. The MachOFile service reads the in-memory header | ||
| // (already cached from BuildId resolution) to check the MH_DYLIB_IN_CACHE flag. | ||
| if (module.Target.OperatingSystem == OSPlatform.OSX && | ||
| !module.BuildId.IsDefaultOrEmpty && | ||
| !IsDyldSharedCacheModule(module)) | ||
| { | ||
| if (!module.BuildId.IsDefaultOrEmpty) | ||
| { | ||
| return new MachOModule(module, symbolService); | ||
| } | ||
| return new MachOModule(module, symbolService); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private static bool IsDyldSharedCacheModule(IModule module) | ||
| { | ||
| MachOFile machOFile = module.Services.GetService<MachOFile>(); | ||
| return machOFile is not null && (machOFile.Header.Flags & MH_DYLIB_IN_CACHE) != 0; | ||
| } |
There was a problem hiding this comment.
@copilot Partially fair, but ultimately not a real issue here. The concern is:
- CreateMachOModule calls IsDyldSharedCacheModule -> GetService()
- MachOFile (from Utilities.CreateMachOFile) uses IMemoryService to read the header
- On dumps, IMemoryService could be ImageMappingMemoryService which falls back to GetService() at line 195
The MachOFile service is created first (separate ServiceExport in Utilities.cs). It reads the in-memory header directly from the memory stream. ImageMappingMemoryService.ReadMemoryFromModule resolves MachOModule only as a fallback for missing bytes - but the MachO header/flags live at the module's ImageBase, which is virtually always present in dumps (it's how the module discovery works and how we get the symbol server uuid). Additionally, there's already _recursionProtection to guard against this exact pattern.
So the scenario described (stack overflow) is possible but practically unreachable for the header bytes. The existing recursion guard makes it safe even in edge cases. The pattern also already exists (ModuleService.cs lhas the same GetService() call)
| // MachO header flag indicating the dylib is part of the dyld shared cache (macOS 11+). | ||
| private const uint MH_DYLIB_IN_CACHE = 0x80000000; | ||
|
|
This pull request improves the handling of Mach-O modules on macOS, specifically addressing modules that are part of the dyld shared cache introduced in macOS 11 (Big Sur). The changes ensure that modules within the shared cache are properly identified and skipped during certain operations, avoiding unnecessary symbol server downloads and timeouts.