[cDAC] Change FrameIdentifiers to be contract literals

[max-charlamb]

Previously these were indirect pointers, this places them in the contract directly.

Indirect pointers are used so the dynamic linker can resolve the addresses. These values are known at compile time and therefore don't need the extra layer of indirection.

Additionally adds to the Target interface to allow TryReadGlobal instead of using exceptions for control-flow

[max-charlamb]

/azp run runtime-diagnostics

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[dotnet-policy-service]

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

[max-charlamb]

/azp run runtime-diagnostics

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Copilot]

Pull Request Overview

This PR updates the contract identifiers by embedding them as literals rather than using indirect pointers, and adds a TryReadGlobal method to avoid exception-driven control flow when accessing globals.

• Added TryReadGlobal overload in ContractDescriptorTarget
• Refactored FrameIterator to use TryReadGlobal instead of exception handling
• Updated the Target abstract class to include the TryReadGlobal method

Reviewed Changes

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

File	Description
src/native/managed/cdacreader/Microsoft.Diagnostics.DataContractReader/ContractDescriptorTarget.cs	Adds new TryReadGlobal method implementation
src/native/managed/cdacreader/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/FrameHandling/FrameIterator.cs	Replaces exception-based logic with a TryReadGlobal call
src/native/managed/cdacreader/Microsoft.Diagnostics.DataContractReader.Abstractions/Target.cs	Introduces an abstract TryReadGlobal method

Files not reviewed (1)

• src/coreclr/debug/runtimeinfo/datadescriptor.h: Language not supported

Comments suppressed due to low confidence (2)

src/native/managed/cdacreader/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/FrameHandling/FrameIterator.cs:137

• [nitpick] Consider caching the concatenated key (frameType.ToString() + "Identifier") in a variable to improve readability and reduce potential issues when updating the identifier naming.

if (target.TryReadGlobal(frameType.ToString() + "Identifier", out ulong? id))

src/native/managed/cdacreader/Microsoft.Diagnostics.DataContractReader/ContractDescriptorTarget.cs:499

• Consider adding unit tests for the new TryReadGlobal method to verify its behavior for both valid and invalid global names.

public override bool TryReadGlobal<T>(string name, [NotNullWhen(true)] out T? value)

[kg]

LGTM but I don't completely understand what's going on in it

[max-charlamb]

LGTM but I don't completely understand what's going on in it

There are two related changes here:

• [cDAC] Stack walk support more Frame types #112997 (comment) Cleaning up FrameIterator to not use exceptions for control-flow by implementing a 'TryGet' for global values.
• Changing the FrameIdentifiers from a 'global pointer' to a 'global literal'. FrameIdentifiers are pointer sized values that represent a specific Frame type. When I originally added them to the contract, I added them as 'global pointers' because they are pointer sized. However, this adds a layer of indirection by referencing an offset into an array of pointers. This is required for some true pointer types because the dynamic linker has to resolve their values. Because FrameIdentifiers are a compile time constant, we can shove the whole value into the contract, removing a layer of indirection.

[kg]

LGTM but I don't completely understand what's going on in it

Changing the FrameIdentifiers from a 'global pointer' to a 'global literal'. FrameIdentifiers are pointer sized values that represent a specific Frame type. When I originally added them to the contract, I added them as 'global pointers' because they are pointer sized. However, this adds a layer of indirection by referencing an offset into an array of pointers. This is required for some true pointer types because the dynamic linker has to resolve their values. Because FrameIdentifiers are a compile time constant, we can shove the whole value into the contract, removing a layer of indirection.

I had missed that these globals are populated into a cache at initialization time, so I was confused by TryReadGlobal only doing a dictionary lookup instead of a memory read from the target. It makes sense now that I know it's a pre-initialized cache. I'd prefer to see these called "global constants" or something to communicate that they aren't read on-demand from the target and are cached, but that can happen in a later PR if it happens at all.

[davmason]

This isn't introduced by your change, but this loop seems expensive for something we do for every frame. You don't need to change it now but we may want to revisit