Replace CordbProcess::GetSharedDomain with GetAppDomain
#117037
Conversation
|
Tagging subscribers to this area: @steveisok, @dotnet/dotnet-diag |
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
| if (m_sharedAppDomain == NULL) | ||
| // Return the one and only app domain | ||
| HASHFIND find; | ||
| CordbAppDomain* appDomain = m_appDomains.FindFirst(&find); |
There was a problem hiding this comment.
Any reason to walk the hashtable and then do the GetCurrentAppDomain dance? Just for faster lookup?
There was a problem hiding this comment.
Yeah, just faster lookup / avoiding the actual dac call if we don't need it.
| m_pAssembly = m_pAppDomain->LookupOrCreateAssembly(dfInfo.vmDomainAssembly); | ||
| } | ||
| else | ||
| { | ||
| // Not yet implemented | ||
| m_pAppDomain = pProcess->GetSharedAppDomain(); | ||
| m_pAppDomain = pProcess->GetAppDomain(); |
There was a problem hiding this comment.
Previously we had two domain objects, each of which had a list of assemblies inside. For our fake shared domain all the assemblies are associated with a VMPTR_Assembly pointer and for our single real app domain all the assemblies are associated with a VMPTR_DomainAssembly pointer.
This change looks like it coallesces the domain objects but I didn't notice anything that coallesces the assemblies. I suspect we'd now end up with a single domain object where every assembly is potentially double registered, once under a DomainAssembly pointer and a 2nd time under an Assembly pointer. This could lead to multiple ICorDebugAssembly interface objects getting exposed for the same assembly in the same domain which breaks the behavior we've promised to API callers.
I'd suggest we pick VMPTR_Assembly as the canonical identifier for assembly lookup and always use that for LookupOrCreateAssembly(). That should prevent any duplicate creation. DAC has GetAssemblyFromDomainAssembly to do the conversion.
As a refactoring step in the future we could eliminate VMPTR_DomainAssembly entirely, using VMPTR_Assembly instead.
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