Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[cdac] Implement GetThreadStoreData in cDAC #102404

Merged
merged 4 commits into from
May 30, 2024
Merged

[cdac] Implement GetThreadStoreData in cDAC #102404

merged 4 commits into from
May 30, 2024

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented May 17, 2024 •
edited
Loading

  • Implement GetThreadStoreData and GetThreadCounts in Thread contract
  • Finish implementing ISOSDacInterface::GetThreadStoreData in cDAC
    • Add specific threads (first in thread store, Finalizer, GC) and counts
  • Make existing DAC call into cDAC for GetThreadData if available
    • Only fills out managed thread ID and next thread right now - always returns E_NOTIMPL
  • Update the example C# API in docs to be closer to what we have now

Contributes to #99298

cc @lambdageek

@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

AaronRobinsonMSFT
AaronRobinsonMSFT reviewed May 21, 2024
View reviewed changes
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

docs/design/datacontracts/contract_csharp_api_design.cs Outdated Show resolved Hide resolved
src/coreclr/debug/runtimeinfo/datadescriptor.h
@@ -105,14 +105,21 @@ CDAC_TYPES_BEGIN()

CDAC_TYPE_BEGIN(Thread)
CDAC_TYPE_INDETERMINATE(Thread)
CDAC_TYPE_FIELD(Thread, /*uint32*/, Id, cdac_offsets<Thread>::Id)
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this comment type. Is that intentional going forward or a temporary mechanism?

Copy link
Member Author

@elinor-fung elinor-fung May 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The type is optional as a mechanism for making the descriptor more compact. I left it off for these (and just put the comment), since I wasn't checking them.

src/native/managed/cdacreader/src/Data/Thread.cs Outdated Show resolved Hide resolved
This was referenced May 23, 2024
Open
Open
lambdageek
lambdageek approved these changes May 28, 2024
View reviewed changes
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

lgtm, although I'm not a huge fan of the TryGet TryRegister pattern.

src/native/managed/cdacreader/src/Contracts/Thread.cs Outdated Show resolved Hide resolved
This was referenced May 30, 2024
Closed
Closed
@elinor-fung elinor-fung merged commit f6701e5 into dotnet:main May 30, 2024
151 checks passed
@elinor-fung elinor-fung deleted the cdac-threadstore branch May 30, 2024 22:02
@steveisok steveisok mentioned this pull request May 31, 2024
Open
25 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Jun 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lambdageek lambdageek lambdageek approved these changes

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT left review comments

@davidwrighton davidwrighton Awaiting requested review from davidwrighton

Assignees

@elinor-fung elinor-fung

Labels
area-Diagnostics-coreclr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@elinor-fung @lambdageek @AaronRobinsonMSFT