Skip to content

[cdac] Implement GetThreadStoreData in cDAC #102404

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
Merged
elinor-fung merged 4 commits into from
May 30, 2024
Merged

[cdac] Implement GetThreadStoreData in cDAC #102404

elinor-fung 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
Copy link
Contributor

dotnet-policy-service bot commented May 17, 2024

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

elinor-fung
elinor-fung commented May 20, 2024
View reviewed changes
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

src/coreclr/debug/runtimeinfo/datadescriptor.h

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

@AaronRobinsonMSFT AaronRobinsonMSFT May 21, 2024

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.

@build-analysis build-analysis bot mentioned this pull request May 23, 2024
Closed
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.

This was referenced May 30, 2024
Closed
Closed
@elinor-fung elinor-fung merged commit f6701e5 into dotnet:main May 30, 2024
@elinor-fung elinor-fung deleted the cdac-threadstore branch May 30, 2024 22:02
@steveisok steveisok mentioned this pull request May 31, 2024
Closed
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