Conversation
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
…ockCleanupInfo_RCWLockBitMasked tests Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
...ive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/SyncBlock_1.cs
Show resolved
Hide resolved
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
… COM data Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
…ct delegates to it; update docs Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
|
@copilot please apply the following patch. |
…to factory, simplify null checks Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Applied in b2c0797. Key changes from the patch:
|
There was a problem hiding this comment.
Pull request overview
This PR implements the GetSyncBlockCleanupData API in the cDAC (Contracts Data for Diagnostics). It also fixes a pre-existing behavior discrepancy: the legacy C++ implementation incorrectly returned E_INVALIDARG for addr=0, while the API contract specifies that addr=0 should start cleanup list traversal from SyncBlockCache.m_pCleanupBlockList. Additionally, it refactors GetBuiltInComData to work directly from a sync block pointer, enabling safe access during cleanup when the associated managed object may not be valid.
Changes:
- Adds
GetSyncBlockFromCleanupList(),GetNextSyncBlock(), andGetBuiltInComData(TargetPointer syncBlock, ...)to theISyncBlockinterface andSyncBlock_1implementation - Refactors
Object_1.GetBuiltInComDatato delegate to the SyncBlock contract, and moves the sync block bit constants (SyncBlockIsHashOrSyncBlockIndex,SyncBlockIsHashCode,SyncBlockIndexMask) to runtime globals exposed via the data descriptor - Adds
GetSyncBlockCleanupDataimplementation inSOSDacImplusing the new SyncBlock contract APIs, and updatesGetSyncBlockDatato usesyncBlock.GetBuiltInComDatadirectly
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
ISyncBlock.cs |
Adds three new cleanup-list and COM-data interface methods |
SyncBlock_1.cs |
Implements new methods; receives syncBlockLinkOffset for list pointer arithmetic |
SyncBlockFactory.cs |
Computes and passes syncBlockLinkOffset to SyncBlock_1 |
Object_1.cs |
Removes SyncBlockValue nested class; reads bit constants from runtime globals; delegates GetBuiltInComData to ISyncBlock |
SOSDacImpl.cs |
Implements GetSyncBlockCleanupData; updates GetSyncBlockData to use SyncBlock contract |
ISOSDacInterface.cs |
Adds DacpSyncBlockCleanupData struct; updates GetSyncBlockCleanupData signature |
Constants.cs |
Adds three new SyncBlock* global name constants |
SyncBlockCache.cs |
Adds CleanupBlockList field |
datadescriptor.inc |
Exposes SyncBlockCache.CleanupBlockList and three bit constants as globals |
syncblk.h |
Adds CleanupBlockList to cdac_data<SyncBlockCache> |
request.cpp |
Removes syncBlock == 0 from E_INVALIDARG guard |
SyncBlockTests.cs |
New unit tests for cleanup list traversal and COM data |
MockDescriptors.SyncBlock.cs |
New test descriptor for SyncBlock cleanup list |
MockDescriptors.Object.cs |
Adds three new globals to Object mock |
MockDescriptors.cs |
Adds CleanupBlockList to SyncBlockCacheFields |
SyncBlock.md |
Documents new APIs, data descriptors |
Object.md |
Documents new globals and delegation pattern |
| } | ||
| #endif | ||
| return hr; | ||
| } |
There was a problem hiding this comment.
There is a missing blank line between the closing brace of GetSyncBlockCleanupData (line 3473) and the start of GetSyncBlockData (line 3474). All other method implementations in this file have a blank line separating them for readability. This is a minor style inconsistency introduced by this PR.
| } | |
| } |
| private const ulong TestSyncBlockCacheGlobalAddress = 0x0000_0100; | ||
| private const ulong TestSyncTableEntriesGlobalAddress = 0x0000_0200; | ||
| private const ulong TestSyncTableEntriesAddress = 0x0000_0300; |
There was a problem hiding this comment.
The constants TestSyncBlockCacheGlobalAddress (0x0000_0100), TestSyncTableEntriesGlobalAddress (0x0000_0200), and TestSyncTableEntriesAddress (0x0000_0300) are defined but never used. The actual global pointer fragments are allocated from _allocator using the bump allocator, not from these fixed addresses. These unused constants should be removed.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Implements
GetSyncBlockCleanupDatain the cDAC. Also correctsaddr=0behavior: the legacy C++ implementation incorrectly returnedE_INVALIDARG; per the API contract,addr=0should start traversal fromSyncBlockCache.m_pCleanupBlockList.Objects on the cleanup list may not be valid managed objects, so COM data must be read directly from the sync block — not via the object header. This required splitting
GetBuiltInComDataacross two contracts.Changes
ISyncBlock/SyncBlock_1— AddGetBuiltInComData(TargetPointer syncBlock, ...)to read RCW/CCW/CCF directly from a sync block pointer (safe during cleanup). AddGetSyncBlockFromCleanupList()andGetNextSyncBlock(TargetPointer)for cleanup-list traversal.Object_1—GetBuiltInComDatakeeps the object-header → sync-table → sync-block lookup (inlined, noSyncBlockValuenested class — bit constants are now runtime globals), then delegates toISyncBlock.GetBuiltInComData. PrivateGetSyncBlockhelper inlined and removed.datadescriptor.inc— ExposeSyncBlockCache.m_pCleanupBlockListand the three sync block bit constants (SyncBlockIsHashOrSyncBlockIndex,SyncBlockIsHashCode,SyncBlockIndexMask) as runtime globals backed by existing C++ macros. RemoveSyncIndex(no longer needed).SyncBlockFactory—syncBlockLinkOffsetcomputation moved fromSyncBlock_1constructor into the factory'sCreateContractand passed as a parameter.SyncBlockCache—CleanupBlockListuses direct field access instead of optionalTryGetValue.SOSDacImpl—GetSyncBlockCleanupData:addr==0walks fromGetSyncBlockFromCleanupList(),addr!=0reads the block directly; both callsyncBlockContract.GetBuiltInComData(syncBlockPtr, ...)without touching the object. Null check fordatamoved inside the try block. DEBUG assertion no longer skipsaddr==0case.GetSyncBlockDatalikewise uses the SyncBlock contract directly.request.cpp— RemovedsyncBlock == 0from theE_INVALIDARGguard to allow the "start from cleanup list head" case.SyncBlock.md/Object.md— Updated to document new APIs, globals, data descriptors, and the delegation pattern.SyncBlockTestscovering cleanup-list traversal,GetBuiltInComDatawith/without interop info, chained linkage.ObjectTests.ComDatawired up with SyncBlock contract. Mock descriptors updated with new globals.🔒 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.