Skip to content

Conversation

rcj1
Copy link
Contributor

@rcj1 rcj1 commented Jul 14, 2025

No description provided.

@Copilot Copilot AI review requested due to automatic review settings July 14, 2025 22:59
Copy link
Contributor

@Copilot 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 introduces a new cDAC implementation for retrieving AppDomain store data and adds the corresponding data structure to the SOS DAC interface.

  • Implements GetAppDomainStoreData in SOSDacImpl by reading global pointers for system and app domains.
  • Defines a new DacpAppDomainStoreData struct in ISOSDacInterface.
  • Adds DEBUG-only assertions to verify parity with the legacy DAC implementation.

Reviewed Changes

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

File Description
src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.cs Added GetAppDomainStoreData method with pointer reads, exception handling, and debug cross-check
src/native/managed/cdac/mscordaccore_universal/Legacy/ISOSDacInterface.cs Introduced DacpAppDomainStoreData struct for the new API
Comments suppressed due to low confidence (3)

src/native/managed/cdac/mscordaccore_universal/Legacy/ISOSDacInterface.cs:31

  • The field name DomainCount uses PascalCase while the other fields use camelCase (sharedDomain, systemDomain). Consider renaming to domainCount for consistency.
    public ulong DomainCount;

src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.cs:119

  • There are no unit tests exercising the new GetAppDomainStoreData API. Consider adding tests under the existing test suite to validate pointer reads and returned values.
    int ISOSDacInterface.GetAppDomainStoreData(void* data)

src/native/managed/cdac/mscordaccore_universal/Legacy/ISOSDacInterface.cs:27

  • The new DacpAppDomainStoreData struct lacks XML documentation. Add <summary> comments for the struct and its fields to improve clarity for consumers.
internal struct DacpAppDomainStoreData

Copy link
Contributor

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

@rcj1 rcj1 requested a review from max-charlamb July 15, 2025 01:06
Copy link
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.

lgtm 👍

@rcj1
Copy link
Contributor Author

rcj1 commented Jul 20, 2025

/ba-g build timeouts

@rcj1 rcj1 merged commit bf92e06 into dotnet:main Jul 20, 2025
46 of 48 checks passed
@rcj1 rcj1 deleted the GetAppDomainStoreData branch July 20, 2025 22:13
@github-actions github-actions bot locked and limited conversation to collaborators Aug 20, 2025
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