[cDAC] Implement SetJITCompilerFlags for cDAC#126595
[cDAC] Implement SetJITCompilerFlags for cDAC#126595barosiak wants to merge 4 commits intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Implements IXCLRDataModule2.SetJITCompilerFlags for the managed cDAC by wiring it to the Loader contract’s module transient-flag debugger bits, adding the necessary contract surface and tests to validate read/write behavior.
Changes:
- Implement
IXCLRDataModule2.SetJITCompilerFlagsinClrDataModuleby updating module debugger control bits viaILoader. - Extend
ILoader/Loader_1withGetDebuggerInfoBitsandSetDebuggerInfoBits, and document the new contract surface/constants. - Update cDAC test infrastructure to support target-memory writes and add Loader tests for debugger-bit get/set and flag preservation.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs | Implements SetJITCompilerFlags using Loader debugger-info bits. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/IXCLRData.cs | Adds managed DebuggerAssemblyControlFlags enum mirroring cordbpriv.h. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ILoader.cs | Adds GetDebuggerInfoBits / SetDebuggerInfoBits to the Loader contract. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs | Implements debugger-info bit get/set by reading/writing Module::Flags. |
| src/native/managed/cdac/tests/TestPlaceholderTarget.cs | Adds write-capable target support for tests. |
| src/native/managed/cdac/tests/MockDescriptors/MockDescriptors.Loader.cs | Allows initializing module transient flags in mocks. |
| src/native/managed/cdac/tests/LoaderTests.cs | Adds new tests for debugger-info bit behavior. |
| docs/design/datacontracts/Loader.md | Documents new Loader contract methods and constants. |
| src/coreclr/vm/ceeload.h | Notes cDAC Loader contract dependency on debugger-info mask/shift constants. |
| src/coreclr/inc/cordbpriv.h | Adds explicit comment that cDAC depends on flag values. |
...native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Loader_1.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| int hr = HResults.S_OK; | ||
| try | ||
| { | ||
| if ((flags != CORDEBUG_JIT_DEFAULT) && (flags != CORDEBUG_JIT_DISABLE_OPTIMIZATION)) | ||
| throw new ArgumentException(); | ||
|
|
There was a problem hiding this comment.
IXCLRDataModule2.SetJITCompilerFlags is now implemented, but there doesn’t appear to be any unit/integration test that exercises this COM entrypoint (argument validation and the expected updates to Module::m_dwTransientFlags via Loader.Get/SetDebuggerInfoBits). Adding a focused test that constructs a ClrDataModule over a mock Module, calls SetJITCompilerFlags with both supported values (and an invalid value), and then asserts the shifted debugger bits in target memory would help prevent regressions.
| int hr = HResults.S_OK; | |
| try | |
| { | |
| if ((flags != CORDEBUG_JIT_DEFAULT) && (flags != CORDEBUG_JIT_DISABLE_OPTIMIZATION)) | |
| throw new ArgumentException(); | |
| if ((flags != CORDEBUG_JIT_DEFAULT) && (flags != CORDEBUG_JIT_DISABLE_OPTIMIZATION)) | |
| { | |
| return HResults.E_INVALIDARG; | |
| } | |
| int hr = HResults.S_OK; | |
| try | |
| { |
docs/design/datacontracts/Loader.md
Outdated
| void SetDebuggerInfoBits(ModuleHandle handle, uint newBits) | ||
| { | ||
| uint currentFlags = // read Module::Flags (uint32) at handle.Address + Flags offset | ||
| uint updated = (currentFlags & ~DebuggerInfoMask) | (newBits << DebuggerInfoShift); |
There was a problem hiding this comment.
The SetDebuggerInfoBits pseudocode writes (newBits << DebuggerInfoShift) without masking to the available bit-range, but the contract implementation masks newBits to (DebuggerInfoMask >> DebuggerInfoShift) before shifting. Please update the documentation snippet to reflect the masking behavior so callers don’t assume higher bits will be preserved/written.
| uint updated = (currentFlags & ~DebuggerInfoMask) | (newBits << DebuggerInfoShift); | |
| uint updated = (currentFlags & ~DebuggerInfoMask) | ((newBits & (DebuggerInfoMask >> DebuggerInfoShift)) << DebuggerInfoShift); |
docs/design/datacontracts/Loader.md
Outdated
| IReadOnlyDictionary<string, TargetPointer> GetLoaderAllocatorHeaps(TargetPointer loaderAllocatorPointer); | ||
|
|
||
| uint GetDebuggerInfoBits(ModuleHandle handle); | ||
| void SetDebuggerInfoBits(ModuleHandle handle, uint newBits); |
There was a problem hiding this comment.
Rather than expose all the bits I'd suggest only exposing DACF_ALLOW_JIT_OPTS and DACF_ENC_ENABLED for now. Later I think we might need to add read-only for DACF_IGNORE_PDBS as part of DBI ENC support.
DACF_USER_OVERRIDE doesn't look like it serves any purpose and could be removed from the runtime. The one place I see in the runtime that reads it is unreachable.
| } | ||
|
|
||
| [Flags] | ||
| public enum DebuggerAssemblyControlFlags : uint |
There was a problem hiding this comment.
I'd treat this enum as part of the contract (document it in the markdown, define it in DataContractReader, and use it as the input/output type for the Get/SetDebuggerBits functions).
| Target.TypeInfo type = _target.GetTypeInfo(DataType.Module); | ||
| ulong flagsAddr = handle.Address + (ulong)type.Fields[nameof(Data.Module.Flags)].Offset; | ||
| uint flags = _target.Read<uint>(flagsAddr); |
There was a problem hiding this comment.
Can use Data.Module.Flags to get this value.
| Target.TypeInfo type = _target.GetTypeInfo(DataType.Module); | ||
| ulong flagsAddr = handle.Address + (ulong)type.Fields[nameof(Data.Module.Flags)].Offset; | ||
| uint currentFlags = _target.Read<uint>(flagsAddr); | ||
| uint debuggerInfoBitsMask = DebuggerInfoMask >> DebuggerInfoShift; | ||
| uint updated = (currentFlags & ~DebuggerInfoMask) | ((newBits & debuggerInfoBitsMask) << DebuggerInfoShift); | ||
| _target.Write<uint>(flagsAddr, updated); |
There was a problem hiding this comment.
We should make sure to update the in memory Data.Module class. Otherwise anyone else that looks at the Module.Flags field will get outdated data (IData classes are cached). Not sure if we have an existing pattern for this, but I wouldn't be opposed to adding support for writing back in the Module class directly.
| DACF_ENC_ENABLED = 0x08, | ||
| DACF_IGNORE_PDBS = 0x20, | ||
| DACF_CONTROL_FLAGS_MASK = 0x2F, | ||
| DACF_CONTROL_FLAGS_MASK = 0x2E, |
| } | ||
|
|
||
| Target.TypeInfo type = _target.GetTypeInfo(DataType.Module); | ||
| ulong flagsAddr = handle.Address + (ulong)type.Fields[nameof(Data.Module.Flags)].Offset; |
| | `MaxWebcilSections` | ushort | Maximum number of COFF sections supported in a Webcil image (must stay in sync with native `WEBCIL_MAX_SECTIONS`) | `16` | | ||
| | `DebuggerInfoMask` | uint | Mask for the debugger info bits within the Module's transient flags | `0x0000Fc00` | | ||
| | `DebuggerInfoShift` | int | Bit shift for the debugger info bits within the Module's transient flags | `10` | | ||
| | `IS_JIT_OPTIMIZATION_DISABLED` | uint | Cached flag: JIT optimizations are disabled | `0x00000002` | |
There was a problem hiding this comment.
These four that are on the module flags belong in the module flags enum
| Data.Module module = _target.ProcessedData.GetOrAdd<Data.Module>(handle.Address); | ||
| uint currentFlags = module.Flags; | ||
| uint debuggerInfoBitsMask = DebuggerInfoMask >> DebuggerInfoShift; | ||
| uint updatedFlags = (currentFlags & ~DebuggerInfoMask) | (((uint)newBits & debuggerInfoBitsMask) << DebuggerInfoShift); |
| if ((updatedFlags & IS_ENC_CAPABLE) != 0) | ||
| { | ||
| TargetPointer configPtr = _target.ReadGlobalPointer(Constants.Globals.EEConfig); | ||
| Data.EEConfig config = _target.ProcessedData.GetOrAdd<Data.EEConfig>(configPtr); | ||
| ClrModifiableAssemblies modifiableAssemblies = (ClrModifiableAssemblies)config.ModifiableAssemblies; | ||
|
|
||
| if (modifiableAssemblies != ClrModifiableAssemblies.None) | ||
| { | ||
| bool encRequested = (newBits & DebuggerAssemblyControlFlags.DACF_ENC_ENABLED) != 0; | ||
| bool jitOptsDisabledForEnc = (updatedFlags & IS_JIT_OPTIMIZATION_DISABLED) != 0; | ||
| bool setEnC = encRequested || (modifiableAssemblies == ClrModifiableAssemblies.Debug && jitOptsDisabledForEnc); | ||
|
|
||
| if (setEnC) | ||
| updatedFlags |= IS_EDIT_AND_CONTINUE; | ||
| } | ||
| } |
| [Flags] | ||
| public enum DebuggerAssemblyControlFlags : uint | ||
| { | ||
| DACF_NONE = 0x00, | ||
| DACF_ALLOW_JIT_OPTS = 0x02, | ||
| DACF_ENC_ENABLED = 0x08, | ||
| DACF_CONTROL_FLAGS_MASK = 0x2E, | ||
| } |
| DACF_ALLOW_JIT_OPTS = 0x02, | ||
| DACF_ENC_ENABLED = 0x08, |
| if (_dataWriter(address, buffer) < 0) | ||
| throw new InvalidOperationException($"Failed to write {buffer.Length} bytes at 0x{address:x8}."); |
f37b5e7 to
d0c1fe4
Compare
Summary
Implement
SetJITCompilerFlagsonIXCLRDataModule2in the cDAC. Changes
SetJITCompilerFlagsimplementation