Replace CordbProcess::GetSharedDomain with GetAppDomain#117037
Merged
elinor-fung merged 7 commits intodotnet:mainfrom Jul 1, 2025
Merged
Replace CordbProcess::GetSharedDomain with GetAppDomain#117037elinor-fung merged 7 commits intodotnet:mainfrom
CordbProcess::GetSharedDomain with GetAppDomain#117037elinor-fung merged 7 commits intodotnet:mainfrom
Conversation
Contributor
|
Tagging subscribers to this area: @steveisok, @dotnet/dotnet-diag |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR removes the legacy “shared domain” concept and thread-specific domain lookups, consolidating all operations on the single AppDomain.
- Drop
GetCurrentAppDomain(VMPTR_Thread)andGetSharedAppDomain(), replacing them with no-argGetCurrentAppDomain()andGetAppDomain() - Remove
m_sharedAppDomain,m_pDefaultAppDomain, and related fallback logic - Update all callers (threads, modules, DAC interface) to use the unified APIs
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/debug/inc/dacdbiinterface.h | Removed thread argument from GetCurrentAppDomain declaration |
| src/coreclr/debug/di/rsthread.cpp | Switched to GetAppDomain() and removed per-thread domain code |
| src/coreclr/debug/di/rspriv.h | Eliminated shared/default domain members and declarations |
| src/coreclr/debug/di/process.cpp | Removed shared/default domain fields and methods; added GetAppDomain |
| src/coreclr/debug/di/module.cpp | Replaced use of shared domain in module init |
| src/coreclr/debug/daccess/dacdbiimpl.h | Dropped thread parameter from GetCurrentAppDomain declaration |
| src/coreclr/debug/daccess/dacdbiimpl.cpp | Updated GetCurrentAppDomain implementation to no-arg form |
Comments suppressed due to low confidence (1)
src/coreclr/debug/di/rsthread.cpp:1894
- There’s a duplicate assignment here which looks like a typo. It should be:
CordbAppDomain* pThreadCurrentDomain = GetProcess()->GetAppDomain(); CordbAppDomain * pThreadCurrentDomain = pThreadCurrentDomain = GetProcess()->GetAppDomain();
elinor-fung
force-pushed
the
noSharedDomain-cordb
branch
from
f1dfd15 to
e532947
Compare
elinor-fung
commented
Jun 26, 2025
This was referenced Jun 26, 2025
hoyosjs
approved these changes
Jun 26, 2025
noahfalk
reviewed
Jun 27, 2025
Member
noahfalk
left a comment
noahfalk
left a comment
There was a problem hiding this comment.
I think there is an assembly duplication issue, but once that is fixed looked good to me!
elinor-fung
added a commit
to elinor-fung/runtime
that referenced
this pull request
Jul 2, 2025
…otnet#117037)" (dotnet#117221) This reverts commit a3929e2.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This removes the 'shared domain' idea (which was basically an empty
CordbAppDomain) from the ICorDebug* implementations. Everything operates on the (one and only) AppDomain. There's plenty more that could be cleaned up with app domains here (around there only being one now), but what's in this change seemed like a reasonably contained step.See also #108618 (comment) - cc @noahfalk @am11