Skip to content

Conversation

rcj1
Copy link
Contributor

@rcj1 rcj1 commented Jul 10, 2025

No description provided.

@Copilot Copilot AI review requested due to automatic review settings July 10, 2025 19:02
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 implements the cDAC (Composite Data Access Component) functionality for GetAppDomainList and GetAssemblyModuleList methods in the SOSDacImpl, replacing stub implementations that previously delegated to legacy implementations.

  • Adds cDAC implementation for retrieving application domain lists from target memory
  • Implements assembly module list retrieval functionality with proper error handling
  • Extends the ILoader contract with a new GetModulePointer method to support assembly-to-module mapping

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
SOSDacImpl.cs Replaces stub implementations with full cDAC logic for GetAppDomainList and GetAssemblyModuleList methods
Loader_1.cs Implements GetModulePointer method to retrieve module pointer from assembly pointer
ILoader.cs Adds GetModulePointer method signature to the ILoader interface
Comments suppressed due to low confidence (2)

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

  • The variable name 'i' is ambiguous. Consider using a more descriptive name like 'appDomainCount' or 'foundCount' to clarify its purpose as a counter for found app domains.
            uint i = 0;

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

  • The variable name 'addr' is ambiguous when there's already an 'assembly' parameter. Consider renaming to 'assemblyPointer' or 'assemblyTargetPointer' to clarify the distinction.
        TargetPointer addr = 0;

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 10, 2025 23:42
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.

This shouldn't require any new contract docs since it only uses existing contracts/datatypes and the ones added in #117452

@risc-vv
Copy link

risc-vv commented Jul 12, 2025

@dotnet/samsung Could you please take a look? These changes may be related to riscv64.

@risc-vv
Copy link

risc-vv commented Jul 12, 2025

33b3d1c is being scheduled for building and testing

GIT: 33b3d1ce8b3ca430e5f1e20f4559a3715c9732cb
REPO: dotnet/runtime
BRANCH: main

Release-build FAILED

buildinfo.json
Cloning into '/godata/pipelines/Release-build/runtime'...
Updating files: 14% (8763/60509)
Updating files: 15% (9077/60509)
Updating files: 16% (9682/60509)
Updating files: 17% (10287/60509)
Updating files: 18% (10892/60509)
Updating files: 19% (11497/60509)
Updating files: 20% (12102/60509)
Updating files: 21% (12707/60509)
Updating files: 22% (13312/60509)
Updating files: 23% (13918/60509)
Updating files: 24% (14523/60509)
Updating files: 25% (15128/60509)
Updating files: 26% (15733/60509)
Updating files: 27% (16338/60509)
Updating files: 28% (16943/60509)
Updating files: 29% (17548/60509)
Updating files: 30% (18153/60509)
Updating files: 31% (18758/60509)
Updating files: 32% (19363/60509)
Updating files: 32% (19609/60509)
Updating files: 33% (19968/60509)
Updating files: 34% (20574/60509)
Updating files: 35% (21179/60509)
Updating files: 36% (21784/60509)
Updating files: 37% (22389/60509)
Updating files: 38% (22994/60509)
Updating files: 39% (23599/60509)
Updating files: 40% (24204/60509)
Updating files: 41% (24809/60509)
Updating files: 42% (25414/60509)
Updating files: 43% (26019/60509)
Updating files: 44% (26624/60509)
Updating files: 45% (27230/60509)
Updating files: 46% (27835/60509)
Updating files: 47% (28440/60509)
Updating files: 48% (29045/60509)
Updating files: 49% (29650/60509)
Updating files: 50% (30255/60509)
Updating files: 51% (30860/60509)
Updating files: 51% (30957/60509)
Updating files: 52% (31465/60509)
Updating files: 53% (32070/60509)
Updating files: 54% (32675/60509)
Updating files: 55% (33280/60509)
Updating files: 56% (33886/60509)
Updating files: 57% (34491/60509)
Updating files: 58% (35096/60509)
Updating files: 59% (35701/60509)
Updating files: 60% (36306/60509)
Updating files: 61% (36911/60509)
Updating files: 62% (37516/60509)
Updating files: 63% (38121/60509)
Updating files: 64% (38726/60509)
Updating files: 65% (39331/60509)
Updating files: 66% (39936/60509)
Updating files: 67% (40542/60509)
Updating files: 68% (41147/60509)
Updating files: 69% (41752/60509)
Updating files: 70% (42357/60509)
Updating files: 71% (42962/60509)
Updating files: 72% (43567/60509)
Updating files: 73% (44172/60509)
Updating files: 74% (44777/60509)
Updating files: 75% (45382/60509)
Updating files: 76% (45987/60509)
Updating files: 76% (46398/60509)
Updating files: 77% (46592/60509)
Updating files: 78% (47198/60509)
Updating files: 79% (47803/60509)
Updating files: 80% (48408/60509)
Updating files: 81% (49013/60509)
Updating files: 82% (49618/60509)
Updating files: 83% (50223/60509)
Updating files: 84% (50828/60509)
Updating files: 85% (51433/60509)
Updating files: 86% (52038/60509)
Updating files: 87% (52643/60509)
Updating files: 88% (53248/60509)
Updating files: 89% (53854/60509)
Updating files: 90% (54459/60509)
Updating files: 91% (55064/60509)
Updating files: 91% (55439/60509)
Updating files: 92% (55669/60509)
Updating files: 93% (56274/60509)
Updating files: 94% (56879/60509)
Updating files: 95% (57484/60509)
Updating files: 96% (58089/60509)
Updating files: 97% (58694/60509)
Updating files: 98% (59299/60509)
Updating files: 99% (59904/60509)
Updating files: 100% (60509/60509)
Updating files: 100% (60509/60509), done.
From https://github.com/rcj1/runtime

  • branch GetAppDomainList -> FETCH_HEAD
  • [new branch] GetAppDomainList -> upstream/GetAppDomainList
    Auto-merging eng/Subsets.props
    Auto-merging eng/Version.Details.xml
    Auto-merging eng/Versions.props
    Auto-merging src/coreclr/debug/daccess/daccess.cpp
    Auto-merging src/coreclr/interpreter/compiler.cpp
    Auto-merging src/coreclr/interpreter/compiler.h
    Auto-merging src/coreclr/interpreter/intops.def
    Auto-merging src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.cs
    CONFLICT (rename/delete): src/tests/async/reflection/reflectionSimple.cs renamed to src/tests/async/reflection/reflection-simple.cs in HEAD, but deleted in upstream/GetAppDomainList.
    CONFLICT (rename/delete): src/tests/async/reflection/reflectionSimple.csproj renamed to src/tests/async/reflection/reflection-simple.csproj in HEAD, but deleted in upstream/GetAppDomainList.
    Automatic merge failed; fix conflicts and then commit the result.

@rcj1 rcj1 force-pushed the GetAppDomainList branch from 2772979 to 71ded2c Compare July 16, 2025 20:29
@rcj1 rcj1 force-pushed the GetAppDomainList branch from e671627 to dcda50e Compare July 16, 2025 22:49
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 rcj1 merged commit 959c2a7 into dotnet:main Jul 21, 2025
48 checks passed
@rcj1 rcj1 deleted the GetAppDomainList branch July 21, 2025 15:18
@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 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.

3 participants