-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Replace CordbProcess::GetSharedDomain
with GetAppDomain
#117037
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Tagging subscribers to this area: @steveisok, @dotnet/dotnet-diag |
There was a problem hiding this 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 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();
f1dfd15
to
e532947
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is an assembly duplication issue, but once that is fixed looked good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
…otnet#117037)" (dotnet#117221) This reverts commit a3929e2.
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