Remove enter-managed chain code#127978
Open
rcj1 wants to merge 1 commit intodotnet:mainfrom
Open
Conversation
Contributor
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the “enter-managed chain” plumbing from the CoreCLR debugging stack-walk path, including related flags/fields that were used to represent or special-case CHAIN_ENTER_MANAGED and its “quick unwind” register-set behavior.
Changes:
- Removed CHAIN_ENTER_MANAGED fabrication/tracking on the EE stackwalker side and updated comments/consumers accordingly.
- Removed chain/quick-unwind payload fields from the IPC stackwalk frame data and corresponding RS types (register set / native frame).
- Simplified shim stackwalk chain construction to no longer inject enter-managed chains, and updated internal state tracking.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/debug/inc/dbgipcevents.h | Removes stackwalk “chain” payload from DebuggerIPCE_STRData (enum value + union members). |
| src/coreclr/debug/ee/frameinfo.h | Drops quickUnwind field and InitForEnterManagedChain declaration. |
| src/coreclr/debug/ee/frameinfo.cpp | Removes enter-managed chain emission logic and updates CHAIN_ENTER_MANAGED commentary. |
| src/coreclr/debug/ee/debugger.cpp | Updates thread-starter comment after enter-managed chain removal. |
| src/coreclr/debug/ee/controller.cpp | Removes special-case skip for CHAIN_ENTER_MANAGED during controller stack walk. |
| src/coreclr/debug/di/shimstackwalk.cpp | Stops injecting CHAIN_ENTER_MANAGED in shim chain building; adjusts managed-context tracking. |
| src/coreclr/debug/di/shimpriv.h | Renames/repurposes shim chain state flag used for managed CONTEXT capture. |
| src/coreclr/debug/di/rsthread.cpp | Removes chain-reason parameter from register-set creation and quick-unwind propagation. |
| src/coreclr/debug/di/rsstackwalk.cpp | Stops passing “quickly unwound” flag when constructing CordbNativeFrame. |
| src/coreclr/debug/di/rsregsetcommon.cpp | Removes quick-unwind parameter/state from CordbRegisterSet construction. |
| src/coreclr/debug/di/rspriv.h | Updates type/member declarations to remove quick-unwind fields/parameters. |
| src/coreclr/debug/di/i386/cordbregisterset.cpp | Removes quick-unwind-based register availability/argument validation gating. |
| src/coreclr/debug/di/amd64/cordbregisterset.cpp | Removes quick-unwind-based register availability/argument validation gating. |
| src/coreclr/debug/daccess/dacdbiimplstackwalk.cpp | Removes now-dead initialization of the removed quicklyUnwound payload. |
Copilot's findings
- Files reviewed: 14/14 changed files
- Comments generated: 3
Comment on lines
1104
to
1109
|
|
||
| DT_CONTEXT * pChainContext = NULL; | ||
| pChainInfo->m_fLeafManagedContextIsValid = false; | ||
| if (fManagedChain) | ||
| { | ||
| // The chain to be added is managed itself. So we don't need to send an enter-managed chain. | ||
| pChainInfo->m_fNeedEnterManagedChain = false; | ||
| pChainContext = &(pChainInfo->m_leafManagedContext); |
Comment on lines
676
to
688
| // This is a helper class used to store the information of a chain during a stackwalk. A chain is marked | ||
| // by the CONTEXT on the leaf boundary and a FramePointer on the root boundary. Also, notice that we | ||
| // are keeping two CONTEXTs. This is because some chain types may cancel a previous unmanaged chain. | ||
| // For example, a CHAIN_FUNC_EVAL chain cancels any CHAIN_ENTER_UNMANAGED chain immediately preceding | ||
| // it. In this case, the leaf boundary of the CHAIN_FUNC_EVAL chain is marked by the CONTEXT of the | ||
| // previous CHAIN_ENTER_MANAGED, not the previous CHAIN_ENTER_UNMANAGED. | ||
| // it. In this case, the leaf boundary of the CHAIN_FUNC_EVAL chain is marked by the CONTEXT saved | ||
| // when the first managed frame was encountered. | ||
| // | ||
|
|
||
| struct ChainInfo | ||
| { | ||
| public: | ||
| ChainInfo() : m_rootFP(LEAF_MOST_FRAME), m_reason(CHAIN_NONE), m_fNeedEnterManagedChain(FALSE), m_fLeafNativeContextIsValid(FALSE) {} | ||
| ChainInfo() : m_rootFP(LEAF_MOST_FRAME), m_reason(CHAIN_NONE), m_fLeafManagedContextIsValid(FALSE), m_fLeafNativeContextIsValid(FALSE) {} | ||
|
|
Comment on lines
1470
to
1476
| // Each interception type in the switch statement below is associated with a chain reason. | ||
| // The other chain reasons are: | ||
| // CHAIN_INTERCEPTION - not used | ||
| // CHAIN_PROCESS_START - not used | ||
| // CHAIN_THREAD_START - thread start | ||
| // CHAIN_ENTER_MANAGED - managed chain | ||
| // CHAIN_ENTER_MANAGED - not used | ||
| // CHAIN_ENTER_UNMANAGED - unmanaged chain |
Member
|
I do not have any context on this. @noahfalk ? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I think this is not needed anymore? I don't see anyone using the output in Concord. Seems to have previously been part of the V2 stack implementation there. No regressions on internal diagnostics tests.