-
Notifications
You must be signed in to change notification settings - Fork 5.1k
[cDAC] Implement contract APIs for x86 stack walking #115433
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
This PR implements new contract APIs for x86 stack walking focused on updating how unwind information and funclet addresses are retrieved. Key changes include:
- Removing the need for a controlPC parameter in GetUnwindInfo calls.
- Introducing a new GetFuncletStartAddress API in the ExecutionManager implementations.
- Updating the IExecutionManager interface and adding an extension IsFunclet for funclet detection.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
Unwinder.cs | Removed the controlPC parameter from the GetUnwindInfo call to align with new API design. |
ExecutionManager_2.cs | Updated API calls to reflect removal of the ip parameter and added GetFuncletStartAddress. |
ExecutionManager_1.cs | Similar update as in ExecutionManager_2.cs for consistency. |
ExecutionManagerCore.cs | Implemented GetFuncletStartAddress and modified GetUnwindInfo calls to use CodeBlockHandle.Address. |
IExecutionManager.cs | Revised interface to remove ip parameter and include GetFuncletStartAddress. |
IExecutionManagerExtensions.cs | Added a helper extension method for funclet detection. |
Comments suppressed due to low confidence (2)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/Context/Unwinder.cs:151
- Ensure that removing the controlPC parameter from the GetUnwindInfo call is intentional and that the new API behavior fully supports x86 stack walking requirements.
TargetPointer unwindInfo = eman.GetUnwindInfo(cbh);
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.cs:188
- Verify that replacing the controlPC parameter with codeInfoHandle.Address.Value in the RangeSection.Find call maintains correct unwinding behavior for x86 stack walking.
RangeSection range = RangeSection.Find(_target, _topRangeSectionMap, _rangeSectionMapLookup, codeInfoHandle.Address.Value);
Tagging subscribers to this area: @tommcdon |
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.
Looks good!
|
||
public static class IExecutionManagerExtensions | ||
{ | ||
public static bool IsFunclet(this IExecutionManager eman, CodeBlockHandle codeBlockHandle) |
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.
Unused? Plan is to make funclet available on all platforms once testing is completed #113985 (comment). cc @filipnavara
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.
The implementation looks reasonable even for FEATURE_EH_FUNCLETS
builds.
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.
These contract APIs are not used yet, but will be used as part of the x86 stack walking. That will probably be in a follow-up PR.
Closing in favor of #116072 |
#110758
Implements
CodeInfo -> Get SavedMethodCode
CodeInfo -> Get RelativeOffset
CodeInfo -> Get FuncletStartAddress
runtime/src/coreclr/vm/codeman.cpp
Lines 1080 to 1101 in 0e06c21
CodeInfo -> Get IsFunclet