Skip to content

Use module as ID for debugger module cache instead of domain assembly - #118414

Merged
tommcdon merged 2 commits into
dotnet:mainfrom
tommcdon:dev/tommcdon/fixmoduleduplication
Aug 6, 2025
Merged

Use module as ID for debugger module cache instead of domain assembly#118414
tommcdon merged 2 commits into
dotnet:mainfrom
tommcdon:dev/tommcdon/fixmoduleduplication

Conversation

@tommcdon

Copy link
Copy Markdown
Member

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.

@tommcdontommcdon added this to the 10.0.0 milestone Aug 5, 2025
@tommcdontommcdon self-assigned this Aug 5, 2025
CopilotAI review requested due to automatic review settings August 5, 2025 21:54

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

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

Comment threadsrc/coreclr/debug/di/rsappdomain.cpp
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Comment threadsrc/coreclr/debug/di/rsappdomain.cpp

@noahfalknoahfalk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍

@tommcdon

tommcdon commented Aug 6, 2025

Copy link
Copy Markdown
MemberAuthor

/ba-g Failures are unrelated

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

Copy link
Copy Markdown
MemberAuthor

/ba-g Failures are unrelated

@tommcdon
tommcdon merged commit 6b4936e into dotnet:mainAug 6, 2025
88 of 90 checks passed
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Sep 6, 2025
Sign up for freeto 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.

4 participants

@tommcdon@noahfalk@hoyosjs