-
Notifications
You must be signed in to change notification settings - Fork 5k
[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
Conversation
There was a problem hiding this 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)
LGTM |
There was a problem hiding this 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(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Tangentially related to #110758
This API allows SOS to print the exact Frame name.
Ran on CDACCompatible SOS tests locally