Skip to content

Conversation

rcj1
Copy link
Contributor

@rcj1 rcj1 commented Jul 28, 2025

  • This PR implements the Write and WriteBuffer methods to write an arbitrary value/buffer respsectively to the data target in the cDAC.
  • Added a unit test to test writing.
  • Implemented SetOtherNotificationFlags cDAC API which requires writing.

rcj1 added 2 commits July 28, 2025 13:19
* implementing SetOtherNotificationFlags cDAC API
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

This PR implements write capabilities and the SetOtherNotificationFlags cDAC API by adding write methods to the data contract reader framework and implementing a specific notification flags API that requires write access.

Key changes:

  • Added Write and WriteBuffer methods to the Target abstraction and ContractDescriptorTarget implementation
  • Implemented SetOtherNotificationFlags cDAC API with proper validation and write functionality
  • Enhanced mock testing infrastructure to support write operations

Reviewed Changes

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

Show a summary per file
File Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Target.cs Added abstract Write and WriteBuffer method declarations to the Target base class
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/ContractDescriptorTarget.cs Implemented write methods, added WriteToTargetDelegate, and refactored to use DataTargetDelegates
src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.IXCLRDataProcess.cs Implemented SetOtherNotificationFlags method with validation and write operations
src/native/managed/cdac/mscordaccore_universal/Legacy/ISOSDacInterface.cs Added CLRDataOtherNotifyFlag enum for notification flag constants
src/native/managed/cdac/mscordaccore_universal/Entrypoints.cs Updated entry points to accept and use write delegate function pointers
src/coreclr/debug/daccess/cdac.cpp Added WriteToTargetCallback function and updated initialization call
src/native/managed/cdac/inc/cdac_reader.h Updated function signature to include write_to_target parameter
src/native/managed/cdac/tests/MockMemorySpace.cs Added WriteContext class with WriteToTarget implementation for testing
src/native/managed/cdac/tests/ContractDescriptor/ContractDescriptorBuilder.cs Updated to use both read and write contexts for target creation
src/native/managed/cdac/tests/ContractDescriptor/TargetTests.cs Added WriteValue test to verify write functionality
src/native/managed/cdac/tests/TestPlaceholderTarget.cs Added WriteBuffer and Write method stubs that throw NotImplementedException
src/tools/StressLogAnalyzer/src/Program.cs Added write delegate that throws NotImplementedException for read-only tool

Copy link
Contributor

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

@max-charlamb
Copy link
Member

Have you tested this API on a dump (where the write should fail)? We should verify that the failure happens in a handeled and expected way.

@rcj1
Copy link
Contributor Author

rcj1 commented Jul 29, 2025

Have you tested this API on a dump (where the write should fail)? We should verify that the failure happens in a handeled and expected way.

I'm working on it

@rcj1
Copy link
Contributor Author

rcj1 commented Jul 29, 2025

Have you tested this API on a dump (where the write should fail)? We should verify that the failure happens in a handeled and expected way.

The cDAC behavior matches that of the DAC; Windbg allows writes to dumps, while dotnet-dump does not (quiet failure from the end user's perspective).

Copy link
Member

@max-charlamb max-charlamb left a comment

Choose a reason for hiding this comment

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

left a couple of comments, but generally looks good to me

@rcj1 rcj1 merged commit ac764af into dotnet:main Jul 30, 2025
95 of 97 checks passed
@rcj1 rcj1 deleted the SetOtherNotificationFlags branch July 30, 2025 23:54
@github-actions github-actions bot locked and limited conversation to collaborators Aug 30, 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.

3 participants