Skip to content

[cDAC] Add diagnostic logging to GetMethodTableName assertion#127399

Closed
max-charlamb wants to merge 1 commit intomainfrom
fix/sos-webapp3-assertion
Closed

[cDAC] Add diagnostic logging to GetMethodTableName assertion#127399
max-charlamb wants to merge 1 commit intomainfrom
fix/sos-webapp3-assertion

Conversation

@max-charlamb
Copy link
Copy Markdown
Member

Note

This PR was generated with the assistance of GitHub Copilot.

Summary

Add \Console.Error.WriteLine\ diagnostic output before the \Debug.Assert\ in \GetMethodTableName\ to capture the mismatched type names when the cDAC and legacy DAC produce different results. The stderr output survives the process crash and appears in CI test logs.

Problem

The \SOS.WebApp3\ test has been crashing in all
untime-diagnostics\ PR builds since April 24 with:
\
Assertion failed: pNeeded == null || *pNeeded == neededLocal
at SOSDacImpl.GetMethodTableName(...)
\\

The assertion fires during !timerinfo\ when the cDAC's \TypeNameBuilder\ produces a different type name than the legacy DAC's \TypeString\ for some method table. The assertion doesn't include the actual names, making diagnosis impossible.

Changes

  • SOSDacImpl.cs: Added stderr logging before the assertion that prints the MT address, both pNeeded values, and both type name strings with a \CDAC_MISMATCH\ prefix
  • DebugExtensions.cs: Added \ValidateOutputStringBuffer\ helper for future soft-fail conversion

Purpose

This is a diagnostic PR to identify the exact type causing the mismatch. Once identified, a follow-up PR will fix the root cause in \TypeNameBuilder.

Add Console.Error.WriteLine before Debug.Assert in GetMethodTableName to
capture both the cDAC and legacy DAC type names when they differ. The
stderr output survives the process crash and appears in CI test logs,
allowing identification of the specific type causing the mismatch.

Also adds ValidateOutputStringBuffer helper to DebugExtensions for
future use in converting other assertions to soft-fail.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dotnet-policy-service
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.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds additional diagnostics to help investigate an assertion mismatch between cDAC and legacy DAC type-name output during ISOSDacInterface.GetMethodTableName, primarily to unblock debugging CI crashes.

Changes:

  • Emit Console.Error diagnostics (with CDAC_MISMATCH prefix) before the Debug.Assert in GetMethodTableName.
  • Add a new DEBUG-only helper (ValidateOutputStringBuffer) intended to compare output-buffer needed sizes and contents.

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/SOSDacImpl.cs Adds stderr logging when pNeeded values differ between cDAC and legacy DAC implementations.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/DebugExtensions.cs Adds DEBUG-only helper to validate output-string buffer size/content mismatches for future reuse.

if (pNeeded != null && *pNeeded != neededLocal)
{
string cdacStr = mtName is not null && count > 0 ? new string(mtName, 0, (int)System.Math.Min(*pNeeded, count) - 1) : "<null>";
string dacStr = neededLocal > 0 ? new string(mtNameLocal, 0, (int)neededLocal - 1) : "<empty>";
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 diagnostic dacStr construction can throw when neededLocal is larger than count (e.g., buffer was too small and legacy DAC reported a bigger required size). Since mtNameLocal is allocated as new char[count], new string(mtNameLocal, 0, (int)neededLocal - 1) will go out of range and may prevent the mismatch details from being logged. Consider clamping the extracted length to the available buffer size (e.g., Math.Min(neededLocal, count)), similar to what’s done for cdacStr.

Suggested change
string dacStr = neededLocal > 0 ? new string(mtNameLocal, 0, (int)neededLocal - 1) : "<empty>";
uint dacCount = System.Math.Min(neededLocal, count);
string dacStr = dacCount > 0 ? new string(mtNameLocal, 0, (int)dacCount - 1) : "<empty>";

Copilot uses AI. Check for mistakes.
Comment on lines +72 to +73
string cdacStr = cdacBuffer is not null && cdacLen > 0 ? new string(cdacBuffer, 0, cdacLen - 1) : "<null>";
string dacStr = dacNeeded > 0 ? new string(dacBuffer, 0, (int)dacNeeded - 1) : "<empty>";
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.

ValidateOutputStringBuffer can throw while trying to build the diagnostic dacStr when dacNeeded is greater than the provided dacBuffer length (often count). In the mismatch case, dacNeeded is exactly the scenario where the caller’s buffer may be too small, so new string(dacBuffer, 0, (int)dacNeeded - 1) risks ArgumentOutOfRangeException and defeats the purpose of soft-fail diagnostics. Clamp the length to the available buffer size (e.g., Math.Min(dacNeeded, (uint)dacBuffer.Length) / Math.Min(dacNeeded, count)) before constructing the string.

Suggested change
string cdacStr = cdacBuffer is not null && cdacLen > 0 ? new string(cdacBuffer, 0, cdacLen - 1) : "<null>";
string dacStr = dacNeeded > 0 ? new string(dacBuffer, 0, (int)dacNeeded - 1) : "<empty>";
int dacLen = (int)Math.Min(dacNeeded, (uint)dacBuffer.Length);
string cdacStr = cdacBuffer is not null && cdacLen > 0 ? new string(cdacBuffer, 0, cdacLen - 1) : "<null>";
string dacStr = dacLen > 0 ? new string(dacBuffer, 0, dacLen - 1) : "<empty>";

Copilot uses AI. Check for mistakes.
@rcj1
Copy link
Copy Markdown
Contributor

rcj1 commented Apr 24, 2026

Duplicate of #127397

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants