cDAC: Implement ISOSDacInterface::GetRCWData#125426
Conversation
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
…M GetRCWData tests Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
…iltInCOM_1 Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Implements ISOSDacInterface::GetRCWData via the BuiltInCOM contract, expanding the RCW data descriptor/model and adding dump + unit tests to validate RCW data and interface cache behavior.
Changes:
- Added
IBuiltInCOM.GetRCWDataand implemented it inBuiltInCOM_1, including RCW flag decoding and disconnected detection. - Extended cDAC data descriptors/models for
RCWandCtxEntry, and wired legacy SOS DACGetRCWDatato the new contract. - Added/updated dump-test documentation and introduced new RCW dump tests + unit tests for
GetRCWData.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/BuiltInCOM_1.cs | Adds RCW flags enum, disconnected detection, and implements GetRCWData. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IBuiltInCOM.cs | Introduces RCWData and adds GetRCWData to contract interface. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/RCW.cs | Extends RCW processed-data model to read additional fields used by GetRCWData. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/CtxEntry.cs | Adds CtxCookie to support RCW disconnected detection. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs | Implements ISOSDacInterface.GetRCWData using cDAC BuiltInCOM. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ISOSDacInterface.cs | Adds DacpRCWData struct and updates GetRCWData signature. |
| src/native/managed/cdac/tests/BuiltInCOMTests.cs | Adds mock descriptors + unit tests covering GetRCWData and disconnected scenarios. |
| src/native/managed/cdac/tests/DumpTests/RCWDumpTests.cs | Adds dump-based tests for RCW interfaces and GetRCWData behavior. |
| src/native/managed/cdac/tests/DumpTests/Debuggees/RCWInterfaces/Program.cs | Expands debuggee to create both plain and contained RCWs and pins them for dump discovery. |
| src/native/managed/cdac/tests/DumpTests/README.md | Documents new dump type and test mapping for RCW. |
| src/coreclr/vm/runtimecallablewrapper.h | Adds cDAC field offsets for additional RCW fields; documents contract dependency on bit encoding. |
| src/coreclr/vm/datadescriptor/datadescriptor.inc | Adds new RCW/CtxEntry fields to cDAC data descriptor. |
| src/coreclr/vm/comcache.h | Adds CtxEntry::CtxCookie field offset for cDAC. |
| docs/design/datacontracts/BuiltInCOM.md | Documents RCWData, new fields, flags, and disconnected detection logic. |
| /// </summary> | ||
| public class RCWDumpTests : DumpTestBase | ||
| { | ||
| protected override string DebuggeeName => "RCW"; |
There was a problem hiding this comment.
DebuggeeName is set to "RCW", but the updated debuggee lives under Debuggees/RCWInterfaces/Program.cs in this diff. If the debuggee/project name wasn’t also renamed to RCW elsewhere, this test will fail to locate/run the debuggee. Align the dump test’s DebuggeeName with the actual debuggee target name (or rename/move the debuggee project/folder so the name is truly RCW).
| protected override string DebuggeeName => "RCW"; | |
| protected override string DebuggeeName => "RCWInterfaces"; |
| | SyncBlockDumpTests | SyncBlock | SyncBlock | | ||
| | BuiltInCOMDumpTests | BuiltInCOM | CCWInterfaces | | ||
| | RCWCleanupListDumpTests | BuiltInCOM | RCWCleanupList | | ||
| | RCWInterfacesDumpTests | BuiltInCOM | RCW | |
There was a problem hiding this comment.
The README maps the new RCW dump type to RCWInterfacesDumpTests, but RCWInterfacesDumpTests.cs was removed and replaced by RCWDumpTests.cs in this PR. Update this row (and any related naming in the README) to reference the actual test class/file name to avoid confusion and stale documentation.
| | RCWInterfacesDumpTests | BuiltInCOM | RCW | | |
| | RCWDumpTests | BuiltInCOM | RCW | |
| // Mirrors IUnkEntry::IsDisconnected in src/coreclr/vm/comcache.h. | ||
| private bool IsRCWDisconnected(Data.RCW rcw) | ||
| { | ||
| if (rcw.UnknownPointer == DisconnectedSentinel) |
There was a problem hiding this comment.
rcw.UnknownPointer is a TargetPointer, while DisconnectedSentinel is a ulong. This comparison is very likely either a compile-time type mismatch or (worse) an unintended comparison via implicit conversion/operator overload. To make the check unambiguous and portable, compare like-for-like (e.g., create a TargetPointer sentinel value once and compare against that).
| if (rcw.UnknownPointer == DisconnectedSentinel) | |
| ulong unknownPointer = rcw.UnknownPointer; | |
| if (unknownPointer == DisconnectedSentinel) |
| // Mirrors RCW::RCWFlags bits in src/coreclr/vm/runtimecallablewrapper.h. | ||
| private enum RCWFlags : uint | ||
| { | ||
| URTAggregated = 0x010u, // bit 4: m_fURTAggregated | ||
| URTContained = 0x020u, // bit 5: m_fURTContained | ||
| MarshalingTypeMask = 0x180u, // bits 7-8: m_MarshalingType | ||
| MarshalingTypeFreeThreaded = 0x100u, // MarshalingType_FreeThreaded (2) in bits 7-8 | ||
| } |
There was a problem hiding this comment.
The PR description says this should be a [Flags] private enum RCWFlags : uint, but the enum in code is missing the [Flags] attribute. Add [Flags] to match the stated intent and existing style (e.g., CCWFlags/ComWrapperFlags) since this enum is used for bitmask operations.
| public struct DacpRCWData | ||
| { | ||
| public ClrDataAddress identityPointer; | ||
| public ClrDataAddress unknownPointer; | ||
| public ClrDataAddress managedObject; | ||
| public ClrDataAddress jupiterObject; | ||
| public ClrDataAddress vtablePtr; | ||
| public ClrDataAddress creatorThread; | ||
| public ClrDataAddress ctxCookie; | ||
| public int refCount; | ||
| public int interfaceCount; | ||
| public Interop.BOOL isJupiterObject; | ||
| public Interop.BOOL supportsIInspectable; | ||
| public Interop.BOOL isAggregated; | ||
| public Interop.BOOL isContained; | ||
| public Interop.BOOL isFreeThreaded; | ||
| public Interop.BOOL isDisconnected; | ||
| } |
There was a problem hiding this comment.
Since DacpRCWData is part of the COM-facing SOS DAC surface, it’s safer to explicitly enforce blittable layout (e.g., [StructLayout(LayoutKind.Sequential)]). This avoids any ambiguity around field ordering/packing assumptions and makes the interop contract clearer.
| data->creatorThread = rcwData.CreatorThread.ToClrDataAddress(_target); | ||
| data->ctxCookie = rcwData.CtxCookie.ToClrDataAddress(_target); | ||
| data->refCount = (int)rcwData.RefCount; | ||
| data->interfaceCount = builtInCom.GetRCWInterfaces(rcwPtr).Count(); |
There was a problem hiding this comment.
Count() will enumerate the full IEnumerable returned by GetRCWInterfaces, which may repeatedly read target memory and allocate enumerator state. If this API is expected to be called frequently (e.g., by diagnostics tooling), consider adding a dedicated contract method that returns interface count without enumerating (or adjusting GetRCWInterfaces to return a collection/array where the count is cheap).
URTAggregatedMask,URTContainedMask,MarshalingTypeMask,MarshalingTypeFreeThreadedValuerawconst uintfields with a[Flags] private enum RCWFlags : uintinBuiltInCOM_1.cs, matching the style of existingCCWFlags/ComMethodTableFlags/ComWrapperFlagsenumsGetRCWData×2,GetRCWCleanupList×1) to use enum casts🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.