[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.
The change ensures these methods update g_CORDebuggerControlFlags (using DBCF_PENDING_ATTACH / DBCF_ATTACHED).

• Contract/API surface

  • Added IDebugger.MarkDebuggerAttachPending()
  • Added IDebugger.MarkDebuggerAttached(bool fAttached)
  • Implemented both in Contracts/Debugger_1.cs with a private flag enum mirroring native values

• DBI implementation

  • Replaced legacy pass-throughs in DacDbiImpl.cs with cDAC-backed implementations
  • Preserved HRESULT translation behavior and DEBUG legacy cross-validation pattern

• Global exposure + native dependency annotation

  • Exposed g_CORDebuggerControlFlags via VM data descriptor (CORDebuggerControlFlags)
  • Added managed constant for the new global in Constants.Globals
  • Annotated native DebuggerControlFlag values in cordbpriv.h with [cDAC] [Debugger] dependency comments

• Documentation + tests

  • Updated docs/design/datacontracts/Debugger.md with new APIs, enum dependency, and correct global usage
  • Extended DebuggerTests to validate flag write semantics against CORDebuggerControlFlags

void IDebugger.MarkDebuggerAttachPending()
{
    TargetPointer addr = _target.ReadGlobalPointer(Constants.Globals.CORDebuggerControlFlags);
    uint flags = _target.Read<uint>(addr.Value);
    _target.Write<uint>(addr.Value, flags | 0x0100); // DBCF_PENDING_ATTACH
}

[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.

[github-actions]

🤖 Copilot Code Review — PR #126794

Note

This review was AI-generated by GitHub Copilot using multi-model analysis (Claude Opus 4.6 primary, Claude Sonnet 4 secondary).

Holistic Assessment

Motivation: The PR adds MarkDebuggerAttachPending and MarkDebuggerAttached to the cDAC debugger contract and DacDbi layer, replacing E_NOTIMPL passthrough stubs with real implementations. It also fixes a behavioral bug in TryGetDebuggerData where it was incorrectly gating on LeftSideInitialized, making the cDAC more restrictive than the native DAC. Both changes are well-motivated.

Approach: The implementation faithfully mirrors the native C++ DacDbiInterfaceImpl in dacdbiimpl.cpp:4812-4883. The TryGetDebuggerData change correctly separates the "debugger object exists" check from the "left side initialized" state, which aligns with how the native DAC handles these checks independently. The DacDbi layer properly follows the existing pattern (try/catch, DEBUG validation, HRESULT propagation).

Summary: ⚠️ Needs Changes. The implementation logic is correct and matches native behavior, but the unit tests cannot pass due to a missing Write<T> implementation in the test infrastructure.

---

Detailed Findings

❌ Unit Tests Will Fail — TestPlaceholderTarget.Write<T> throws NotImplementedException

The three new tests (MarkDebuggerAttachPending_SetsPendingAttachFlag, MarkDebuggerAttached_SetsAttachedFlag_WhenTrue, MarkDebuggerAttached_ClearsAttachedAndPending_WhenFalse) exercise Debugger_1.MarkDebuggerAttachPending() and Debugger_1.MarkDebuggerAttached(), which call _target.Write<uint>().

TestPlaceholderTarget (used by BuildTarget) throws NotImplementedException for both Write<T> (TestPlaceholderTarget.cs:328) and WriteBuffer (TestPlaceholderTarget.cs:209).

The underlying MockMemorySpace.MemoryContext does support writes via WriteToTarget() (MockMemorySpace.cs:187), and ContractDescriptorTarget.Write<T> works correctly (ContractDescriptorTarget.cs:458). Only TestPlaceholderTarget is missing the wiring.

Fix: Override Write<T> and WriteBuffer in TestPlaceholderTarget to delegate to the MemoryContext, similar to how ReadBuffer delegates to _dataReader. This likely requires plumbing a write delegate (or the MemoryContext itself) through the Builder.Build() method.

This issue was independently flagged by both models (Claude Opus 4.6 and Claude Sonnet 4).

---

✅ Correctness — Contract Implementation Matches Native DAC

The Debugger_1 implementation precisely mirrors the native C++ in dacdbiimpl.cpp:

• MarkDebuggerAttachPending: reads flags, ORs with 0x0100 (DBCF_PENDING_ATTACH), writes back. ✓
• MarkDebuggerAttached(true): ORs with 0x0200 (DBCF_ATTACHED). ✓
• MarkDebuggerAttached(false): clears both DBCF_ATTACHED | DBCF_PENDING_ATTACH. ✓
• Flag constants 0x0100/0x0200 match cordbpriv.h. ✓

The DacDbi layer correctly handles error cases:

• MarkDebuggerAttachPending: throws CORDBG_E_NOTREADY when TryGetDebuggerData returns false (equivalent to native g_pDebugger == NULL check). ✓
• MarkDebuggerAttached(false) with debugger not ready: silently returns S_OK (matching native: "If we're detaching, then don't throw because we don't care."). ✓

---

✅ TryGetDebuggerData Behavioral Change Is Correct

The change to no longer gate on LeftSideInitialized is a bug fix. The native DAC implementations of GetDefinesBitField, GetMDStructuresVersion, MarkDebuggerAttachPending, and MarkDebuggerAttached all check g_pDebugger != NULL, not LeftSideInitialized. The old cDAC behavior was overly restrictive.

All callers that need IsLeftSideInitialized now check it explicitly:

• DacDbiImpl.IsLeftSideInitialized: TryGetDebuggerData(out data) && data.IsLeftSideInitialized ✓
• DacDbiDebuggerDumpTests: Updated cross-validation to check both ✓
• GetDefinesBitField/GetMDStructuresVersion: Only need g_pDebugger != NULL (now correct) ✓

---

✅ Documentation & Infrastructure

• Debugger.md properly documents the new APIs, the new CORDebuggerControlFlags global, and the DebuggerControlFlag_1 enum.
• cordbpriv.h annotations ([cDAC] [Debugger]) correctly mark values depended on by the contract.
• datadescriptor.inc correctly adds the new global pointer under the existing #if defined(DEBUGGING_SUPPORTED) && !defined(TARGET_WASM) guard (fixing the intermediate !TARGET_WASM syntax error from the 2nd commit).
• Constants.cs global name added in alphabetical order. ✓

---

💡 Suggestion — Consider DacDbi-Level Integration Tests for Write Operations

Once the TestPlaceholderTarget.Write<T> issue is fixed, consider adding DacDbi-level tests that exercise MarkDebuggerAttachPending/MarkDebuggerAttached through the DacDbiImpl layer (similar to the existing IsLeftSideInitialized_CrossValidateWithContract pattern in DacDbiDebuggerDumpTests). This would validate the full stack including the TryGetDebuggerData guard and the CORDBG_E_NOTREADY error path.

Generated by Code Review for issue #126794 · ◷

[rcj1]

Test helpers tbd in #126595

[jkotas]

DBCF_FIBERMODE and m_bHostingInFiber can be deleted - dead code