TEST by rcj1 · Pull Request #127397 · dotnet/runtime

rcj1 · 2026-04-24T18:52:05Z

No description provided.

Copilot

Pull request overview

This PR updates a DEBUG-time assertion in GetMethodTableName to include additional diagnostic details when the legacy and new implementations disagree.

Changes:

Adds a formatted message to a Debug.Assert comparing needed sizes and method table names.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

…er.Legacy/SOSDacImpl.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

Copilot · 2026-04-24T19:22:34Z

+                if (!(pNeeded is null || *pNeeded == neededLocal))
+                {
+                    string localNameString = new(mtNameLocal, 0, localNameLength);
+                    string nameString = mtName is null ? string.Empty : new(mtName, 0, nameLength);
+                    Debug.Fail($"local name = {localNameString}, name = {nameString}");
+                }


The pNeeded validation uses a manual if + Debug.Fail(...), which is inconsistent with the surrounding pattern in this file (other callsites use Debug.Assert(pNeeded == null || *pNeeded == neededLocal, ...)). Consider switching back to Debug.Assert with a message and include the mismatched *pNeeded/neededLocal values (and optionally count) so the failure is actionable.

Suggested change

if (!(pNeeded is null || *pNeeded == neededLocal))

{

string localNameString = new(mtNameLocal, 0, localNameLength);

string nameString = mtName is null ? string.Empty : new(mtName, 0, nameLength);

Debug.Fail($"local name = {localNameString}, name = {nameString}");

}

string localNameString = new(mtNameLocal, 0, localNameLength);

string nameString = mtName is null ? string.Empty : new(mtName, 0, nameLength);

Debug.Assert(

pNeeded is null || *pNeeded == neededLocal,

$"pNeeded mismatch: actual={(pNeeded is null ? "null" : (*pNeeded).ToString())}, expected={neededLocal}, count={count}, local name = {localNameString}, name = {nameString}");

dotnet-policy-service · 2026-04-24T20:25:13Z

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

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

Copilot · 2026-04-25T05:18:14Z

@@ -2946,6 +2948,10 @@ int ISOSDacInterface.GetMethodTableName(ClrDataAddress mt, uint count, char* mtN
            Contracts.TypeHandle methodTableHandle = typeSystemContract.GetTypeHandle(mt.ToTargetPointer(_target, overrideCheck: true));
            if (typeSystemContract.IsFreeObjectMethodTable(methodTableHandle))
            {
+                fixed (char* pMtNametest = mtNametest)
+                {
+                    OutputBufferHelpers.CopyStringToBuffer(pMtNametest, counttest, pNeeded, "Free");
+                }


counttest is declared as int, but it's passed to APIs that require uint (OutputBufferHelpers.CopyStringToBuffer(..., uint bufferSize, ...) and ISOSDacInterface.GetMethodTableName(..., uint count, ...)). This won’t compile without an explicit cast; consider making this a const uint (or uint countTest = 4096u) to match the API surface.

Copilot · 2026-04-25T05:18:14Z

@@ -2946,6 +2948,10 @@ int ISOSDacInterface.GetMethodTableName(ClrDataAddress mt, uint count, char* mtN
            Contracts.TypeHandle methodTableHandle = typeSystemContract.GetTypeHandle(mt.ToTargetPointer(_target, overrideCheck: true));
            if (typeSystemContract.IsFreeObjectMethodTable(methodTableHandle))
            {
+                fixed (char* pMtNametest = mtNametest)
+                {
+                    OutputBufferHelpers.CopyStringToBuffer(pMtNametest, counttest, pNeeded, "Free");
+                }
                OutputBufferHelpers.CopyStringToBuffer(mtName, count, pNeeded, "Free");
            }
            else
@@ -2954,6 +2960,10 @@ int ISOSDacInterface.GetMethodTableName(ClrDataAddress mt, uint count, char* mtN
                Contracts.ModuleHandle moduleHandle = loader.GetModuleHandleFromModulePtr(modulePointer);
                if (!loader.TryGetLoadedImageContents(moduleHandle, out _, out _, out _))
                {
+                    fixed (char* pMtNametest = mtNametest)
+                    {
+                        OutputBufferHelpers.CopyStringToBuffer(pMtNametest, counttest, pNeeded, "<Unloaded Type>");
+                    }
                    OutputBufferHelpers.CopyStringToBuffer(mtName, count, pNeeded, "<Unloaded Type>");
                }
                else
@@ -2972,6 +2982,10 @@ int ISOSDacInterface.GetMethodTableName(ClrDataAddress mt, uint count, char* mtN
                            methodTableName.Append(fallbackName);
                        }
                    }
+                    fixed (char* pMtNametest = mtNametest)
+                    {
+                    OutputBufferHelpers.CopyStringToBuffer(pMtNametest, counttest, pNeeded, methodTableName.ToString());
+                    }
                    OutputBufferHelpers.CopyStringToBuffer(mtName, count, pNeeded, methodTableName.ToString());
                }


mtNametest/counttest and the extra CopyStringToBuffer calls are executed in all builds, but they are only used by the #if DEBUG validation logic later. This adds extra allocations and string copies to a potentially hot path; consider moving the temp buffer + copies under #if DEBUG (or using a stackalloc/Span-based approach guarded by DEBUG).

Copilot

Pull request overview

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

Copilot · 2026-04-25T06:08:21Z

@@ -2946,6 +2948,10 @@ int ISOSDacInterface.GetMethodTableName(ClrDataAddress mt, uint count, char* mtN
            Contracts.TypeHandle methodTableHandle = typeSystemContract.GetTypeHandle(mt.ToTargetPointer(_target, overrideCheck: true));
            if (typeSystemContract.IsFreeObjectMethodTable(methodTableHandle))
            {
+                fixed (char* pMtNametest = mtNametest)
+                {
+                    OutputBufferHelpers.CopyStringToBuffer(pMtNametest, counttest, pNeeded, "Free");
+                }
                OutputBufferHelpers.CopyStringToBuffer(mtName, count, pNeeded, "Free");
            }
            else
@@ -2954,6 +2960,10 @@ int ISOSDacInterface.GetMethodTableName(ClrDataAddress mt, uint count, char* mtN
                Contracts.ModuleHandle moduleHandle = loader.GetModuleHandleFromModulePtr(modulePointer);
                if (!loader.TryGetLoadedImageContents(moduleHandle, out _, out _, out _))
                {
+                    fixed (char* pMtNametest = mtNametest)
+                    {
+                        OutputBufferHelpers.CopyStringToBuffer(pMtNametest, counttest, pNeeded, "<Unloaded Type>");
+                    }
                    OutputBufferHelpers.CopyStringToBuffer(mtName, count, pNeeded, "<Unloaded Type>");
                }
                else
@@ -2972,6 +2982,10 @@ int ISOSDacInterface.GetMethodTableName(ClrDataAddress mt, uint count, char* mtN
                            methodTableName.Append(fallbackName);
                        }
                    }
+                    fixed (char* pMtNametest = mtNametest)
+                    {
+                    OutputBufferHelpers.CopyStringToBuffer(pMtNametest, counttest, pNeeded, methodTableName.ToString());
+                    }
                    OutputBufferHelpers.CopyStringToBuffer(mtName, count, pNeeded, methodTableName.ToString());
                }


mtNametest / counttest are allocated and used unconditionally (outside #if DEBUG), but they only appear to support the DEBUG-only legacy validation later. This adds extra buffer allocation and extra CopyStringToBuffer calls (and an extra ToString() allocation) to the release path. Consider moving the test buffer + its writes under #if DEBUG (or removing entirely) and caching methodTableName.ToString() into a local string once before copying to buffers.

Copilot · 2026-04-25T06:08:22Z

+            char[] mtNameLocal = new char[counttest];
            uint neededLocal;
            int hrLocal;
            fixed (char* ptr = mtNameLocal)
            {
-                hrLocal = _legacyImpl.GetMethodTableName(mt, count, ptr, &neededLocal);
+                hrLocal = _legacyImpl.GetMethodTableName(mt, counttest, ptr, &neededLocal);


In the #if DEBUG block, new char[counttest] uses a uint length, which does not compile. Use an int length (e.g., const int bufferLen = 4096; and allocate new char[bufferLen]) and pass the corresponding uint to the DAC API.

…er.Legacy/SOSDacImpl.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

Copilot · 2026-04-25T06:14:24Z

@@ -2946,6 +2948,10 @@ int ISOSDacInterface.GetMethodTableName(ClrDataAddress mt, uint count, char* mtN
            Contracts.TypeHandle methodTableHandle = typeSystemContract.GetTypeHandle(mt.ToTargetPointer(_target, overrideCheck: true));
            if (typeSystemContract.IsFreeObjectMethodTable(methodTableHandle))
            {
+                fixed (char* pMtNametest = mtNametest)
+                {
+                    OutputBufferHelpers.CopyStringToBuffer(pMtNametest, counttest, pNeeded, "Free");
+                }
                OutputBufferHelpers.CopyStringToBuffer(mtName, count, pNeeded, "Free");
            }
            else
@@ -2954,6 +2960,10 @@ int ISOSDacInterface.GetMethodTableName(ClrDataAddress mt, uint count, char* mtN
                Contracts.ModuleHandle moduleHandle = loader.GetModuleHandleFromModulePtr(modulePointer);
                if (!loader.TryGetLoadedImageContents(moduleHandle, out _, out _, out _))
                {
+                    fixed (char* pMtNametest = mtNametest)
+                    {
+                        OutputBufferHelpers.CopyStringToBuffer(pMtNametest, counttest, pNeeded, "<Unloaded Type>");
+                    }
                    OutputBufferHelpers.CopyStringToBuffer(mtName, count, pNeeded, "<Unloaded Type>");


mtNametest/counttest are allocated unconditionally and the method now performs extra CopyStringToBuffer calls into this temporary buffer in all builds. This adds per-call allocations and duplicate work, and also writes pNeeded twice. If this buffer is only meant for DEBUG validation, move the temp buffer + copy under #if DEBUG (or remove entirely).

Copilot · 2026-04-25T06:14:24Z

+            char[] mtNameLocal = new char[counttest];
            uint neededLocal;
            int hrLocal;
            fixed (char* ptr = mtNameLocal)
            {
-                hrLocal = _legacyImpl.GetMethodTableName(mt, count, ptr, &neededLocal);
+                hrLocal = _legacyImpl.GetMethodTableName(mt, counttest, ptr, &neededLocal);
            }
            Debug.ValidateHResult(hr, hrLocal);
            if (hr == HResults.S_OK)
            {
-                Debug.Assert(pNeeded == null || *pNeeded == neededLocal);
-                Debug.Assert(mtName == null || new ReadOnlySpan<char>(mtNameLocal, 0, (int)neededLocal - 1).SequenceEqual(new string(mtName)));
+                int maxComparableLength = 4095;
+                ReadOnlySpan<char> localComparableSpan = mtNameLocal.AsSpan(0, maxComparableLength);
+                int localNameLength = localComparableSpan.IndexOf('\0');
+                if (localNameLength < 0)
+                {
+                    localNameLength = maxComparableLength;
+                }
+
+                int nameLength = 0;
+                if (mtName is not null)
+                {
+                    ReadOnlySpan<char> comparableSpan = mtNametest.AsSpan(0, maxComparableLength);
+                    nameLength = comparableSpan.IndexOf('\0');
+                    if (nameLength < 0)
+                    {
+                        nameLength = maxComparableLength;
+                    }
+                }
+
+                if (!(pNeeded is null || *pNeeded == neededLocal))
+                {
+                    string localNameString = new(mtNameLocal, 0, localNameLength);
+                    string nameString = mtNametest is null ? string.Empty : new(mtNametest, 0, nameLength);
+                    Debug.Fail($"local name = {localNameString}, name = {nameString}, neededlocal = {neededLocal}, pneeded = {(pNeeded == null ? "null" : *pNeeded)}");
+                }


The DEBUG cross-check no longer validates that the produced name matches the legacy DAC output (it only checks pNeeded), and it uses a hard-coded counttest when calling _legacyImpl instead of using the caller-provided count. This deviates from the established pattern in this file (e.g., GetAppDomainName at SOSDacImpl.cs:284-300) and can mask output mismatches. Consider restoring name comparison and passing count through, while making the comparison robust against truncation (e.g., compare up to the first \0 or Math.Min(count, neededLocal)).

Copilot · 2026-04-25T06:14:24Z

+                if (!(pNeeded is null || *pNeeded == neededLocal))
+                {
+                    string localNameString = new(mtNameLocal, 0, localNameLength);
+                    string nameString = mtNametest is null ? string.Empty : new(mtNametest, 0, nameLength);


mtNametest is a local array and can never be null, so mtNametest is null ? ... is dead code. If the intent is to avoid reading from an uninitialized buffer, gate on whether the temp buffer was actually populated or (preferably) build the string from the actual output pointer (mtName) when non-null.

Suggested change

string nameString = mtNametest is null ? string.Empty : new(mtNametest, 0, nameLength);

string nameString = mtName is null ? string.Empty : new(mtNametest, 0, nameLength);

TEST

6612fac

rcj1 added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 24, 2026

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

rcj1 added the NO-REVIEW Experimental/testing PR, do NOT review it label Apr 24, 2026

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

rcj1 removed the area-Diagnostics-coreclr label Apr 24, 2026

Copilot started reviewing on behalf of rcj1 April 24, 2026 18:53 View session

dotnet-policy-service Bot assigned rcj1 Apr 24, 2026

Copilot AI reviewed Apr 24, 2026

View reviewed changes

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

Apply suggestion from @Copilot

7ad8332

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings April 24, 2026 19:03

Copilot started reviewing on behalf of rcj1 April 24, 2026 19:04 View session

Copilot AI reviewed Apr 24, 2026

View reviewed changes

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

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

Update src/native/managed/cdac/Microsoft.Diagnostics.DataContractRead…

8b536d2

…er.Legacy/SOSDacImpl.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

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

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

Copilot AI reviewed Apr 24, 2026

View reviewed changes

rcj1 mentioned this pull request Apr 24, 2026

[cDAC] Add diagnostic logging to GetMethodTableName assertion #127399

Closed

rcj1 removed the area-Diagnostics-coreclr label Apr 24, 2026

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

rcj1 added 2 commits April 24, 2026 14:22

e

d5622ea

Fix maxComparableLength and update variable references

a386185

Copilot AI review requested due to automatic review settings April 25, 2026 05:14

Copilot started reviewing on behalf of rcj1 April 25, 2026 05:16 View session

Copilot AI reviewed Apr 25, 2026

View reviewed changes

rcj1 added 2 commits April 24, 2026 22:22

Update SOSDacImpl.cs

494e3a2

Update SOSDacImpl.cs

1fc4110

Copilot AI review requested due to automatic review settings April 25, 2026 06:03

Copilot started reviewing on behalf of rcj1 April 25, 2026 06:05 View session

Copilot AI reviewed Apr 25, 2026

View reviewed changes

Update src/native/managed/cdac/Microsoft.Diagnostics.DataContractRead…

d925fff

…er.Legacy/SOSDacImpl.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings April 25, 2026 06:10

Copilot started reviewing on behalf of rcj1 April 25, 2026 06:11 View session

Copilot AI reviewed Apr 25, 2026

View reviewed changes

rcj1 closed this Apr 25, 2026

	string nameString = mtNametest is null ? string.Empty : new(mtNametest, 0, nameLength);
	string nameString = mtName is null ? string.Empty : new(mtNametest, 0, nameLength);

Conversation

rcj1 commented Apr 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants