Uh oh!
There was an error while loading. Please reload this page.
cDAC stress build break: match GetObjectStringData's quirky dac behavior - #129297
Conversation
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
Updates the legacy cDAC ISOSDacInterface.GetObjectStringData implementation to better match the legacy DAC’s HRESULT behavior for length-only string queries by returning E_INVALIDARG while still reporting the required buffer length.
Changes:
- After copying/string-length computation via
OutputBufferHelpers.CopyStringToBuffer, returnE_INVALIDARGwhen there is no output buffer (stringData == null || count == 0). - Add an explanatory comment documenting the legacy DAC parity intent.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs | Adjusts GetObjectStringData HRESULT for length-only queries to align with legacy DAC behavior. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 1
Uh oh!
There was an error while loading. Please reload this page.
The legacy DAC (ClrDataAccess::GetObjectStringData) returns E_INVALIDARG when no output buffer is available (count == 0) even on a valid string, while still populating *pNeeded. The cDAC implementation returned S_OK, which trips the cDAC-vs-DAC HResult parity assert when CLRMA (clrma under dotnet-dump) sizes an exception string via GetObjectStringData(obj, 0, nullptr, &needed). Mirror the DAC: validate args as it does (so a non-null buffer with count == 0 still reports the needed size), populate *pNeeded via CopyStringToBuffer, then return E_INVALIDARG when count == 0. CLRMA explicitly ignores that HRESULT and uses the reported size. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
5de372f to
629af16CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Invoke the legacy DAC with the same stringData/pNeeded nullness the caller passed to the cDAC, so the debug HRESULT comparison is apples-to-apples and can't manufacture a spurious divergence from substituted arguments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ng it Rather than tolerating the divergence via AllowCdacSuccess, fix the legacy DAC: ClrDataAccess::GetObjectStringData no longer returns E_INVALIDARG for a size-only query (no output buffer) on a valid string -- it reports the needed size via *pNeeded and succeeds, matching the cDAC. The cDAC debug parity check returns to the default validation mode. Also mirror the caller's stringData/pNeeded nullness into the legacy DAC call in the debug validation, and derive the content-compare length from the cDAC string (neededLocal is only populated when a size-out is requested), avoiding an out-of-range span when pNeeded is null. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…ior (#129297) The legacy DAC (ClrDataAccess::GetObjectStringData) returns E_INVALIDARG when called on a valid string with no output buffer (stringData == null or count == 0), while still populating *pNeeded. The cDAC implementation returned S_OK in that case, which trips the cDAC-vs-DAC HResult parity assert (SOSDacImpl.cs ValidateHResult) when clrma sizes an exception string. Mirror the DAC: populate *pNeeded via CopyStringToBuffer, then return E_INVALIDARG for the no-buffer case. This fixes the build/test-break in the cDac introduced by dotnet/diagnostics#5865. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The legacy DAC (ClrDataAccess::GetObjectStringData) returns E_INVALIDARG when called on a valid string with no output buffer (stringData == null or count == 0), while still populating *pNeeded. The cDAC implementation returned S_OK in that case, which trips the cDAC-vs-DAC HResult parity assert (SOSDacImpl.cs ValidateHResult) when clrma sizes an exception string. Mirror the cDac: fix the weird behavior that we don't depend on in the legacy dac, and assert similar data here.
This fixes the build/test-break in the cDac introduced by dotnet/diagnostics#5865.