Skip to content

Conversation

@tommcdon
Copy link
Member

@tommcdon tommcdon commented Aug 5, 2025

This pull request addresses an issue caused by #117224 found during Visual Studio debugger testing, where ICorDebugClass.GetModule returns a different instance of ICorDebugModule compared to the instance received via ICorDebugManagedCallback::LoadModule.

@tommcdon tommcdon added this to the 10.0.0 milestone Aug 5, 2025
@tommcdon tommcdon self-assigned this Aug 5, 2025
Copilot AI review requested due to automatic review settings August 5, 2025 21:54
Copy link
Contributor

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 pull request fixes a debugger module cache inconsistency where ICorDebugClass.GetModule returned a different ICorDebugModule instance compared to the one received via ICorDebugManagedCallback::LoadModule. The fix changes the debugger's module caching strategy to consistently use the module pointer as the cache key instead of sometimes using the domain assembly pointer.

Key changes:

  • Standardizes module cache lookups to always use the module pointer as the key
  • Ensures module pointer is resolved early when only domain assembly is available
  • Updates the CordbModule constructor to use module pointer for its base identifier

Reviewed Changes

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

File Description
src/coreclr/debug/di/rsappdomain.cpp Updates module cache lookup to use module pointer consistently and resolves module pointer early
src/coreclr/debug/di/process.cpp Changes class lookup to resolve and use module pointer instead of domain assembly for cache access
src/coreclr/debug/di/module.cpp Updates CordbModule constructor to use module pointer for base class identifier
Comments suppressed due to low confidence (1)

@dotnet-policy-service
Copy link
Contributor

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

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@tommcdon
Copy link
Member Author

tommcdon commented Aug 6, 2025

/ba-g Failures are unrelated

@tommcdon tommcdon enabled auto-merge (squash) August 6, 2025 21:31
@tommcdon
Copy link
Member Author

tommcdon commented Aug 6, 2025

/ba-g Failures are unrelated

@tommcdon tommcdon merged commit 6b4936e into dotnet:main Aug 6, 2025
88 of 90 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 6, 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