Skip to content

[DAC] Properly dereference internal_root_array - #119396

Merged
max-charlamb merged 3 commits into
dotnet:mainfrom
max-charlamb:dac-internal-root-array
Sep 9, 2025
Merged

[DAC] Properly dereference internal_root_array#119396
max-charlamb merged 3 commits into
dotnet:mainfrom
max-charlamb:dac-internal-root-array

Conversation

@max-charlamb

@max-charlambmax-charlamb commented Sep 5, 2025

Copy link
Copy Markdown
Member

Found while working on #119324

GetHeapAnalyzeStaticData is inconsistent with its pair GetHeapAnalyzeData. The former returns a uint8_t*** whereas the latter returns a uint8_t**. I checked the usage of these APIs in clrmd and it expects both to return the same format.

// gcpriv.h - declarationclassgc_heap
{
...
PER_HEAP_FIELD_DIAG_ONLYuint8_t** internal_root_array;
PER_HEAP_FIELD_DIAG_ONLYsize_t internal_root_array_index;
PER_HEAP_FIELD_DIAG_ONLYBOOL heap_analyze_success;
...
}// gc.cppPopulateDacVars(...)
{
...
gcDacVars->internal_root_array = &gc_heap::internal_root_array;
gcDacVars->internal_root_array_index = &gc_heap::internal_root_array_index;
gcDacVars->heap_analyze_success = &gc_heap::heap_analyze_success;
...
}

I modified GetHeapAnalyzeStaticData to dereference the g_gcDacGlobals->internal_root_array matching GetHeapAnalyzeData.

dotnet/diagnostics#5562 adds a test FindRootsOlderGeneration which verifies this behaves correctly. The test fails in the current main branch.
Pipeline with fixes: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1143226&view=results

CopilotAI review requested due to automatic review settings September 5, 2025 16:10

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 fixes a data access consistency issue in the DAC (Data Access Component) by properly dereferencing the internal_root_array pointer in GetHeapAnalyzeStaticData. The change ensures that both GetHeapAnalyzeStaticData and GetHeapAnalyzeData return the same data format (uint8_t** instead of uint8_t***), which is expected by external tools like ClrMD.

  • Adds proper dereferencing of g_gcDacGlobals->internal_root_array to match the expected return type
  • Ensures consistency between GetHeapAnalyzeStaticData and GetHeapAnalyzeData APIs
  • Fixes compatibility with external diagnostic tools that expect uniform data formats

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@noahfalknoahfalk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@max-charlamb

Copy link
Copy Markdown
MemberAuthor

/ba-g test failures unrelated to DAC change

@max-charlamb
max-charlamb merged commit b6127f9 into dotnet:mainSep 9, 2025
98 of 101 checks passed
@max-charlamb
max-charlamb deleted the dac-internal-root-array branch September 9, 2025 15:51
@max-charlamb

Copy link
Copy Markdown
MemberAuthor

/backport to release/10.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/10.0: https://github.com/dotnet/runtime/actions/runs/17588280032

@max-charlambmax-charlamb mentioned this pull request Sep 10, 2025
9 tasks
max-charlamb added a commit to dotnet/diagnostics that referenced this pull request Sep 26, 2025
* Adds new DumpGCData test to verify bug fixed in dotnet/runtime#119393
* Adds new FindRootsOlderGeneration to verify bug fixed in dotnet/runtime#119396
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Oct 10, 2025
Sign up for freeto 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

@max-charlamb@noahfalk