[cDAC] Implement ISOSDacInterface::GetFrameName#113769
[cDAC] Implement ISOSDacInterface::GetFrameName#113769max-charlamb merged 2 commits intodotnet:mainfrom
Conversation
ghost
added
the
max-charlamb
requested review from
a team,
davidwrighton,
davmason and
steveisok
There was a problem hiding this comment.
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 |
| @@ -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.
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.
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