[cDAC] Implement SetCompilerFlags for cDAC

[barosiak]

Summary

Implement SetCompilerFlags in cDAC DacDbiImpl using ILoader contract to set debugger assembly control flags, using a post-write EnC capability check instead of native DAC's pre-write one.

Changes

• DacDbiImpl.cs - SetCompilerFlags implementation
• CorDbHResults.cs - Add CORDBG_S_NOT_ALL_BITS_SET success HRESULT
• LoaderTests.cs - Add 8 test cases covering both flags set/unset, EnC-not-capable, and JIT opts toggling across
  architectures

[Copilot]

Pull request overview

Implements SetCompilerFlags in the cDAC DacDbiImpl using the ILoader contract to update debugger assembly control flags and adds coverage/tests and a missing success HRESULT constant to match native DAC semantics.

Changes:

• Implement DacDbiImpl.SetCompilerFlags via ILoader and return CORDBG_S_NOT_ALL_BITS_SET when EnC cannot be enabled.
• Add CORDBG_S_NOT_ALL_BITS_SET to CorDbgHResults.
• Add SetCompilerFlagsTests covering flag set/unset and EnC capability behavior across architectures.

Reviewed changes

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

File	Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Adds SetCompilerFlags implementation using ILoader and post-write EnC enablement check
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/CorDbHResults.cs	Adds missing CORDBG_S_NOT_ALL_BITS_SET HRESULT constant
src/native/managed/cdac/tests/LoaderTests.cs	Adds new test coverage for SetCompilerFlags behavior

[Copilot]

The comment says this is checking whether the module is “not capable”, but the condition is checking whether ModuleFlags.EditAndContinue ended up enabled after SetDebuggerInfoBits. That bit can remain unset for reasons other than capability (e.g., global EEConfig policy). Consider rewording the comment to reflect what’s actually being validated (whether EnC was successfully enabled).

Suggested change

	// Check if EnC was requested but the module was not capable.
	// Check if EnC was requested but was not successfully enabled.

[github-actions]

🤖 Copilot Code Review — PR #127244

Note

This review was generated by GitHub Copilot (Claude Opus 4.6), with additional perspectives from GPT-5.3-Codex and Claude Haiku 4.5.

Holistic Assessment

Motivation: The PR replaces the SetCompilerFlags legacy-fallback stub in DacDbiImpl with a full cDAC implementation using the ILoader contract. This is a natural continuation of the cDAC migration work (companion to the existing GetCompilerFlags implementation) and is clearly justified.

Approach: The implementation follows the established cDAC patterns: try/catch with HRESULT return, #if DEBUG legacy validation, and delegation to the ILoader contract for low-level flag manipulation. The approach of delegating the "can we enable EnC" decision to SetDebuggerInfoBits (rather than replicating CanSetEnCBits logic) is a reasonable cDAC design choice, though it introduces a subtle internal state difference from the C++ implementation.

Summary: ⚠️ Needs Human Review. The implementation is structurally correct and follows cDAC conventions well. The HRESULT and observable flag outcomes match the C++ behavior. However, there is a subtle difference in how DACF_ENC_ENABLED is stored in the private debugger bits when EnC cannot be enabled — the managed version stores it unconditionally while C++ does not. I believe this is benign but want a domain expert to confirm. Additionally, one test could be strengthened.

---

Detailed Findings

⚠️ Internal State Divergence — DACF_ENC_ENABLED stored unconditionally (flagged by all 3 models)

In the C++ implementation (dacdbiimpl.cpp:785-798), DACF_ENC_ENABLED is only added to dwBits if CanSetEnCBits(pModule) succeeds. If it fails, the bit is NOT passed to SetDebuggerInfoBits:

if (fEnableEnC) {
    if (CanSetEnCBits(pModule))
        dwBits |= DACF_ENC_ENABLED;   // only if capable
    else
        hr = CORDBG_S_NOT_ALL_BITS_SET;
}
pModule->SetDebuggerInfoBits((DebuggerAssemblyControlFlags)dwBits);

In the managed implementation (DacDbiImpl.cs:334-336), DACF_ENC_ENABLED is always added when fEnableEnC is TRUE, then passed to SetDebuggerInfoBits:

if (fEnableEnC == Interop.BOOL.TRUE)
    controlFlags |= Contracts.DebuggerAssemblyControlFlags.DACF_ENC_ENABLED; // always
loader.SetDebuggerInfoBits(handle, controlFlags);

Impact analysis: The private debugger bit at position 0x2000 (DEBUGGER_ENC_ENABLED_PRIV) will be stored in the module flags in the managed version even when EnC can't be enabled. However:

• The HRESULT is correct (CORDBG_S_NOT_ALL_BITS_SET when appropriate)
• The IS_EDIT_AND_CONTINUE module flag (0x8) is correctly NOT set (the contract's SetDebuggerInfoBits handles this)
• GetCompilerFlags reads ModuleFlags, not debugger info bits, so results are unaffected
• Subsequent SetCompilerFlags calls clear DACF_ENC_ENABLED before reuse (line 326), so the persisted bit doesn't compound
• The #if DEBUG validation will catch any HRESULT mismatch against the C++ legacy implementation at runtime

I believe this is likely benign because no code path I can find reads DACF_ENC_ENABLED from stored debugger bits and acts on it differently. However, a domain expert should confirm there are no other consumers of GetDebuggerInfoBits that would be affected by the divergent private bit state.

⚠️ Test Gap — SetCompilerFlags_EnCRequested_NotCapable doesn't verify flag state (flagged by 2 models)

SetCompilerFlags_EnCRequested_NotCapable only asserts the HRESULT:

Assert.Equal(CorDbgHResults.CORDBG_S_NOT_ALL_BITS_SET, hr);

It would be stronger to also verify that IS_EDIT_AND_CONTINUE (0x8) was NOT set in the raw module flags, consistent with how SetCompilerFlags_BothFlagsSet_EncCapable verifies flags ARE set on the success path. This is advisory — the current test is not wrong, just not as thorough as its sibling.

💡 Missing Edge Case — EnC-capable module with ModifiableAssemblies.None

The tests cover EnC-capable + Debug config (succeeds) and non-capable + None config (returns CORDBG_S_NOT_ALL_BITS_SET). But there's no test for a module that IS EncCapable but where ModifiableAssemblies is None. In Loader_1.SetDebuggerInfoBits (line 427), this condition skips the EnC block entirely, so IS_EDIT_AND_CONTINUE won't be set and SetCompilerFlags should return CORDBG_S_NOT_ALL_BITS_SET. This is a contract-level test gap more than a DacDbiImpl-level one — follow-up suggestion.

💡 Pre-existing Design Note — CanSetEnCBits parity

The C++ CanSetEnCBits checks IsEditAndContinueCapable(), !CORProfilerPresent(), and DACF_IGNORE_PDBS. The managed Loader_1.SetDebuggerInfoBits checks EncCapable + ModifiableAssemblies config but not profiler presence or DACF_IGNORE_PDBS. This is a pre-existing difference in the Loader contract (not introduced by this PR) and the #if DEBUG HRESULT validation should catch any divergence at runtime. Flagging for awareness only.

✅ HRESULT Constant — Correct

CORDBG_S_NOT_ALL_BITS_SET = 0x00131c13 matches the native SMAKEHR(0x1c13) definition in corerror.h.

✅ Pattern Consistency — Follows established cDAC conventions

The implementation correctly follows the standard DacDbiImpl pattern: int hr = HResults.S_OK, try/catch with hr = ex.HResult, #if DEBUG validation via Debug.ValidateHResult. This matches ~60+ other methods in the file.

✅ Visibility Change — Appropriate

Changing CreateLoaderContractWithTarget from private to internal enables reuse by the new SetCompilerFlagsTests class. This is test-only code with no public API impact.

✅ Test Coverage — Good overall

Four well-structured tests covering: both flags enabled (EnC-capable), both flags disabled, EnC requested on non-capable module, and JIT opts toggling. Tests use [Theory] with [ClassData] for architecture coverage.

Generated by Code Review for issue #127244 · ◷

[rcj1]

Nit: prefer Marshal.GetExceptionForHR