Skip to content

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

Merged
merged 7 commits into from
Jul 1, 2025

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented Jun 26, 2025

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

Copy link
Contributor

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

Copy link
Contributor

@Copilot Copilot AI left a 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) and GetSharedAppDomain(), replacing them with no-arg GetCurrentAppDomain() and GetAppDomain()
  • 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 elinor-fung force-pushed the noSharedDomain-cordb branch from f1dfd15 to e532947 Compare June 26, 2025 17:00
Copy link
Member

@noahfalk noahfalk left a 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!

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@elinor-fung elinor-fung merged commit e0e9d7c into dotnet:main Jul 1, 2025
93 of 97 checks passed
davidwrighton added a commit that referenced this pull request Jul 1, 2025
@elinor-fung elinor-fung deleted the noSharedDomain-cordb branch July 1, 2025 22:31
@elinor-fung elinor-fung restored the noSharedDomain-cordb branch July 1, 2025 22:35
davidwrighton added a commit that referenced this pull request Jul 2, 2025
elinor-fung added a commit to elinor-fung/runtime that referenced this pull request Jul 2, 2025
@elinor-fung elinor-fung deleted the noSharedDomain-cordb branch July 2, 2025 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants