Conversation
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
This PR fixes cDAC thread “dead” semantics by treating ThreadState.ReportDead as dead for enumeration and IsThreadMarkedDead, while still allowing callers to distinguish Dead vs ReportDead when needed (e.g., monitor-lock ownership queries).
Changes:
- Update thread enumeration and
IsThreadMarkedDeadto considerReportDeadas dead. - Extend the managed thread contract to surface
ReportDeadand convert runtime state bits into contract flags. - Adjust Thread contract documentation to reflect that raw runtime state requires conversion to the contract enum.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs | Treat ReportDead threads as dead for enumeration and IsThreadMarkedDead. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Thread_1.cs | Add mapping from runtime Thread::State bits (incl. TS_ReportDead) into contract ThreadState. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IThread.cs | Add ThreadState.ReportDead to the public contract enum. |
| docs/design/datacontracts/Thread.md | Note that Thread::State should be converted to the contract enum rather than used as a raw integer. |
| Background = 0x00000200, // Thread is a background thread | ||
| Unstarted = 0x00000400, // Thread has never been started | ||
| Dead = 0x00000800, // Thread is dead | ||
| ReportDead = 0x00001000, // Thread is dead for the purposes of the debugger |
There was a problem hiding this comment.
ThreadState.ReportDead is assigned 0x00001000, but the runtime’s Thread::TS_ReportDead flag is 0x00010000 (see src/coreclr/vm/threads.h). This mismatch is error-prone for consumers and also conflicts with the native TS_WeOwn bit at 0x00001000. Update the contract value to 0x00010000 to match the runtime flag (and keep ThreadState values aligned with Thread::ThreadState).
| ReportDead = 0x00001000, // Thread is dead for the purposes of the debugger | |
| ReportDead = 0x00010000, // Thread is dead for the purposes of the debugger |
| @@ -339,7 +339,7 @@ public int EnumerateThreads(nint fpCallback, nint pUserData) | |||
| { | |||
| Contracts.ThreadData threadData = threadContract.GetThreadData(currentThread); | |||
| // Match native: skip dead and unstarted threads | |||
There was a problem hiding this comment.
The comment says “skip dead and unstarted threads”, but the condition now also skips ReportDead. Update the comment to reflect the actual filter so future readers don’t miss that ReportDead threads are excluded too.
| // Match native: skip dead and unstarted threads | |
| // Match native: skip dead, report-dead, and unstarted threads |
| try | ||
| { | ||
| Contracts.ThreadData threadData = _target.Contracts.Thread.GetThreadData(new TargetPointer(vmThread)); | ||
| *pResult = (threadData.State & Contracts.ThreadState.Dead) != 0 ? Interop.BOOL.TRUE : Interop.BOOL.FALSE; | ||
| *pResult = (threadData.State & (Contracts.ThreadState.Dead | Contracts.ThreadState.ReportDead)) != 0 ? Interop.BOOL.TRUE : Interop.BOOL.FALSE; | ||
| } |
There was a problem hiding this comment.
This changes the semantics of IsThreadMarkedDead to treat ReportDead as dead. There are existing dump tests that cross-validate this API against the contract’s ThreadState.Dead bit only; those should be updated to use (Dead | ReportDead) so the test reflects the new intended behavior and prevents regressions.
| Id: target.Read<uint>(address + /* Thread::Id offset */), | ||
| OSId: target.ReadNUInt(address + /* Thread::OSId offset */), | ||
| State: target.Read<uint>(address + /* Thread::State offset */), | ||
| State: target.Read<uint>(address + /* Thread::State offset */) /* -> convert to contract enum */, |
There was a problem hiding this comment.
The contract documentation now notes that the raw Thread::State value should be converted to the contract enum, but ThreadState earlier in this doc is still missing the newly added ReportDead flag. Please update the documented ThreadState enum and/or add a brief note describing how TS_ReportDead maps to ThreadState.ReportDead so the doc matches the contract surface area.
| State: target.Read<uint>(address + /* Thread::State offset */) /* -> convert to contract enum */, | |
| State: target.Read<uint>(address + /* Thread::State offset */) /* -> convert to contract enum; for example, TS_ReportDead maps to ThreadState.ReportDead */, |
| Background = 0x00000200, // Thread is a background thread | ||
| Unstarted = 0x00000400, // Thread has never been started | ||
| Dead = 0x00000800, // Thread is dead | ||
| ReportDead = 0x00001000, // Thread is dead for the purposes of the debugger |
There was a problem hiding this comment.
What does it mean "for the purpose of the debugger"?
We have to ensure that the thread is reported as dead when either
DeadorReportDeadare set. There are, however, certain cases where it is useful to distinguish between the two, for exampleGetThreadOwningMonitorLock.