[cDAC] Implement thread-related DacDbi APIs

[Copilot]

• Implement GetThreadObject, HasUnhandledException, GetCurrentCustomDebuggerNotification, and GetCurrentException

[Copilot]

Pull request overview

Implements additional thread-related DacDbi APIs in the cDAC-based DacDbiImpl, extending the Thread contract/data descriptors and adding dump-based coverage to ensure the new APIs return values consistent with contract-provided thread state/handles.

Changes:

• Implement GetThreadObject, HasUnhandledException, GetCurrentCustomDebuggerNotification, and update GetCurrentException in DacDbiImpl using the Thread contract.
• Extend Thread contract data (ThreadData) and contract implementation (Thread_1) to surface exposed thread object handle, unhandled-exception state, and custom debugger notification handle.
• Update CoreCLR cDAC descriptors (threads.h, datadescriptor.inc) and docs, plus add/adjust dump + mock-based tests.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/native/managed/cdac/tests/MockDescriptors/MockDescriptors.Thread.cs	Extends mock Thread layout with additional fields used by updated Thread contract/API behavior.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiThreadDumpTests.cs	Adds dump-based integration tests for the newly implemented DacDbi thread APIs and adjusts the current-exception test.
src/native/managed/cdac/tests/ClrDataExceptionStateTests.cs	Updates mocked ThreadData construction to include newly added record fields.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements the thread-related DacDbi APIs via cDAC contracts (with DEBUG cross-validation against legacy DAC where available).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Thread.cs	Adds reading of new Thread data descriptor fields (exposed object, LTO-unhandled flag, custom notification).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Thread_1.cs	Extends v1 Thread contract to compute detached state + unhandled-exception semantics and populate the expanded ThreadData.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/CorDbHResults.cs	Adds CORDBG_E_BAD_THREAD_STATE constant used by GetThreadObject.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IThread.cs	Expands ThreadState/ThreadData and removes GetThrowableObject from the abstraction contract.
src/coreclr/vm/threads.h	Adds cDAC field offsets for last-thrown-unhandled flag and current custom notification handle.
src/coreclr/vm/datadescriptor/datadescriptor.inc	Adds Thread data descriptor fields and fixes naming/typing for handle-based fields.
docs/design/datacontracts/Thread.md	Updates Thread contract documentation to reflect the expanded ThreadData and required data descriptors.

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

[github-actions]

🤖 Copilot Code Review — PR #126975

Note

This review was generated by Copilot.

Holistic Assessment

Motivation: The PR renames the cDAC data type GCHandle → ObjectHandle across the data descriptor and managed reader, which avoids naming confusion with System.Runtime.InteropServices.GCHandle. It also makes ExposedObject and CurrentCustomDebuggerNotification on Data.Thread use the ObjectHandle data type (matching what LastThrownObject already did), and hardens the ObjectHandle constructor with TryReadPointer.

Approach: The rename itself is clean and mechanical. However, the method used to read the handle fields on Data.Thread introduces an indirection-level bug. ReadDataField<ObjectHandle> treats the field as an inline struct at the field's memory address, while the correct pattern (used by InflightTLSData, LoaderAllocator, and SyncBlock elsewhere) is GetOrAdd<ObjectHandle>(ReadPointerField(...)) — read the handle value first, then construct ObjectHandle from it.

Summary: ❌ Needs Changes. The switch to ReadDataField<ObjectHandle> changes what ObjectHandle.Handle contains from the OBJECTHANDLE value to the address of the field inside the Thread struct. All downstream consumers (SOSDacImpl, DacDbiImpl, ClrDataTask) receive the wrong value. This is a correctness bug that affects the debugger interface.

---

Detailed Findings

❌ Wrong indirection level — ReadDataField<ObjectHandle> vs GetOrAdd<ObjectHandle>(ReadPointerField(...))

In Data/Thread.cs, all three handle fields now use ReadDataField<ObjectHandle>:

ExposedObject = target.ReadDataField<ObjectHandle>(address, type, nameof(ExposedObject));
LastThrownObject = target.ReadDataField<ObjectHandle>(address, type, nameof(LastThrownObject));
CurrentCustomDebuggerNotification = target.ReadDataField<ObjectHandle>(address, type, nameof(CurrentCustomDebuggerNotification));

ReadDataField<ObjectHandle> computes fieldAddr = threadAddress + fieldOffset and passes that to the ObjectHandle constructor. This means:

• Handle = fieldAddr (address of the field in the Thread struct)
• Object = ReadPointer(fieldAddr) = the OBJECTHANDLE value

But the old code (and the pattern used by every other ObjectHandle consumer in the codebase) was:

// InflightTLSData.cs:16, LoaderAllocator.cs:31-32, old Thread.cs for LastThrownObject
target.ProcessedData.GetOrAdd<ObjectHandle>(target.ReadPointerField(address, type, fieldName));

Which first reads the pointer value at the field (= the OBJECTHANDLE), then constructs ObjectHandle:

• Handle = OBJECTHANDLE value H
• Object = ReadPointer(H) = the managed object

Then in Thread_1.cs, thread.ExposedObject.Handle / thread.LastThrownObject.Handle / thread.CurrentCustomDebuggerNotification.Handle are passed to ThreadData.ExposedObjectHandle / LastThrownObjectHandle / CurrentCustomDebuggerNotificationHandle, which downstream consumers (SOSDacImpl.cs:4264, DacDbiImpl.cs:393, DacDbiImpl.cs:582, ClrDataTask.cs:151) use as the actual OBJECTHANDLE. After this change, they receive the field address instead.

Impact: The DEBUG-mode validation in DacDbiImpl (line 595) would catch this mismatch against the legacy DAC, but release builds would silently return wrong handle values to the debugger.

Fix: Revert to the established two-step pattern:

ExposedObject = target.ProcessedData.GetOrAdd<ObjectHandle>(
    target.ReadPointerField(address, type, nameof(ExposedObject)));
LastThrownObject = target.ProcessedData.GetOrAdd<ObjectHandle>(
    target.ReadPointerField(address, type, nameof(LastThrownObject)));
CurrentCustomDebuggerNotification = target.ProcessedData.GetOrAdd<ObjectHandle>(
    target.ReadPointerField(address, type, nameof(CurrentCustomDebuggerNotification)));

Or, if the intent is to use typed fields via ReadDataField, the ObjectHandle constructor would need to be redesigned to perform the extra dereference (read the handle value at the given address, then read the object through the handle) — but this would break the existing consumers (InflightTLSData, LoaderAllocator, SyncBlock).

⚠️ Mock test descriptors not updated for ObjectHandle field type

In MockDescriptors.Thread.cs, the ExposedObject, LastThrownObject, and CurrentCustomDebuggerNotification fields are still registered via .AddPointerField(...) (lines 203-206), with no TypeName. The Debug.Assert in ReadDataField<T> passes when TypeName is null or empty, so this won't catch the type mismatch. If the field type in the descriptor were set to "ObjectHandle", the assertion would validate the intent correctly.

✅ GCHandle → ObjectHandle rename is clean

The rename across datadescriptor.inc, DataType.cs, and test files is consistent and complete. No remaining references to DataType.GCHandle exist in the codebase. The new name ObjectHandle is more precise and avoids confusion with System.Runtime.InteropServices.GCHandle.

✅ TryReadPointer hardening in ObjectHandle constructor

Changing from ReadPointer to TryReadPointer in the ObjectHandle constructor is a reasonable defensive improvement that gracefully handles unreadable memory (e.g., corrupt or truncated dumps) instead of throwing.

Generated by Code Review for issue #126975 · ◷