[cDAC] Implement GetModuleSimpleName for cDAC - #127415

Merged
barosiak merged 10 commits into
mainfrom
copilot/implement-get-module-simple-name
Apr 30, 2026
Merged

[cDAC] Implement GetModuleSimpleName for cDAC#127415
barosiak merged 10 commits into
mainfrom
copilot/implement-get-module-simple-name

Conversation

@barosiak

@barosiakbarosiak commented Apr 24, 2026•
edited
Loading

Copy link
Copy Markdown
Member

Summary

Implement GetModuleSimpleName on DacDbiImpl using the ILoader contract, replacing legacy-only delegation.

Changes

  • DacDbiImpl.cs - Implement GetModuleSimpleName via ILoader.GetSimpleName
  • DacDbiLoaderDumpTests.cs - Add dump test verifying all modules return S_OK with a non-empty simple name

CopilotAIand others added 2 commits April 23, 2026 21:53
@barosiak
Implement GetModuleSimpleName on DacDbiImpl using ILoader contract
07d0209
Replace the legacy-delegation stub with a contract-based implementation
that uses ILoader.GetModuleHandleFromModulePtr and TryGetSimpleName.
Follows the GetAppDomainFullName pattern with try/catch and DEBUG
legacy validation. Add dump-based integration test.
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/d565028d-e96a-4a77-961a-592e8f591a34
Co-authored-by: barosiak <76071368+barosiak@users.noreply.github.com>
@barosiak
Code cleanup
baa787b
@barosiakbarosiak self-assigned this Apr 24, 2026
CopilotAI review requested due to automatic review settings April 24, 2026 23:18
@dotnet-policy-service

dotnet-policy-serviceBot commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

CopilotAI reviewed Apr 24, 2026
View reviewed changes

CopilotAI 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

Implements IDacDbiInterface.GetModuleSimpleName in the managed cDAC DBI (DacDbiImpl) using the ILoader contract, and adds a dump-based integration test to validate the API returns S_OK with a non-empty name for modules in the MultiModule full dump.

Changes:

  • Implement DacDbiImpl.GetModuleSimpleName using ILoader.GetModuleHandleFromModulePtr + ILoader.TryGetSimpleName.
  • Add a dump test that iterates all modules and asserts GetModuleSimpleName returns S_OK with a non-empty string.

Reviewed changes

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

FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csAdds contract-based implementation of GetModuleSimpleName (with DEBUG-time validation against legacy DAC).
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds integration coverage to ensure module simple names are returned successfully and are non-empty in dump scenarios.

Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@github-actions

This comment has been minimized.

Sign in to view
noahfalk
noahfalk reviewed Apr 26, 2026
View reviewed changes
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs Outdated
CopilotAI review requested due to automatic review settings April 27, 2026 22:05
CopilotAI reviewed Apr 27, 2026
View reviewed changes

CopilotAI 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 implements DacDbiImpl.GetModuleSimpleName using the cDAC ILoader contract (TryGetSimpleName) instead of relying on legacy DAC delegation, and adds a dump-based integration test to ensure module simple names are available and non-empty.

Changes:

  • Implement DacDbiImpl.GetModuleSimpleName via ILoader.GetModuleHandleFromModulePtr + ILoader.TryGetSimpleName, including DEBUG-only validation against legacy output.
  • Add a dump test that iterates all modules in the MultiModule full dump and asserts GetModuleSimpleName returns S_OK and a non-empty string for each.

Reviewed changes

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

FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName using the loader contract and adds DEBUG-only string comparison against legacy DAC output.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds a dump integration test validating GetModuleSimpleName succeeds and returns a non-empty name for all modules.

Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs Outdated
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@barosiak
barosiak requested a review from noahfalkApril 28, 2026 00:09
CopilotAI review requested due to automatic review settings April 28, 2026 18:29
CopilotAI reviewed Apr 28, 2026
View reviewed changes

CopilotAI 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

Implements DacDbiImpl.GetModuleSimpleName using the cDAC ILoader contract (TryGetSimpleName) instead of delegating to the legacy DAC-only implementation, and adds dump-based coverage to ensure module simple names are returned successfully.

Changes:

  • Implement DacDbiImpl.GetModuleSimpleName via Target.Contracts.Loader.TryGetSimpleName and copy into the native IStringHolder.
  • Add a dump test that calls GetModuleSimpleName for all modules and asserts S_OK with a non-empty result.
  • Fix NativeStringHolder’s namespace to align with the Legacy assembly and simplify the HResults reference.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName using the ILoader contract and retains DEBUG cross-checking against legacy.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds dump-based integration test coverage for GetModuleSimpleName.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/NativeStringHolder.csAligns helper type namespace with the Legacy assembly and updates HRESULT reference.

Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
@github-actions

This comment has been minimized.

Sign in to view
CopilotAI review requested due to automatic review settings April 30, 2026 00:04
CopilotAI reviewed Apr 30, 2026
View reviewed changes

CopilotAI 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 wires DacDbiImpl.GetModuleSimpleName to the cDAC ILoader contract (instead of delegating only to legacy DAC), and adds dump-test coverage to ensure module simple names are available in dump-based scenarios.

Changes:

  • Replace ILoader.TryGetSimpleName with ILoader.GetSimpleName and update the contract implementation/docs accordingly.
  • Implement DacDbiImpl.GetModuleSimpleName using ILoader.GetSimpleName with DEBUG-only validation against legacy (when available).
  • Add/update unit + dump tests for the new simple-name retrieval behavior.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file
FileDescription
src/native/managed/cdac/tests/LoaderTests.csUpdates loader unit tests to use GetSimpleName instead of TryGetSimpleName.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csSwitches debuggee and adds a dump test asserting GetModuleSimpleName returns S_OK and a non-empty name per module.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/NativeStringHolder.csFixes the namespace and aligns HRESULT usage for the native string-holder test helper.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName via the cDAC loader contract and adds DEBUG validation against legacy output.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.csUpdates IXCLRDataModule.GetName to use ILoader.GetSimpleName.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.csImplements ILoader.GetSimpleName in the Loader v1 contract.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.csChanges the public ILoader interface surface to GetSimpleName.
docs/design/datacontracts/Loader.mdUpdates contract documentation to reflect GetSimpleName.

Comment thread...native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs Outdated
Comment thread...tive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
Comment threadsrc/native/managed/cdac/tests/LoaderTests.cs
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@github-actions

github-actionsBot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #127415

Note

This review was generated by Copilot.

Holistic Assessment

Motivation: The change makes ILoader.GetSimpleName handle a null SimpleName pointer gracefully by returning string.Empty instead of likely crashing in ReadUtf8String. This is a real scenario (modules without simple names) and is clearly needed.

Approach: The fix follows the exact same pattern already used by GetPath (line 461) and GetFileName (line 469) in the same file — a ternary null check returning string.Empty. This is the right approach: consistent, minimal, and correct.

Summary: ✅ LGTM. The implementation is consistent with sibling methods, properly tested (including the null case and the existing happy path), and the dump test now validates cDAC/DacDbi agreement. No blocking issues found.

Detailed Findings

✅ Correctness — Null check follows established pattern

The ternary pattern:

returnmodule.SimpleName!=TargetPointer.Null?_target.ReadUtf8String(module.SimpleName,strict:true):string.Empty;

is identical to GetPath and GetFileName immediately below it. This is correct and consistent.

✅ Test Coverage — Both positive and null cases covered

The PR:

  • Keeps the existing GetSimpleName test for the happy path (non-null simple name)
  • Adds GetSimpleName_NullSimpleName which explicitly creates a module without a simple name and verifies string.Empty is returned
  • Adds an assertion in the dump test comparing DacDbi output to the cDAC contract output (Assert.Equal(loader.GetSimpleName(module), holder.Value))

The test refactoring also correctly removes the unnecessary inner scope block from the original test, improving readability.

✅ Dump test comment update — Accurate

The comment update (removing "which loads assemblies from multiple ALCs") is a minor doc improvement that keeps the comment accurate after the debuggee change.

💡 Minor observation — Test structure consistency

The existing GetFileName test (lines ~82-110) tests both the non-empty and empty cases within a single [Theory] method using two module addresses. The new GetSimpleName_NullSimpleName uses a separate method instead. Both patterns exist in this file, so this is fine — just noting the difference in style. No action needed.

Generated by Code Review for issue #127415 · ◷

@barosiak
barosiak merged commit 9ef2b5e into mainApr 30, 2026
51 of 59 checks passed
@barosiak
barosiak deleted the copilot/implement-get-module-simple-name branch April 30, 2026 23:55
@dotnet-maestrodotnet-maestroBot mentioned this pull request May 1, 2026
Merged
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 31, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@noahfalknoahfalknoahfalk approved these changes
@rcj1rcj1rcj1 approved these changes
@max-charlambmax-charlambAwaiting requested review from max-charlamb
+1 more reviewer
@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@barosiakbarosiak

Labels

area-Diagnostics-coreclr

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@barosiak@am11@noahfalk@max-charlamb@rcj1
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
[cDAC] Implement GetModuleSimpleName for cDAC - #127415
Merged
barosiak merged 10 commits into
mainfrom 
copilot/implement-get-module-simple-name
Apr 30, 2026
Merged
[cDAC] Implement GetModuleSimpleName for cDAC#127415
barosiak merged 10 commits into
mainfrom 
copilot/implement-get-module-simple-name
Conversation
@barosiak
@barosiakbarosiak
commented
Apr 24, 2026•

edited

Loading

Copy link


Copy Markdown

Member
Summary
Implement GetModuleSimpleName on DacDbiImpl using the ILoader contract, replacing legacy-only delegation.
Changes
  • DacDbiImpl.cs - Implement GetModuleSimpleName via ILoader.GetSimpleName
  • DacDbiLoaderDumpTests.cs - Add dump test verifying all modules return S_OK with a non-empty simple name
CopilotAIand others
added 2 commits
April 23, 2026 21:53
@barosiak
Implement GetModuleSimpleName on DacDbiImpl using ILoader contract
07d0209
Replace the legacy-delegation stub with a contract-based implementation
that uses ILoader.GetModuleHandleFromModulePtr and TryGetSimpleName.
Follows the GetAppDomainFullName pattern with try/catch and DEBUG
legacy validation. Add dump-based integration test.
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/d565028d-e96a-4a77-961a-592e8f591a34
Co-authored-by: barosiak <76071368+barosiak@users.noreply.github.com>
@barosiak
Code cleanup
baa787b
@barosiakbarosiak
self-assigned this
Apr 24, 2026
CopilotAI
review requested due to automatic review settings
April 24, 2026 23:18
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 24, 2026

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag

See info in area-owners.md if you want to be subscribed.
CopilotAI
reviewed
Apr 24, 2026
View reviewed changes
CopilotAI
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
Implements IDacDbiInterface.GetModuleSimpleName in the managed cDAC DBI (DacDbiImpl) using the ILoader contract, and adds a dump-based integration test to validate the API returns S_OK with a non-empty name for modules in the MultiModule full dump.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName using ILoader.GetModuleHandleFromModulePtr + ILoader.TryGetSimpleName.
  • Add a dump test that iterates all modules and asserts GetModuleSimpleName returns S_OK with a non-empty string.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csAdds contract-based implementation of GetModuleSimpleName (with DEBUG-time validation against legacy DAC).
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds integration coverage to ensure module simple names are returned successfully and are non-empty in dump scenarios.
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@github-actions

This comment has been minimized.

Sign in to view
noahfalk
noahfalk
reviewed
Apr 26, 2026
View reviewed changes
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Outdated

CopilotAI
review requested due to automatic review settings
April 27, 2026 22:05
CopilotAI
reviewed
Apr 27, 2026
View reviewed changes
CopilotAI
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 implements DacDbiImpl.GetModuleSimpleName using the cDAC ILoader contract (TryGetSimpleName) instead of relying on legacy DAC delegation, and adds a dump-based integration test to ensure module simple names are available and non-empty.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName via ILoader.GetModuleHandleFromModulePtr + ILoader.TryGetSimpleName, including DEBUG-only validation against legacy output.
  • Add a dump test that iterates all modules in the MultiModule full dump and asserts GetModuleSimpleName returns S_OK and a non-empty string for each.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName using the loader contract and adds DEBUG-only string comparison against legacy DAC output.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds a dump integration test validating GetModuleSimpleName succeeds and returns a non-empty name for all modules.
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Outdated

Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@barosiak
barosiak
requested a review
from noahfalkApril 28, 2026 00:09
CopilotAI
review requested due to automatic review settings
April 28, 2026 18:29
CopilotAI
reviewed
Apr 28, 2026
View reviewed changes
CopilotAI
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
Implements DacDbiImpl.GetModuleSimpleName using the cDAC ILoader contract (TryGetSimpleName) instead of delegating to the legacy DAC-only implementation, and adds dump-based coverage to ensure module simple names are returned successfully.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName via Target.Contracts.Loader.TryGetSimpleName and copy into the native IStringHolder.
  • Add a dump test that calls GetModuleSimpleName for all modules and asserts S_OK with a non-empty result.
  • Fix NativeStringHolder’s namespace to align with the Legacy assembly and simplify the HResults reference.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName using the ILoader contract and retains DEBUG cross-checking against legacy.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds dump-based integration test coverage for GetModuleSimpleName.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/NativeStringHolder.csAligns helper type namespace with the Legacy assembly and updates HRESULT reference.
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
@github-actions

This comment has been minimized.

Sign in to view
CopilotAI
review requested due to automatic review settings
April 30, 2026 00:04
CopilotAI
reviewed
Apr 30, 2026
View reviewed changes
CopilotAI
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 wires DacDbiImpl.GetModuleSimpleName to the cDAC ILoader contract (instead of delegating only to legacy DAC), and adds dump-test coverage to ensure module simple names are available in dump-based scenarios.
Changes:
  • Replace ILoader.TryGetSimpleName with ILoader.GetSimpleName and update the contract implementation/docs accordingly.
  • Implement DacDbiImpl.GetModuleSimpleName using ILoader.GetSimpleName with DEBUG-only validation against legacy (when available).
  • Add/update unit + dump tests for the new simple-name retrieval behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
FileDescription
src/native/managed/cdac/tests/LoaderTests.csUpdates loader unit tests to use GetSimpleName instead of TryGetSimpleName.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csSwitches debuggee and adds a dump test asserting GetModuleSimpleName returns S_OK and a non-empty name per module.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/NativeStringHolder.csFixes the namespace and aligns HRESULT usage for the native string-holder test helper.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName via the cDAC loader contract and adds DEBUG validation against legacy output.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.csUpdates IXCLRDataModule.GetName to use ILoader.GetSimpleName.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.csImplements ILoader.GetSimpleName in the Loader v1 contract.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.csChanges the public ILoader interface surface to GetSimpleName.
docs/design/datacontracts/Loader.mdUpdates contract documentation to reflect GetSimpleName.
Comment thread...native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs
Outdated

Comment thread...tive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
Comment threadsrc/native/managed/cdac/tests/LoaderTests.cs
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@github-actions
github-actionsBot
commented
Apr 30, 2026

Copy link


Copy Markdown

Contributor
🤖 Copilot Code Review — PR #127415
Note
This review was generated by Copilot.
Holistic Assessment
Motivation: The change makes ILoader.GetSimpleName handle a null SimpleName pointer gracefully by returning string.Empty instead of likely crashing in ReadUtf8String. This is a real scenario (modules without simple names) and is clearly needed.
Approach: The fix follows the exact same pattern already used by GetPath (line 461) and GetFileName (line 469) in the same file — a ternary null check returning string.Empty. This is the right approach: consistent, minimal, and correct.
Summary: ✅ LGTM. The implementation is consistent with sibling methods, properly tested (including the null case and the existing happy path), and the dump test now validates cDAC/DacDbi agreement. No blocking issues found.
Detailed Findings
✅ Correctness — Null check follows established pattern
The ternary pattern:
returnmodule.SimpleName!=TargetPointer.Null?_target.ReadUtf8String(module.SimpleName,strict:true):string.Empty;
is identical to GetPath and GetFileName immediately below it. This is correct and consistent.
✅ Test Coverage — Both positive and null cases covered
The PR:
  • Keeps the existing GetSimpleName test for the happy path (non-null simple name)
  • Adds GetSimpleName_NullSimpleName which explicitly creates a module without a simple name and verifies string.Empty is returned
  • Adds an assertion in the dump test comparing DacDbi output to the cDAC contract output (Assert.Equal(loader.GetSimpleName(module), holder.Value))
The test refactoring also correctly removes the unnecessary inner scope block from the original test, improving readability.
✅ Dump test comment update — Accurate
The comment update (removing "which loads assemblies from multiple ALCs") is a minor doc improvement that keeps the comment accurate after the debuggee change.
💡 Minor observation — Test structure consistency
The existing GetFileName test (lines ~82-110) tests both the non-empty and empty cases within a single [Theory] method using two module addresses. The new GetSimpleName_NullSimpleName uses a separate method instead. Both patterns exist in this file, so this is fine — just noting the difference in style. No action needed.
Generated by Code Review for issue #127415 · ◷
@barosiak
barosiak
merged commit 9ef2b5e
into
mainApr 30, 2026

51 of 59 checks passed

@barosiak
barosiak
deleted the

copilot/implement-get-module-simple-name

branch
April 30, 2026 23:55
@dotnet-maestrodotnet-maestroBot
mentioned this pull request
May 1, 2026
 Merged

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 31, 2026
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@noahfalknoahfalknoahfalk approved these changes
@rcj1rcj1rcj1 approved these changes
@max-charlambmax-charlambAwaiting requested review from max-charlamb

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@barosiakbarosiak

Labels

area-Diagnostics-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@barosiak@am11@noahfalk@max-charlamb@rcj1

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[cDAC] Implement GetModuleSimpleName for cDAC - #127415
Merged
barosiak merged 10 commits into
mainfrom 
copilot/implement-get-module-simple-name
Apr 30, 2026
Merged
[cDAC] Implement GetModuleSimpleName for cDAC#127415
barosiak merged 10 commits into
mainfrom 
copilot/implement-get-module-simple-name
Conversation
@barosiak
@barosiakbarosiak
commented
Apr 24, 2026•

edited

Loading

Copy link


Copy Markdown

Member
Summary
Implement GetModuleSimpleName on DacDbiImpl using the ILoader contract, replacing legacy-only delegation.
Changes
  • DacDbiImpl.cs - Implement GetModuleSimpleName via ILoader.GetSimpleName
  • DacDbiLoaderDumpTests.cs - Add dump test verifying all modules return S_OK with a non-empty simple name
CopilotAIand others
added 2 commits
April 23, 2026 21:53
@barosiak
Implement GetModuleSimpleName on DacDbiImpl using ILoader contract
07d0209
Replace the legacy-delegation stub with a contract-based implementation
that uses ILoader.GetModuleHandleFromModulePtr and TryGetSimpleName.
Follows the GetAppDomainFullName pattern with try/catch and DEBUG
legacy validation. Add dump-based integration test.
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/d565028d-e96a-4a77-961a-592e8f591a34
Co-authored-by: barosiak <76071368+barosiak@users.noreply.github.com>
@barosiak
Code cleanup
baa787b
@barosiakbarosiak
self-assigned this
Apr 24, 2026
CopilotAI
review requested due to automatic review settings
April 24, 2026 23:18
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 24, 2026

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag

See info in area-owners.md if you want to be subscribed.
CopilotAI
reviewed
Apr 24, 2026
View reviewed changes
CopilotAI
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
Implements IDacDbiInterface.GetModuleSimpleName in the managed cDAC DBI (DacDbiImpl) using the ILoader contract, and adds a dump-based integration test to validate the API returns S_OK with a non-empty name for modules in the MultiModule full dump.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName using ILoader.GetModuleHandleFromModulePtr + ILoader.TryGetSimpleName.
  • Add a dump test that iterates all modules and asserts GetModuleSimpleName returns S_OK with a non-empty string.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csAdds contract-based implementation of GetModuleSimpleName (with DEBUG-time validation against legacy DAC).
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds integration coverage to ensure module simple names are returned successfully and are non-empty in dump scenarios.
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@github-actions

This comment has been minimized.

Sign in to view
noahfalk
noahfalk
reviewed
Apr 26, 2026
View reviewed changes
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Outdated

CopilotAI
review requested due to automatic review settings
April 27, 2026 22:05
CopilotAI
reviewed
Apr 27, 2026
View reviewed changes
CopilotAI
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 implements DacDbiImpl.GetModuleSimpleName using the cDAC ILoader contract (TryGetSimpleName) instead of relying on legacy DAC delegation, and adds a dump-based integration test to ensure module simple names are available and non-empty.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName via ILoader.GetModuleHandleFromModulePtr + ILoader.TryGetSimpleName, including DEBUG-only validation against legacy output.
  • Add a dump test that iterates all modules in the MultiModule full dump and asserts GetModuleSimpleName returns S_OK and a non-empty string for each.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName using the loader contract and adds DEBUG-only string comparison against legacy DAC output.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds a dump integration test validating GetModuleSimpleName succeeds and returns a non-empty name for all modules.
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Outdated

Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@barosiak
barosiak
requested a review
from noahfalkApril 28, 2026 00:09
CopilotAI
review requested due to automatic review settings
April 28, 2026 18:29
CopilotAI
reviewed
Apr 28, 2026
View reviewed changes
CopilotAI
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
Implements DacDbiImpl.GetModuleSimpleName using the cDAC ILoader contract (TryGetSimpleName) instead of delegating to the legacy DAC-only implementation, and adds dump-based coverage to ensure module simple names are returned successfully.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName via Target.Contracts.Loader.TryGetSimpleName and copy into the native IStringHolder.
  • Add a dump test that calls GetModuleSimpleName for all modules and asserts S_OK with a non-empty result.
  • Fix NativeStringHolder’s namespace to align with the Legacy assembly and simplify the HResults reference.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName using the ILoader contract and retains DEBUG cross-checking against legacy.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds dump-based integration test coverage for GetModuleSimpleName.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/NativeStringHolder.csAligns helper type namespace with the Legacy assembly and updates HRESULT reference.
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
@github-actions

This comment has been minimized.

Sign in to view
CopilotAI
review requested due to automatic review settings
April 30, 2026 00:04
CopilotAI
reviewed
Apr 30, 2026
View reviewed changes
CopilotAI
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 wires DacDbiImpl.GetModuleSimpleName to the cDAC ILoader contract (instead of delegating only to legacy DAC), and adds dump-test coverage to ensure module simple names are available in dump-based scenarios.
Changes:
  • Replace ILoader.TryGetSimpleName with ILoader.GetSimpleName and update the contract implementation/docs accordingly.
  • Implement DacDbiImpl.GetModuleSimpleName using ILoader.GetSimpleName with DEBUG-only validation against legacy (when available).
  • Add/update unit + dump tests for the new simple-name retrieval behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
FileDescription
src/native/managed/cdac/tests/LoaderTests.csUpdates loader unit tests to use GetSimpleName instead of TryGetSimpleName.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csSwitches debuggee and adds a dump test asserting GetModuleSimpleName returns S_OK and a non-empty name per module.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/NativeStringHolder.csFixes the namespace and aligns HRESULT usage for the native string-holder test helper.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName via the cDAC loader contract and adds DEBUG validation against legacy output.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.csUpdates IXCLRDataModule.GetName to use ILoader.GetSimpleName.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.csImplements ILoader.GetSimpleName in the Loader v1 contract.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.csChanges the public ILoader interface surface to GetSimpleName.
docs/design/datacontracts/Loader.mdUpdates contract documentation to reflect GetSimpleName.
Comment thread...native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs
Outdated

Comment thread...tive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
Comment threadsrc/native/managed/cdac/tests/LoaderTests.cs
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@github-actions
github-actionsBot
commented
Apr 30, 2026

Copy link


Copy Markdown

Contributor
🤖 Copilot Code Review — PR #127415
Note
This review was generated by Copilot.
Holistic Assessment
Motivation: The change makes ILoader.GetSimpleName handle a null SimpleName pointer gracefully by returning string.Empty instead of likely crashing in ReadUtf8String. This is a real scenario (modules without simple names) and is clearly needed.
Approach: The fix follows the exact same pattern already used by GetPath (line 461) and GetFileName (line 469) in the same file — a ternary null check returning string.Empty. This is the right approach: consistent, minimal, and correct.
Summary: ✅ LGTM. The implementation is consistent with sibling methods, properly tested (including the null case and the existing happy path), and the dump test now validates cDAC/DacDbi agreement. No blocking issues found.
Detailed Findings
✅ Correctness — Null check follows established pattern
The ternary pattern:
returnmodule.SimpleName!=TargetPointer.Null?_target.ReadUtf8String(module.SimpleName,strict:true):string.Empty;
is identical to GetPath and GetFileName immediately below it. This is correct and consistent.
✅ Test Coverage — Both positive and null cases covered
The PR:
  • Keeps the existing GetSimpleName test for the happy path (non-null simple name)
  • Adds GetSimpleName_NullSimpleName which explicitly creates a module without a simple name and verifies string.Empty is returned
  • Adds an assertion in the dump test comparing DacDbi output to the cDAC contract output (Assert.Equal(loader.GetSimpleName(module), holder.Value))
The test refactoring also correctly removes the unnecessary inner scope block from the original test, improving readability.
✅ Dump test comment update — Accurate
The comment update (removing "which loads assemblies from multiple ALCs") is a minor doc improvement that keeps the comment accurate after the debuggee change.
💡 Minor observation — Test structure consistency
The existing GetFileName test (lines ~82-110) tests both the non-empty and empty cases within a single [Theory] method using two module addresses. The new GetSimpleName_NullSimpleName uses a separate method instead. Both patterns exist in this file, so this is fine — just noting the difference in style. No action needed.
Generated by Code Review for issue #127415 · ◷
@barosiak
barosiak
merged commit 9ef2b5e
into
mainApr 30, 2026

51 of 59 checks passed

@barosiak
barosiak
deleted the

copilot/implement-get-module-simple-name

branch
April 30, 2026 23:55
@dotnet-maestrodotnet-maestroBot
mentioned this pull request
May 1, 2026
 Merged

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 31, 2026
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@noahfalknoahfalknoahfalk approved these changes
@rcj1rcj1rcj1 approved these changes
@max-charlambmax-charlambAwaiting requested review from max-charlamb

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@barosiakbarosiak

Labels

area-Diagnostics-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@barosiak@am11@noahfalk@max-charlamb@rcj1

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[cDAC] Implement GetModuleSimpleName for cDAC - #127415
Merged
barosiak merged 10 commits into
mainfrom 
copilot/implement-get-module-simple-name
Apr 30, 2026
Merged
[cDAC] Implement GetModuleSimpleName for cDAC#127415
barosiak merged 10 commits into
mainfrom 
copilot/implement-get-module-simple-name
Conversation
@barosiak
@barosiakbarosiak
commented
Apr 24, 2026•

edited

Loading

Copy link


Copy Markdown

Member
Summary
Implement GetModuleSimpleName on DacDbiImpl using the ILoader contract, replacing legacy-only delegation.
Changes
  • DacDbiImpl.cs - Implement GetModuleSimpleName via ILoader.GetSimpleName
  • DacDbiLoaderDumpTests.cs - Add dump test verifying all modules return S_OK with a non-empty simple name
CopilotAIand others
added 2 commits
April 23, 2026 21:53
@barosiak
Implement GetModuleSimpleName on DacDbiImpl using ILoader contract
07d0209
Replace the legacy-delegation stub with a contract-based implementation
that uses ILoader.GetModuleHandleFromModulePtr and TryGetSimpleName.
Follows the GetAppDomainFullName pattern with try/catch and DEBUG
legacy validation. Add dump-based integration test.
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/d565028d-e96a-4a77-961a-592e8f591a34
Co-authored-by: barosiak <76071368+barosiak@users.noreply.github.com>
@barosiak
Code cleanup
baa787b
@barosiakbarosiak
self-assigned this
Apr 24, 2026
CopilotAI
review requested due to automatic review settings
April 24, 2026 23:18
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 24, 2026

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag

See info in area-owners.md if you want to be subscribed.
CopilotAI
reviewed
Apr 24, 2026
View reviewed changes
CopilotAI
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
Implements IDacDbiInterface.GetModuleSimpleName in the managed cDAC DBI (DacDbiImpl) using the ILoader contract, and adds a dump-based integration test to validate the API returns S_OK with a non-empty name for modules in the MultiModule full dump.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName using ILoader.GetModuleHandleFromModulePtr + ILoader.TryGetSimpleName.
  • Add a dump test that iterates all modules and asserts GetModuleSimpleName returns S_OK with a non-empty string.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csAdds contract-based implementation of GetModuleSimpleName (with DEBUG-time validation against legacy DAC).
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds integration coverage to ensure module simple names are returned successfully and are non-empty in dump scenarios.
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@github-actions

This comment has been minimized.

Sign in to view
noahfalk
noahfalk
reviewed
Apr 26, 2026
View reviewed changes
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Outdated

CopilotAI
review requested due to automatic review settings
April 27, 2026 22:05
CopilotAI
reviewed
Apr 27, 2026
View reviewed changes
CopilotAI
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 implements DacDbiImpl.GetModuleSimpleName using the cDAC ILoader contract (TryGetSimpleName) instead of relying on legacy DAC delegation, and adds a dump-based integration test to ensure module simple names are available and non-empty.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName via ILoader.GetModuleHandleFromModulePtr + ILoader.TryGetSimpleName, including DEBUG-only validation against legacy output.
  • Add a dump test that iterates all modules in the MultiModule full dump and asserts GetModuleSimpleName returns S_OK and a non-empty string for each.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName using the loader contract and adds DEBUG-only string comparison against legacy DAC output.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds a dump integration test validating GetModuleSimpleName succeeds and returns a non-empty name for all modules.
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Outdated

Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@barosiak
barosiak
requested a review
from noahfalkApril 28, 2026 00:09
CopilotAI
review requested due to automatic review settings
April 28, 2026 18:29
CopilotAI
reviewed
Apr 28, 2026
View reviewed changes
CopilotAI
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
Implements DacDbiImpl.GetModuleSimpleName using the cDAC ILoader contract (TryGetSimpleName) instead of delegating to the legacy DAC-only implementation, and adds dump-based coverage to ensure module simple names are returned successfully.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName via Target.Contracts.Loader.TryGetSimpleName and copy into the native IStringHolder.
  • Add a dump test that calls GetModuleSimpleName for all modules and asserts S_OK with a non-empty result.
  • Fix NativeStringHolder’s namespace to align with the Legacy assembly and simplify the HResults reference.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName using the ILoader contract and retains DEBUG cross-checking against legacy.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds dump-based integration test coverage for GetModuleSimpleName.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/NativeStringHolder.csAligns helper type namespace with the Legacy assembly and updates HRESULT reference.
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
@github-actions

This comment has been minimized.

Sign in to view
CopilotAI
review requested due to automatic review settings
April 30, 2026 00:04
CopilotAI
reviewed
Apr 30, 2026
View reviewed changes
CopilotAI
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 wires DacDbiImpl.GetModuleSimpleName to the cDAC ILoader contract (instead of delegating only to legacy DAC), and adds dump-test coverage to ensure module simple names are available in dump-based scenarios.
Changes:
  • Replace ILoader.TryGetSimpleName with ILoader.GetSimpleName and update the contract implementation/docs accordingly.
  • Implement DacDbiImpl.GetModuleSimpleName using ILoader.GetSimpleName with DEBUG-only validation against legacy (when available).
  • Add/update unit + dump tests for the new simple-name retrieval behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
FileDescription
src/native/managed/cdac/tests/LoaderTests.csUpdates loader unit tests to use GetSimpleName instead of TryGetSimpleName.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csSwitches debuggee and adds a dump test asserting GetModuleSimpleName returns S_OK and a non-empty name per module.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/NativeStringHolder.csFixes the namespace and aligns HRESULT usage for the native string-holder test helper.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName via the cDAC loader contract and adds DEBUG validation against legacy output.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.csUpdates IXCLRDataModule.GetName to use ILoader.GetSimpleName.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.csImplements ILoader.GetSimpleName in the Loader v1 contract.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.csChanges the public ILoader interface surface to GetSimpleName.
docs/design/datacontracts/Loader.mdUpdates contract documentation to reflect GetSimpleName.
Comment thread...native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs
Outdated

Comment thread...tive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
Comment threadsrc/native/managed/cdac/tests/LoaderTests.cs
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@github-actions
github-actionsBot
commented
Apr 30, 2026

Copy link


Copy Markdown

Contributor
🤖 Copilot Code Review — PR #127415
Note
This review was generated by Copilot.
Holistic Assessment
Motivation: The change makes ILoader.GetSimpleName handle a null SimpleName pointer gracefully by returning string.Empty instead of likely crashing in ReadUtf8String. This is a real scenario (modules without simple names) and is clearly needed.
Approach: The fix follows the exact same pattern already used by GetPath (line 461) and GetFileName (line 469) in the same file — a ternary null check returning string.Empty. This is the right approach: consistent, minimal, and correct.
Summary: ✅ LGTM. The implementation is consistent with sibling methods, properly tested (including the null case and the existing happy path), and the dump test now validates cDAC/DacDbi agreement. No blocking issues found.
Detailed Findings
✅ Correctness — Null check follows established pattern
The ternary pattern:
returnmodule.SimpleName!=TargetPointer.Null?_target.ReadUtf8String(module.SimpleName,strict:true):string.Empty;
is identical to GetPath and GetFileName immediately below it. This is correct and consistent.
✅ Test Coverage — Both positive and null cases covered
The PR:
  • Keeps the existing GetSimpleName test for the happy path (non-null simple name)
  • Adds GetSimpleName_NullSimpleName which explicitly creates a module without a simple name and verifies string.Empty is returned
  • Adds an assertion in the dump test comparing DacDbi output to the cDAC contract output (Assert.Equal(loader.GetSimpleName(module), holder.Value))
The test refactoring also correctly removes the unnecessary inner scope block from the original test, improving readability.
✅ Dump test comment update — Accurate
The comment update (removing "which loads assemblies from multiple ALCs") is a minor doc improvement that keeps the comment accurate after the debuggee change.
💡 Minor observation — Test structure consistency
The existing GetFileName test (lines ~82-110) tests both the non-empty and empty cases within a single [Theory] method using two module addresses. The new GetSimpleName_NullSimpleName uses a separate method instead. Both patterns exist in this file, so this is fine — just noting the difference in style. No action needed.
Generated by Code Review for issue #127415 · ◷
@barosiak
barosiak
merged commit 9ef2b5e
into
mainApr 30, 2026

51 of 59 checks passed

@barosiak
barosiak
deleted the

copilot/implement-get-module-simple-name

branch
April 30, 2026 23:55
@dotnet-maestrodotnet-maestroBot
mentioned this pull request
May 1, 2026
 Merged

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 31, 2026
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@noahfalknoahfalknoahfalk approved these changes
@rcj1rcj1rcj1 approved these changes
@max-charlambmax-charlambAwaiting requested review from max-charlamb

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@barosiakbarosiak

Labels

area-Diagnostics-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@barosiak@am11@noahfalk@max-charlamb@rcj1

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
[cDAC] Implement GetModuleSimpleName for cDAC - #127415
Merged
barosiak merged 10 commits into
mainfrom 
copilot/implement-get-module-simple-name
Apr 30, 2026
Merged
[cDAC] Implement GetModuleSimpleName for cDAC#127415
barosiak merged 10 commits into
mainfrom 
copilot/implement-get-module-simple-name
Conversation
@barosiak
@barosiakbarosiak
commented
Apr 24, 2026•

edited

Loading

Copy link


Copy Markdown

Member
Summary
Implement GetModuleSimpleName on DacDbiImpl using the ILoader contract, replacing legacy-only delegation.
Changes
  • DacDbiImpl.cs - Implement GetModuleSimpleName via ILoader.GetSimpleName
  • DacDbiLoaderDumpTests.cs - Add dump test verifying all modules return S_OK with a non-empty simple name
CopilotAIand others
added 2 commits
April 23, 2026 21:53
@barosiak
Implement GetModuleSimpleName on DacDbiImpl using ILoader contract
07d0209
Replace the legacy-delegation stub with a contract-based implementation
that uses ILoader.GetModuleHandleFromModulePtr and TryGetSimpleName.
Follows the GetAppDomainFullName pattern with try/catch and DEBUG
legacy validation. Add dump-based integration test.
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/d565028d-e96a-4a77-961a-592e8f591a34
Co-authored-by: barosiak <76071368+barosiak@users.noreply.github.com>
@barosiak
Code cleanup
baa787b
@barosiakbarosiak
self-assigned this
Apr 24, 2026
CopilotAI
review requested due to automatic review settings
April 24, 2026 23:18
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 24, 2026

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag

See info in area-owners.md if you want to be subscribed.
CopilotAI
reviewed
Apr 24, 2026
View reviewed changes
CopilotAI
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
Implements IDacDbiInterface.GetModuleSimpleName in the managed cDAC DBI (DacDbiImpl) using the ILoader contract, and adds a dump-based integration test to validate the API returns S_OK with a non-empty name for modules in the MultiModule full dump.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName using ILoader.GetModuleHandleFromModulePtr + ILoader.TryGetSimpleName.
  • Add a dump test that iterates all modules and asserts GetModuleSimpleName returns S_OK with a non-empty string.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csAdds contract-based implementation of GetModuleSimpleName (with DEBUG-time validation against legacy DAC).
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds integration coverage to ensure module simple names are returned successfully and are non-empty in dump scenarios.
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@github-actions

This comment has been minimized.

Sign in to view
noahfalk
noahfalk
reviewed
Apr 26, 2026
View reviewed changes
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Outdated

CopilotAI
review requested due to automatic review settings
April 27, 2026 22:05
CopilotAI
reviewed
Apr 27, 2026
View reviewed changes
CopilotAI
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 implements DacDbiImpl.GetModuleSimpleName using the cDAC ILoader contract (TryGetSimpleName) instead of relying on legacy DAC delegation, and adds a dump-based integration test to ensure module simple names are available and non-empty.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName via ILoader.GetModuleHandleFromModulePtr + ILoader.TryGetSimpleName, including DEBUG-only validation against legacy output.
  • Add a dump test that iterates all modules in the MultiModule full dump and asserts GetModuleSimpleName returns S_OK and a non-empty string for each.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName using the loader contract and adds DEBUG-only string comparison against legacy DAC output.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds a dump integration test validating GetModuleSimpleName succeeds and returns a non-empty name for all modules.
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Outdated

Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@barosiak
barosiak
requested a review
from noahfalkApril 28, 2026 00:09
CopilotAI
review requested due to automatic review settings
April 28, 2026 18:29
CopilotAI
reviewed
Apr 28, 2026
View reviewed changes
CopilotAI
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
Implements DacDbiImpl.GetModuleSimpleName using the cDAC ILoader contract (TryGetSimpleName) instead of delegating to the legacy DAC-only implementation, and adds dump-based coverage to ensure module simple names are returned successfully.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName via Target.Contracts.Loader.TryGetSimpleName and copy into the native IStringHolder.
  • Add a dump test that calls GetModuleSimpleName for all modules and asserts S_OK with a non-empty result.
  • Fix NativeStringHolder’s namespace to align with the Legacy assembly and simplify the HResults reference.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName using the ILoader contract and retains DEBUG cross-checking against legacy.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds dump-based integration test coverage for GetModuleSimpleName.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/NativeStringHolder.csAligns helper type namespace with the Legacy assembly and updates HRESULT reference.
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
@github-actions

This comment has been minimized.

Sign in to view
CopilotAI
review requested due to automatic review settings
April 30, 2026 00:04
CopilotAI
reviewed
Apr 30, 2026
View reviewed changes
CopilotAI
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 wires DacDbiImpl.GetModuleSimpleName to the cDAC ILoader contract (instead of delegating only to legacy DAC), and adds dump-test coverage to ensure module simple names are available in dump-based scenarios.
Changes:
  • Replace ILoader.TryGetSimpleName with ILoader.GetSimpleName and update the contract implementation/docs accordingly.
  • Implement DacDbiImpl.GetModuleSimpleName using ILoader.GetSimpleName with DEBUG-only validation against legacy (when available).
  • Add/update unit + dump tests for the new simple-name retrieval behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
FileDescription
src/native/managed/cdac/tests/LoaderTests.csUpdates loader unit tests to use GetSimpleName instead of TryGetSimpleName.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csSwitches debuggee and adds a dump test asserting GetModuleSimpleName returns S_OK and a non-empty name per module.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/NativeStringHolder.csFixes the namespace and aligns HRESULT usage for the native string-holder test helper.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName via the cDAC loader contract and adds DEBUG validation against legacy output.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.csUpdates IXCLRDataModule.GetName to use ILoader.GetSimpleName.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.csImplements ILoader.GetSimpleName in the Loader v1 contract.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.csChanges the public ILoader interface surface to GetSimpleName.
docs/design/datacontracts/Loader.mdUpdates contract documentation to reflect GetSimpleName.
Comment thread...native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs
Outdated

Comment thread...tive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
Comment threadsrc/native/managed/cdac/tests/LoaderTests.cs
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@github-actions
github-actionsBot
commented
Apr 30, 2026

Copy link


Copy Markdown

Contributor
🤖 Copilot Code Review — PR #127415
Note
This review was generated by Copilot.
Holistic Assessment
Motivation: The change makes ILoader.GetSimpleName handle a null SimpleName pointer gracefully by returning string.Empty instead of likely crashing in ReadUtf8String. This is a real scenario (modules without simple names) and is clearly needed.
Approach: The fix follows the exact same pattern already used by GetPath (line 461) and GetFileName (line 469) in the same file — a ternary null check returning string.Empty. This is the right approach: consistent, minimal, and correct.
Summary: ✅ LGTM. The implementation is consistent with sibling methods, properly tested (including the null case and the existing happy path), and the dump test now validates cDAC/DacDbi agreement. No blocking issues found.
Detailed Findings
✅ Correctness — Null check follows established pattern
The ternary pattern:
returnmodule.SimpleName!=TargetPointer.Null?_target.ReadUtf8String(module.SimpleName,strict:true):string.Empty;
is identical to GetPath and GetFileName immediately below it. This is correct and consistent.
✅ Test Coverage — Both positive and null cases covered
The PR:
  • Keeps the existing GetSimpleName test for the happy path (non-null simple name)
  • Adds GetSimpleName_NullSimpleName which explicitly creates a module without a simple name and verifies string.Empty is returned
  • Adds an assertion in the dump test comparing DacDbi output to the cDAC contract output (Assert.Equal(loader.GetSimpleName(module), holder.Value))
The test refactoring also correctly removes the unnecessary inner scope block from the original test, improving readability.
✅ Dump test comment update — Accurate
The comment update (removing "which loads assemblies from multiple ALCs") is a minor doc improvement that keeps the comment accurate after the debuggee change.
💡 Minor observation — Test structure consistency
The existing GetFileName test (lines ~82-110) tests both the non-empty and empty cases within a single [Theory] method using two module addresses. The new GetSimpleName_NullSimpleName uses a separate method instead. Both patterns exist in this file, so this is fine — just noting the difference in style. No action needed.
Generated by Code Review for issue #127415 · ◷
@barosiak
barosiak
merged commit 9ef2b5e
into
mainApr 30, 2026

51 of 59 checks passed

@barosiak
barosiak
deleted the

copilot/implement-get-module-simple-name

branch
April 30, 2026 23:55
@dotnet-maestrodotnet-maestroBot
mentioned this pull request
May 1, 2026
 Merged

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 31, 2026
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@noahfalknoahfalknoahfalk approved these changes
@rcj1rcj1rcj1 approved these changes
@max-charlambmax-charlambAwaiting requested review from max-charlamb

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@barosiakbarosiak

Labels

area-Diagnostics-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@barosiak@am11@noahfalk@max-charlamb@rcj1

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[cDAC] Implement GetModuleSimpleName for cDAC - #127415
Merged
barosiak merged 10 commits into
mainfrom 
copilot/implement-get-module-simple-name
Apr 30, 2026
Merged
[cDAC] Implement GetModuleSimpleName for cDAC#127415
barosiak merged 10 commits into
mainfrom 
copilot/implement-get-module-simple-name
Conversation
@barosiak
@barosiakbarosiak
commented
Apr 24, 2026•

edited

Loading

Copy link


Copy Markdown

Member
Summary
Implement GetModuleSimpleName on DacDbiImpl using the ILoader contract, replacing legacy-only delegation.
Changes
  • DacDbiImpl.cs - Implement GetModuleSimpleName via ILoader.GetSimpleName
  • DacDbiLoaderDumpTests.cs - Add dump test verifying all modules return S_OK with a non-empty simple name
CopilotAIand others
added 2 commits
April 23, 2026 21:53
@barosiak
Implement GetModuleSimpleName on DacDbiImpl using ILoader contract
07d0209
Replace the legacy-delegation stub with a contract-based implementation
that uses ILoader.GetModuleHandleFromModulePtr and TryGetSimpleName.
Follows the GetAppDomainFullName pattern with try/catch and DEBUG
legacy validation. Add dump-based integration test.
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/d565028d-e96a-4a77-961a-592e8f591a34
Co-authored-by: barosiak <76071368+barosiak@users.noreply.github.com>
@barosiak
Code cleanup
baa787b
@barosiakbarosiak
self-assigned this
Apr 24, 2026
CopilotAI
review requested due to automatic review settings
April 24, 2026 23:18
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 24, 2026

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag

See info in area-owners.md if you want to be subscribed.
CopilotAI
reviewed
Apr 24, 2026
View reviewed changes
CopilotAI
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
Implements IDacDbiInterface.GetModuleSimpleName in the managed cDAC DBI (DacDbiImpl) using the ILoader contract, and adds a dump-based integration test to validate the API returns S_OK with a non-empty name for modules in the MultiModule full dump.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName using ILoader.GetModuleHandleFromModulePtr + ILoader.TryGetSimpleName.
  • Add a dump test that iterates all modules and asserts GetModuleSimpleName returns S_OK with a non-empty string.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csAdds contract-based implementation of GetModuleSimpleName (with DEBUG-time validation against legacy DAC).
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds integration coverage to ensure module simple names are returned successfully and are non-empty in dump scenarios.
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@github-actions

This comment has been minimized.

Sign in to view
noahfalk
noahfalk
reviewed
Apr 26, 2026
View reviewed changes
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Outdated

CopilotAI
review requested due to automatic review settings
April 27, 2026 22:05
CopilotAI
reviewed
Apr 27, 2026
View reviewed changes
CopilotAI
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 implements DacDbiImpl.GetModuleSimpleName using the cDAC ILoader contract (TryGetSimpleName) instead of relying on legacy DAC delegation, and adds a dump-based integration test to ensure module simple names are available and non-empty.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName via ILoader.GetModuleHandleFromModulePtr + ILoader.TryGetSimpleName, including DEBUG-only validation against legacy output.
  • Add a dump test that iterates all modules in the MultiModule full dump and asserts GetModuleSimpleName returns S_OK and a non-empty string for each.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName using the loader contract and adds DEBUG-only string comparison against legacy DAC output.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds a dump integration test validating GetModuleSimpleName succeeds and returns a non-empty name for all modules.
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Outdated

Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@barosiak
barosiak
requested a review
from noahfalkApril 28, 2026 00:09
CopilotAI
review requested due to automatic review settings
April 28, 2026 18:29
CopilotAI
reviewed
Apr 28, 2026
View reviewed changes
CopilotAI
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
Implements DacDbiImpl.GetModuleSimpleName using the cDAC ILoader contract (TryGetSimpleName) instead of delegating to the legacy DAC-only implementation, and adds dump-based coverage to ensure module simple names are returned successfully.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName via Target.Contracts.Loader.TryGetSimpleName and copy into the native IStringHolder.
  • Add a dump test that calls GetModuleSimpleName for all modules and asserts S_OK with a non-empty result.
  • Fix NativeStringHolder’s namespace to align with the Legacy assembly and simplify the HResults reference.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName using the ILoader contract and retains DEBUG cross-checking against legacy.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds dump-based integration test coverage for GetModuleSimpleName.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/NativeStringHolder.csAligns helper type namespace with the Legacy assembly and updates HRESULT reference.
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
@github-actions

This comment has been minimized.

Sign in to view
CopilotAI
review requested due to automatic review settings
April 30, 2026 00:04
CopilotAI
reviewed
Apr 30, 2026
View reviewed changes
CopilotAI
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 wires DacDbiImpl.GetModuleSimpleName to the cDAC ILoader contract (instead of delegating only to legacy DAC), and adds dump-test coverage to ensure module simple names are available in dump-based scenarios.
Changes:
  • Replace ILoader.TryGetSimpleName with ILoader.GetSimpleName and update the contract implementation/docs accordingly.
  • Implement DacDbiImpl.GetModuleSimpleName using ILoader.GetSimpleName with DEBUG-only validation against legacy (when available).
  • Add/update unit + dump tests for the new simple-name retrieval behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
FileDescription
src/native/managed/cdac/tests/LoaderTests.csUpdates loader unit tests to use GetSimpleName instead of TryGetSimpleName.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csSwitches debuggee and adds a dump test asserting GetModuleSimpleName returns S_OK and a non-empty name per module.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/NativeStringHolder.csFixes the namespace and aligns HRESULT usage for the native string-holder test helper.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName via the cDAC loader contract and adds DEBUG validation against legacy output.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.csUpdates IXCLRDataModule.GetName to use ILoader.GetSimpleName.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.csImplements ILoader.GetSimpleName in the Loader v1 contract.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.csChanges the public ILoader interface surface to GetSimpleName.
docs/design/datacontracts/Loader.mdUpdates contract documentation to reflect GetSimpleName.
Comment thread...native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs
Outdated

Comment thread...tive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
Comment threadsrc/native/managed/cdac/tests/LoaderTests.cs
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@github-actions
github-actionsBot
commented
Apr 30, 2026

Copy link


Copy Markdown

Contributor
🤖 Copilot Code Review — PR #127415
Note
This review was generated by Copilot.
Holistic Assessment
Motivation: The change makes ILoader.GetSimpleName handle a null SimpleName pointer gracefully by returning string.Empty instead of likely crashing in ReadUtf8String. This is a real scenario (modules without simple names) and is clearly needed.
Approach: The fix follows the exact same pattern already used by GetPath (line 461) and GetFileName (line 469) in the same file — a ternary null check returning string.Empty. This is the right approach: consistent, minimal, and correct.
Summary: ✅ LGTM. The implementation is consistent with sibling methods, properly tested (including the null case and the existing happy path), and the dump test now validates cDAC/DacDbi agreement. No blocking issues found.
Detailed Findings
✅ Correctness — Null check follows established pattern
The ternary pattern:
returnmodule.SimpleName!=TargetPointer.Null?_target.ReadUtf8String(module.SimpleName,strict:true):string.Empty;
is identical to GetPath and GetFileName immediately below it. This is correct and consistent.
✅ Test Coverage — Both positive and null cases covered
The PR:
  • Keeps the existing GetSimpleName test for the happy path (non-null simple name)
  • Adds GetSimpleName_NullSimpleName which explicitly creates a module without a simple name and verifies string.Empty is returned
  • Adds an assertion in the dump test comparing DacDbi output to the cDAC contract output (Assert.Equal(loader.GetSimpleName(module), holder.Value))
The test refactoring also correctly removes the unnecessary inner scope block from the original test, improving readability.
✅ Dump test comment update — Accurate
The comment update (removing "which loads assemblies from multiple ALCs") is a minor doc improvement that keeps the comment accurate after the debuggee change.
💡 Minor observation — Test structure consistency
The existing GetFileName test (lines ~82-110) tests both the non-empty and empty cases within a single [Theory] method using two module addresses. The new GetSimpleName_NullSimpleName uses a separate method instead. Both patterns exist in this file, so this is fine — just noting the difference in style. No action needed.
Generated by Code Review for issue #127415 · ◷
@barosiak
barosiak
merged commit 9ef2b5e
into
mainApr 30, 2026

51 of 59 checks passed

@barosiak
barosiak
deleted the

copilot/implement-get-module-simple-name

branch
April 30, 2026 23:55
@dotnet-maestrodotnet-maestroBot
mentioned this pull request
May 1, 2026
 Merged

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 31, 2026
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@noahfalknoahfalknoahfalk approved these changes
@rcj1rcj1rcj1 approved these changes
@max-charlambmax-charlambAwaiting requested review from max-charlamb

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@barosiakbarosiak

Labels

area-Diagnostics-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@barosiak@am11@noahfalk@max-charlamb@rcj1

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[cDAC] Implement GetModuleSimpleName for cDAC - #127415
Merged
barosiak merged 10 commits into
mainfrom 
copilot/implement-get-module-simple-name
Apr 30, 2026
Merged
[cDAC] Implement GetModuleSimpleName for cDAC#127415
barosiak merged 10 commits into
mainfrom 
copilot/implement-get-module-simple-name
Conversation
@barosiak
@barosiakbarosiak
commented
Apr 24, 2026•

edited

Loading

Copy link


Copy Markdown

Member
Summary
Implement GetModuleSimpleName on DacDbiImpl using the ILoader contract, replacing legacy-only delegation.
Changes
  • DacDbiImpl.cs - Implement GetModuleSimpleName via ILoader.GetSimpleName
  • DacDbiLoaderDumpTests.cs - Add dump test verifying all modules return S_OK with a non-empty simple name
CopilotAIand others
added 2 commits
April 23, 2026 21:53
@barosiak
Implement GetModuleSimpleName on DacDbiImpl using ILoader contract
07d0209
Replace the legacy-delegation stub with a contract-based implementation
that uses ILoader.GetModuleHandleFromModulePtr and TryGetSimpleName.
Follows the GetAppDomainFullName pattern with try/catch and DEBUG
legacy validation. Add dump-based integration test.
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/d565028d-e96a-4a77-961a-592e8f591a34
Co-authored-by: barosiak <76071368+barosiak@users.noreply.github.com>
@barosiak
Code cleanup
baa787b
@barosiakbarosiak
self-assigned this
Apr 24, 2026
CopilotAI
review requested due to automatic review settings
April 24, 2026 23:18
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 24, 2026

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag

See info in area-owners.md if you want to be subscribed.
CopilotAI
reviewed
Apr 24, 2026
View reviewed changes
CopilotAI
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
Implements IDacDbiInterface.GetModuleSimpleName in the managed cDAC DBI (DacDbiImpl) using the ILoader contract, and adds a dump-based integration test to validate the API returns S_OK with a non-empty name for modules in the MultiModule full dump.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName using ILoader.GetModuleHandleFromModulePtr + ILoader.TryGetSimpleName.
  • Add a dump test that iterates all modules and asserts GetModuleSimpleName returns S_OK with a non-empty string.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csAdds contract-based implementation of GetModuleSimpleName (with DEBUG-time validation against legacy DAC).
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds integration coverage to ensure module simple names are returned successfully and are non-empty in dump scenarios.
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@github-actions

This comment has been minimized.

Sign in to view
noahfalk
noahfalk
reviewed
Apr 26, 2026
View reviewed changes
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Outdated

CopilotAI
review requested due to automatic review settings
April 27, 2026 22:05
CopilotAI
reviewed
Apr 27, 2026
View reviewed changes
CopilotAI
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 implements DacDbiImpl.GetModuleSimpleName using the cDAC ILoader contract (TryGetSimpleName) instead of relying on legacy DAC delegation, and adds a dump-based integration test to ensure module simple names are available and non-empty.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName via ILoader.GetModuleHandleFromModulePtr + ILoader.TryGetSimpleName, including DEBUG-only validation against legacy output.
  • Add a dump test that iterates all modules in the MultiModule full dump and asserts GetModuleSimpleName returns S_OK and a non-empty string for each.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName using the loader contract and adds DEBUG-only string comparison against legacy DAC output.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds a dump integration test validating GetModuleSimpleName succeeds and returns a non-empty name for all modules.
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Outdated

Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@barosiak
barosiak
requested a review
from noahfalkApril 28, 2026 00:09
CopilotAI
review requested due to automatic review settings
April 28, 2026 18:29
CopilotAI
reviewed
Apr 28, 2026
View reviewed changes
CopilotAI
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
Implements DacDbiImpl.GetModuleSimpleName using the cDAC ILoader contract (TryGetSimpleName) instead of delegating to the legacy DAC-only implementation, and adds dump-based coverage to ensure module simple names are returned successfully.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName via Target.Contracts.Loader.TryGetSimpleName and copy into the native IStringHolder.
  • Add a dump test that calls GetModuleSimpleName for all modules and asserts S_OK with a non-empty result.
  • Fix NativeStringHolder’s namespace to align with the Legacy assembly and simplify the HResults reference.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName using the ILoader contract and retains DEBUG cross-checking against legacy.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds dump-based integration test coverage for GetModuleSimpleName.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/NativeStringHolder.csAligns helper type namespace with the Legacy assembly and updates HRESULT reference.
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
@github-actions

This comment has been minimized.

Sign in to view
CopilotAI
review requested due to automatic review settings
April 30, 2026 00:04
CopilotAI
reviewed
Apr 30, 2026
View reviewed changes
CopilotAI
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 wires DacDbiImpl.GetModuleSimpleName to the cDAC ILoader contract (instead of delegating only to legacy DAC), and adds dump-test coverage to ensure module simple names are available in dump-based scenarios.
Changes:
  • Replace ILoader.TryGetSimpleName with ILoader.GetSimpleName and update the contract implementation/docs accordingly.
  • Implement DacDbiImpl.GetModuleSimpleName using ILoader.GetSimpleName with DEBUG-only validation against legacy (when available).
  • Add/update unit + dump tests for the new simple-name retrieval behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
FileDescription
src/native/managed/cdac/tests/LoaderTests.csUpdates loader unit tests to use GetSimpleName instead of TryGetSimpleName.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csSwitches debuggee and adds a dump test asserting GetModuleSimpleName returns S_OK and a non-empty name per module.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/NativeStringHolder.csFixes the namespace and aligns HRESULT usage for the native string-holder test helper.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName via the cDAC loader contract and adds DEBUG validation against legacy output.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.csUpdates IXCLRDataModule.GetName to use ILoader.GetSimpleName.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.csImplements ILoader.GetSimpleName in the Loader v1 contract.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.csChanges the public ILoader interface surface to GetSimpleName.
docs/design/datacontracts/Loader.mdUpdates contract documentation to reflect GetSimpleName.
Comment thread...native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs
Outdated

Comment thread...tive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
Comment threadsrc/native/managed/cdac/tests/LoaderTests.cs
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@github-actions
github-actionsBot
commented
Apr 30, 2026

Copy link


Copy Markdown

Contributor
🤖 Copilot Code Review — PR #127415
Note
This review was generated by Copilot.
Holistic Assessment
Motivation: The change makes ILoader.GetSimpleName handle a null SimpleName pointer gracefully by returning string.Empty instead of likely crashing in ReadUtf8String. This is a real scenario (modules without simple names) and is clearly needed.
Approach: The fix follows the exact same pattern already used by GetPath (line 461) and GetFileName (line 469) in the same file — a ternary null check returning string.Empty. This is the right approach: consistent, minimal, and correct.
Summary: ✅ LGTM. The implementation is consistent with sibling methods, properly tested (including the null case and the existing happy path), and the dump test now validates cDAC/DacDbi agreement. No blocking issues found.
Detailed Findings
✅ Correctness — Null check follows established pattern
The ternary pattern:
returnmodule.SimpleName!=TargetPointer.Null?_target.ReadUtf8String(module.SimpleName,strict:true):string.Empty;
is identical to GetPath and GetFileName immediately below it. This is correct and consistent.
✅ Test Coverage — Both positive and null cases covered
The PR:
  • Keeps the existing GetSimpleName test for the happy path (non-null simple name)
  • Adds GetSimpleName_NullSimpleName which explicitly creates a module without a simple name and verifies string.Empty is returned
  • Adds an assertion in the dump test comparing DacDbi output to the cDAC contract output (Assert.Equal(loader.GetSimpleName(module), holder.Value))
The test refactoring also correctly removes the unnecessary inner scope block from the original test, improving readability.
✅ Dump test comment update — Accurate
The comment update (removing "which loads assemblies from multiple ALCs") is a minor doc improvement that keeps the comment accurate after the debuggee change.
💡 Minor observation — Test structure consistency
The existing GetFileName test (lines ~82-110) tests both the non-empty and empty cases within a single [Theory] method using two module addresses. The new GetSimpleName_NullSimpleName uses a separate method instead. Both patterns exist in this file, so this is fine — just noting the difference in style. No action needed.
Generated by Code Review for issue #127415 · ◷
@barosiak
barosiak
merged commit 9ef2b5e
into
mainApr 30, 2026

51 of 59 checks passed

@barosiak
barosiak
deleted the

copilot/implement-get-module-simple-name

branch
April 30, 2026 23:55
@dotnet-maestrodotnet-maestroBot
mentioned this pull request
May 1, 2026
 Merged

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 31, 2026
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@noahfalknoahfalknoahfalk approved these changes
@rcj1rcj1rcj1 approved these changes
@max-charlambmax-charlambAwaiting requested review from max-charlamb

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@barosiakbarosiak

Labels

area-Diagnostics-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@barosiak@am11@noahfalk@max-charlamb@rcj1

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
[cDAC] Implement GetModuleSimpleName for cDAC - #127415
Merged
barosiak merged 10 commits into
mainfrom 
copilot/implement-get-module-simple-name
Apr 30, 2026
Merged
[cDAC] Implement GetModuleSimpleName for cDAC#127415
barosiak merged 10 commits into
mainfrom 
copilot/implement-get-module-simple-name
Conversation
@barosiak
@barosiakbarosiak
commented
Apr 24, 2026•

edited

Loading

Copy link


Copy Markdown

Member
Summary
Implement GetModuleSimpleName on DacDbiImpl using the ILoader contract, replacing legacy-only delegation.
Changes
  • DacDbiImpl.cs - Implement GetModuleSimpleName via ILoader.GetSimpleName
  • DacDbiLoaderDumpTests.cs - Add dump test verifying all modules return S_OK with a non-empty simple name
CopilotAIand others
added 2 commits
April 23, 2026 21:53
@barosiak
Implement GetModuleSimpleName on DacDbiImpl using ILoader contract
07d0209
Replace the legacy-delegation stub with a contract-based implementation
that uses ILoader.GetModuleHandleFromModulePtr and TryGetSimpleName.
Follows the GetAppDomainFullName pattern with try/catch and DEBUG
legacy validation. Add dump-based integration test.
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/d565028d-e96a-4a77-961a-592e8f591a34
Co-authored-by: barosiak <76071368+barosiak@users.noreply.github.com>
@barosiak
Code cleanup
baa787b
@barosiakbarosiak
self-assigned this
Apr 24, 2026
CopilotAI
review requested due to automatic review settings
April 24, 2026 23:18
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 24, 2026

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag

See info in area-owners.md if you want to be subscribed.
CopilotAI
reviewed
Apr 24, 2026
View reviewed changes
CopilotAI
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
Implements IDacDbiInterface.GetModuleSimpleName in the managed cDAC DBI (DacDbiImpl) using the ILoader contract, and adds a dump-based integration test to validate the API returns S_OK with a non-empty name for modules in the MultiModule full dump.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName using ILoader.GetModuleHandleFromModulePtr + ILoader.TryGetSimpleName.
  • Add a dump test that iterates all modules and asserts GetModuleSimpleName returns S_OK with a non-empty string.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csAdds contract-based implementation of GetModuleSimpleName (with DEBUG-time validation against legacy DAC).
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds integration coverage to ensure module simple names are returned successfully and are non-empty in dump scenarios.
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@github-actions

This comment has been minimized.

Sign in to view
noahfalk
noahfalk
reviewed
Apr 26, 2026
View reviewed changes
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Outdated

CopilotAI
review requested due to automatic review settings
April 27, 2026 22:05
CopilotAI
reviewed
Apr 27, 2026
View reviewed changes
CopilotAI
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 implements DacDbiImpl.GetModuleSimpleName using the cDAC ILoader contract (TryGetSimpleName) instead of relying on legacy DAC delegation, and adds a dump-based integration test to ensure module simple names are available and non-empty.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName via ILoader.GetModuleHandleFromModulePtr + ILoader.TryGetSimpleName, including DEBUG-only validation against legacy output.
  • Add a dump test that iterates all modules in the MultiModule full dump and asserts GetModuleSimpleName returns S_OK and a non-empty string for each.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName using the loader contract and adds DEBUG-only string comparison against legacy DAC output.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds a dump integration test validating GetModuleSimpleName succeeds and returns a non-empty name for all modules.
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
Outdated

Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@barosiak
barosiak
requested a review
from noahfalkApril 28, 2026 00:09
CopilotAI
review requested due to automatic review settings
April 28, 2026 18:29
CopilotAI
reviewed
Apr 28, 2026
View reviewed changes
CopilotAI
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
Implements DacDbiImpl.GetModuleSimpleName using the cDAC ILoader contract (TryGetSimpleName) instead of delegating to the legacy DAC-only implementation, and adds dump-based coverage to ensure module simple names are returned successfully.
Changes:
  • Implement DacDbiImpl.GetModuleSimpleName via Target.Contracts.Loader.TryGetSimpleName and copy into the native IStringHolder.
  • Add a dump test that calls GetModuleSimpleName for all modules and asserts S_OK with a non-empty result.
  • Fix NativeStringHolder’s namespace to align with the Legacy assembly and simplify the HResults reference.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
FileDescription
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName using the ILoader contract and retains DEBUG cross-checking against legacy.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csAdds dump-based integration test coverage for GetModuleSimpleName.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/NativeStringHolder.csAligns helper type namespace with the Legacy assembly and updates HRESULT reference.
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
@github-actions

This comment has been minimized.

Sign in to view
CopilotAI
review requested due to automatic review settings
April 30, 2026 00:04
CopilotAI
reviewed
Apr 30, 2026
View reviewed changes
CopilotAI
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 wires DacDbiImpl.GetModuleSimpleName to the cDAC ILoader contract (instead of delegating only to legacy DAC), and adds dump-test coverage to ensure module simple names are available in dump-based scenarios.
Changes:
  • Replace ILoader.TryGetSimpleName with ILoader.GetSimpleName and update the contract implementation/docs accordingly.
  • Implement DacDbiImpl.GetModuleSimpleName using ILoader.GetSimpleName with DEBUG-only validation against legacy (when available).
  • Add/update unit + dump tests for the new simple-name retrieval behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
FileDescription
src/native/managed/cdac/tests/LoaderTests.csUpdates loader unit tests to use GetSimpleName instead of TryGetSimpleName.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.csSwitches debuggee and adds a dump test asserting GetModuleSimpleName returns S_OK and a non-empty name per module.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/NativeStringHolder.csFixes the namespace and aligns HRESULT usage for the native string-holder test helper.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.csImplements GetModuleSimpleName via the cDAC loader contract and adds DEBUG validation against legacy output.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.csUpdates IXCLRDataModule.GetName to use ILoader.GetSimpleName.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.csImplements ILoader.GetSimpleName in the Loader v1 contract.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.csChanges the public ILoader interface surface to GetSimpleName.
docs/design/datacontracts/Loader.mdUpdates contract documentation to reflect GetSimpleName.
Comment thread...native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs
Outdated

Comment thread...tive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
Comment threadsrc/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs
Comment threadsrc/native/managed/cdac/tests/LoaderTests.cs
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs
@github-actions
github-actionsBot
commented
Apr 30, 2026

Copy link


Copy Markdown

Contributor
🤖 Copilot Code Review — PR #127415
Note
This review was generated by Copilot.
Holistic Assessment
Motivation: The change makes ILoader.GetSimpleName handle a null SimpleName pointer gracefully by returning string.Empty instead of likely crashing in ReadUtf8String. This is a real scenario (modules without simple names) and is clearly needed.
Approach: The fix follows the exact same pattern already used by GetPath (line 461) and GetFileName (line 469) in the same file — a ternary null check returning string.Empty. This is the right approach: consistent, minimal, and correct.
Summary: ✅ LGTM. The implementation is consistent with sibling methods, properly tested (including the null case and the existing happy path), and the dump test now validates cDAC/DacDbi agreement. No blocking issues found.
Detailed Findings
✅ Correctness — Null check follows established pattern
The ternary pattern:
returnmodule.SimpleName!=TargetPointer.Null?_target.ReadUtf8String(module.SimpleName,strict:true):string.Empty;
is identical to GetPath and GetFileName immediately below it. This is correct and consistent.
✅ Test Coverage — Both positive and null cases covered
The PR:
  • Keeps the existing GetSimpleName test for the happy path (non-null simple name)
  • Adds GetSimpleName_NullSimpleName which explicitly creates a module without a simple name and verifies string.Empty is returned
  • Adds an assertion in the dump test comparing DacDbi output to the cDAC contract output (Assert.Equal(loader.GetSimpleName(module), holder.Value))
The test refactoring also correctly removes the unnecessary inner scope block from the original test, improving readability.
✅ Dump test comment update — Accurate
The comment update (removing "which loads assemblies from multiple ALCs") is a minor doc improvement that keeps the comment accurate after the debuggee change.
💡 Minor observation — Test structure consistency
The existing GetFileName test (lines ~82-110) tests both the non-empty and empty cases within a single [Theory] method using two module addresses. The new GetSimpleName_NullSimpleName uses a separate method instead. Both patterns exist in this file, so this is fine — just noting the difference in style. No action needed.
Generated by Code Review for issue #127415 · ◷
@barosiak
barosiak
merged commit 9ef2b5e
into
mainApr 30, 2026

51 of 59 checks passed

@barosiak
barosiak
deleted the

copilot/implement-get-module-simple-name

branch
April 30, 2026 23:55
@dotnet-maestrodotnet-maestroBot
mentioned this pull request
May 1, 2026
 Merged

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 31, 2026
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@noahfalknoahfalknoahfalk approved these changes
@rcj1rcj1rcj1 approved these changes
@max-charlambmax-charlambAwaiting requested review from max-charlamb

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@barosiakbarosiak

Labels

area-Diagnostics-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@barosiak@am11@noahfalk@max-charlamb@rcj1