Skip to content

Conversation

rcj1
Copy link
Contributor

@rcj1 rcj1 commented Jul 16, 2025

No description provided.

@Copilot Copilot AI review requested due to automatic review settings July 16, 2025 23:00
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds the new GetClrNotification cDAC API and its supporting global constants.

  • Implements ISOSDacInterface4.GetClrNotification in SOSDacImpl with direct reads from target memory and optional legacy cross-checks in DEBUG.
  • Defines two new global names (MaxClrNotificationArgs and ClrNotificationArguments) in the managed constants and registers them in the native descriptor header.

Reviewed Changes

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

File Description
src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.cs Added full implementation of GetClrNotification
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs Introduced MaxClrNotificationArgs and ClrNotificationArguments
src/coreclr/debug/runtimeinfo/datadescriptor.h Registered the two new globals with CDAC_GLOBAL/CDAC_GLOBAL_POINTER
Comments suppressed due to low confidence (2)

src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.cs:1776

  • This new GetClrNotification implementation lacks accompanying unit tests to verify behavior under different scenarios (e.g., when there are no notifications, at maximum argument count, and during exception paths). Adding tests in the existing test suite will help ensure correctness and prevent regressions.
    int ISOSDacInterface4.GetClrNotification(ClrDataAddress[] arguments, int count, int* pNeeded)

src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.cs:1792

  • The pointer arithmetic uses sizeof(ClrDataAddress), which may not match the actual pointer size on all architectures. Consider using the target pointer size (e.g., (ulong)IntPtr.Size) or a defined address-size constant to compute the correct offset.
                    arguments[i] = _target.Read<ulong>(basePtr.Value + (ulong)(i * sizeof(ClrDataAddress)));

Copy link
Contributor

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

@rcj1 rcj1 merged commit 078f8ca into dotnet:main Jul 24, 2025
95 of 97 checks passed
@rcj1 rcj1 deleted the GetClrNotification branch July 24, 2025 18:10
@github-actions github-actions bot locked and limited conversation to collaborators Aug 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants