Skip to content

[cDAC] : Implement DacDBI GetThreadHandle and GetThreadAllocInfo#128388

Merged
rcj1 merged 4 commits into
mainfrom
copilot/implement-cdac-dacdbi-apis
May 22, 2026
Merged

[cDAC] : Implement DacDBI GetThreadHandle and GetThreadAllocInfo#128388
rcj1 merged 4 commits into
mainfrom
copilot/implement-cdac-dacdbi-apis

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 20, 2026

Implement DacDBI GetThreadHandle and GetThreadAllocInfo. No new contracts.

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot May 20, 2026 00:17
@rcj1 rcj1 changed the title Remove new DacDbi impl tests; make Thread.ThreadHandle required [cDAC] : Implement DacDBI GetThreadHandle and GetThreadAllocInfo May 20, 2026
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

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

@rcj1 rcj1 marked this pull request as ready for review May 20, 2026 01:10
Copilot AI review requested due to automatic review settings May 20, 2026 01:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the cDAC Thread contract and the legacy DacDbi implementation to surface an OS thread handle and to implement IDacDbiInterface::GetThreadHandle / GetThreadAllocInfo using cDAC-backed data.

Changes:

  • Add ThreadHandle to the Thread data descriptor pipeline (CoreCLR cdac_data<Thread> + datadescriptor.inc, managed data reader, contract DTO, and docs).
  • Implement DacDbiImpl.GetThreadHandle and DacDbiImpl.GetThreadAllocInfo using the IThread contract instead of returning E_NOTIMPL.
  • Add/adjust tests and mock descriptors to carry the new ThreadHandle field.
Show a summary per file
File Description
src/native/managed/cdac/tests/ThreadTests.cs Adds a contract-level test validating ThreadData.ThreadHandle.
src/native/managed/cdac/tests/MockDescriptors/MockDescriptors.Thread.cs Extends the mock Thread layout/view to include a ThreadHandle pointer field.
src/native/managed/cdac/tests/ClrDataExceptionStateTests.cs Updates ThreadData construction sites to include the new ThreadHandle field.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/IDacDbiInterface.cs Refines COM signature for GetThreadHandle to an output pointer-to-handle shape.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs Implements GetThreadHandle and GetThreadAllocInfo using cDAC contracts (with DEBUG parity checks).
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/Thread.cs Reads the new ThreadHandle field from the Thread type descriptor into the managed data model.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Thread_1.cs Plumbs ThreadHandle into the ThreadData returned by IThread.GetThreadData.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IThread.cs Extends the public ThreadData record struct with a new positional ThreadHandle field.
src/coreclr/vm/threads.h Exposes Thread::m_ThreadHandle offset via cdac_data<Thread>.
src/coreclr/vm/datadescriptor/datadescriptor.inc Adds ThreadHandle to the Thread data descriptor field list.
docs/design/datacontracts/Thread.md Documents the new ThreadHandle field and descriptor dependency.

Copilot's findings

  • Files reviewed: 11/11 changed files
  • Comments generated: 6

Comment thread docs/design/datacontracts/Thread.md
Comment thread src/coreclr/vm/threads.h
@max-charlamb
Copy link
Copy Markdown
Member

Should the native Dbi GetThreadHandle be updated for this new behavior (returning 0 on non-windows platforms)?

Copilot AI review requested due to automatic review settings May 21, 2026 20:26
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 12/12 changed files
  • Comments generated: 6

Comment thread docs/design/datacontracts/Thread.md Outdated
Copy link
Copy Markdown
Member

@max-charlamb max-charlamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks reasonable. small nit regarding documenting that this API is not x-bitness safe

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 22, 2026 18:02
@rcj1
Copy link
Copy Markdown
Contributor

rcj1 commented May 22, 2026

/ba-g tests already ran, nit comment change

@rcj1
Copy link
Copy Markdown
Contributor

rcj1 commented May 22, 2026

/ba-g nit comment change, tests already ran

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 12/12 changed files
  • Comments generated: 3

@rcj1
Copy link
Copy Markdown
Contributor

rcj1 commented May 22, 2026

/ba-g nit comment, already ran tests

@rcj1 rcj1 merged commit a36215e into main May 22, 2026
120 of 128 checks passed
@rcj1 rcj1 deleted the copilot/implement-cdac-dacdbi-apis branch May 22, 2026 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants