Skip to content

Conversation

rcj1
Copy link
Contributor

@rcj1 rcj1 commented Sep 12, 2025

  • Adding GetCodeHeaderData cDAC API
  • Implementing unwind data size on platforms other than x86 in helper class
  • required additions to hot/cold lookup helpers

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 adds the GetCodeHeaderData cDAC API implementation, which retrieves code header information for a given instruction pointer. The implementation includes hot/cold region detection and unwind data size calculations across different architectures.

  • Replaces the legacy interface implementation with a proper cDAC contract-based implementation
  • Adds support for calculating unwind data sizes on all supported architectures (X64, ARM, ARM64, LoongArch64, RiscV64)
  • Implements hot/cold method region detection and size calculation through execution manager extensions

Reviewed Changes

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

Show a summary per file
File Description
SOSDacImpl.cs Implements GetCodeHeaderData with proper error handling and debug validation
ISOSDacInterface.cs Adds strongly-typed DacpCodeHeaderData struct and JITTypes enum
UnwindInfo.cs Extends UnwindInfo data class to support unwind codes on non-x86 platforms
UnwindDataSize.cs New helper class for calculating unwind data sizes across all architectures
HotColdLookup.cs Adds method to retrieve hot/cold region boundaries
ExecutionManager files Adds new APIs for method region info, JIT type detection, and non-virtual entry lookup
datadescriptor.inc Updates UnwindInfo and adds UnwindCode type definitions
ExecutionManager.md Documents the new ExecutionManager APIs and their implementation details

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
Copy link
Contributor Author

rcj1 commented Sep 12, 2025

Separate PR idea: more detailed hot/cold map documentation? @elinor-fung

if (/* no corresponding range section */)
return null;

if (/* range flags indicate RangeList */)
Copy link
Contributor Author

@rcj1 rcj1 Sep 12, 2025

Choose a reason for hiding this comment

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

@elinor-fung @jkotas any way to simplify this, can these checks be absorbed into GetMethodDescFromStubAddress?

In any case I think we need the capability to turn this additional search on and off because SOS uses the failure case of GetMethodDescPtrFromIP to indicate some things about the type of method we are looking at. Here is an (undocumented) example but there are more:

https://github.com/dotnet/diagnostics/blob/23d4e5f6eadc48f12ce8cda6174e3a8b85e3c638/src/SOS/Strike/strike.cpp#L9262

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to have two different non-legacy methods:

  • API that takes IP that points to the start or into middle of an actual method code, and maps it MethodDesc, hot/cold region sizes, GCInfo, etc. GetMethodRegionInfo looks close.

  • API that take method entrypoint (that may not actually be a pointer to the actual code in some configuration - see my other comment), and maps it to MethodDesc. Equivalent of runtime's NonVirtualEntry2MethodDesc.

The legacy methods can be hopefully implemented using these two APIs in a compatible-enough way.

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.

It looks like our method for computing method size (summing RUNTIME_FUNCTIONs lengths) is different than the runtimes (reading the GCInfo). Will this always yield the same result?

Comment on lines 59 to 65
private enum JITTypes
{
TYPE_UNKNOWN = 0,
TYPE_JIT = 1,
TYPE_PJIT = 2,
TYPE_INTERPRETER = 3
};
Copy link
Member

Choose a reason for hiding this comment

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

We can make this public in IExecutionManager and change the return type of GetJitType to JitType.

Comment on lines 596 to 607
if (codeBlockHandle == null)
{
TargetPointer methodDesc = executionManager.NonVirtualEntry2MethodDesc(targetCodePointer);
if (methodDesc == TargetPointer.Null)
throw new ArgumentException();
data->MethodDescPtr = methodDesc.ToClrDataAddress(_target);
data->JITType = JITTypes.TYPE_UNKNOWN;
data->GCInfo = 0;
data->MethodStart = 0;
data->MethodSize = 0;
data->ColdRegionStart = 0;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason that we don't set HotRegionSize or ColdRegionSize here? I see that the DAC doesn't, but that was probably an oversight and could get updated for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

It is how this API communicates that the address is a stub (most likely a precode these days) and not an actual code.

HotRegionSize/ColdRegionSize may not be even readily available for some of the stubs.

@jkotas
Copy link
Member

jkotas commented Sep 13, 2025

It looks like our method for computing method size (summing RUNTIME_FUNCTIONs lengths) is different than the runtimes (reading the GCInfo). Will this always yield the same result?

I do not think these will yield the same result. There can be padding or data that's not included in the unwind infos in between the unwind infos or at the end.

@rcj1
Copy link
Contributor Author

rcj1 commented Sep 13, 2025

It looks like our method for computing method size (summing RUNTIME_FUNCTIONs lengths) is different than the runtimes (reading the GCInfo). Will this always yield the same result?

I do not think these will yield the same result. There can be padding or data that's not included in the unwind infos in between the unwind infos or at the end.

Yes I now notice the results being different in CI with the cDAC giving significantly higher method lengths.

@rcj1 rcj1 marked this pull request as draft September 15, 2025 18:26
@rcj1
Copy link
Contributor Author

rcj1 commented Sep 15, 2025

Waiting for GetStackReferences to merge (this should include GCInfo decoding stuff)

Copy link
Contributor

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

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