Skip to content

[SymbolManifestGenerator] Extend case-insensitive matching#5897

Open
mdh1418 wants to merge 3 commits into
dotnet:mainfrom
mdh1418:fix/symbol-manifest-case-insensitive-pe
Open

[SymbolManifestGenerator] Extend case-insensitive matching#5897
mdh1418 wants to merge 3 commits into
dotnet:mainfrom
mdh1418:fix/symbol-manifest-case-insensitive-pe

Conversation

@mdh1418

@mdh1418 mdh1418 commented Jun 23, 2026

Copy link
Copy Markdown
Member

Some users have special diagnostic files that aren't lower-case, and the SymbolManifestGenerator logic currently expects exact lowercase matches.

This PR changes the logic to match files case-insensitive

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@mdh1418 mdh1418 requested review from Copilot and hoyosjs June 23, 2026 18:18
@mdh1418 mdh1418 requested a review from a team as a code owner June 23, 2026 18:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates symbol key generation and manifest correlation logic to tolerate runtime/special file names that differ by casing (e.g., CLR.DLL instead of clr.dll), which helps users with diagnostic artifacts that are not consistently lower-cased.

Changes:

  • Make PEFileKeyGenerator treat runtime module names and CLR “special file” detection as case-insensitive.
  • Extend SymbolManifestGenerator correlated-file lookup to use case-insensitive filename matching (including detection of multiple candidates).
  • Add unit tests validating case-insensitive behavior for runtime keys, CLR keys, and identity keys.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/tests/Microsoft.SymbolStore.UnitTests/PEFileKeyGenerationTests.cs Adds coverage for case-insensitive runtime/special file naming scenarios.
src/Microsoft.SymbolStore/KeyGenerators/PEFileKeyGenerator.cs Implements case-insensitive comparisons for runtime module detection and CLR special-file classification.
src/Microsoft.SymbolManifestGenerator/SymbolManifestGenerator.cs Updates correlated-file resolution to match adjacent/runtime special files case-insensitively and detect ambiguity.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Microsoft.SymbolManifestGenerator/SymbolManifestGenerator.cs Outdated
Comment thread src/Microsoft.SymbolManifestGenerator/SymbolManifestGenerator.cs Outdated
mdh1418 and others added 2 commits June 23, 2026 14:29
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
if ((flags & KeyTypeFlags.RuntimeKeys) != 0 && (GetFileName(_path) == CoreClrFileName || GetFileName(_path) == ClrFileName))
string runtimeFileName = GetFileName(_path);
if ((flags & KeyTypeFlags.RuntimeKeys) != 0 &&
(runtimeFileName.Equals(CoreClrFileName, StringComparison.OrdinalIgnoreCase) ||

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is used on linux where we can't do case insensitive matching

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we only accept lower case coreclr.dll/clr.dll? or just have case insensitive matching on Windows?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whats the background for this PR? My understanding is that these code paths would only be running when trying to do special indexing of files the .NET team produces so is this occurring in our own .NET build or somewhere else?

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