Skip to content

Revert "Replace CordbProcess::GetSharedDomain with GetAppDomain" - #117221

Merged
davidwrighton merged 1 commit into
mainfrom
revert-117037-noSharedDomain-cordb
Jul 2, 2025
Merged

Revert "Replace CordbProcess::GetSharedDomain with GetAppDomain"#117221
davidwrighton merged 1 commit into
mainfrom
revert-117037-noSharedDomain-cordb

Conversation

@davidwrighton

@davidwrightondavidwrighton commented Jul 1, 2025

Copy link
Copy Markdown
Member

Reverts #117037 This breaks debugging basics.

Notably, enumerating the list of assemblies results in an infinite loop as it builds more and more CorDbAssembly objects.

@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.

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

Reverts previous PR (#117037) that replaced thread-specific domain lookup with a single-domain model, restoring per-thread GetCurrentAppDomain behavior and introducing a shared default AppDomain fallback.

  • Updated IDacDbiInterface and its implementation to take a vmThread parameter for GetCurrentAppDomain.
  • Modified all callers in CordbThread, CordbProcess, and related classes to pass their thread token.
  • Added GetSharedAppDomain, m_sharedAppDomain fallback, and UpdateThreadsForAdUnload to manage default domain unloading and thread cleanup.

Reviewed Changes

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

Show a summary per file
FileDescription
src/coreclr/debug/inc/dacdbiinterface.hChanged GetCurrentAppDomain to accept a vmThread argument.
src/coreclr/debug/di/rsthread.cppUpdated calls to GetCurrentAppDomain with vmThread.
src/coreclr/debug/di/rspriv.hAdjusted CacheAssembly overload signatures; renamed GetAppDomain.
src/coreclr/debug/di/rsappdomain.cppRefactored assembly caching/removal to use VMPTR_DomainAssembly.
src/coreclr/debug/di/process.cppAdded m_sharedAppDomain, GetSharedAppDomain, and thread cleanup.
src/coreclr/debug/di/module.cppFallback to GetSharedAppDomain when no DomainAssembly is available.
src/coreclr/debug/daccess/dacdbiimpl.h/.cppAdded thread-parameter overload for GetCurrentAppDomain.

Comment threadsrc/coreclr/debug/daccess/dacdbiimpl.cpp
Comment threadsrc/coreclr/debug/di/module.cpp

@elinor-fungelinor-fung 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.

Thanks. I only tried VS and SOS unit tests as basic testing, which apparently don't go down the broken path. Clearly needed windbg too.

@elinor-fung

elinor-fung commented Jul 2, 2025

Copy link
Copy Markdown
Member

#117221 is the fix for assemblies not being found in the cache. Assuming this goes in first, I'll update that to be the revert of the revert.

@davidwrighton
davidwrighton merged commit a3929e2 into mainJul 2, 2025
elinor-fung added a commit to elinor-fung/runtime that referenced this pull request Jul 2, 2025
@hoyosjs
hoyosjs deleted the revert-117037-noSharedDomain-cordb branch July 2, 2025 00:22
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 1, 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

@davidwrighton@elinor-fung@hoyosjs