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.

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

[cdac] Implement IXCLRDataProcess::GetTaskByOSThreadID #109230

Merged
merged 5 commits into from
Oct 28, 2024

Conversation

elinor-fung
Copy link
Member

  • Add ClrDataTask to cDAC as its implementation of IXCLRDataTask
    • Currently delegates everything to legacy implementation
  • Implement IXCLRDataProcess::GetTaskByOSThreadID
    • Uses Thread contract to find the address of the thread corresponding to the specified OS thread ID.
    • Gets the result from the legacy DAC and passes it to the cDAC ClrDataTask
  • Add tests for Thread contract

Contributes to #108553

cc @AaronRobinsonMSFT @davidwrighton

Copy link
Contributor

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

namespace Microsoft.Diagnostics.DataContractReader.Legacy;

[GeneratedComClass]
internal sealed unsafe partial class ClrDataTask : IXCLRDataTask
Copy link
Member Author

Choose a reason for hiding this comment

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

I kind of wanted to call this ClrDataThread, since I think that maps more clearly to what it is representing. But I stuck with matching the IXCLRDataTask name and the IXCLRData*::GetTask* APIs that return it (and also the name of the brittle DAC's implementation).

if (matchingThread == TargetPointer.Null)
return HResults.E_INVALIDARG;

ComWrappers cw = new StrategyBasedComWrappers();
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR specifically, but I've wondered in the past what this thing is and whether it's expensive

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an implementation of ComWrappers that gives you our default marshalling (static instances of IUnknown*Strategy implementations), but also lets you customize it (we don't use the customization). It shouldn't be expensive.

That said, I think I must have been doing something weird when I was trying to switch the parameters out IXCLRData* before - so I'm going to look at that again. That would re-use the same StrategyBasedComWrappers (and the same static instances of IUnknnown*Strategy implementations) and be nicer in C#.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants