[cDAC] ISOSDacInterface::GetSyncBlockData #119320
Conversation
|
Tagging subscribers to this area: @steveisok, @dotnet/dotnet-diag |
max-charlamb
requested review from
Copilot,
noahfalk,
davidwrighton and
rcj1
There was a problem hiding this comment.
Pull Request Overview
This PR implements the ISOSDacInterface::GetSyncBlockData method for the cDAC (Contract-based Data Access Component) system. It adds comprehensive support for retrieving synchronization block information from the runtime, including lock states, thread ownership, COM interop data, and waiter counts.
Key changes:
- Implements a complete SyncBlock contract with version 1 support
- Adds new data structures for AwareLock, SyncBlockCache, and enhanced SyncTableEntry/SyncBlock definitions
- Updates the Object contract to delegate COM data retrieval to the SyncBlock contract
- Provides comprehensive test coverage and debug validation against legacy DAC implementation
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.cs | Implements GetSyncBlockData with full cDAC logic and debug validation |
| src/native/managed/cdac/mscordaccore_universal/Legacy/ISOSDacInterface.cs | Adds DacpSyncBlockData and DacpSyncBlockCleanupData structs with proper type safety |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/CachingContractRegistry.cs | Registers SyncBlock contract factory |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/SyncBlock_1.cs | Core SyncBlock contract implementation with lock state calculations |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/*.cs | New data structures for AwareLock, SyncBlockCache, and updated SyncTableEntry/SyncBlock |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/*.cs | Contract interfaces and data type definitions |
| src/coreclr/vm/syncblk.h | Adds cdac_data template specializations for runtime structures |
| src/coreclr/vm/datadescriptor/. | Defines data descriptors and contract version for SyncBlock |
| src/coreclr/debug/daccess/request.cpp | Fixes bug in legacy DAC implementation |
| docs/design/datacontracts/*.md | Documentation for SyncBlock contract and Object contract updates |
| src/native/managed/cdac/tests/*.cs | Test infrastructure updates |
Comments suppressed due to low confidence (1)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/SyncBlock_1.cs:1
- Missing semicolon should be removed. This appears to be documentation code that incorrectly includes a semicolon after the method signature in what should be a method body context.
// Licensed to the .NET Foundation under one or more agreements.
|
fyi @AaronRobinsonMSFT - The sync block contract exposes some COM interop info (it was already exposed before, just refactored a bit here) |
| ## APIs of contract | ||
|
|
||
| ```csharp | ||
| public readonly struct SyncBlockData |
There was a problem hiding this comment.
I think we should hold off on this for a bit. @jkoritzinsky is reworking the monitors and how they exist on the sync block. See #118371.
| uint GetSyncBlockCount() => throw new NotImplementedException(); | ||
| SyncBlockData GetSyncBlockData(uint index) => throw new NotImplementedException(); | ||
| uint GetAdditionalThreadCount(uint index, uint maximumIterations = 1000) => throw new NotImplementedException(); | ||
| bool TryGetBuiltInComData(uint index, out TargetPointer rcw, out TargetPointer ccw, out TargetPointer cf) => throw new NotImplementedException(); |
There was a problem hiding this comment.
I think we should reconsider this. This approach is coupling COM and the sync block, which is an implementation detail I would avoid. Can we instead place this on the object and abstract away the sync block?
There was a problem hiding this comment.
Discussed with Jeremy, we will plan to make a new BuiltInCOM (naming TBD) contract. This would handle reading the COM data off an InteropSyncBlock with the SyncBlock/Object contracts as dependencies.
To get the CCW/RCW of an Object, the BuiltInCOM would expose something like as follows:
public bool GetObjectRCW(TargetPointer objectAddress)
{
IObject object = _target.Contracts.Object;
ISyncBlock sync = _target.Contracts.SyncBlock;
uint syncBlockIndex = object.GetSyncBlockIndex(objectAddress);
TargetPointer interopSyncBlockAddress = sync.GetInteropData(syncBlockIndex);
Data.InteropSyncBlockInfo interopInfo = _target.ProcessedData.GetOrAdd<Data.InteropSyncBlock>(interopSyncBlockAddress);
return interopInfo.rcw;
}| None = 0x0, | ||
| CCW = 0x1, | ||
| RCW = 0x2, | ||
| CF = 0x4, |
There was a problem hiding this comment.
Let's call this ClassFactory. Also, what scenario does this data support?
There was a problem hiding this comment.
It is called in ClrMD, but I don't see the COMFlags used anywhere.
No description provided.