Skip to content

Conversation

rcj1
Copy link
Contributor

@rcj1 rcj1 commented Jul 9, 2025

Adding the implementation of the GetAssemblyModuleList API to the cDAC

@Copilot Copilot AI review requested due to automatic review settings July 9, 2025 04: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

Adds the cDAC implementation for GetAssemblyModuleList and extends the loader contracts to support fetching a module pointer from an assembly reference.

  • Implements ISOSDacInterface.GetAssemblyModuleList in SOSDacImpl.cs with argument validation, loader dispatch, HRESULT mapping, and debug-time consistency checks against the legacy DAC.
  • Introduces a new GetModulePointer method on the ILoader interface (abstraction and contract) and provides its implementation in Loader_1.cs.
  • Updates interface definitions in ILoader.cs to include the new API.

Reviewed Changes

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

File Description
src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.cs Full implementation of GetAssemblyModuleList, including error handling and DEBUG assertions
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs Implements ILoader.GetModulePointer, throwing on null or missing module
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs Adds default interface method GetModulePointer to the loader abstraction
Comments suppressed due to low confidence (2)

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

  • Add unit tests for GetAssemblyModuleList covering edge cases (null/zero assembly, zero count, and assemblies with multiple modules) to validate the new implementation.
    int ISOSDacInterface.GetAssemblyModuleList(ClrDataAddress assembly, uint count, [In, MarshalUsing(CountElementName = "count"), Out] ClrDataAddress[] modules, uint* pNeeded)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs:80

  • [nitpick] The new GetModulePointer interface method lacks XML doc comments. Consider adding a summary and parameter/return descriptions to maintain documentation consistency.
    TargetPointer GetModulePointer(TargetPointer assemblyPointer) => throw new NotImplementedException();

@rcj1 rcj1 requested a review from max-charlamb July 10, 2025 18:17
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.

Code changes lgtm 👍
After the runtime-diagnostics pipeline passes lets merge this.

@rcj1 rcj1 enabled auto-merge (squash) July 15, 2025 17:37
@rcj1 rcj1 disabled auto-merge July 15, 2025 17:38
@rcj1 rcj1 enabled auto-merge (squash) July 15, 2025 20:43
@rcj1 rcj1 merged commit 2fcafaf into dotnet:main Jul 15, 2025
49 checks passed
@rcj1 rcj1 deleted the GetAssemblyModuleList branch July 16, 2025 00:49
@github-actions github-actions bot locked and limited conversation to collaborators Aug 15, 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