[dac] Make GetObjectStringData return the needed buffer element count instead of byte count
#110540
Conversation
|
Tagging subscribers to this area: @tommcdon |
|
We don't call this in ClrMD, and it doesn't appear on the list of publicly documented methods here: https://learn.microsoft.com/en-us/dotnet/framework/unmanaged-api/debugging/isosdacinterface-interface. This means that no one at Microsoft is using it. I do see other folks using it in the wild: https://github.com/lordmilko/ClrDebug/blob/3b49aec309e9ce8a4de142435b11ea71791fdcfe/ClrDebug/Managed/SOS/SOSDacInterface.cs#L1137 I have no strong opinion on changing it or not. I'm just reporting in what I found looking for uses. |
| @@ -1571,12 +1571,18 @@ ClrDataAccess::GetObjectStringData(CLRDATA_ADDRESS obj, unsigned int count, _Ino | |||
| count = needed; | |||
|
|
|||
| TADDR pszStr = TO_TADDR(obj)+offsetof(StringObject, m_FirstChar); | |||
| hr = m_pTarget->ReadVirtual(pszStr, (PBYTE)stringData, count * sizeof(WCHAR), &needed); | |||
| ULONG32 bytesRead; | |||
| hr = m_pTarget->ReadVirtual(pszStr, (PBYTE)stringData, count * sizeof(WCHAR), &bytesRead); | |||
There was a problem hiding this comment.
This looks good as long as ReadVirtual fails on partial reads. I don't remember if it does.
There was a problem hiding this comment.
Hm, seems like it does not - succeeds as long as any bytesRead > 0.
…nt instead of byte count (dotnet#110540) `GetObjectStringData` is returning the byte count instead of the buffer element count. This is inconsistent with all the other APIs that (optionally) return a needed count which return the buffer element count, not the byte count - for example, for strings specifically: `GetAssemblyName`, `GetMethodDescName`, `GetObjectClassName`, `GetMethodTableName`, `GetFrameName`, `GetPEFileName`.
GetObjectStringDatais returning the byte count instead of the buffer element count. This is inconsistent with all the other APIs that (optionally) return a needed count which return the buffer element count, not the byte count - for example, for strings specifically:GetAssemblyName,GetMethodDescName,GetObjectClassName,GetMethodTableName,GetFrameName,GetPEFileName.The only place I found using it (that is, not passing in
NULL) is:https://github.com/dotnet/diagnostics/blob/d1077f536993e79a013c6f97593af84cfbbf0134/src/SOS/Strike/clrma/managedanalysis.cpp#L673-L679
which just treats it as the buffer element count (so creates a larger buffer than necessary)
@mikem8361 / @dotnet/dotnet-diag From what I can tell, this was not actually intended. Is this okay for us to change?