[cDAC] Implement GetModuleSimpleName for cDAC by barosiak · Pull Request #127415 · dotnet/runtime

barosiak · 2026-04-24T23:18:14Z

Summary

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

Changes

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

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>

dotnet-policy-service · 2026-04-24T23:20:48Z

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

Copilot

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.

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

Copilot

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.

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

Copilot

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.

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

github-actions · 2026-04-28T18:45:19Z

🤖 Copilot Code Review — PR #127415

Note

This review was AI/Copilot-generated. Models contributing: Claude Opus 4.6 (primary), Claude Sonnet 4.5.

Holistic Assessment

Motivation: The PR replaces a legacy-delegation stub for GetModuleSimpleName with a contract-based implementation using ILoader. This is part of the ongoing cDAC effort to move away from legacy DAC dependencies — the motivation is clear and the change is well-justified.

Approach: The implementation correctly follows the established try/catch + #if DEBUG legacy validation pattern used by sibling methods (GetAppDomainFullName, GetAssemblyPath, GetModulePath). The NativeStringHolder move from tests to the Legacy library is a sensible de-duplication. The approach is sound.

Summary: ✅ LGTM. The implementation is correct, follows established patterns, and includes both DEBUG validation and a dump-based integration test. One minor advisory note below about test strictness, but it is not blocking.

Detailed Findings

✅ Correctness & Pattern Consistency — Verified

The new GetModuleSimpleName implementation correctly:

Uses ILoader.GetModuleHandleFromModulePtr + TryGetSimpleName to retrieve the module name via the cDAC contract path
Returns E_FAIL when TryGetSimpleName returns false (null SimpleName pointer)
Propagates StringHolderAssignCopy HRESULT on success
Catches all exceptions via catch (System.Exception ex) and returns ex.HResult
Matches the established pattern of sibling methods in this file

The DEBUG validation block is an improvement over the existing GetAppDomainFullName validation (line 140), which passes the actual pStrName to the legacy call — overwriting the cDAC result in the caller's string holder. This PR correctly uses a separate NativeStringHolder for the legacy comparison, comparing strings independently without side effects.

✅ NativeStringHolder Move — Correctly Executed

Namespace changed from DumpTests to Legacy (appropriate)
System.HResults.S_OK simplified to HResults.S_OK (correct — the Legacy csproj links Common/src/System/HResults.cs)
The Legacy csproj uses InternalsVisibleTo for the DumpTests project (confirmed), so the internal sealed class remains accessible to tests
Uses default glob-based compilation — no csproj <Compile> entry needed

✅ Error Handling — Complete

All error paths are handled: contract failures throw exceptions caught by the catch block, TryGetSimpleName returning false maps to E_FAIL, and StringHolderAssignCopy failures propagate through the HRESULT return. No resource leaks — NativeStringHolder is properly using-disposed in the DEBUG block.

✅ Test Quality — Good

The new GetModuleSimpleName_ReturnsNonEmpty test follows the exact same pattern as sibling tests (GetModuleData_ReturnsValidFields, GetAppDomainFullName_ReturnsNonEmpty): iterates all modules from the MultiModule dump, asserts S_OK, and confirms non-empty values. The testedAtLeastOne guard ensures meaningful execution.

💡 Advisory: Test Strictness Assumption

The test asserts S_OK for every module in the dump. The implementation returns E_FAIL when TryGetSimpleName returns false (module has null SimpleName pointer, e.g., potentially dynamic or in-memory modules). This is fine for the current MultiModule debuggee (which is expected to have simple names for all loaded modules), but if the dump ever includes modules without simple names, the test would fail. This is a minor robustness consideration for the future, not a blocking concern.

Generated by Code Review for issue #127415 · ◷

Copilot

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

File	Description
src/native/managed/cdac/tests/LoaderTests.cs	Updates loader unit tests to use `GetSimpleName` instead of `TryGetSimpleName`.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs	Switches 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.cs	Fixes the namespace and aligns HRESULT usage for the native string-holder test helper.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements `GetModuleSimpleName` via the cDAC loader contract and adds DEBUG validation against legacy output.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs	Updates `IXCLRDataModule.GetName` to use `ILoader.GetSimpleName`.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs	Implements `ILoader.GetSimpleName` in the Loader v1 contract.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs	Changes the public `ILoader` interface surface to `GetSimpleName`.
docs/design/datacontracts/Loader.md	Updates contract documentation to reflect `GetSimpleName`.

+
+        return _target.ReadUtf8String(module.SimpleName, strict: true);


@@ -103,7 +103,7 @@ public interface ILoader : IContract
    bool IsProbeExtensionResultValid(ModuleHandle handle) => throw new NotImplementedException();
    ModuleFlags GetFlags(ModuleHandle handle) => throw new NotImplementedException();
    bool IsReadyToRun(ModuleHandle handle) => throw new NotImplementedException();


 /// <summary>
 /// Dump-based integration tests for DacDbiImpl loader, assembly, and module methods.
-/// Uses the MultiModule debuggee (full dump), which loads assemblies from multiple ALCs.
+/// Uses the StackRefs debuggee (full dump), which loads assemblies from multiple ALCs.
 /// </summary>
 public class DacDbiLoaderDumpTests : DumpTestBase
 {
-    protected override string DebuggeeName => "MultiModule";
+    protected override string DebuggeeName => "StackRefs";


+            TargetPointer moduleAddr = loader.GetModule(module);
+
+            using var holder = new NativeStringHolder();
+            int hr = dbi.GetModuleSimpleName(moduleAddr.Value, holder.Ptr);
+            Assert.Equal(System.HResults.S_OK, hr);
+            Assert.False(string.IsNullOrEmpty(holder.Value), "Module simple name should not be empty");


    [Theory]
    [ClassData(typeof(MockTarget.StdArch))]
-    public void TryGetSimpleName(MockTarget.Architecture arch)
+    public void GetSimpleName(MockTarget.Architecture arch)
    {
        string expected = "TestModule";
        TargetPointer moduleAddr = TargetPointer.Null;
-        TargetPointer moduleAddrEmptyName = TargetPointer.Null;

        ILoader contract = CreateLoaderContract(arch, loader =>
        {
            moduleAddr = loader.AddModule(simpleName: expected).Address;
-            moduleAddrEmptyName = loader.AddModule().Address;
        });

        {
            Contracts.ModuleHandle handle = contract.GetModuleHandleFromModulePtr(moduleAddr);
-            bool result = contract.TryGetSimpleName(handle, out string actual);
-            Assert.True(result);
+            string actual = contract.GetSimpleName(handle);
            Assert.Equal(expected, actual);
        }
-        {
-            Contracts.ModuleHandle handle = contract.GetModuleHandleFromModulePtr(moduleAddrEmptyName);
-            bool result = contract.TryGetSimpleName(handle, out string actual);
-            Assert.False(result);
-            Assert.Equal(string.Empty, actual);
-        }
    }


+            Contracts.ILoader loader = _target.Contracts.Loader;
+            Contracts.ModuleHandle handle = loader.GetModuleHandleFromModulePtr(new TargetPointer(vmModule));
+            cdacSimpleName = loader.GetSimpleName(handle);
+            hr = StringHolderAssignCopy(pStrFilename, cdacSimpleName);


Copilot AI and others added 2 commits April 23, 2026 21:53

Code cleanup

baa787b

barosiak requested review from max-charlamb, noahfalk and rcj1 April 24, 2026 23:18

barosiak self-assigned this Apr 24, 2026

Copilot AI review requested due to automatic review settings April 24, 2026 23:18

github-actions Bot added the area-Diagnostics-coreclr label Apr 24, 2026

Copilot started reviewing on behalf of barosiak April 24, 2026 23:19 View session

Merge branch 'main' into copilot/implement-get-module-simple-name

3871192

Copilot AI reviewed Apr 24, 2026

View reviewed changes

Comment thread src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs

Comment thread src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs

This comment has been minimized.

Sign in to view

am11 reviewed Apr 25, 2026

View reviewed changes

Comment thread src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs

noahfalk reviewed Apr 26, 2026

View reviewed changes

Comment thread src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs Outdated

barosiak and others added 2 commits April 27, 2026 12:44

Address pr comments

83df67b

Merge branch 'main' into copilot/implement-get-module-simple-name

3deb5d1

Copilot AI review requested due to automatic review settings April 27, 2026 22:05

Copilot started reviewing on behalf of barosiak April 27, 2026 22:07 View session

Copilot AI reviewed Apr 27, 2026

View reviewed changes

Comment thread src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs Outdated

Comment thread src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs

barosiak requested a review from noahfalk April 28, 2026 00:09

max-charlamb reviewed Apr 28, 2026

View reviewed changes

Comment thread src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs Outdated

De-duplicate NativeStringHolder

6a51bb6

barosiak requested a review from max-charlamb April 28, 2026 18:13

Merge branch 'main' into copilot/implement-get-module-simple-name

55b9e65

Copilot AI review requested due to automatic review settings April 28, 2026 18:29

Copilot started reviewing on behalf of barosiak April 28, 2026 18:29 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

Comment thread src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs

trygetsimplename -> getssimplename

b078810

rcj1 approved these changes Apr 29, 2026

View reviewed changes

noahfalk approved these changes Apr 29, 2026

View reviewed changes

Change debuggee

ce00eeb

Copilot AI review requested due to automatic review settings April 30, 2026 00:04

Copilot started reviewing on behalf of barosiak April 30, 2026 00:04 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cDAC] Implement GetModuleSimpleName for cDAC#127415

[cDAC] Implement GetModuleSimpleName for cDAC#127415
barosiak wants to merge 9 commits intomainfrom
copilot/implement-get-module-simple-name

barosiak commented Apr 24, 2026

Uh oh!

dotnet-policy-service Bot commented Apr 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

github-actions Bot commented Apr 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants


		return _target.ReadUtf8String(module.SimpleName, strict: true);

Conversation

barosiak commented Apr 24, 2026

Summary

Changes

Uh oh!

dotnet-policy-service Bot commented Apr 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

github-actions Bot commented Apr 28, 2026

🤖 Copilot Code Review — PR #127415

Holistic Assessment

Detailed Findings

✅ Correctness & Pattern Consistency — Verified

✅ NativeStringHolder Move — Correctly Executed

✅ Error Handling — Complete

✅ Test Quality — Good

💡 Advisory: Test Strictness Assumption

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants