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

🤖 Copilot Code Review — PR #125931

Holistic Assessment

Motivation: This PR adds TryParseNotification to the cDAC Notifications contract, implementing the TranslateExceptionRecordToNotification API in the managed cDAC (SOSDacImpl). The change is well-motivated — it's replacing a void*-based forwarding stub with a proper managed implementation that parses exception records into typed notification objects and dispatches them to the appropriate IXCLRDataExceptionNotification* interface methods. The ClrDataModule.Request() method is also enhanced to handle CLRDATA_REQUEST_REVISION and DACDATAMODULEPRIV_REQUEST_GET_MODULEPTR requests with debug-mode validation against the legacy implementation.

Approach: The approach is clean and well-structured. Using C# records with an abstract base type (NotificationData) for pattern matching is idiomatic and makes the dispatch logic in SOSDacImpl easy to read. The separation between parsing (in the contract) and dispatch (in the legacy bridge) is a sound design. The internal NotificationType_1 enum properly decouples the wire format from the public API enum.

Summary: ⚠️ Needs Human Review. The implementation is generally correct and well-tested, but there are a few concerns around defensive bounds checking in the contract API, inconsistent error handling across notification types, and unchecked HRESULT returns from legacy calls that a human reviewer should evaluate.

Detailed Findings

⚠️ Missing bounds validation in TryParseNotification — could throw IndexOutOfRangeException for short spans

Notifications_1.TryParseNotification() checks exceptionInformation.IsEmpty but then directly accesses exceptionInformation[1] and exceptionInformation[2] without validating the span length. The current caller in SOSDacImpl always passes 15 elements (from EXCEPTION_RECORD64.ExceptionMaximumParameters), so this is safe today. However, the INotifications contract interface accepts an arbitrary ReadOnlySpan<TargetPointer>, meaning future callers passing fewer elements would get an unhandled IndexOutOfRangeException instead of a clean false return.

File: Notifications_1.cs:46-58

Consider adding a minimum-length check:

// Before the switch, validate we have enough elements for the notification type
if (exceptionInformation.Length < 2)
    return false;

Or validate per-branch (e.g., Jit2 and ExceptionCatcherEnter need 3 elements while others need 2).

⚠️ Inconsistent error handling when notify doesn't implement required interface versions

In TranslateExceptionRecordToNotification, the notification cases handle missing interface versions differently:

• Exception: Returns E_INVALIDARG if notify is not IXCLRDataExceptionNotification2
• GC: Silently returns S_OK if notify is not IXCLRDataExceptionNotification3
• ExceptionCatcherEnter: Silently returns S_OK if notify is not IXCLRDataExceptionNotification4
• JIT (OnCodeGenerated2): Silently skips if notify is not IXCLRDataExceptionNotification5

The JIT case is defensible (it always calls OnCodeGenerated on the base interface, and OnCodeGenerated2 is optional). But the GC and ExceptionCatcherEnter cases silently succeed without dispatching, which could mask issues. A human reviewer should verify this matches the native DAC behavior.

File: SOSDacImpl.IXCLRDataProcess.cs:556-558 vs 577-583 vs 594-596

⚠️ GetModule HRESULT return value is not checked

In the ModuleLoad and ModuleUnload notification cases, _legacyImpl.GetModule(...) is called but its return value (HRESULT) is not checked. If the module lookup fails, legacyModuleOut.Interface will be null, which is passed to ClrDataModule. While ClrDataModule accepts nullable legacy modules, silently ignoring a failed lookup could hide real issues during debugging.

File: SOSDacImpl.IXCLRDataProcess.cs:510-512, 524-526

💡 Consider at least logging the HRESULT in debug builds.

✅ Contract API design — clean use of records for pattern matching

The use of abstract record NotificationData with derived record types is well-designed. It enables clean C# pattern matching in the dispatch logic and follows modern C# idioms. The NotificationType public enum provides a discriminator while the record hierarchy carries type-specific data. XML doc comments are present on all public types.

✅ COM object lifetime management is correct

The UniqueComInterfaceMarshaller<IXCLRDataExceptionNotification> with ComObject.FinalRelease() in the finally block correctly handles the stack-allocated COM object lifetime. The comment explains the rationale well. All return statements are inside the try block, so the finally always executes.

✅ Test coverage is good

NotificationsTests.cs covers all 6 notification types, empty span, unknown types (including legacy JIT_NOTIFICATION = 3), and both supported/unsupported GC events. Tests use [Theory] with [InlineData] for the unknown-type case, and specific [Fact] methods for each notification type with exact value assertions.

✅ ClrDataModule.Request() refactoring

The Request() method is properly refactored to handle CLRDATA_REQUEST_REVISION and DACDATAMODULEPRIV_REQUEST_GET_MODULEPTR requests natively, with debug-mode byte-for-byte validation against the legacy implementation. The validation is correctly moved to the outer Request() scope so it covers all request types uniformly.

💡 Duplicated legacy module retrieval code

The ModuleLoad and ModuleUnload cases contain identical code for retrieving the legacy module. This could be extracted into a small local function to reduce duplication. Minor style concern — not blocking.

File: SOSDacImpl.IXCLRDataProcess.cs:506-514 and 518-528

💡 Documentation matches implementation

Notifications.md accurately documents the new TryParseNotification API, all notification types with correct internal enum values, and the parsing pseudocode. The documentation and implementation are consistent.

Generated by Code Review for issue #125931 · ◷