Skip to content

Conversation

max-charlamb
Copy link
Member

@max-charlamb max-charlamb commented Mar 21, 2025

Tangentially related to #110758

This API allows SOS to print the exact Frame name.

Ran on CDACCompatible SOS tests locally

@Copilot Copilot AI review requested due to automatic review settings March 21, 2025 18:54
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

A PR to implement the GetFrameName API for SOS, enabling the diagnostics tool to print precise frame names.

  • Updated SOSDacImpl to include robust error checking and a new implementation for ISOSDacInterface.GetFrameName.
  • Extended FrameIterator with a new GetFrameName method and updated GetFrameType overloads to accept an explicit target parameter.
  • Introduced a new virtual GetFrameName method in the IStackWalk interface and its implementation in StackWalk_1.

Reviewed Changes

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

File Description
src/native/managed/cdacreader/src/Legacy/SOSDacImpl.cs Implements ISOSDacInterface.GetFrameName with error-checking, exception handling, and a debug comparison block.
src/native/managed/cdacreader/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/FrameIterator.cs Adds a new public static GetFrameName method, updates GetFrameType usage with explicit target parameter, and adjusts inline call frame checks.
src/native/managed/cdacreader/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IStackWalk.cs Introduces a new virtual GetFrameName method to the interface.
src/native/managed/cdacreader/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/StackWalk_1.cs Implements the new IStackWalk.GetFrameName by delegating to the FrameIterator.
Comments suppressed due to low confidence (2)

src/native/managed/cdacreader/src/Legacy/SOSDacImpl.cs:169

  • Consider logging or capturing additional details from the caught exception before returning its HResult to help with future debugging.
catch (System.Exception ex)

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

  • [nitpick] Returning an empty string for unknown frame types might be ambiguous; consider using a distinct indicator or documented constant to clearly differentiate a lack of frame name from a valid empty name.
public static string GetFrameName(Target target, TargetPointer frameIdentifier)

@max-charlamb max-charlamb changed the title [cDAC} Implement ISOSDacInterface::GetFrameName [cDAC] Implement ISOSDacInterface::GetFrameName Mar 21, 2025
@kg
Copy link
Member

kg commented Mar 21, 2025

LGTM

Copy link
Contributor

@davmason davmason left a comment

Choose a reason for hiding this comment

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

LGTM

{
static string IContract.Name => nameof(StackWalk);

public virtual IEnumerable<IStackDataFrameHandle> CreateStackWalk(ThreadData threadData) => throw new NotImplementedException();
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 interface members are public by default aren't they? So you can leave off the 'public', I think.

I'm also not sure whether you need the 'virtual' since these appear to be DIMs, I know interface methods are usually virtual but providing a body might make them nonvirtuals

Copy link
Member Author

Choose a reason for hiding this comment

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

They are public by default. My understanding is that the virtual makes a small difference in allowing inherited classes to override the functions, as they are virtual, but this can also be marked in the implementing class/struct.

I took a look, and we are inconsistent across the cDAC contracts. I'll remove public virtual from these and work towards unifying on that.

@max-charlamb max-charlamb merged commit 0b6e62e into dotnet:main Mar 27, 2025
146 of 149 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 2025
@max-charlamb max-charlamb deleted the cdac-frame-name branch June 10, 2025 16:37
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