Skip to content

[cDAC] Implement ISOSDacInterface::GetFrameName #113769

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 27, 2025

Conversation

max-charlamb
Copy link
Contributor

@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

@@ -15,6 +15,7 @@ public interface IStackWalk : IContract
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
Contributor 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
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