[cDAC] Implement MarkDebuggerAttach* DacDbi APIs

[Copilot]

• Implements MarkDebuggerAttachPending and MarkDebuggerAttached in cDAC DacDbiImpl by adding corresponding Debugger_1 contract APIs and wiring them to target-memory writes of debugger control flags.
• Removes fibermode dead code

[Copilot]

Pull request overview

This PR extends the cDAC Debugger contract and the legacy DacDbiImpl surface to support MarkDebuggerAttachPending / MarkDebuggerAttached by writing the appropriate bits into g_CORDebuggerControlFlags, and adds unit test coverage plus documentation updates.

Changes:

• Added IDebugger.MarkDebuggerAttachPending() and IDebugger.MarkDebuggerAttached(bool) APIs and implemented them in Debugger_1 via target-memory writes.
• Updated DacDbiImpl to call the cDAC contract implementations (with DEBUG-only legacy cross-validation).
• Exposed g_CORDebuggerControlFlags via the CoreCLR data descriptor and added tests/docs for the new flag semantics.

Reviewed changes

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

Show a summary per file

File	Description
src/native/managed/cdac/tests/DebuggerTests.cs	Adds test target plumbing + new unit tests validating control-flag writes.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements the two DBI methods by calling the cDAC Debugger contract and translating errors to HRESULTs.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger_1.cs	Adds the flag enum and implements the two new attach APIs by updating CORDebuggerControlFlags.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs	Introduces the new Globals.CORDebuggerControlFlags name constant.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IDebugger.cs	Extends the contract interface with the two new methods.
src/coreclr/vm/datadescriptor/datadescriptor.inc	Exposes g_CORDebuggerControlFlags and adjusts Debugger global gating.
src/coreclr/inc/cordbpriv.h	Adds cDAC dependency annotations to DBCF_PENDING_ATTACH / DBCF_ATTACHED.
docs/design/datacontracts/Debugger.md	Documents the new APIs, global dependency, and flag behavior.

[Copilot]

Pull request overview

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

[dotnet-policy-service]

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

[rcj1]

Test helpers tbd in #126595

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

[github-actions]

🤖 Copilot Code Review — PR #126794

Note

This review was generated by Copilot (Claude Opus 4.6) with additional analysis from Claude Sonnet 4.5.

Holistic Assessment

Motivation: This PR implements debugger attach flag methods (MarkDebuggerAttachPending, MarkDebuggerAttached) in the cDAC debugger contract and DacDbi layer, migrates the debugging infrastructure from VMPTR_Assembly to VMPTR_DomainAssembly to better model AppDomain affinity, and removes dead fiber-mode debugging code. The architectural direction is sound — DomainAssembly is the correct abstraction for ICorDebug's module concept.

Approach: The change systematically threads VMPTR_DomainAssembly and VMPTR_AppDomain through the entire DacDbi interface (IDL, interface headers, DI, DAC, and cDAC implementations). The shift from a single m_pAppDomain pointer to a CordbSafeHashTable<CordbAppDomain> m_appDomains in CordbProcess is the right structural change. The dead code removal (fiber mode, unused enum flags) is clean.

Summary: ⚠️Needs Human Review. The code is architecturally well-structured and the core implementations look correct. However, there are several items that require maintainer attention: incomplete dead code cleanup, a parameter naming inconsistency in the IDL, and the DB_IPCE_EXIT_APP_DOMAIN re-enablement warrants explicit acknowledgment. The protocol breaking change counter question also needs a maintainer decision.

---

Detailed Findings

⚠️ DacDbi Protocol Breaking Change Counter — Not bumped

src/coreclr/debug/inc/dacdbistructures.h:692 — kCurrentDacDbiProtocolBreakingChangeCounter remains at 1 despite extensive breaking changes to IDacDbiInterface: ~30+ method signatures changed (Assembly → DomainAssembly), new methods added (GetAppDomainFromId, EnumerateAppDomains, GetAssemblyFromDomainAssembly, etc.), callback typedefs changed, and struct fields modified.

Per the comment at line 699-710, the counter should be explicitly bumped and documented when breaking changes are introduced. The auto-generated MD5 hash mechanism (described at line 616-618 in process.cpp) will detect the change automatically for remote transport scenarios, so this is unlikely to cause a runtime crash. However, the convention says to bump and document the counter. A maintainer should decide if a bump is needed here or if the hash is sufficient.

⚠️ Incomplete Dead Code Cleanup — CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS

src/coreclr/debug/di/process.cpp:237,8575 — The fiber-mode code was removed (DBCF_FIBERMODE, m_bHostingInFiber, the fiber check in VerifyControlBlock), but two references to CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS remain:

1. Line 237: In IsLegalFatalError() — lists this error as a legal fatal error. Since no code path can produce this error anymore, this entry is dead.
2. Line 8575: if (!IsLegalFatalError(errorHR) || (errorHR != CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS)) — This special-cases the fiber error to skip setting m_unrecoverableError. Since the error is never produced, the special case is also dead.

These should be cleaned up for consistency with the fiber-mode removal. Not blocking, but creates confusing dead code.

⚠️ IDL Parameter Naming Inconsistency — vmAssembly for DomainAssembly type

src/coreclr/inc/dacdbi.idl — The EnumerateModulesInAssembly method has:

HRESULT EnumerateModulesInAssembly([in] VMPTR_DomainAssembly vmAssembly, ...);

The parameter is named vmAssembly but the type is VMPTR_DomainAssembly. This should be vmDomainAssembly for consistency with all other methods in this PR.

⚠️DB_IPCE_EXIT_APP_DOMAIN Re-enabled — Needs documentation

src/coreclr/debug/inc/dbgipceventtypes.h:49 — This event type was previously commented out and is now re-enabled. The handler in process.cpp is well-written (with proper null checks and assertions). This is clearly needed for the new multi-AppDomain hash table support. However, it would be helpful to have a brief comment in the code or commit message explaining why this was re-enabled, since the previous comment suggested it was intentionally disabled.

✅ cDAC Debugger Contract Implementation — Correct

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger_1.cs — The new MarkDebuggerAttachPending() and MarkDebuggerAttached() implementations:

• Flag values (0x0100, 0x0200) match cordbpriv.h constants ✅
• Bitwise operations match the native DAC implementation in dacdbiimpl.cpp:4798-4869 ✅
• MarkDebuggerAttached(false) correctly clears both Attached and PendingAttach flags ✅
• The read-modify-write pattern is not atomic, but this matches the native code which relies on the DD lock for synchronization ✅

✅ TryGetDebuggerData Behavior Change — Intentional and correct

Debugger_1.cs:21-35 — Previously returned false when LeftSideInitialized == 0; now returns true with IsLeftSideInitialized in the data. This better matches the native DAC behavior: GetDefinesBitField and GetMDStructuresVersion (dacdbiimpl.cpp:7985-8009) check g_pDebugger == NULL but NOT LeftSideInitialized. The DacDbiImpl.cs wrapper at line 64 correctly uses data.IsLeftSideInitialized for the IsLeftSideInitialized DBI method. The dump test at line 93-94 was updated to cross-validate correctly.

✅ Struct Layout Binary Compatibility — Correct

src/coreclr/debug/inc/dbgipcevents.h — Replacing bool m_bHostingInFiber with BYTE padding1 maintains the same 1-byte layout within the DWORD-aligned group (m_checkedBuild + padding1 + padding2 + padding3 = 4 bytes). The transport marshaling in dbgtransportsession.cpp correctly omits the padding field. Both DebuggerIPCControlBlock and DebuggerIPCControlBlockTransport are updated consistently.

✅ Test Changes — Correct and comprehensive

src/native/managed/cdac/tests/DebuggerTests.cs — The removal of memBuilder.AddHeapFragment(debuggerControlFlagsFrag) is correct: BumpAllocator.Allocate auto-registers heap fragments with the builder. The three new tests (MarkDebuggerAttachPending_SetsPendingAttachFlag, MarkDebuggerAttached_SetsAttachedFlag_WhenTrue, MarkDebuggerAttached_ClearsAttachedAndPending_WhenFalse) provide good coverage with distinct initial states. The DacDbi dump tests add coverage for GetAppDomainFromId, EnumerateAppDomains, IsAssemblyFullyTrusted, and GetAppDomainIdFromVmObjectHandle.

✅ AppDomain Management Rearchitecture — Well-structured

src/coreclr/debug/di/process.cpp, rspriv.h — The migration from CordbAppDomain* m_pAppDomain to CordbSafeHashTable<CordbAppDomain> m_appDomains is clean:

• NeuterChildren correctly uses NeuterAndClear instead of manual pointer cleanup
• NeuterChildrenLeftSideResources correctly copies to an auxiliary list before releasing the lock
• CacheAppDomain properly uses AddBaseOrThrow with RSInitHolder RAII
• PrepopulateAppDomainsOrThrow gates on IsDacInitialized()
• ContinueInternal handles the hash table properly with CopyToArray

💡 GetInstantiationFieldInfo Uses AppDomain::GetCurrentDomain()

src/coreclr/debug/daccess/dacdbiimpl.cpp:1817 — This method takes a VMPTR_DomainAssembly parameter but uses AppDomain::GetCurrentDomain() for CollectFields instead of deriving the AppDomain from the parameter. In modern .NET (single AppDomain), this is functionally correct and mirrors how the runtime operates. Noted as a potential follow-up if multi-domain support were ever reconsidered.

💡 Mixed Concerns in PR

The PR bundles debugger attach flag implementation, Assembly→DomainAssembly migration, fiber-mode dead code removal, RequiresAlign8 removal from RuntimeTypeSystem, and ContractRegistry simplification. While these are related to the broader cDAC migration, splitting the dead code removal and RequiresAlign8 changes into separate PRs would make the changes easier to review and bisect.

Generated by Code Review for issue #126794 · ◷

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/coreclr/debug/di/process.cpp:8386

• CordbProcess::UnrecoverableError no longer sets m_unrecoverableError for legal fatal errors (e.g., CORDBG_E_INCOMPATIBLE_PROTOCOL), because the condition changed from effectively "all except CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS" to only !IsLegalFatalError(errorHR). This is a behavior change beyond removing fiber-mode handling and may leave the process in a non-zombie state after an unrecoverable condition. Consider restoring the previous intent (e.g., set m_unrecoverableError for legal fatal errors too, or otherwise document/handle why legal fatal errors should avoid zombie state).

 CONSISTENCY_CHECK_MSGF(IsLegalFatalError(errorHR), ("Unrecoverable internal error: hr=0x%08x!", errorHR));
if (!IsLegalFatalError(errorHR))
{
// This will throw everything into a Zombie state. The ATT_ macros will check this and fail immediately.
m_unrecoverableError = true;
//
// Mark the process as no longer synchronized.
//
Lock();
SetSynchronized(false);
IncStopCount();
Unlock();
}

[rcj1]

/ba-g wasm

[Copilot]

• Implements MarkDebuggerAttachPending and MarkDebuggerAttached in cDAC DacDbiImpl by adding corresponding Debugger_1 contract APIs and wiring them to target-memory writes of debugger control flags.
• Removes fibermode dead code

[Copilot]

Pull request overview

This PR extends the cDAC Debugger contract and the legacy DacDbiImpl surface to support MarkDebuggerAttachPending / MarkDebuggerAttached by writing the appropriate bits into g_CORDebuggerControlFlags, and adds unit test coverage plus documentation updates.

Changes:

• Added IDebugger.MarkDebuggerAttachPending() and IDebugger.MarkDebuggerAttached(bool) APIs and implemented them in Debugger_1 via target-memory writes.
• Updated DacDbiImpl to call the cDAC contract implementations (with DEBUG-only legacy cross-validation).
• Exposed g_CORDebuggerControlFlags via the CoreCLR data descriptor and added tests/docs for the new flag semantics.

Reviewed changes

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

Show a summary per file

File	Description
src/native/managed/cdac/tests/DebuggerTests.cs	Adds test target plumbing + new unit tests validating control-flag writes.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements the two DBI methods by calling the cDAC Debugger contract and translating errors to HRESULTs.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger_1.cs	Adds the flag enum and implements the two new attach APIs by updating CORDebuggerControlFlags.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs	Introduces the new Globals.CORDebuggerControlFlags name constant.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IDebugger.cs	Extends the contract interface with the two new methods.
src/coreclr/vm/datadescriptor/datadescriptor.inc	Exposes g_CORDebuggerControlFlags and adjusts Debugger global gating.
src/coreclr/inc/cordbpriv.h	Adds cDAC dependency annotations to DBCF_PENDING_ATTACH / DBCF_ATTACHED.
docs/design/datacontracts/Debugger.md	Documents the new APIs, global dependency, and flag behavior.

[Copilot]

Pull request overview

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

[dotnet-policy-service]

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

[rcj1]

Test helpers tbd in #126595

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

[github-actions]

🤖 Copilot Code Review — PR #126794

Note

This review was generated by Copilot (Claude Opus 4.6) with additional analysis from Claude Sonnet 4.5.

Holistic Assessment

Motivation: This PR implements debugger attach flag methods (MarkDebuggerAttachPending, MarkDebuggerAttached) in the cDAC debugger contract and DacDbi layer, migrates the debugging infrastructure from VMPTR_Assembly to VMPTR_DomainAssembly to better model AppDomain affinity, and removes dead fiber-mode debugging code. The architectural direction is sound — DomainAssembly is the correct abstraction for ICorDebug's module concept.

Approach: The change systematically threads VMPTR_DomainAssembly and VMPTR_AppDomain through the entire DacDbi interface (IDL, interface headers, DI, DAC, and cDAC implementations). The shift from a single m_pAppDomain pointer to a CordbSafeHashTable<CordbAppDomain> m_appDomains in CordbProcess is the right structural change. The dead code removal (fiber mode, unused enum flags) is clean.

Summary: ⚠️Needs Human Review. The code is architecturally well-structured and the core implementations look correct. However, there are several items that require maintainer attention: incomplete dead code cleanup, a parameter naming inconsistency in the IDL, and the DB_IPCE_EXIT_APP_DOMAIN re-enablement warrants explicit acknowledgment. The protocol breaking change counter question also needs a maintainer decision.

---

Detailed Findings

⚠️ DacDbi Protocol Breaking Change Counter — Not bumped

src/coreclr/debug/inc/dacdbistructures.h:692 — kCurrentDacDbiProtocolBreakingChangeCounter remains at 1 despite extensive breaking changes to IDacDbiInterface: ~30+ method signatures changed (Assembly → DomainAssembly), new methods added (GetAppDomainFromId, EnumerateAppDomains, GetAssemblyFromDomainAssembly, etc.), callback typedefs changed, and struct fields modified.

Per the comment at line 699-710, the counter should be explicitly bumped and documented when breaking changes are introduced. The auto-generated MD5 hash mechanism (described at line 616-618 in process.cpp) will detect the change automatically for remote transport scenarios, so this is unlikely to cause a runtime crash. However, the convention says to bump and document the counter. A maintainer should decide if a bump is needed here or if the hash is sufficient.

⚠️ Incomplete Dead Code Cleanup — CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS

src/coreclr/debug/di/process.cpp:237,8575 — The fiber-mode code was removed (DBCF_FIBERMODE, m_bHostingInFiber, the fiber check in VerifyControlBlock), but two references to CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS remain:

1. Line 237: In IsLegalFatalError() — lists this error as a legal fatal error. Since no code path can produce this error anymore, this entry is dead.
2. Line 8575: if (!IsLegalFatalError(errorHR) || (errorHR != CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS)) — This special-cases the fiber error to skip setting m_unrecoverableError. Since the error is never produced, the special case is also dead.

These should be cleaned up for consistency with the fiber-mode removal. Not blocking, but creates confusing dead code.

⚠️ IDL Parameter Naming Inconsistency — vmAssembly for DomainAssembly type

src/coreclr/inc/dacdbi.idl — The EnumerateModulesInAssembly method has:

HRESULT EnumerateModulesInAssembly([in] VMPTR_DomainAssembly vmAssembly, ...);

The parameter is named vmAssembly but the type is VMPTR_DomainAssembly. This should be vmDomainAssembly for consistency with all other methods in this PR.

⚠️DB_IPCE_EXIT_APP_DOMAIN Re-enabled — Needs documentation

src/coreclr/debug/inc/dbgipceventtypes.h:49 — This event type was previously commented out and is now re-enabled. The handler in process.cpp is well-written (with proper null checks and assertions). This is clearly needed for the new multi-AppDomain hash table support. However, it would be helpful to have a brief comment in the code or commit message explaining why this was re-enabled, since the previous comment suggested it was intentionally disabled.

✅ cDAC Debugger Contract Implementation — Correct

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger_1.cs — The new MarkDebuggerAttachPending() and MarkDebuggerAttached() implementations:

• Flag values (0x0100, 0x0200) match cordbpriv.h constants ✅
• Bitwise operations match the native DAC implementation in dacdbiimpl.cpp:4798-4869 ✅
• MarkDebuggerAttached(false) correctly clears both Attached and PendingAttach flags ✅
• The read-modify-write pattern is not atomic, but this matches the native code which relies on the DD lock for synchronization ✅

✅ TryGetDebuggerData Behavior Change — Intentional and correct

Debugger_1.cs:21-35 — Previously returned false when LeftSideInitialized == 0; now returns true with IsLeftSideInitialized in the data. This better matches the native DAC behavior: GetDefinesBitField and GetMDStructuresVersion (dacdbiimpl.cpp:7985-8009) check g_pDebugger == NULL but NOT LeftSideInitialized. The DacDbiImpl.cs wrapper at line 64 correctly uses data.IsLeftSideInitialized for the IsLeftSideInitialized DBI method. The dump test at line 93-94 was updated to cross-validate correctly.

✅ Struct Layout Binary Compatibility — Correct

src/coreclr/debug/inc/dbgipcevents.h — Replacing bool m_bHostingInFiber with BYTE padding1 maintains the same 1-byte layout within the DWORD-aligned group (m_checkedBuild + padding1 + padding2 + padding3 = 4 bytes). The transport marshaling in dbgtransportsession.cpp correctly omits the padding field. Both DebuggerIPCControlBlock and DebuggerIPCControlBlockTransport are updated consistently.

✅ Test Changes — Correct and comprehensive

src/native/managed/cdac/tests/DebuggerTests.cs — The removal of memBuilder.AddHeapFragment(debuggerControlFlagsFrag) is correct: BumpAllocator.Allocate auto-registers heap fragments with the builder. The three new tests (MarkDebuggerAttachPending_SetsPendingAttachFlag, MarkDebuggerAttached_SetsAttachedFlag_WhenTrue, MarkDebuggerAttached_ClearsAttachedAndPending_WhenFalse) provide good coverage with distinct initial states. The DacDbi dump tests add coverage for GetAppDomainFromId, EnumerateAppDomains, IsAssemblyFullyTrusted, and GetAppDomainIdFromVmObjectHandle.

✅ AppDomain Management Rearchitecture — Well-structured

src/coreclr/debug/di/process.cpp, rspriv.h — The migration from CordbAppDomain* m_pAppDomain to CordbSafeHashTable<CordbAppDomain> m_appDomains is clean:

• NeuterChildren correctly uses NeuterAndClear instead of manual pointer cleanup
• NeuterChildrenLeftSideResources correctly copies to an auxiliary list before releasing the lock
• CacheAppDomain properly uses AddBaseOrThrow with RSInitHolder RAII
• PrepopulateAppDomainsOrThrow gates on IsDacInitialized()
• ContinueInternal handles the hash table properly with CopyToArray

💡 GetInstantiationFieldInfo Uses AppDomain::GetCurrentDomain()

src/coreclr/debug/daccess/dacdbiimpl.cpp:1817 — This method takes a VMPTR_DomainAssembly parameter but uses AppDomain::GetCurrentDomain() for CollectFields instead of deriving the AppDomain from the parameter. In modern .NET (single AppDomain), this is functionally correct and mirrors how the runtime operates. Noted as a potential follow-up if multi-domain support were ever reconsidered.

💡 Mixed Concerns in PR

The PR bundles debugger attach flag implementation, Assembly→DomainAssembly migration, fiber-mode dead code removal, RequiresAlign8 removal from RuntimeTypeSystem, and ContractRegistry simplification. While these are related to the broader cDAC migration, splitting the dead code removal and RequiresAlign8 changes into separate PRs would make the changes easier to review and bisect.

Generated by Code Review for issue #126794 · ◷

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/coreclr/debug/di/process.cpp:8386

• CordbProcess::UnrecoverableError no longer sets m_unrecoverableError for legal fatal errors (e.g., CORDBG_E_INCOMPATIBLE_PROTOCOL), because the condition changed from effectively "all except CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS" to only !IsLegalFatalError(errorHR). This is a behavior change beyond removing fiber-mode handling and may leave the process in a non-zombie state after an unrecoverable condition. Consider restoring the previous intent (e.g., set m_unrecoverableError for legal fatal errors too, or otherwise document/handle why legal fatal errors should avoid zombie state).

 CONSISTENCY_CHECK_MSGF(IsLegalFatalError(errorHR), ("Unrecoverable internal error: hr=0x%08x!", errorHR));
if (!IsLegalFatalError(errorHR))
{
// This will throw everything into a Zombie state. The ATT_ macros will check this and fail immediately.
m_unrecoverableError = true;
//
// Mark the process as no longer synchronized.
//
Lock();
SetSynchronized(false);
IncStopCount();
Unlock();
}

[rcj1]

/ba-g wasm

[Copilot]

• Implements MarkDebuggerAttachPending and MarkDebuggerAttached in cDAC DacDbiImpl by adding corresponding Debugger_1 contract APIs and wiring them to target-memory writes of debugger control flags.
• Removes fibermode dead code

[Copilot]

Pull request overview

This PR extends the cDAC Debugger contract and the legacy DacDbiImpl surface to support MarkDebuggerAttachPending / MarkDebuggerAttached by writing the appropriate bits into g_CORDebuggerControlFlags, and adds unit test coverage plus documentation updates.

Changes:

• Added IDebugger.MarkDebuggerAttachPending() and IDebugger.MarkDebuggerAttached(bool) APIs and implemented them in Debugger_1 via target-memory writes.
• Updated DacDbiImpl to call the cDAC contract implementations (with DEBUG-only legacy cross-validation).
• Exposed g_CORDebuggerControlFlags via the CoreCLR data descriptor and added tests/docs for the new flag semantics.

Reviewed changes

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

Show a summary per file

File	Description
src/native/managed/cdac/tests/DebuggerTests.cs	Adds test target plumbing + new unit tests validating control-flag writes.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements the two DBI methods by calling the cDAC Debugger contract and translating errors to HRESULTs.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger_1.cs	Adds the flag enum and implements the two new attach APIs by updating CORDebuggerControlFlags.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs	Introduces the new Globals.CORDebuggerControlFlags name constant.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IDebugger.cs	Extends the contract interface with the two new methods.
src/coreclr/vm/datadescriptor/datadescriptor.inc	Exposes g_CORDebuggerControlFlags and adjusts Debugger global gating.
src/coreclr/inc/cordbpriv.h	Adds cDAC dependency annotations to DBCF_PENDING_ATTACH / DBCF_ATTACHED.
docs/design/datacontracts/Debugger.md	Documents the new APIs, global dependency, and flag behavior.

[Copilot]

Pull request overview

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

[dotnet-policy-service]

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

[rcj1]

Test helpers tbd in #126595

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

[github-actions]

🤖 Copilot Code Review — PR #126794

Note

This review was generated by Copilot (Claude Opus 4.6) with additional analysis from Claude Sonnet 4.5.

Holistic Assessment

Motivation: This PR implements debugger attach flag methods (MarkDebuggerAttachPending, MarkDebuggerAttached) in the cDAC debugger contract and DacDbi layer, migrates the debugging infrastructure from VMPTR_Assembly to VMPTR_DomainAssembly to better model AppDomain affinity, and removes dead fiber-mode debugging code. The architectural direction is sound — DomainAssembly is the correct abstraction for ICorDebug's module concept.

Approach: The change systematically threads VMPTR_DomainAssembly and VMPTR_AppDomain through the entire DacDbi interface (IDL, interface headers, DI, DAC, and cDAC implementations). The shift from a single m_pAppDomain pointer to a CordbSafeHashTable<CordbAppDomain> m_appDomains in CordbProcess is the right structural change. The dead code removal (fiber mode, unused enum flags) is clean.

Summary: ⚠️Needs Human Review. The code is architecturally well-structured and the core implementations look correct. However, there are several items that require maintainer attention: incomplete dead code cleanup, a parameter naming inconsistency in the IDL, and the DB_IPCE_EXIT_APP_DOMAIN re-enablement warrants explicit acknowledgment. The protocol breaking change counter question also needs a maintainer decision.

---

Detailed Findings

⚠️ DacDbi Protocol Breaking Change Counter — Not bumped

src/coreclr/debug/inc/dacdbistructures.h:692 — kCurrentDacDbiProtocolBreakingChangeCounter remains at 1 despite extensive breaking changes to IDacDbiInterface: ~30+ method signatures changed (Assembly → DomainAssembly), new methods added (GetAppDomainFromId, EnumerateAppDomains, GetAssemblyFromDomainAssembly, etc.), callback typedefs changed, and struct fields modified.

Per the comment at line 699-710, the counter should be explicitly bumped and documented when breaking changes are introduced. The auto-generated MD5 hash mechanism (described at line 616-618 in process.cpp) will detect the change automatically for remote transport scenarios, so this is unlikely to cause a runtime crash. However, the convention says to bump and document the counter. A maintainer should decide if a bump is needed here or if the hash is sufficient.

⚠️ Incomplete Dead Code Cleanup — CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS

src/coreclr/debug/di/process.cpp:237,8575 — The fiber-mode code was removed (DBCF_FIBERMODE, m_bHostingInFiber, the fiber check in VerifyControlBlock), but two references to CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS remain:

1. Line 237: In IsLegalFatalError() — lists this error as a legal fatal error. Since no code path can produce this error anymore, this entry is dead.
2. Line 8575: if (!IsLegalFatalError(errorHR) || (errorHR != CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS)) — This special-cases the fiber error to skip setting m_unrecoverableError. Since the error is never produced, the special case is also dead.

These should be cleaned up for consistency with the fiber-mode removal. Not blocking, but creates confusing dead code.

⚠️ IDL Parameter Naming Inconsistency — vmAssembly for DomainAssembly type

src/coreclr/inc/dacdbi.idl — The EnumerateModulesInAssembly method has:

HRESULT EnumerateModulesInAssembly([in] VMPTR_DomainAssembly vmAssembly, ...);

The parameter is named vmAssembly but the type is VMPTR_DomainAssembly. This should be vmDomainAssembly for consistency with all other methods in this PR.

⚠️DB_IPCE_EXIT_APP_DOMAIN Re-enabled — Needs documentation

src/coreclr/debug/inc/dbgipceventtypes.h:49 — This event type was previously commented out and is now re-enabled. The handler in process.cpp is well-written (with proper null checks and assertions). This is clearly needed for the new multi-AppDomain hash table support. However, it would be helpful to have a brief comment in the code or commit message explaining why this was re-enabled, since the previous comment suggested it was intentionally disabled.

✅ cDAC Debugger Contract Implementation — Correct

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger_1.cs — The new MarkDebuggerAttachPending() and MarkDebuggerAttached() implementations:

• Flag values (0x0100, 0x0200) match cordbpriv.h constants ✅
• Bitwise operations match the native DAC implementation in dacdbiimpl.cpp:4798-4869 ✅
• MarkDebuggerAttached(false) correctly clears both Attached and PendingAttach flags ✅
• The read-modify-write pattern is not atomic, but this matches the native code which relies on the DD lock for synchronization ✅

✅ TryGetDebuggerData Behavior Change — Intentional and correct

Debugger_1.cs:21-35 — Previously returned false when LeftSideInitialized == 0; now returns true with IsLeftSideInitialized in the data. This better matches the native DAC behavior: GetDefinesBitField and GetMDStructuresVersion (dacdbiimpl.cpp:7985-8009) check g_pDebugger == NULL but NOT LeftSideInitialized. The DacDbiImpl.cs wrapper at line 64 correctly uses data.IsLeftSideInitialized for the IsLeftSideInitialized DBI method. The dump test at line 93-94 was updated to cross-validate correctly.

✅ Struct Layout Binary Compatibility — Correct

src/coreclr/debug/inc/dbgipcevents.h — Replacing bool m_bHostingInFiber with BYTE padding1 maintains the same 1-byte layout within the DWORD-aligned group (m_checkedBuild + padding1 + padding2 + padding3 = 4 bytes). The transport marshaling in dbgtransportsession.cpp correctly omits the padding field. Both DebuggerIPCControlBlock and DebuggerIPCControlBlockTransport are updated consistently.

✅ Test Changes — Correct and comprehensive

src/native/managed/cdac/tests/DebuggerTests.cs — The removal of memBuilder.AddHeapFragment(debuggerControlFlagsFrag) is correct: BumpAllocator.Allocate auto-registers heap fragments with the builder. The three new tests (MarkDebuggerAttachPending_SetsPendingAttachFlag, MarkDebuggerAttached_SetsAttachedFlag_WhenTrue, MarkDebuggerAttached_ClearsAttachedAndPending_WhenFalse) provide good coverage with distinct initial states. The DacDbi dump tests add coverage for GetAppDomainFromId, EnumerateAppDomains, IsAssemblyFullyTrusted, and GetAppDomainIdFromVmObjectHandle.

✅ AppDomain Management Rearchitecture — Well-structured

src/coreclr/debug/di/process.cpp, rspriv.h — The migration from CordbAppDomain* m_pAppDomain to CordbSafeHashTable<CordbAppDomain> m_appDomains is clean:

• NeuterChildren correctly uses NeuterAndClear instead of manual pointer cleanup
• NeuterChildrenLeftSideResources correctly copies to an auxiliary list before releasing the lock
• CacheAppDomain properly uses AddBaseOrThrow with RSInitHolder RAII
• PrepopulateAppDomainsOrThrow gates on IsDacInitialized()
• ContinueInternal handles the hash table properly with CopyToArray

💡 GetInstantiationFieldInfo Uses AppDomain::GetCurrentDomain()

src/coreclr/debug/daccess/dacdbiimpl.cpp:1817 — This method takes a VMPTR_DomainAssembly parameter but uses AppDomain::GetCurrentDomain() for CollectFields instead of deriving the AppDomain from the parameter. In modern .NET (single AppDomain), this is functionally correct and mirrors how the runtime operates. Noted as a potential follow-up if multi-domain support were ever reconsidered.

💡 Mixed Concerns in PR

The PR bundles debugger attach flag implementation, Assembly→DomainAssembly migration, fiber-mode dead code removal, RequiresAlign8 removal from RuntimeTypeSystem, and ContractRegistry simplification. While these are related to the broader cDAC migration, splitting the dead code removal and RequiresAlign8 changes into separate PRs would make the changes easier to review and bisect.

Generated by Code Review for issue #126794 · ◷

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/coreclr/debug/di/process.cpp:8386

• CordbProcess::UnrecoverableError no longer sets m_unrecoverableError for legal fatal errors (e.g., CORDBG_E_INCOMPATIBLE_PROTOCOL), because the condition changed from effectively "all except CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS" to only !IsLegalFatalError(errorHR). This is a behavior change beyond removing fiber-mode handling and may leave the process in a non-zombie state after an unrecoverable condition. Consider restoring the previous intent (e.g., set m_unrecoverableError for legal fatal errors too, or otherwise document/handle why legal fatal errors should avoid zombie state).

 CONSISTENCY_CHECK_MSGF(IsLegalFatalError(errorHR), ("Unrecoverable internal error: hr=0x%08x!", errorHR));
if (!IsLegalFatalError(errorHR))
{
// This will throw everything into a Zombie state. The ATT_ macros will check this and fail immediately.
m_unrecoverableError = true;
//
// Mark the process as no longer synchronized.
//
Lock();
SetSynchronized(false);
IncStopCount();
Unlock();
}

[rcj1]

/ba-g wasm

[Copilot]

• Implements MarkDebuggerAttachPending and MarkDebuggerAttached in cDAC DacDbiImpl by adding corresponding Debugger_1 contract APIs and wiring them to target-memory writes of debugger control flags.
• Removes fibermode dead code

[Copilot]

Pull request overview

This PR extends the cDAC Debugger contract and the legacy DacDbiImpl surface to support MarkDebuggerAttachPending / MarkDebuggerAttached by writing the appropriate bits into g_CORDebuggerControlFlags, and adds unit test coverage plus documentation updates.

Changes:

• Added IDebugger.MarkDebuggerAttachPending() and IDebugger.MarkDebuggerAttached(bool) APIs and implemented them in Debugger_1 via target-memory writes.
• Updated DacDbiImpl to call the cDAC contract implementations (with DEBUG-only legacy cross-validation).
• Exposed g_CORDebuggerControlFlags via the CoreCLR data descriptor and added tests/docs for the new flag semantics.

Reviewed changes

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

Show a summary per file

File	Description
src/native/managed/cdac/tests/DebuggerTests.cs	Adds test target plumbing + new unit tests validating control-flag writes.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements the two DBI methods by calling the cDAC Debugger contract and translating errors to HRESULTs.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger_1.cs	Adds the flag enum and implements the two new attach APIs by updating CORDebuggerControlFlags.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs	Introduces the new Globals.CORDebuggerControlFlags name constant.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IDebugger.cs	Extends the contract interface with the two new methods.
src/coreclr/vm/datadescriptor/datadescriptor.inc	Exposes g_CORDebuggerControlFlags and adjusts Debugger global gating.
src/coreclr/inc/cordbpriv.h	Adds cDAC dependency annotations to DBCF_PENDING_ATTACH / DBCF_ATTACHED.
docs/design/datacontracts/Debugger.md	Documents the new APIs, global dependency, and flag behavior.

[Copilot]

Pull request overview

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

[dotnet-policy-service]

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

[rcj1]

Test helpers tbd in #126595

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

[github-actions]

🤖 Copilot Code Review — PR #126794

Note

This review was generated by Copilot (Claude Opus 4.6) with additional analysis from Claude Sonnet 4.5.

Holistic Assessment

Motivation: This PR implements debugger attach flag methods (MarkDebuggerAttachPending, MarkDebuggerAttached) in the cDAC debugger contract and DacDbi layer, migrates the debugging infrastructure from VMPTR_Assembly to VMPTR_DomainAssembly to better model AppDomain affinity, and removes dead fiber-mode debugging code. The architectural direction is sound — DomainAssembly is the correct abstraction for ICorDebug's module concept.

Approach: The change systematically threads VMPTR_DomainAssembly and VMPTR_AppDomain through the entire DacDbi interface (IDL, interface headers, DI, DAC, and cDAC implementations). The shift from a single m_pAppDomain pointer to a CordbSafeHashTable<CordbAppDomain> m_appDomains in CordbProcess is the right structural change. The dead code removal (fiber mode, unused enum flags) is clean.

Summary: ⚠️Needs Human Review. The code is architecturally well-structured and the core implementations look correct. However, there are several items that require maintainer attention: incomplete dead code cleanup, a parameter naming inconsistency in the IDL, and the DB_IPCE_EXIT_APP_DOMAIN re-enablement warrants explicit acknowledgment. The protocol breaking change counter question also needs a maintainer decision.

---

Detailed Findings

⚠️ DacDbi Protocol Breaking Change Counter — Not bumped

src/coreclr/debug/inc/dacdbistructures.h:692 — kCurrentDacDbiProtocolBreakingChangeCounter remains at 1 despite extensive breaking changes to IDacDbiInterface: ~30+ method signatures changed (Assembly → DomainAssembly), new methods added (GetAppDomainFromId, EnumerateAppDomains, GetAssemblyFromDomainAssembly, etc.), callback typedefs changed, and struct fields modified.

Per the comment at line 699-710, the counter should be explicitly bumped and documented when breaking changes are introduced. The auto-generated MD5 hash mechanism (described at line 616-618 in process.cpp) will detect the change automatically for remote transport scenarios, so this is unlikely to cause a runtime crash. However, the convention says to bump and document the counter. A maintainer should decide if a bump is needed here or if the hash is sufficient.

⚠️ Incomplete Dead Code Cleanup — CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS

src/coreclr/debug/di/process.cpp:237,8575 — The fiber-mode code was removed (DBCF_FIBERMODE, m_bHostingInFiber, the fiber check in VerifyControlBlock), but two references to CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS remain:

1. Line 237: In IsLegalFatalError() — lists this error as a legal fatal error. Since no code path can produce this error anymore, this entry is dead.
2. Line 8575: if (!IsLegalFatalError(errorHR) || (errorHR != CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS)) — This special-cases the fiber error to skip setting m_unrecoverableError. Since the error is never produced, the special case is also dead.

These should be cleaned up for consistency with the fiber-mode removal. Not blocking, but creates confusing dead code.

⚠️ IDL Parameter Naming Inconsistency — vmAssembly for DomainAssembly type

src/coreclr/inc/dacdbi.idl — The EnumerateModulesInAssembly method has:

HRESULT EnumerateModulesInAssembly([in] VMPTR_DomainAssembly vmAssembly, ...);

The parameter is named vmAssembly but the type is VMPTR_DomainAssembly. This should be vmDomainAssembly for consistency with all other methods in this PR.

⚠️DB_IPCE_EXIT_APP_DOMAIN Re-enabled — Needs documentation

src/coreclr/debug/inc/dbgipceventtypes.h:49 — This event type was previously commented out and is now re-enabled. The handler in process.cpp is well-written (with proper null checks and assertions). This is clearly needed for the new multi-AppDomain hash table support. However, it would be helpful to have a brief comment in the code or commit message explaining why this was re-enabled, since the previous comment suggested it was intentionally disabled.

✅ cDAC Debugger Contract Implementation — Correct

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger_1.cs — The new MarkDebuggerAttachPending() and MarkDebuggerAttached() implementations:

• Flag values (0x0100, 0x0200) match cordbpriv.h constants ✅
• Bitwise operations match the native DAC implementation in dacdbiimpl.cpp:4798-4869 ✅
• MarkDebuggerAttached(false) correctly clears both Attached and PendingAttach flags ✅
• The read-modify-write pattern is not atomic, but this matches the native code which relies on the DD lock for synchronization ✅

✅ TryGetDebuggerData Behavior Change — Intentional and correct

Debugger_1.cs:21-35 — Previously returned false when LeftSideInitialized == 0; now returns true with IsLeftSideInitialized in the data. This better matches the native DAC behavior: GetDefinesBitField and GetMDStructuresVersion (dacdbiimpl.cpp:7985-8009) check g_pDebugger == NULL but NOT LeftSideInitialized. The DacDbiImpl.cs wrapper at line 64 correctly uses data.IsLeftSideInitialized for the IsLeftSideInitialized DBI method. The dump test at line 93-94 was updated to cross-validate correctly.

✅ Struct Layout Binary Compatibility — Correct

src/coreclr/debug/inc/dbgipcevents.h — Replacing bool m_bHostingInFiber with BYTE padding1 maintains the same 1-byte layout within the DWORD-aligned group (m_checkedBuild + padding1 + padding2 + padding3 = 4 bytes). The transport marshaling in dbgtransportsession.cpp correctly omits the padding field. Both DebuggerIPCControlBlock and DebuggerIPCControlBlockTransport are updated consistently.

✅ Test Changes — Correct and comprehensive

src/native/managed/cdac/tests/DebuggerTests.cs — The removal of memBuilder.AddHeapFragment(debuggerControlFlagsFrag) is correct: BumpAllocator.Allocate auto-registers heap fragments with the builder. The three new tests (MarkDebuggerAttachPending_SetsPendingAttachFlag, MarkDebuggerAttached_SetsAttachedFlag_WhenTrue, MarkDebuggerAttached_ClearsAttachedAndPending_WhenFalse) provide good coverage with distinct initial states. The DacDbi dump tests add coverage for GetAppDomainFromId, EnumerateAppDomains, IsAssemblyFullyTrusted, and GetAppDomainIdFromVmObjectHandle.

✅ AppDomain Management Rearchitecture — Well-structured

src/coreclr/debug/di/process.cpp, rspriv.h — The migration from CordbAppDomain* m_pAppDomain to CordbSafeHashTable<CordbAppDomain> m_appDomains is clean:

• NeuterChildren correctly uses NeuterAndClear instead of manual pointer cleanup
• NeuterChildrenLeftSideResources correctly copies to an auxiliary list before releasing the lock
• CacheAppDomain properly uses AddBaseOrThrow with RSInitHolder RAII
• PrepopulateAppDomainsOrThrow gates on IsDacInitialized()
• ContinueInternal handles the hash table properly with CopyToArray

💡 GetInstantiationFieldInfo Uses AppDomain::GetCurrentDomain()

src/coreclr/debug/daccess/dacdbiimpl.cpp:1817 — This method takes a VMPTR_DomainAssembly parameter but uses AppDomain::GetCurrentDomain() for CollectFields instead of deriving the AppDomain from the parameter. In modern .NET (single AppDomain), this is functionally correct and mirrors how the runtime operates. Noted as a potential follow-up if multi-domain support were ever reconsidered.

💡 Mixed Concerns in PR

The PR bundles debugger attach flag implementation, Assembly→DomainAssembly migration, fiber-mode dead code removal, RequiresAlign8 removal from RuntimeTypeSystem, and ContractRegistry simplification. While these are related to the broader cDAC migration, splitting the dead code removal and RequiresAlign8 changes into separate PRs would make the changes easier to review and bisect.

Generated by Code Review for issue #126794 · ◷

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/coreclr/debug/di/process.cpp:8386

• CordbProcess::UnrecoverableError no longer sets m_unrecoverableError for legal fatal errors (e.g., CORDBG_E_INCOMPATIBLE_PROTOCOL), because the condition changed from effectively "all except CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS" to only !IsLegalFatalError(errorHR). This is a behavior change beyond removing fiber-mode handling and may leave the process in a non-zombie state after an unrecoverable condition. Consider restoring the previous intent (e.g., set m_unrecoverableError for legal fatal errors too, or otherwise document/handle why legal fatal errors should avoid zombie state).

 CONSISTENCY_CHECK_MSGF(IsLegalFatalError(errorHR), ("Unrecoverable internal error: hr=0x%08x!", errorHR));
if (!IsLegalFatalError(errorHR))
{
// This will throw everything into a Zombie state. The ATT_ macros will check this and fail immediately.
m_unrecoverableError = true;
//
// Mark the process as no longer synchronized.
//
Lock();
SetSynchronized(false);
IncStopCount();
Unlock();
}

[rcj1]

/ba-g wasm

[Copilot]

• Implements MarkDebuggerAttachPending and MarkDebuggerAttached in cDAC DacDbiImpl by adding corresponding Debugger_1 contract APIs and wiring them to target-memory writes of debugger control flags.
• Removes fibermode dead code

[Copilot]

Pull request overview

This PR extends the cDAC Debugger contract and the legacy DacDbiImpl surface to support MarkDebuggerAttachPending / MarkDebuggerAttached by writing the appropriate bits into g_CORDebuggerControlFlags, and adds unit test coverage plus documentation updates.

Changes:

• Added IDebugger.MarkDebuggerAttachPending() and IDebugger.MarkDebuggerAttached(bool) APIs and implemented them in Debugger_1 via target-memory writes.
• Updated DacDbiImpl to call the cDAC contract implementations (with DEBUG-only legacy cross-validation).
• Exposed g_CORDebuggerControlFlags via the CoreCLR data descriptor and added tests/docs for the new flag semantics.

Reviewed changes

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

Show a summary per file

File	Description
src/native/managed/cdac/tests/DebuggerTests.cs	Adds test target plumbing + new unit tests validating control-flag writes.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements the two DBI methods by calling the cDAC Debugger contract and translating errors to HRESULTs.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger_1.cs	Adds the flag enum and implements the two new attach APIs by updating CORDebuggerControlFlags.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs	Introduces the new Globals.CORDebuggerControlFlags name constant.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IDebugger.cs	Extends the contract interface with the two new methods.
src/coreclr/vm/datadescriptor/datadescriptor.inc	Exposes g_CORDebuggerControlFlags and adjusts Debugger global gating.
src/coreclr/inc/cordbpriv.h	Adds cDAC dependency annotations to DBCF_PENDING_ATTACH / DBCF_ATTACHED.
docs/design/datacontracts/Debugger.md	Documents the new APIs, global dependency, and flag behavior.

[Copilot]

Pull request overview

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

[dotnet-policy-service]

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

[rcj1]

Test helpers tbd in #126595

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

[github-actions]

🤖 Copilot Code Review — PR #126794

Note

This review was generated by Copilot (Claude Opus 4.6) with additional analysis from Claude Sonnet 4.5.

Holistic Assessment

Motivation: This PR implements debugger attach flag methods (MarkDebuggerAttachPending, MarkDebuggerAttached) in the cDAC debugger contract and DacDbi layer, migrates the debugging infrastructure from VMPTR_Assembly to VMPTR_DomainAssembly to better model AppDomain affinity, and removes dead fiber-mode debugging code. The architectural direction is sound — DomainAssembly is the correct abstraction for ICorDebug's module concept.

Approach: The change systematically threads VMPTR_DomainAssembly and VMPTR_AppDomain through the entire DacDbi interface (IDL, interface headers, DI, DAC, and cDAC implementations). The shift from a single m_pAppDomain pointer to a CordbSafeHashTable<CordbAppDomain> m_appDomains in CordbProcess is the right structural change. The dead code removal (fiber mode, unused enum flags) is clean.

Summary: ⚠️Needs Human Review. The code is architecturally well-structured and the core implementations look correct. However, there are several items that require maintainer attention: incomplete dead code cleanup, a parameter naming inconsistency in the IDL, and the DB_IPCE_EXIT_APP_DOMAIN re-enablement warrants explicit acknowledgment. The protocol breaking change counter question also needs a maintainer decision.

---

Detailed Findings

⚠️ DacDbi Protocol Breaking Change Counter — Not bumped

src/coreclr/debug/inc/dacdbistructures.h:692 — kCurrentDacDbiProtocolBreakingChangeCounter remains at 1 despite extensive breaking changes to IDacDbiInterface: ~30+ method signatures changed (Assembly → DomainAssembly), new methods added (GetAppDomainFromId, EnumerateAppDomains, GetAssemblyFromDomainAssembly, etc.), callback typedefs changed, and struct fields modified.

Per the comment at line 699-710, the counter should be explicitly bumped and documented when breaking changes are introduced. The auto-generated MD5 hash mechanism (described at line 616-618 in process.cpp) will detect the change automatically for remote transport scenarios, so this is unlikely to cause a runtime crash. However, the convention says to bump and document the counter. A maintainer should decide if a bump is needed here or if the hash is sufficient.

⚠️ Incomplete Dead Code Cleanup — CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS

src/coreclr/debug/di/process.cpp:237,8575 — The fiber-mode code was removed (DBCF_FIBERMODE, m_bHostingInFiber, the fiber check in VerifyControlBlock), but two references to CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS remain:

1. Line 237: In IsLegalFatalError() — lists this error as a legal fatal error. Since no code path can produce this error anymore, this entry is dead.
2. Line 8575: if (!IsLegalFatalError(errorHR) || (errorHR != CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS)) — This special-cases the fiber error to skip setting m_unrecoverableError. Since the error is never produced, the special case is also dead.

These should be cleaned up for consistency with the fiber-mode removal. Not blocking, but creates confusing dead code.

⚠️ IDL Parameter Naming Inconsistency — vmAssembly for DomainAssembly type

src/coreclr/inc/dacdbi.idl — The EnumerateModulesInAssembly method has:

HRESULT EnumerateModulesInAssembly([in] VMPTR_DomainAssembly vmAssembly, ...);

The parameter is named vmAssembly but the type is VMPTR_DomainAssembly. This should be vmDomainAssembly for consistency with all other methods in this PR.

⚠️DB_IPCE_EXIT_APP_DOMAIN Re-enabled — Needs documentation

src/coreclr/debug/inc/dbgipceventtypes.h:49 — This event type was previously commented out and is now re-enabled. The handler in process.cpp is well-written (with proper null checks and assertions). This is clearly needed for the new multi-AppDomain hash table support. However, it would be helpful to have a brief comment in the code or commit message explaining why this was re-enabled, since the previous comment suggested it was intentionally disabled.

✅ cDAC Debugger Contract Implementation — Correct

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger_1.cs — The new MarkDebuggerAttachPending() and MarkDebuggerAttached() implementations:

• Flag values (0x0100, 0x0200) match cordbpriv.h constants ✅
• Bitwise operations match the native DAC implementation in dacdbiimpl.cpp:4798-4869 ✅
• MarkDebuggerAttached(false) correctly clears both Attached and PendingAttach flags ✅
• The read-modify-write pattern is not atomic, but this matches the native code which relies on the DD lock for synchronization ✅

✅ TryGetDebuggerData Behavior Change — Intentional and correct

Debugger_1.cs:21-35 — Previously returned false when LeftSideInitialized == 0; now returns true with IsLeftSideInitialized in the data. This better matches the native DAC behavior: GetDefinesBitField and GetMDStructuresVersion (dacdbiimpl.cpp:7985-8009) check g_pDebugger == NULL but NOT LeftSideInitialized. The DacDbiImpl.cs wrapper at line 64 correctly uses data.IsLeftSideInitialized for the IsLeftSideInitialized DBI method. The dump test at line 93-94 was updated to cross-validate correctly.

✅ Struct Layout Binary Compatibility — Correct

src/coreclr/debug/inc/dbgipcevents.h — Replacing bool m_bHostingInFiber with BYTE padding1 maintains the same 1-byte layout within the DWORD-aligned group (m_checkedBuild + padding1 + padding2 + padding3 = 4 bytes). The transport marshaling in dbgtransportsession.cpp correctly omits the padding field. Both DebuggerIPCControlBlock and DebuggerIPCControlBlockTransport are updated consistently.

✅ Test Changes — Correct and comprehensive

src/native/managed/cdac/tests/DebuggerTests.cs — The removal of memBuilder.AddHeapFragment(debuggerControlFlagsFrag) is correct: BumpAllocator.Allocate auto-registers heap fragments with the builder. The three new tests (MarkDebuggerAttachPending_SetsPendingAttachFlag, MarkDebuggerAttached_SetsAttachedFlag_WhenTrue, MarkDebuggerAttached_ClearsAttachedAndPending_WhenFalse) provide good coverage with distinct initial states. The DacDbi dump tests add coverage for GetAppDomainFromId, EnumerateAppDomains, IsAssemblyFullyTrusted, and GetAppDomainIdFromVmObjectHandle.

✅ AppDomain Management Rearchitecture — Well-structured

src/coreclr/debug/di/process.cpp, rspriv.h — The migration from CordbAppDomain* m_pAppDomain to CordbSafeHashTable<CordbAppDomain> m_appDomains is clean:

• NeuterChildren correctly uses NeuterAndClear instead of manual pointer cleanup
• NeuterChildrenLeftSideResources correctly copies to an auxiliary list before releasing the lock
• CacheAppDomain properly uses AddBaseOrThrow with RSInitHolder RAII
• PrepopulateAppDomainsOrThrow gates on IsDacInitialized()
• ContinueInternal handles the hash table properly with CopyToArray

💡 GetInstantiationFieldInfo Uses AppDomain::GetCurrentDomain()

src/coreclr/debug/daccess/dacdbiimpl.cpp:1817 — This method takes a VMPTR_DomainAssembly parameter but uses AppDomain::GetCurrentDomain() for CollectFields instead of deriving the AppDomain from the parameter. In modern .NET (single AppDomain), this is functionally correct and mirrors how the runtime operates. Noted as a potential follow-up if multi-domain support were ever reconsidered.

💡 Mixed Concerns in PR

The PR bundles debugger attach flag implementation, Assembly→DomainAssembly migration, fiber-mode dead code removal, RequiresAlign8 removal from RuntimeTypeSystem, and ContractRegistry simplification. While these are related to the broader cDAC migration, splitting the dead code removal and RequiresAlign8 changes into separate PRs would make the changes easier to review and bisect.

Generated by Code Review for issue #126794 · ◷

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/coreclr/debug/di/process.cpp:8386

• CordbProcess::UnrecoverableError no longer sets m_unrecoverableError for legal fatal errors (e.g., CORDBG_E_INCOMPATIBLE_PROTOCOL), because the condition changed from effectively "all except CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS" to only !IsLegalFatalError(errorHR). This is a behavior change beyond removing fiber-mode handling and may leave the process in a non-zombie state after an unrecoverable condition. Consider restoring the previous intent (e.g., set m_unrecoverableError for legal fatal errors too, or otherwise document/handle why legal fatal errors should avoid zombie state).

 CONSISTENCY_CHECK_MSGF(IsLegalFatalError(errorHR), ("Unrecoverable internal error: hr=0x%08x!", errorHR));
if (!IsLegalFatalError(errorHR))
{
// This will throw everything into a Zombie state. The ATT_ macros will check this and fail immediately.
m_unrecoverableError = true;
//
// Mark the process as no longer synchronized.
//
Lock();
SetSynchronized(false);
IncStopCount();
Unlock();
}

[rcj1]

/ba-g wasm

[Copilot]

• Implements MarkDebuggerAttachPending and MarkDebuggerAttached in cDAC DacDbiImpl by adding corresponding Debugger_1 contract APIs and wiring them to target-memory writes of debugger control flags.
• Removes fibermode dead code

[Copilot]

Pull request overview

This PR extends the cDAC Debugger contract and the legacy DacDbiImpl surface to support MarkDebuggerAttachPending / MarkDebuggerAttached by writing the appropriate bits into g_CORDebuggerControlFlags, and adds unit test coverage plus documentation updates.

Changes:

• Added IDebugger.MarkDebuggerAttachPending() and IDebugger.MarkDebuggerAttached(bool) APIs and implemented them in Debugger_1 via target-memory writes.
• Updated DacDbiImpl to call the cDAC contract implementations (with DEBUG-only legacy cross-validation).
• Exposed g_CORDebuggerControlFlags via the CoreCLR data descriptor and added tests/docs for the new flag semantics.

Reviewed changes

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

Show a summary per file

File	Description
src/native/managed/cdac/tests/DebuggerTests.cs	Adds test target plumbing + new unit tests validating control-flag writes.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements the two DBI methods by calling the cDAC Debugger contract and translating errors to HRESULTs.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger_1.cs	Adds the flag enum and implements the two new attach APIs by updating CORDebuggerControlFlags.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs	Introduces the new Globals.CORDebuggerControlFlags name constant.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IDebugger.cs	Extends the contract interface with the two new methods.
src/coreclr/vm/datadescriptor/datadescriptor.inc	Exposes g_CORDebuggerControlFlags and adjusts Debugger global gating.
src/coreclr/inc/cordbpriv.h	Adds cDAC dependency annotations to DBCF_PENDING_ATTACH / DBCF_ATTACHED.
docs/design/datacontracts/Debugger.md	Documents the new APIs, global dependency, and flag behavior.

[Copilot]

Pull request overview

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

[dotnet-policy-service]

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

[rcj1]

Test helpers tbd in #126595

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

[github-actions]

🤖 Copilot Code Review — PR #126794

Note

This review was generated by Copilot (Claude Opus 4.6) with additional analysis from Claude Sonnet 4.5.

Holistic Assessment

Motivation: This PR implements debugger attach flag methods (MarkDebuggerAttachPending, MarkDebuggerAttached) in the cDAC debugger contract and DacDbi layer, migrates the debugging infrastructure from VMPTR_Assembly to VMPTR_DomainAssembly to better model AppDomain affinity, and removes dead fiber-mode debugging code. The architectural direction is sound — DomainAssembly is the correct abstraction for ICorDebug's module concept.

Approach: The change systematically threads VMPTR_DomainAssembly and VMPTR_AppDomain through the entire DacDbi interface (IDL, interface headers, DI, DAC, and cDAC implementations). The shift from a single m_pAppDomain pointer to a CordbSafeHashTable<CordbAppDomain> m_appDomains in CordbProcess is the right structural change. The dead code removal (fiber mode, unused enum flags) is clean.

Summary: ⚠️Needs Human Review. The code is architecturally well-structured and the core implementations look correct. However, there are several items that require maintainer attention: incomplete dead code cleanup, a parameter naming inconsistency in the IDL, and the DB_IPCE_EXIT_APP_DOMAIN re-enablement warrants explicit acknowledgment. The protocol breaking change counter question also needs a maintainer decision.

---

Detailed Findings

⚠️ DacDbi Protocol Breaking Change Counter — Not bumped

src/coreclr/debug/inc/dacdbistructures.h:692 — kCurrentDacDbiProtocolBreakingChangeCounter remains at 1 despite extensive breaking changes to IDacDbiInterface: ~30+ method signatures changed (Assembly → DomainAssembly), new methods added (GetAppDomainFromId, EnumerateAppDomains, GetAssemblyFromDomainAssembly, etc.), callback typedefs changed, and struct fields modified.

Per the comment at line 699-710, the counter should be explicitly bumped and documented when breaking changes are introduced. The auto-generated MD5 hash mechanism (described at line 616-618 in process.cpp) will detect the change automatically for remote transport scenarios, so this is unlikely to cause a runtime crash. However, the convention says to bump and document the counter. A maintainer should decide if a bump is needed here or if the hash is sufficient.

⚠️ Incomplete Dead Code Cleanup — CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS

src/coreclr/debug/di/process.cpp:237,8575 — The fiber-mode code was removed (DBCF_FIBERMODE, m_bHostingInFiber, the fiber check in VerifyControlBlock), but two references to CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS remain:

1. Line 237: In IsLegalFatalError() — lists this error as a legal fatal error. Since no code path can produce this error anymore, this entry is dead.
2. Line 8575: if (!IsLegalFatalError(errorHR) || (errorHR != CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS)) — This special-cases the fiber error to skip setting m_unrecoverableError. Since the error is never produced, the special case is also dead.

These should be cleaned up for consistency with the fiber-mode removal. Not blocking, but creates confusing dead code.

⚠️ IDL Parameter Naming Inconsistency — vmAssembly for DomainAssembly type

src/coreclr/inc/dacdbi.idl — The EnumerateModulesInAssembly method has:

HRESULT EnumerateModulesInAssembly([in] VMPTR_DomainAssembly vmAssembly, ...);

The parameter is named vmAssembly but the type is VMPTR_DomainAssembly. This should be vmDomainAssembly for consistency with all other methods in this PR.

⚠️DB_IPCE_EXIT_APP_DOMAIN Re-enabled — Needs documentation

src/coreclr/debug/inc/dbgipceventtypes.h:49 — This event type was previously commented out and is now re-enabled. The handler in process.cpp is well-written (with proper null checks and assertions). This is clearly needed for the new multi-AppDomain hash table support. However, it would be helpful to have a brief comment in the code or commit message explaining why this was re-enabled, since the previous comment suggested it was intentionally disabled.

✅ cDAC Debugger Contract Implementation — Correct

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger_1.cs — The new MarkDebuggerAttachPending() and MarkDebuggerAttached() implementations:

• Flag values (0x0100, 0x0200) match cordbpriv.h constants ✅
• Bitwise operations match the native DAC implementation in dacdbiimpl.cpp:4798-4869 ✅
• MarkDebuggerAttached(false) correctly clears both Attached and PendingAttach flags ✅
• The read-modify-write pattern is not atomic, but this matches the native code which relies on the DD lock for synchronization ✅

✅ TryGetDebuggerData Behavior Change — Intentional and correct

Debugger_1.cs:21-35 — Previously returned false when LeftSideInitialized == 0; now returns true with IsLeftSideInitialized in the data. This better matches the native DAC behavior: GetDefinesBitField and GetMDStructuresVersion (dacdbiimpl.cpp:7985-8009) check g_pDebugger == NULL but NOT LeftSideInitialized. The DacDbiImpl.cs wrapper at line 64 correctly uses data.IsLeftSideInitialized for the IsLeftSideInitialized DBI method. The dump test at line 93-94 was updated to cross-validate correctly.

✅ Struct Layout Binary Compatibility — Correct

src/coreclr/debug/inc/dbgipcevents.h — Replacing bool m_bHostingInFiber with BYTE padding1 maintains the same 1-byte layout within the DWORD-aligned group (m_checkedBuild + padding1 + padding2 + padding3 = 4 bytes). The transport marshaling in dbgtransportsession.cpp correctly omits the padding field. Both DebuggerIPCControlBlock and DebuggerIPCControlBlockTransport are updated consistently.

✅ Test Changes — Correct and comprehensive

src/native/managed/cdac/tests/DebuggerTests.cs — The removal of memBuilder.AddHeapFragment(debuggerControlFlagsFrag) is correct: BumpAllocator.Allocate auto-registers heap fragments with the builder. The three new tests (MarkDebuggerAttachPending_SetsPendingAttachFlag, MarkDebuggerAttached_SetsAttachedFlag_WhenTrue, MarkDebuggerAttached_ClearsAttachedAndPending_WhenFalse) provide good coverage with distinct initial states. The DacDbi dump tests add coverage for GetAppDomainFromId, EnumerateAppDomains, IsAssemblyFullyTrusted, and GetAppDomainIdFromVmObjectHandle.

✅ AppDomain Management Rearchitecture — Well-structured

src/coreclr/debug/di/process.cpp, rspriv.h — The migration from CordbAppDomain* m_pAppDomain to CordbSafeHashTable<CordbAppDomain> m_appDomains is clean:

• NeuterChildren correctly uses NeuterAndClear instead of manual pointer cleanup
• NeuterChildrenLeftSideResources correctly copies to an auxiliary list before releasing the lock
• CacheAppDomain properly uses AddBaseOrThrow with RSInitHolder RAII
• PrepopulateAppDomainsOrThrow gates on IsDacInitialized()
• ContinueInternal handles the hash table properly with CopyToArray

💡 GetInstantiationFieldInfo Uses AppDomain::GetCurrentDomain()

src/coreclr/debug/daccess/dacdbiimpl.cpp:1817 — This method takes a VMPTR_DomainAssembly parameter but uses AppDomain::GetCurrentDomain() for CollectFields instead of deriving the AppDomain from the parameter. In modern .NET (single AppDomain), this is functionally correct and mirrors how the runtime operates. Noted as a potential follow-up if multi-domain support were ever reconsidered.

💡 Mixed Concerns in PR

The PR bundles debugger attach flag implementation, Assembly→DomainAssembly migration, fiber-mode dead code removal, RequiresAlign8 removal from RuntimeTypeSystem, and ContractRegistry simplification. While these are related to the broader cDAC migration, splitting the dead code removal and RequiresAlign8 changes into separate PRs would make the changes easier to review and bisect.

Generated by Code Review for issue #126794 · ◷

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/coreclr/debug/di/process.cpp:8386

• CordbProcess::UnrecoverableError no longer sets m_unrecoverableError for legal fatal errors (e.g., CORDBG_E_INCOMPATIBLE_PROTOCOL), because the condition changed from effectively "all except CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS" to only !IsLegalFatalError(errorHR). This is a behavior change beyond removing fiber-mode handling and may leave the process in a non-zombie state after an unrecoverable condition. Consider restoring the previous intent (e.g., set m_unrecoverableError for legal fatal errors too, or otherwise document/handle why legal fatal errors should avoid zombie state).

 CONSISTENCY_CHECK_MSGF(IsLegalFatalError(errorHR), ("Unrecoverable internal error: hr=0x%08x!", errorHR));
if (!IsLegalFatalError(errorHR))
{
// This will throw everything into a Zombie state. The ATT_ macros will check this and fail immediately.
m_unrecoverableError = true;
//
// Mark the process as no longer synchronized.
//
Lock();
SetSynchronized(false);
IncStopCount();
Unlock();
}

[rcj1]

/ba-g wasm

[Copilot]

• Implements MarkDebuggerAttachPending and MarkDebuggerAttached in cDAC DacDbiImpl by adding corresponding Debugger_1 contract APIs and wiring them to target-memory writes of debugger control flags.
• Removes fibermode dead code

[Copilot]

Pull request overview

This PR extends the cDAC Debugger contract and the legacy DacDbiImpl surface to support MarkDebuggerAttachPending / MarkDebuggerAttached by writing the appropriate bits into g_CORDebuggerControlFlags, and adds unit test coverage plus documentation updates.

Changes:

• Added IDebugger.MarkDebuggerAttachPending() and IDebugger.MarkDebuggerAttached(bool) APIs and implemented them in Debugger_1 via target-memory writes.
• Updated DacDbiImpl to call the cDAC contract implementations (with DEBUG-only legacy cross-validation).
• Exposed g_CORDebuggerControlFlags via the CoreCLR data descriptor and added tests/docs for the new flag semantics.

Reviewed changes

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

Show a summary per file

File	Description
src/native/managed/cdac/tests/DebuggerTests.cs	Adds test target plumbing + new unit tests validating control-flag writes.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements the two DBI methods by calling the cDAC Debugger contract and translating errors to HRESULTs.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger_1.cs	Adds the flag enum and implements the two new attach APIs by updating CORDebuggerControlFlags.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs	Introduces the new Globals.CORDebuggerControlFlags name constant.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IDebugger.cs	Extends the contract interface with the two new methods.
src/coreclr/vm/datadescriptor/datadescriptor.inc	Exposes g_CORDebuggerControlFlags and adjusts Debugger global gating.
src/coreclr/inc/cordbpriv.h	Adds cDAC dependency annotations to DBCF_PENDING_ATTACH / DBCF_ATTACHED.
docs/design/datacontracts/Debugger.md	Documents the new APIs, global dependency, and flag behavior.

[Copilot]

Pull request overview

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

[dotnet-policy-service]

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

[rcj1]

Test helpers tbd in #126595

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

[github-actions]

🤖 Copilot Code Review — PR #126794

Note

This review was generated by Copilot (Claude Opus 4.6) with additional analysis from Claude Sonnet 4.5.

Holistic Assessment

Motivation: This PR implements debugger attach flag methods (MarkDebuggerAttachPending, MarkDebuggerAttached) in the cDAC debugger contract and DacDbi layer, migrates the debugging infrastructure from VMPTR_Assembly to VMPTR_DomainAssembly to better model AppDomain affinity, and removes dead fiber-mode debugging code. The architectural direction is sound — DomainAssembly is the correct abstraction for ICorDebug's module concept.

Approach: The change systematically threads VMPTR_DomainAssembly and VMPTR_AppDomain through the entire DacDbi interface (IDL, interface headers, DI, DAC, and cDAC implementations). The shift from a single m_pAppDomain pointer to a CordbSafeHashTable<CordbAppDomain> m_appDomains in CordbProcess is the right structural change. The dead code removal (fiber mode, unused enum flags) is clean.

Summary: ⚠️Needs Human Review. The code is architecturally well-structured and the core implementations look correct. However, there are several items that require maintainer attention: incomplete dead code cleanup, a parameter naming inconsistency in the IDL, and the DB_IPCE_EXIT_APP_DOMAIN re-enablement warrants explicit acknowledgment. The protocol breaking change counter question also needs a maintainer decision.

---

Detailed Findings

⚠️ DacDbi Protocol Breaking Change Counter — Not bumped

src/coreclr/debug/inc/dacdbistructures.h:692 — kCurrentDacDbiProtocolBreakingChangeCounter remains at 1 despite extensive breaking changes to IDacDbiInterface: ~30+ method signatures changed (Assembly → DomainAssembly), new methods added (GetAppDomainFromId, EnumerateAppDomains, GetAssemblyFromDomainAssembly, etc.), callback typedefs changed, and struct fields modified.

Per the comment at line 699-710, the counter should be explicitly bumped and documented when breaking changes are introduced. The auto-generated MD5 hash mechanism (described at line 616-618 in process.cpp) will detect the change automatically for remote transport scenarios, so this is unlikely to cause a runtime crash. However, the convention says to bump and document the counter. A maintainer should decide if a bump is needed here or if the hash is sufficient.

⚠️ Incomplete Dead Code Cleanup — CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS

src/coreclr/debug/di/process.cpp:237,8575 — The fiber-mode code was removed (DBCF_FIBERMODE, m_bHostingInFiber, the fiber check in VerifyControlBlock), but two references to CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS remain:

1. Line 237: In IsLegalFatalError() — lists this error as a legal fatal error. Since no code path can produce this error anymore, this entry is dead.
2. Line 8575: if (!IsLegalFatalError(errorHR) || (errorHR != CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS)) — This special-cases the fiber error to skip setting m_unrecoverableError. Since the error is never produced, the special case is also dead.

These should be cleaned up for consistency with the fiber-mode removal. Not blocking, but creates confusing dead code.

⚠️ IDL Parameter Naming Inconsistency — vmAssembly for DomainAssembly type

src/coreclr/inc/dacdbi.idl — The EnumerateModulesInAssembly method has:

HRESULT EnumerateModulesInAssembly([in] VMPTR_DomainAssembly vmAssembly, ...);

The parameter is named vmAssembly but the type is VMPTR_DomainAssembly. This should be vmDomainAssembly for consistency with all other methods in this PR.

⚠️DB_IPCE_EXIT_APP_DOMAIN Re-enabled — Needs documentation

src/coreclr/debug/inc/dbgipceventtypes.h:49 — This event type was previously commented out and is now re-enabled. The handler in process.cpp is well-written (with proper null checks and assertions). This is clearly needed for the new multi-AppDomain hash table support. However, it would be helpful to have a brief comment in the code or commit message explaining why this was re-enabled, since the previous comment suggested it was intentionally disabled.

✅ cDAC Debugger Contract Implementation — Correct

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger_1.cs — The new MarkDebuggerAttachPending() and MarkDebuggerAttached() implementations:

• Flag values (0x0100, 0x0200) match cordbpriv.h constants ✅
• Bitwise operations match the native DAC implementation in dacdbiimpl.cpp:4798-4869 ✅
• MarkDebuggerAttached(false) correctly clears both Attached and PendingAttach flags ✅
• The read-modify-write pattern is not atomic, but this matches the native code which relies on the DD lock for synchronization ✅

✅ TryGetDebuggerData Behavior Change — Intentional and correct

Debugger_1.cs:21-35 — Previously returned false when LeftSideInitialized == 0; now returns true with IsLeftSideInitialized in the data. This better matches the native DAC behavior: GetDefinesBitField and GetMDStructuresVersion (dacdbiimpl.cpp:7985-8009) check g_pDebugger == NULL but NOT LeftSideInitialized. The DacDbiImpl.cs wrapper at line 64 correctly uses data.IsLeftSideInitialized for the IsLeftSideInitialized DBI method. The dump test at line 93-94 was updated to cross-validate correctly.

✅ Struct Layout Binary Compatibility — Correct

src/coreclr/debug/inc/dbgipcevents.h — Replacing bool m_bHostingInFiber with BYTE padding1 maintains the same 1-byte layout within the DWORD-aligned group (m_checkedBuild + padding1 + padding2 + padding3 = 4 bytes). The transport marshaling in dbgtransportsession.cpp correctly omits the padding field. Both DebuggerIPCControlBlock and DebuggerIPCControlBlockTransport are updated consistently.

✅ Test Changes — Correct and comprehensive

src/native/managed/cdac/tests/DebuggerTests.cs — The removal of memBuilder.AddHeapFragment(debuggerControlFlagsFrag) is correct: BumpAllocator.Allocate auto-registers heap fragments with the builder. The three new tests (MarkDebuggerAttachPending_SetsPendingAttachFlag, MarkDebuggerAttached_SetsAttachedFlag_WhenTrue, MarkDebuggerAttached_ClearsAttachedAndPending_WhenFalse) provide good coverage with distinct initial states. The DacDbi dump tests add coverage for GetAppDomainFromId, EnumerateAppDomains, IsAssemblyFullyTrusted, and GetAppDomainIdFromVmObjectHandle.

✅ AppDomain Management Rearchitecture — Well-structured

src/coreclr/debug/di/process.cpp, rspriv.h — The migration from CordbAppDomain* m_pAppDomain to CordbSafeHashTable<CordbAppDomain> m_appDomains is clean:

• NeuterChildren correctly uses NeuterAndClear instead of manual pointer cleanup
• NeuterChildrenLeftSideResources correctly copies to an auxiliary list before releasing the lock
• CacheAppDomain properly uses AddBaseOrThrow with RSInitHolder RAII
• PrepopulateAppDomainsOrThrow gates on IsDacInitialized()
• ContinueInternal handles the hash table properly with CopyToArray

💡 GetInstantiationFieldInfo Uses AppDomain::GetCurrentDomain()

src/coreclr/debug/daccess/dacdbiimpl.cpp:1817 — This method takes a VMPTR_DomainAssembly parameter but uses AppDomain::GetCurrentDomain() for CollectFields instead of deriving the AppDomain from the parameter. In modern .NET (single AppDomain), this is functionally correct and mirrors how the runtime operates. Noted as a potential follow-up if multi-domain support were ever reconsidered.

💡 Mixed Concerns in PR

The PR bundles debugger attach flag implementation, Assembly→DomainAssembly migration, fiber-mode dead code removal, RequiresAlign8 removal from RuntimeTypeSystem, and ContractRegistry simplification. While these are related to the broader cDAC migration, splitting the dead code removal and RequiresAlign8 changes into separate PRs would make the changes easier to review and bisect.

Generated by Code Review for issue #126794 · ◷

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/coreclr/debug/di/process.cpp:8386

• CordbProcess::UnrecoverableError no longer sets m_unrecoverableError for legal fatal errors (e.g., CORDBG_E_INCOMPATIBLE_PROTOCOL), because the condition changed from effectively "all except CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS" to only !IsLegalFatalError(errorHR). This is a behavior change beyond removing fiber-mode handling and may leave the process in a non-zombie state after an unrecoverable condition. Consider restoring the previous intent (e.g., set m_unrecoverableError for legal fatal errors too, or otherwise document/handle why legal fatal errors should avoid zombie state).

 CONSISTENCY_CHECK_MSGF(IsLegalFatalError(errorHR), ("Unrecoverable internal error: hr=0x%08x!", errorHR));
if (!IsLegalFatalError(errorHR))
{
// This will throw everything into a Zombie state. The ATT_ macros will check this and fail immediately.
m_unrecoverableError = true;
//
// Mark the process as no longer synchronized.
//
Lock();
SetSynchronized(false);
IncStopCount();
Unlock();
}

[rcj1]

/ba-g wasm

[Copilot]

• Implements MarkDebuggerAttachPending and MarkDebuggerAttached in cDAC DacDbiImpl by adding corresponding Debugger_1 contract APIs and wiring them to target-memory writes of debugger control flags.
• Removes fibermode dead code

[Copilot]

Pull request overview

This PR extends the cDAC Debugger contract and the legacy DacDbiImpl surface to support MarkDebuggerAttachPending / MarkDebuggerAttached by writing the appropriate bits into g_CORDebuggerControlFlags, and adds unit test coverage plus documentation updates.

Changes:

• Added IDebugger.MarkDebuggerAttachPending() and IDebugger.MarkDebuggerAttached(bool) APIs and implemented them in Debugger_1 via target-memory writes.
• Updated DacDbiImpl to call the cDAC contract implementations (with DEBUG-only legacy cross-validation).
• Exposed g_CORDebuggerControlFlags via the CoreCLR data descriptor and added tests/docs for the new flag semantics.

Reviewed changes

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

Show a summary per file

File	Description
src/native/managed/cdac/tests/DebuggerTests.cs	Adds test target plumbing + new unit tests validating control-flag writes.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Implements the two DBI methods by calling the cDAC Debugger contract and translating errors to HRESULTs.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger_1.cs	Adds the flag enum and implements the two new attach APIs by updating CORDebuggerControlFlags.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs	Introduces the new Globals.CORDebuggerControlFlags name constant.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IDebugger.cs	Extends the contract interface with the two new methods.
src/coreclr/vm/datadescriptor/datadescriptor.inc	Exposes g_CORDebuggerControlFlags and adjusts Debugger global gating.
src/coreclr/inc/cordbpriv.h	Adds cDAC dependency annotations to DBCF_PENDING_ATTACH / DBCF_ATTACHED.
docs/design/datacontracts/Debugger.md	Documents the new APIs, global dependency, and flag behavior.

[Copilot]

Pull request overview

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

[dotnet-policy-service]

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

[rcj1]

Test helpers tbd in #126595

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

[github-actions]

🤖 Copilot Code Review — PR #126794

Note

This review was generated by Copilot (Claude Opus 4.6) with additional analysis from Claude Sonnet 4.5.

Holistic Assessment

Motivation: This PR implements debugger attach flag methods (MarkDebuggerAttachPending, MarkDebuggerAttached) in the cDAC debugger contract and DacDbi layer, migrates the debugging infrastructure from VMPTR_Assembly to VMPTR_DomainAssembly to better model AppDomain affinity, and removes dead fiber-mode debugging code. The architectural direction is sound — DomainAssembly is the correct abstraction for ICorDebug's module concept.

Approach: The change systematically threads VMPTR_DomainAssembly and VMPTR_AppDomain through the entire DacDbi interface (IDL, interface headers, DI, DAC, and cDAC implementations). The shift from a single m_pAppDomain pointer to a CordbSafeHashTable<CordbAppDomain> m_appDomains in CordbProcess is the right structural change. The dead code removal (fiber mode, unused enum flags) is clean.

Summary: ⚠️Needs Human Review. The code is architecturally well-structured and the core implementations look correct. However, there are several items that require maintainer attention: incomplete dead code cleanup, a parameter naming inconsistency in the IDL, and the DB_IPCE_EXIT_APP_DOMAIN re-enablement warrants explicit acknowledgment. The protocol breaking change counter question also needs a maintainer decision.

---

Detailed Findings

⚠️ DacDbi Protocol Breaking Change Counter — Not bumped

src/coreclr/debug/inc/dacdbistructures.h:692 — kCurrentDacDbiProtocolBreakingChangeCounter remains at 1 despite extensive breaking changes to IDacDbiInterface: ~30+ method signatures changed (Assembly → DomainAssembly), new methods added (GetAppDomainFromId, EnumerateAppDomains, GetAssemblyFromDomainAssembly, etc.), callback typedefs changed, and struct fields modified.

Per the comment at line 699-710, the counter should be explicitly bumped and documented when breaking changes are introduced. The auto-generated MD5 hash mechanism (described at line 616-618 in process.cpp) will detect the change automatically for remote transport scenarios, so this is unlikely to cause a runtime crash. However, the convention says to bump and document the counter. A maintainer should decide if a bump is needed here or if the hash is sufficient.

⚠️ Incomplete Dead Code Cleanup — CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS

src/coreclr/debug/di/process.cpp:237,8575 — The fiber-mode code was removed (DBCF_FIBERMODE, m_bHostingInFiber, the fiber check in VerifyControlBlock), but two references to CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS remain:

1. Line 237: In IsLegalFatalError() — lists this error as a legal fatal error. Since no code path can produce this error anymore, this entry is dead.
2. Line 8575: if (!IsLegalFatalError(errorHR) || (errorHR != CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS)) — This special-cases the fiber error to skip setting m_unrecoverableError. Since the error is never produced, the special case is also dead.

These should be cleaned up for consistency with the fiber-mode removal. Not blocking, but creates confusing dead code.

⚠️ IDL Parameter Naming Inconsistency — vmAssembly for DomainAssembly type

src/coreclr/inc/dacdbi.idl — The EnumerateModulesInAssembly method has:

HRESULT EnumerateModulesInAssembly([in] VMPTR_DomainAssembly vmAssembly, ...);

The parameter is named vmAssembly but the type is VMPTR_DomainAssembly. This should be vmDomainAssembly for consistency with all other methods in this PR.

⚠️DB_IPCE_EXIT_APP_DOMAIN Re-enabled — Needs documentation

src/coreclr/debug/inc/dbgipceventtypes.h:49 — This event type was previously commented out and is now re-enabled. The handler in process.cpp is well-written (with proper null checks and assertions). This is clearly needed for the new multi-AppDomain hash table support. However, it would be helpful to have a brief comment in the code or commit message explaining why this was re-enabled, since the previous comment suggested it was intentionally disabled.

✅ cDAC Debugger Contract Implementation — Correct

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger_1.cs — The new MarkDebuggerAttachPending() and MarkDebuggerAttached() implementations:

• Flag values (0x0100, 0x0200) match cordbpriv.h constants ✅
• Bitwise operations match the native DAC implementation in dacdbiimpl.cpp:4798-4869 ✅
• MarkDebuggerAttached(false) correctly clears both Attached and PendingAttach flags ✅
• The read-modify-write pattern is not atomic, but this matches the native code which relies on the DD lock for synchronization ✅

✅ TryGetDebuggerData Behavior Change — Intentional and correct

Debugger_1.cs:21-35 — Previously returned false when LeftSideInitialized == 0; now returns true with IsLeftSideInitialized in the data. This better matches the native DAC behavior: GetDefinesBitField and GetMDStructuresVersion (dacdbiimpl.cpp:7985-8009) check g_pDebugger == NULL but NOT LeftSideInitialized. The DacDbiImpl.cs wrapper at line 64 correctly uses data.IsLeftSideInitialized for the IsLeftSideInitialized DBI method. The dump test at line 93-94 was updated to cross-validate correctly.

✅ Struct Layout Binary Compatibility — Correct

src/coreclr/debug/inc/dbgipcevents.h — Replacing bool m_bHostingInFiber with BYTE padding1 maintains the same 1-byte layout within the DWORD-aligned group (m_checkedBuild + padding1 + padding2 + padding3 = 4 bytes). The transport marshaling in dbgtransportsession.cpp correctly omits the padding field. Both DebuggerIPCControlBlock and DebuggerIPCControlBlockTransport are updated consistently.

✅ Test Changes — Correct and comprehensive

src/native/managed/cdac/tests/DebuggerTests.cs — The removal of memBuilder.AddHeapFragment(debuggerControlFlagsFrag) is correct: BumpAllocator.Allocate auto-registers heap fragments with the builder. The three new tests (MarkDebuggerAttachPending_SetsPendingAttachFlag, MarkDebuggerAttached_SetsAttachedFlag_WhenTrue, MarkDebuggerAttached_ClearsAttachedAndPending_WhenFalse) provide good coverage with distinct initial states. The DacDbi dump tests add coverage for GetAppDomainFromId, EnumerateAppDomains, IsAssemblyFullyTrusted, and GetAppDomainIdFromVmObjectHandle.

✅ AppDomain Management Rearchitecture — Well-structured

src/coreclr/debug/di/process.cpp, rspriv.h — The migration from CordbAppDomain* m_pAppDomain to CordbSafeHashTable<CordbAppDomain> m_appDomains is clean:

• NeuterChildren correctly uses NeuterAndClear instead of manual pointer cleanup
• NeuterChildrenLeftSideResources correctly copies to an auxiliary list before releasing the lock
• CacheAppDomain properly uses AddBaseOrThrow with RSInitHolder RAII
• PrepopulateAppDomainsOrThrow gates on IsDacInitialized()
• ContinueInternal handles the hash table properly with CopyToArray

💡 GetInstantiationFieldInfo Uses AppDomain::GetCurrentDomain()

src/coreclr/debug/daccess/dacdbiimpl.cpp:1817 — This method takes a VMPTR_DomainAssembly parameter but uses AppDomain::GetCurrentDomain() for CollectFields instead of deriving the AppDomain from the parameter. In modern .NET (single AppDomain), this is functionally correct and mirrors how the runtime operates. Noted as a potential follow-up if multi-domain support were ever reconsidered.

💡 Mixed Concerns in PR

The PR bundles debugger attach flag implementation, Assembly→DomainAssembly migration, fiber-mode dead code removal, RequiresAlign8 removal from RuntimeTypeSystem, and ContractRegistry simplification. While these are related to the broader cDAC migration, splitting the dead code removal and RequiresAlign8 changes into separate PRs would make the changes easier to review and bisect.

Generated by Code Review for issue #126794 · ◷

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/coreclr/debug/di/process.cpp:8386

• CordbProcess::UnrecoverableError no longer sets m_unrecoverableError for legal fatal errors (e.g., CORDBG_E_INCOMPATIBLE_PROTOCOL), because the condition changed from effectively "all except CORDBG_E_CANNOT_DEBUG_FIBER_PROCESS" to only !IsLegalFatalError(errorHR). This is a behavior change beyond removing fiber-mode handling and may leave the process in a non-zombie state after an unrecoverable condition. Consider restoring the previous intent (e.g., set m_unrecoverableError for legal fatal errors too, or otherwise document/handle why legal fatal errors should avoid zombie state).

 CONSISTENCY_CHECK_MSGF(IsLegalFatalError(errorHR), ("Unrecoverable internal error: hr=0x%08x!", errorHR));
if (!IsLegalFatalError(errorHR))
{
// This will throw everything into a Zombie state. The ATT_ macros will check this and fail immediately.
m_unrecoverableError = true;
//
// Mark the process as no longer synchronized.
//
Lock();
SetSynchronized(false);
IncStopCount();
Unlock();
}

[rcj1]

/ba-g wasm