Skip to content

TEST #127397

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
rcj1 wants to merge 6 commits into
base: main
Choose a base branch
from
+41 −4
Draft

TEST #127397

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6612fac
TEST
rcj1 Apr 24, 2026
7ad8332
Apply suggestion from @Copilot
rcj1 Apr 24, 2026
8b536d2
Update src/native/managed/cdac/Microsoft.Diagnostics.DataContractRead…
rcj1 Apr 24, 2026
d5622ea
e
rcj1 Apr 24, 2026
a386185
Fix maxComparableLength and update variable references
rcj1 Apr 25, 2026
494e3a2
Update SOSDacImpl.cs
rcj1 Apr 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 41 additions & 4 deletions src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2937,6 +2937,8 @@ int ISOSDacInterface.GetMethodTableForEEClass(ClrDataAddress eeClassReallyCanonM
int ISOSDacInterface.GetMethodTableName(ClrDataAddress mt, uint count, char* mtName, uint* pNeeded)
{
int hr = HResults.S_OK;
char[] mtNametest = new char[4096];
uint counttest = 4096;
try
{
if (mt == 0)
Expand All @@ -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");
}
Comment on lines 2940 to +2954
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
OutputBufferHelpers.CopyStringToBuffer(mtName, count, pNeeded, "Free");
}
else
Expand All @@ -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
Expand All @@ -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());
}
Comment on lines 2939 to 2990
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
}
Expand All @@ -2985,18 +2999,41 @@ int ISOSDacInterface.GetMethodTableName(ClrDataAddress mt, uint count, char* mtN
#if DEBUG
if (_legacyImpl is not null)
{
char[] mtNameLocal = new char[count];
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 = new(mtNametest, 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}");
Comment thread
rcj1 marked this conversation as resolved.
}
Comment on lines +3031 to +3036
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

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}");

Copilot uses AI. Check for mistakes.
}
}
#endif
Expand Down
Loading