Add cDAC JIT notification contract with lazy table allocation#127114
Add cDAC JIT notification contract with lazy table allocation#127114max-charlamb wants to merge 9 commits intomainfrom
Conversation
Add SetCodeNotification/GetCodeNotification/SetAllCodeNotifications APIs to the INotifications contract, enabling the cDAC to manage the JIT notification table directly instead of falling back to the legacy DAC. Key changes: - Runtime: Pre-allocate JIT notification table on all platforms (not just Unix), with proper bookkeeping initialization in slot 0 - Data descriptors: Expose JITNotification type and g_pNotificationTable global in the Notifications c1 contract - Contract: Notifications_1 reads/writes entries directly in target memory (no host-side copy needed, unlike legacy DAC) - Legacy layer: Wire up ClrDataMethodDefinition and SOSDacImpl batch APIs to use the contract, with DEBUG validation against legacy DAC - Remove SetCodeNotification from legacy fallback allowlist - Add 9 unit tests for JIT notification table operations - Update Notifications.md design documentation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add AllocateMemory virtual method to Target abstraction with NotSupportedException default - Add AllocVirtualDelegate and new TryCreate overload to ContractDescriptorTarget - Wire ICLRDataTarget2.AllocVirtual through Entrypoints.cs - Revert Windows pre-allocation: put InitializeJITNotificationTable back under TARGET_UNIX - Handle null table in Notifications_1.cs: Get→NONE, SetAll→no-op, Set→lazy allocate - Replace magic numbers in IsValidMethodCodeNotification with named constants - Add 5 new tests: null table read, clear, no-allocator, lazy allocation - Update Notifications.md to document lazy allocation behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR implements the cDAC JIT code notification APIs on the existing Notifications contract, including lazy allocation of the JIT notification table via ICLRDataTarget2.AllocVirtual, and removes the legacy DAC fallback allowlist entry for SetCodeNotification.
Changes:
- Added JIT notification table support to
INotifications(SetCodeNotification,GetCodeNotification,SetAllCodeNotifications) with null-table semantics and lazy allocation viaTarget.AllocateMemory. - Wired optional target allocation support end-to-end (
ICLRDataTarget2.AllocVirtual→ContractDescriptorTarget→Target.AllocateMemory) and updated legacy COM wrappers to use the new contract APIs. - Added/expanded tests and documentation for the new contract behavior and table layout.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/tests/TestPlaceholderTarget.cs | Adds test-target hook to support Target.AllocateMemory for lazy allocation tests. |
| src/native/managed/cdac/tests/NotificationsTests.cs | Adds extensive unit tests for JIT notification table get/set/clear, null-table behavior, and lazy allocation. |
| src/native/managed/cdac/mscordaccore_universal/Entrypoints.cs | Attempts to plumb ICLRDataTarget2.AllocVirtual into ContractDescriptorTarget.TryCreate. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/ContractDescriptorTarget.cs | Adds AllocVirtualDelegate and a new TryCreate overload; implements AllocateMemory using AllocVirtual when available. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.IXCLRDataProcess.cs | Implements code notification APIs (Get/Set/SetAll*) using the cDAC Notifications contract (with DEBUG validation against legacy). |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/LegacyFallbackHelper.cs | Removes SetCodeNotification from the legacy fallback allowlist. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs | Exposes module address for use by code notification wrappers. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs | Implements IXCLRDataMethodDefinition.GetCodeNotification/SetCodeNotification via the contract. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/JITNotification.cs | Adds contract data model for reading JITNotification entries. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Notifications_1.cs | Implements JIT notification table logic, including lazy allocation and entry management. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs | Adds new globals for table pointer and capacity. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Target.cs | Adds Target.AllocateMemory(uint size) virtual API (default not supported). |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/DataType.cs | Adds DataType.JITNotification. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/INotifications.cs | Extends the Notifications contract interface with code notification APIs. |
| src/coreclr/vm/util.hpp | Updates Unix table initialization to also initialize the bookkeeping slot (length/capacity). |
| src/coreclr/vm/datadescriptor/datadescriptor.inc | Adds JITNotification type + new globals (JITNotificationTable, JITNotificationTableSize) to the contract descriptor. |
| docs/design/datacontracts/Notifications.md | Documents the new APIs, table layout, and lazy allocation behavior. |
Comments suppressed due to low confidence (1)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/ContractDescriptorTarget.cs:123
- There is a stray
/// </summary>XML doc line here that doesn't match any open<summary>tag (likely left behind when adding the new TryCreate overload). This will produce XML doc warnings/errors; remove it or restore the intended XML doc comment for the following member.
/// </summary>
/// <param name="contractDescriptor">The contract descriptor to use for this target</param>
/// <param name="globalPointerValues">The values for any global pointers specified in the contract descriptor.</param>
/// <param name="readFromTarget">A callback to read memory blocks at a given address from the target</param>
/// <param name="writeToTarget">A callback to write memory blocks at a given address to the target</param>
| ClrDataAddress addr; | ||
| int result = dataTarget2.AllocVirtual(0, (uint)size, 0x1000 /* MEM_COMMIT */, 0x04 /* PAGE_READWRITE */, &addr); |
There was a problem hiding this comment.
ClrDataAddress addr is not definitely assigned before being read to set allocatedAddress. This will fail compilation (use of unassigned local) and/or propagate garbage on allocation failure. Initialize addr (e.g., ClrDataAddress addr = 0;) and set allocatedAddress = 0 when AllocVirtual returns a failure HRESULT; also consider validating size fits in uint before casting.
| ClrDataAddress addr; | |
| int result = dataTarget2.AllocVirtual(0, (uint)size, 0x1000 /* MEM_COMMIT */, 0x04 /* PAGE_READWRITE */, &addr); | |
| if (size > uint.MaxValue) | |
| { | |
| allocatedAddress = 0; | |
| return unchecked((int)0x80070057); // E_INVALIDARG | |
| } | |
| ClrDataAddress addr = 0; | |
| int result = dataTarget2.AllocVirtual(0, (uint)size, 0x1000 /* MEM_COMMIT */, 0x04 /* PAGE_READWRITE */, &addr); | |
| if (result != 0) | |
| { | |
| allocatedAddress = 0; | |
| return result; | |
| } |
| flags[i] = _target.Contracts.Notifications.GetCodeNotification(moduleAddr, tokens[i]); | ||
| } |
There was a problem hiding this comment.
This indexes flags[i]/tokens[i] up to numTokens without validating that the managed arrays are at least numTokens long. If a caller passes shorter arrays, this will throw IndexOutOfRangeException and return an unexpected HRESULT. Add explicit checks like numTokens <= tokens.Length and numTokens <= flags.Length and return E_INVALIDARG on mismatch.
| for (uint check = 0; check < numTokens; check++) | ||
| { | ||
| if (!IsValidMethodCodeNotification(flags[check])) | ||
| throw new ArgumentException("Invalid code notification flags"); | ||
| } |
There was a problem hiding this comment.
The validation loop indexes flags[check] up to numTokens without checking that flags.Length >= numTokens (and later flags[i] similarly). If the arrays are shorter than numTokens, this will throw and surface as an unexpected HRESULT. Add explicit length checks (and also validate tokens.Length >= numTokens) and return E_INVALIDARG when violated.
| #if DEBUG | ||
| if (_legacyProcess is not null) | ||
| { | ||
| int hrLocal = _legacyProcess.SetCodeNotifications(numTokens, mods, singleMod, tokens, flags!, singleFlags); |
There was a problem hiding this comment.
In the DEBUG validation block, the legacy call passes flags! even though the method logic allows flags to be null (using singleFlags instead). If flags is ever null, this will throw and break DEBUG builds. Ensure the DEBUG legacy call is only made with a non-null array (e.g., allocate a temporary array filled with singleFlags when needed).
| int hrLocal = _legacyProcess.SetCodeNotifications(numTokens, mods, singleMod, tokens, flags!, singleFlags); | |
| uint[] flagsForLegacy = flags; | |
| if (flagsForLegacy is null) | |
| { | |
| flagsForLegacy = new uint[checked((int)numTokens)]; | |
| Array.Fill(flagsForLegacy, singleFlags); | |
| } | |
| int hrLocal = _legacyProcess.SetCodeNotifications(numTokens, mods, singleMod, tokens, flagsForLegacy, singleFlags); |
… ICLRDataTarget2 - Remove #if DEBUG validation blocks from all five JIT notification methods (SetCodeNotification, SetAllCodeNotifications, SetCodeNotifications, GetCodeNotification, GetCodeNotifications). Write operations cause dual-write corruption when both cDAC and legacy DAC independently allocate and write to g_pNotificationTable. Read operations have HResult mismatches when table is NULL (cDAC returns S_OK+NONE, legacy DAC returns E_OUTOFMEMORY). - Replace 'as ICLRDataTarget2' cast with explicit Marshal.QueryInterface and raw vtable function pointer call. The 'as' cast on a StrategyBasedComWrappers RCW may not reliably perform QI for derived [GeneratedComInterface] interfaces. The explicit QI approach mirrors the native DAC's QueryInterface pattern in daccess.cpp. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
max-charlamb
force-pushed
the
cdac/jit-notification-contract
branch
from
c052fb3 to
d951242
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/ContractDescriptorTarget.cs:124
- There’s a stray XML doc line
/// </summary>after the newTryCreateoverload. This produces malformed XML documentation (and can fail the build if warnings are treated as errors). Remove it or restore the missing opening documentation element for the following member.
}
/// </summary>
/// <param name="contractDescriptor">The contract descriptor to use for this target</param>
/// <param name="globalPointerValues">The values for any global pointers specified in the contract descriptor.</param>
/// <param name="readFromTarget">A callback to read memory blocks at a given address from the target</param>
/// <param name="writeToTarget">A callback to write memory blocks at a given address to the target</param>
/// <param name="getThreadContext">A callback to fetch a thread's context</param>
| uint tableByteSize = entrySize * (capacity + 1); | ||
| TargetPointer tablePointer = _target.AllocateMemory(tableByteSize); | ||
|
|
||
| // Zero-initialize the entire table | ||
| byte[] zeros = new byte[tableByteSize]; |
There was a problem hiding this comment.
AllocateTable uses uint tableByteSize and then new byte[tableByteSize]. Array lengths are int, so this won’t compile (and also needs an overflow check when casting). Consider using an int byte count (checked) for the managed buffer size, or allocate/write in chunks to avoid a large managed allocation.
| uint tableByteSize = entrySize * (capacity + 1); | |
| TargetPointer tablePointer = _target.AllocateMemory(tableByteSize); | |
| // Zero-initialize the entire table | |
| byte[] zeros = new byte[tableByteSize]; | |
| uint tableByteSize = checked(entrySize * checked(capacity + 1)); | |
| TargetPointer tablePointer = _target.AllocateMemory(tableByteSize); | |
| // Zero-initialize the entire table | |
| int managedTableByteSize = checked((int)tableByteSize); | |
| byte[] zeros = new byte[managedTableByteSize]; |
| private static TargetPointer GetModuleAddress(void* comModulePtr) | ||
| { | ||
| StrategyBasedComWrappers cw = new(); | ||
| object obj = cw.GetOrCreateObjectForComInstance((nint)comModulePtr, CreateObjectFlags.None); | ||
| if (obj is ClrDataModule cdm) | ||
| return cdm.Address; |
There was a problem hiding this comment.
GetModuleAddress constructs a new StrategyBasedComWrappers instance each time it’s called. Since GetCodeNotifications/SetCodeNotifications call this in a loop, this can create many ComWrappers instances and defeat per-instance caching (potentially creating multiple managed wrappers for the same COM pointer). Consider creating a single StrategyBasedComWrappers per API call (outside the loop) and reusing it when resolving module addresses.
| private static TargetPointer GetModuleAddress(void* comModulePtr) | |
| { | |
| StrategyBasedComWrappers cw = new(); | |
| object obj = cw.GetOrCreateObjectForComInstance((nint)comModulePtr, CreateObjectFlags.None); | |
| if (obj is ClrDataModule cdm) | |
| return cdm.Address; | |
| private static readonly StrategyBasedComWrappers s_comWrappers = new(); | |
| private static TargetPointer GetModuleAddress(void* comModulePtr) | |
| { | |
| object obj = s_comWrappers.GetOrCreateObjectForComInstance((nint)comModulePtr, CreateObjectFlags.None); | |
| if (obj is ClrDataModule cdm) | |
| return cdm.Address; |
| [Fact] | ||
| public void SetCodeNotification_InvalidFlags_Throws() | ||
| { | ||
| INotifications contract = CreateContractWithJITTable(); | ||
| TargetPointer module = new(0x1234); |
There was a problem hiding this comment.
The PR description mentions test coverage for the “table full” case, but there doesn’t appear to be a test exercising the InvalidOperationException("JIT notification table is full") path. Consider adding a test that fills the table to capacity and verifies the expected failure behavior.
This comment has been minimized.
This comment has been minimized.
Add alloc_virtual callback parameter to cdac_reader_init so the CI test path (cdac_reader_init -> cdac_reader_create_sos_interface) has access to ICLRDataTarget2::AllocVirtual for lazy JIT notification table allocation. Changes: - cdac_reader.h: Add alloc_virtual callback parameter (nullable) - cdac.cpp: Add AllocVirtualCallback that QIs ICorDebugDataTarget for ICLRDataTarget2, probe at init time and pass callback or nullptr - cdacstress.cpp: Pass nullptr for alloc_virtual (in-process stress) - Entrypoints.cs: Accept alloc_virtual function pointer, wrap as AllocVirtualDelegate, call 6-parameter TryCreate overload - Notifications.md: Update docs to reflect alloc_virtual availability Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🤖 Copilot Code Review — PR #127114Note This review was generated by Copilot using multi-model analysis (Claude Opus 4.6 primary, GPT-5.3-Codex and Claude Sonnet 4.5 sub-agents). Holistic AssessmentMotivation: This PR is well-motivated. It migrates JIT notification table management from the legacy C++ DAC to the managed cDAC, consistent with the ongoing effort to replace the legacy DAC. The notification table protocol is faithfully reproduced, and the lazy allocation pattern for Windows (where the table starts null) is a sensible addition. Approach: The approach is sound and follows established cDAC conventions — data descriptors in Summary: Detailed Findings
|
…nify constant types, reuse WriteNUInt - Fix stray </summary> XML doc tag on ContractDescriptorTarget.Create - Replace StrategyBasedComWrappers with ComWrappers.TryGetObject in GetModuleAddress - Make CLRDATA_METHNOTIFY_NONE uint to match other constants - Reuse WriteNUInt helper in AllocateTable instead of duplicating logic Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| WriteToTargetDelegate writeToTarget, | ||
| GetTargetThreadContextDelegate getThreadContext) | ||
| GetTargetThreadContextDelegate getThreadContext, | ||
| AllocVirtualDelegate? allocVirtual = null) |
There was a problem hiding this comment.
Lets make this non-nullable, if we don't have a callback, we can create one that throws an exception.
| { | ||
| return writeToTarget(address, buffer); | ||
| } | ||
| public bool HasAllocVirtual => allocVirtual is not null; |
There was a problem hiding this comment.
We can remove this and just call the method directly now that it is non-nullable.
| if (!IsValidMethodCodeNotification(flags)) | ||
| throw new ArgumentException("Invalid code notification flags", nameof(flags)); | ||
|
|
||
| Target.TypeInfo jitNotifType = _target.GetTypeInfo(DataType.JITNotification); |
There was a problem hiding this comment.
Lets make JITNotification a IData class with write-back capabilities. We should avoid using TypeInfo inside the contracts.
…ble AllocVirtual - JITNotification Data class now has write-back methods (WriteState, WriteClrModule, WriteMethodToken, Clear, WriteEntry) matching the established Module.WriteFlags pattern - Notifications_1 refactored to use Data.JITNotification for all reads and writes instead of raw TypeInfo offset lookups - AllocVirtual delegate made non-nullable in DataTargetDelegates with ThrowAllocVirtual as the default, removing HasAllocVirtual and null checks Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| allocVirtual = (ulong size, out ulong allocatedAddress) => | ||
| { | ||
| ClrDataAddress addr; | ||
| int result = dataTarget2.AllocVirtual(0, (uint)size, 0x1000 /* MEM_COMMIT */, 0x04 /* PAGE_READWRITE */, &addr); | ||
| allocatedAddress = (ulong)addr; | ||
| return result; |
There was a problem hiding this comment.
The AllocVirtual call uses raw constants (0x1000 for MEM_COMMIT and 0x04 for PAGE_READWRITE). Consider replacing these with named constants (or local const fields) to make the flags unambiguous and reduce the chance of accidental misuse.
| void INotifications.SetCodeNotification(TargetPointer module, uint methodToken, uint flags) | ||
| { | ||
| if (!IsValidMethodCodeNotification(flags)) | ||
| throw new ArgumentException("Invalid code notification flags", nameof(flags)); | ||
|
|
||
| uint entrySize = GetEntrySize(); | ||
| TargetPointer tablePointer = ReadTablePointer(); | ||
|
|
||
| // If table is null and we're clearing, nothing to do | ||
| if (tablePointer == TargetPointer.Null && flags == CLRDATA_METHNOTIFY_NONE) | ||
| return; | ||
|
|
||
| // If table is null and we're setting, lazily allocate | ||
| if (tablePointer == TargetPointer.Null) | ||
| { | ||
| tablePointer = AllocateTable(entrySize); | ||
| } |
There was a problem hiding this comment.
INotifications.SetCodeNotification currently allows module == TargetPointer.Null, but the underlying runtime implementation rejects a null module (see JITNotifications::SetNotification returning FALSE when clrModule == NULL). Consider validating module != TargetPointer.Null and throwing ArgumentException to avoid inserting entries with a null module pointer.
| uint INotifications.GetCodeNotification(TargetPointer module, uint methodToken) | ||
| { | ||
| uint entrySize = GetEntrySize(); | ||
| TargetPointer tablePointer = ReadTablePointer(); | ||
|
|
||
| if (tablePointer == TargetPointer.Null) | ||
| return CLRDATA_METHNOTIFY_NONE; | ||
|
|
There was a problem hiding this comment.
INotifications.GetCodeNotification also treats module == TargetPointer.Null as a normal lookup key. If null modules are invalid inputs (as in the native JIT notification table), it would be better to reject TargetPointer.Null here too (e.g., throw ArgumentException) rather than returning a potentially misleading value.
| int IXCLRDataMethodDefinition.SetCodeNotification(uint flags) | ||
| => LegacyFallbackHelper.CanFallback() && _legacyImpl is not null ? _legacyImpl.SetCodeNotification(flags) : HResults.E_NOTIMPL; | ||
| { | ||
| int hr = HResults.S_OK; | ||
| try | ||
| { | ||
| _target.Contracts.Notifications.SetCodeNotification(_module, _token, flags); | ||
| } | ||
| catch (System.ArgumentException) | ||
| { | ||
| hr = HResults.E_INVALIDARG; | ||
| } | ||
| catch (System.InvalidOperationException) | ||
| { | ||
| hr = HResults.E_OUTOFMEMORY; | ||
| } | ||
| catch (System.Exception ex) | ||
| { | ||
| hr = ex.HResult; | ||
| } |
There was a problem hiding this comment.
SetCodeNotification can throw NotSupportedException when AllocateMemory isn't available (e.g., target doesn't support ICLRDataTarget2). The current catch-all will return NotSupportedException.HResult (COR_E_NOTSUPPORTED) to COM callers, which is typically not the expected HRESULT for an unsupported DAC API. Consider catching NotSupportedException explicitly and translating it to E_NOTIMPL (or another agreed HRESULT) at the COM boundary.
| if (obj is ClrDataModule cdm) | ||
| return cdm.Address; | ||
| } | ||
| throw new InvalidOperationException("Could not resolve module address from COM pointer"); |
There was a problem hiding this comment.
GetModuleAddress throws InvalidOperationException when the COM pointer can't be resolved. That exception type currently gets translated to E_OUTOFMEMORY by GetCodeNotifications/SetCodeNotifications, which is misleading. Consider throwing ArgumentException (or a COMException with E_INVALIDARG) here so callers reliably get E_INVALIDARG for bad module pointers.
| throw new InvalidOperationException("Could not resolve module address from COM pointer"); | |
| throw new ArgumentException("Could not resolve module address from COM pointer", nameof(comModulePtr)); |
| int IXCLRDataProcess.SetAllCodeNotifications(IXCLRDataModule? mod, uint flags) | ||
| => LegacyFallbackHelper.CanFallback() && _legacyProcess is not null ? _legacyProcess.SetAllCodeNotifications(mod, flags) : HResults.E_NOTIMPL; | ||
| { | ||
| int hr = HResults.S_OK; | ||
| try | ||
| { | ||
| TargetPointer moduleAddr = TargetPointer.Null; | ||
| if (mod is not null) | ||
| { | ||
| if (mod is not ClrDataModule cdm) | ||
| throw new ArgumentException(); | ||
| moduleAddr = cdm.Address; | ||
| } | ||
|
|
||
| _target.Contracts.Notifications.SetAllCodeNotifications(moduleAddr, flags); | ||
| } | ||
| catch (System.ArgumentException) | ||
| { | ||
| hr = HResults.E_INVALIDARG; | ||
| } | ||
| catch (System.InvalidOperationException) | ||
| { | ||
| hr = HResults.E_OUTOFMEMORY; | ||
| } | ||
| catch (System.Exception ex) | ||
| { | ||
| hr = ex.HResult; | ||
| } |
There was a problem hiding this comment.
SetAllCodeNotifications may indirectly trigger lazy table allocation, which can throw NotSupportedException when the target doesn't support allocation. This method currently falls through to the generic exception handler and returns NotSupportedException.HResult (COR_E_NOTSUPPORTED). Consider catching NotSupportedException explicitly and returning E_NOTIMPL (or an agreed HRESULT) to keep COM-facing behavior consistent.
| @@ -699,7 +779,66 @@ int IXCLRDataProcess.SetCodeNotifications( | |||
| [In, MarshalUsing(CountElementName = nameof(numTokens))] /*mdMethodDef */ uint[] tokens, | |||
| [In, MarshalUsing(CountElementName = nameof(numTokens))] uint[] flags, | |||
| uint singleFlags) | |||
| => LegacyFallbackHelper.CanFallback() && _legacyProcess is not null ? _legacyProcess.SetCodeNotifications(numTokens, mods, singleMod, tokens, flags, singleFlags) : HResults.E_NOTIMPL; | |||
| { | |||
| int hr = HResults.S_OK; | |||
| try | |||
| { | |||
| ArgumentNullException.ThrowIfNull(tokens); | |||
| if ((mods is null && singleMod is null) || (mods is not null && singleMod is not null)) | |||
| throw new ArgumentException(); | |||
|
|
|||
| // Validate flags | |||
| if (flags is not null) | |||
| { | |||
| for (uint check = 0; check < numTokens; check++) | |||
| { | |||
| if (!IsValidMethodCodeNotification(flags[check])) | |||
| throw new ArgumentException("Invalid code notification flags"); | |||
| } | |||
| } | |||
| else if (!IsValidMethodCodeNotification(singleFlags)) | |||
| { | |||
| throw new ArgumentException("Invalid code notification flags"); | |||
| } | |||
|
|
|||
| TargetPointer singleModuleAddr = TargetPointer.Null; | |||
| if (singleMod is not null) | |||
| { | |||
| if (singleMod is not ClrDataModule singleCdm) | |||
| throw new ArgumentException(); | |||
| singleModuleAddr = singleCdm.Address; | |||
| } | |||
|
|
|||
| for (uint i = 0; i < numTokens; i++) | |||
| { | |||
| TargetPointer moduleAddr = singleModuleAddr; | |||
| if (mods is not null) | |||
| { | |||
| moduleAddr = GetModuleAddress(mods[i]); | |||
| } | |||
|
|
|||
| uint f = flags is not null ? flags[i] : singleFlags; | |||
| _target.Contracts.Notifications.SetCodeNotification(moduleAddr, tokens[i], f); | |||
| } | |||
| } | |||
| catch (System.ArgumentException) | |||
| { | |||
| hr = HResults.E_INVALIDARG; | |||
| } | |||
| catch (System.InvalidOperationException) | |||
| { | |||
| hr = HResults.E_OUTOFMEMORY; | |||
| } | |||
| catch (System.Exception ex) | |||
| { | |||
| hr = ex.HResult; | |||
| } | |||
There was a problem hiding this comment.
SetCodeNotifications can also hit the lazy-allocation path and throw NotSupportedException when ICLRDataTarget2/AllocVirtual isn't available. As written, that will return COR_E_NOTSUPPORTED via the generic catch, which is an uncommon HRESULT for COM APIs here. Consider catching NotSupportedException explicitly and translating to E_NOTIMPL (or another agreed HRESULT).
Extract SetCodeNotification, GetCodeNotification, and SetAllCodeNotifications into a new ICodeNotifications contract, leaving INotifications focused on read-only event decoding (TryParseNotification, SetGcNotification). The two responsibilities are materially different: INotifications decodes exception-record payloads raised by the runtime; ICodeNotifications writes into an in-process allowlist and lazily allocates the table via AllocVirtual. Also adds GetCodeNotificationCapacity() to the new contract in preparation for an upfront capacity check in batch SOSDacImpl.SetCodeNotifications. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two DAC-parity fixes surfaced by self-review of PR #127114: 1. Batch IXCLRDataProcess::SetCodeNotifications now rejects upfront with E_OUTOFMEMORY when numTokens exceeds the table capacity. Matches the legacy DAC (daccess.cpp: 'numTokens > jn.GetTableSize()') and prevents partial writes when a batch overflows mid-loop. 2. ICodeNotifications.GetCodeNotification now throws InvalidOperationException (mapped to E_OUTOFMEMORY by wrappers) when the table has not been allocated, matching legacy DAC behavior (JITNotifications::IsActive()). Previously returned CLRDATA_METHNOTIFY_NONE, which was an unintentional behavioral divergence. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Previously the cDAC read the JIT notification table capacity from slot 0's ClrModule field, which required the runtime to initialize that field at startup on Unix (where the table is pre-allocated) so the cDAC would see a valid capacity. This coupled runtime init to the cDAC's direct-read model. The capacity is a compile-time invariant (1000) already exposed via the JITNotificationTableSize global descriptor. Switch to reading the global, which: - Removes the runtime-side bookkeeping init (util.hpp change reverted). - Removes the bookkeeping capacity write from AllocateTable; zero-filling the buffer is sufficient (length starts at 0 naturally). - Leaves slot 0's ClrModule field unused. Default-construction (Unix) and AllocVirtual zero-init (Windows) both give length = 0 in slot 0 without any explicit writes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| var arch = new MockTarget.Architecture { IsLittleEndian = true, Is64Bit = true }; | ||
| var helpers = new TargetTestHelpers(arch); | ||
|
|
||
| int totalTableSize = EntrySize * ((int)TableCapacity + 1); | ||
| byte[] allocatedTableData = new byte[totalTableSize]; | ||
| const ulong AllocatedTableAddress = 0x3_0000; |
There was a problem hiding this comment.
Unused local helpers will produce compiler warning CS0219 and can break builds if warnings are treated as errors (common in CI). Remove the variable or use it for table initialization in this test.
| if (obj is ClrDataModule cdm) | ||
| return cdm.Address; | ||
| } | ||
| throw new InvalidOperationException("Could not resolve module address from COM pointer"); |
There was a problem hiding this comment.
GetModuleAddress throws InvalidOperationException, but callers (e.g., GetCodeNotifications) catch InvalidOperationException and translate it to E_OUTOFMEMORY. Failing to resolve the COM module pointer should map to E_INVALIDARG instead; throw ArgumentException (or a dedicated exception type) here, or adjust the catch blocks to avoid misclassifying this failure as OOM.
| throw new InvalidOperationException("Could not resolve module address from COM pointer"); | |
| throw new ArgumentException("Could not resolve module address from COM pointer", nameof(comModulePtr)); |
| for (uint i = 0; i < numTokens; i++) | ||
| { | ||
| TargetPointer moduleAddr = singleModuleAddr; | ||
| if (mods is not null) | ||
| { | ||
| moduleAddr = GetModuleAddress(mods[i]); | ||
| } | ||
|
|
||
| flags[i] = _target.Contracts.CodeNotifications.GetCodeNotification(moduleAddr, tokens[i]); | ||
| } |
There was a problem hiding this comment.
This loop indexes mods[i], tokens[i], and flags[i] without validating that the provided arrays have lengths >= numTokens. If a caller passes inconsistent lengths, this will throw IndexOutOfRangeException and return an unintended HRESULT. Add explicit length checks for tokens, flags, and (when non-null) mods, and return E_INVALIDARG on mismatch.
| for (uint check = 0; check < numTokens; check++) | ||
| { | ||
| if (!IsValidMethodCodeNotification(flags[check])) | ||
| throw new ArgumentException("Invalid code notification flags"); | ||
| } |
There was a problem hiding this comment.
When flags is non-null, this loop assumes flags.Length >= numTokens. If shorter, it will throw IndexOutOfRangeException (unexpected HRESULT). Validate flags.Length (and mods.Length when applicable) against numTokens up front and return E_INVALIDARG for inconsistent inputs.
| allocVirtual = (ulong size, out ulong allocatedAddress) => | ||
| { | ||
| ClrDataAddress addr; | ||
| int result = dataTarget2.AllocVirtual(0, (uint)size, 0x1000 /* MEM_COMMIT */, 0x04 /* PAGE_READWRITE */, &addr); | ||
| allocatedAddress = (ulong)addr; | ||
| return result; | ||
| }; |
There was a problem hiding this comment.
This introduces Windows API magic numbers for MEM_COMMIT (0x1000) and PAGE_READWRITE (0x04). Replace them with named constants (e.g., from an existing interop/constants location in the repo, or local const uint fields) to make the intent clear and reduce the chance of flag misuse.
| Target.TypeInfo type = target.GetTypeInfo(DataType.JITNotification); | ||
|
|
||
| State = target.ReadField<ushort>(address, type, nameof(State)); | ||
| ClrModule = target.ReadNUIntField(address, type, nameof(ClrModule)); | ||
| MethodToken = target.ReadField<uint>(address, type, nameof(MethodToken)); |
There was a problem hiding this comment.
JITNotification repeatedly calls target.GetTypeInfo(DataType.JITNotification) in the constructor and again in every write helper. In loops over the table (up to 1000 entries), this can add avoidable overhead. Consider caching the TypeInfo (and/or field offsets) once per contract instance and passing it into JITNotification, or storing it in the JITNotification instance so write helpers reuse the same TypeInfo.
Note
This PR was created with assistance from GitHub Copilot.
Summary
Implement the cDAC JIT code notification contract, removing the legacy DAC allowlist fallback for
SetCodeNotification. Adds a new dedicatedICodeNotificationscontract (c1) coveringSetCodeNotification,GetCodeNotification,SetAllCodeNotifications, andGetCodeNotificationCapacity.Key Design Decisions
ICodeNotificationscontract: JIT code notification table management is split out of the existing read-onlyINotificationscontract (which now only decodes debugger events).INotifications= read-only event decoding;ICodeNotifications= mutable in-target table management.ICLRDataTarget2::AllocVirtual. This avoids a ~24 KB per-process memory cost for apps that never use JIT notifications.JITNotificationTableSizeglobal descriptor. Slot 0 of the table stores only thelength(in itsmethodTokenfield);clrModuleis unused. This avoids coupling the runtime's table-allocation path to the cDAC's direct-read model — no runtime changes are needed to seed bookkeeping fields.GetCodeNotification→ throwsInvalidOperationException→E_OUTOFMEMORY(matches legacy DAC)SetAllCodeNotifications→ no-opSetCodeNotificationwith NONE → no-opSetCodeNotificationwith flags → lazy-allocates the table, then setsIXCLRDataProcess::SetCodeNotificationsrejects upfront withE_OUTOFMEMORYwhennumTokensexceeds table capacity, matching legacy DAC and preventing partial writes on batch overflow.Changes
Target Abstraction
AllocateMemory(uint size)virtual method toTarget(defaults toNotSupportedException)AllocVirtualDelegateand newTryCreateoverload toContractDescriptorTargetICLRDataTarget2::AllocVirtualthroughCLRDataCreateInstanceand thecdac_reader_initentrypointcdac_reader.h/cdac.cpp/cdacstress.cppto pass an optionalalloc_virtualcallbackRuntime
JITNotification+ globalsJITNotificationTable,JITNotificationTableSizeCDAC_GLOBAL_CONTRACT(CodeNotifications, c1)InitializeJITNotificationTable— default-construction zeroes slot 0'smethodToken(length) naturally, and capacity lives in the globalContract Implementation
ICodeNotificationsabstraction andCodeNotifications_1implementationData.JITNotificationIData class with typed read/write helpersLegacy COM Wrappers
IXCLRDataMethodDefinition::GetCodeNotification/SetCodeNotificationonClrDataMethodDefinitionIXCLRDataProcess::SetCodeNotifications/GetCodeNotifications/SetAllCodeNotificationsonSOSDacImplCLRDATA_METHNOTIFY_GENERATED/DISCARDED)SetCodeNotificationfrom theLegacyFallbackHelperallowlistTests
NotificationsTests, 16 table-ops tests inCodeNotificationsTests)TestPlaceholderTargetextended with anAllocateMemoryDelegatehook for lazy-allocation testsDocumentation
docs/design/datacontracts/CodeNotifications.mdwith APIs, data descriptors, and pseudocodedocs/design/datacontracts/Notifications.mdto event decoding and cross-linked toCodeNotifications.mdFixes #126760