Skip to content

Comments

Implement ISOSDacInterface::GetThreadAllocData in cDAC SOSDacImpl#124817

Open
Copilot wants to merge 8 commits intomainfrom
copilot/implement-get-thread-alloc-data
Open

Implement ISOSDacInterface::GetThreadAllocData in cDAC SOSDacImpl#124817
Copilot wants to merge 8 commits intomainfrom
copilot/implement-get-thread-alloc-data

Conversation

Copy link
Contributor

Copilot AI commented Feb 24, 2026

Description

Implements GetThreadAllocData in the cDAC SOSDacImpl, based on ClrDataAccess::GetThreadAllocData in request.cpp. This exposes per-thread GC allocation byte counts (alloc_bytes and alloc_bytes_uoh from gc_alloc_context) through the cDAC diagnostic interface.

Data descriptor & contract changes:

  • Add AllocBytes and AllocBytesLoh fields to GCAllocContext in datadescriptor.inc, mapped from gc_alloc_context::alloc_bytes and gc_alloc_context::alloc_bytes_uoh
  • Extend managed GCAllocContext data class with corresponding long properties
  • Add GetThreadAllocContext with out long allocBytes, out long allocBytesLoh parameters to IThread contract; implement in Thread_1

SOSDacImpl changes:

  • Define managed DacpAllocData struct (matches native layout: two ClrDataAddress fields)
  • Replace delegating stub with full implementation following existing patterns (parameter validation inside try/catch, throw new ArgumentException() for invalid address, throw new NullReferenceException() for null data pointer to produce E_POINTER, contract call, #if DEBUG legacy comparison)

Tests:

  • Unit tests: GetThreadAllocContext (non-zero values) and GetThreadAllocContext_ZeroValues across all 4 mock architectures
  • Dump test: walks all threads validating GetThreadAllocContext returns non-negative values
  • Extended MockDescriptors.Thread to set up RuntimeThreadLocals/EEAllocContext/GCAllocContext type hierarchy

Documentation:

  • Updated Thread.md and GC.md with new GCAllocContext field descriptors

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Copilot AI and others added 2 commits February 24, 2026 21:02
- Add AllocBytes and AllocBytesLoh fields to GCAllocContext data descriptor
- Add ThreadAllocData record struct and GetThreadAllocData to IThread contract
- Implement GetThreadAllocData in Thread_1.cs contract
- Define DacpAllocData managed struct in ISOSDacInterface.cs
- Implement GetThreadAllocData in SOSDacImpl.cs with DEBUG comparison
- Add unit tests for GetThreadAllocData with non-zero and zero values
- Add dump test for GetThreadAllocData
- Update Thread.md and GC.md documentation

Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Copilot AI changed the title [WIP] Implement GetThreadAllocData in SOSDacImpl Implement ISOSDacInterface::GetThreadAllocData in cDAC SOSDacImpl Feb 24, 2026
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.

@copilot , address feedback

…eption for E_POINTER, rename to GetThreadAllocContext

Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 24, 2026

@copilot , address feedback

All three review comments addressed in 9fbc6c9. All 840 tests pass.

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.

@copilot , address feedback

@max-charlamb max-charlamb marked this pull request as ready for review February 24, 2026 21:52
Copilot AI review requested due to automatic review settings February 24, 2026 21:52
Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Copy link
Contributor

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

Implements ISOSDacInterface::GetThreadAllocData in the cDAC-based SOSDacImpl by exposing per-thread GC allocation byte counters from gc_alloc_context through the managed contracts and data descriptors.

Changes:

  • Added AllocBytes / AllocBytesLoh fields to the GCAllocContext data descriptor and managed data model.
  • Extended the IThread contract with GetThreadAllocContext returning a new ThreadAllocData record, and wired it into SOSDacImpl.GetThreadAllocData.
  • Added unit + dump tests and updated mock descriptors and design docs to cover/describe the new fields.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/native/managed/cdac/tests/ThreadTests.cs Adds unit tests for GetThreadAllocContext.
src/native/managed/cdac/tests/MockDescriptors/MockDescriptors.Thread.cs Extends mock type layouts and thread creation to include alloc-context hierarchy and alloc byte values.
src/native/managed/cdac/tests/DumpTests/ThreadDumpTests.cs Adds dump test to validate alloc context can be read across all threads.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs Implements GetThreadAllocData using the IThread contract and follows existing try/catch + DEBUG-compare patterns.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ISOSDacInterface.cs Introduces DacpAllocData and updates the interface signature to use it.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/GCAllocContext.cs Reads new alloc-byte fields from the target in managed data representation.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Thread_1.cs Implements IThread.GetThreadAllocContext.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IThread.cs Adds ThreadAllocData record and the new contract method.
src/coreclr/vm/datadescriptor/datadescriptor.inc Adds GCAllocContext field descriptors for alloc byte counts.
docs/design/datacontracts/Thread.md Documents new GCAllocContext field dependencies for the Thread contract.
docs/design/datacontracts/GC.md Documents new GCAllocContext field descriptors used by GC-related contracts.

Comment on lines 126 to 136
[Theory]
[ClassData(typeof(MockTarget.StdArch))]
public void GetThreadAllocContext(MockTarget.Architecture arch)
{
long allocBytes = 1024;
long allocBytesLoh = 4096;

TargetPointer addr = thread.AddThread(1, new TargetNUInt(1234), allocBytes, allocBytesLoh);

Target target = CreateTarget(thread);
IThread contract = target.Contracts.Thread;
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

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

GetThreadAllocContext test references a thread variable that isn't declared in the method scope (unlike the other tests that create TargetTestHelpers/MockMemorySpace.Builder/MockDescriptors.Thread). This will not compile and also means the arch parameter isn't used to build the target. Create a local MockDescriptors.Thread using the provided arch before calling AddThread/CreateTarget.

Copilot uses AI. Check for mistakes.
@max-charlamb max-charlamb requested a review from rcj1 February 24, 2026 22:36
@max-charlamb
Copy link
Member

@copilot, please fix build break

…dAllocContext test and fix trailing whitespace

Co-authored-by: max-charlamb <44248479+max-charlamb@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 24, 2026

@copilot, please fix build break

Fixed in 0e5922e — the GetThreadAllocContext test was missing TargetTestHelpers/MockMemorySpace.Builder/MockDescriptors.Thread setup. Also cleaned up remaining trailing whitespace in Thread_1.cs. All 24 thread tests pass.

TargetPointer LastThrownObjectHandle,
TargetPointer NextThread);

public record struct ThreadAllocData(
Copy link
Contributor

Choose a reason for hiding this comment

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

Use out variables

Copy link
Contributor

Choose a reason for hiding this comment

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

@copilot use out variables instead of this struct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in fa4b867 — replaced ThreadAllocData record struct with out long allocBytes, out long allocBytesLoh parameters on GetThreadAllocContext, matching the pattern used by GetStackLimitData. Updated Thread_1 implementation, SOSDacImpl caller, and all tests.

…ontext

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants