You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains 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
When suspending for the debugger is in progress (the debugger is waiting for some threads to reach a safe point for suspension), a thread that is not yet suspended may trigger another runtime suspension due to triggering a GC from allocation or for deleting call counters. Suspending the runtime at that point is currently not allowed because the order of operations conflicts with requirements to send GC events for managed data breakpoints to work correctly. Instead, the thread suspends for the debugger first, and after the runtime is resumed by the debugger, continues suspending for the other reason.
However, the thread may also be in a forbid-suspend-for-debugger region since it holds the slot backpatching lock and suspending for the debugger at that point could cause FuncEvals to deadlock
So the thread is not allowed to suspend for the debugger and not allowed to suspend for the other reason, and the runtime deadlocks
The plan is to change managed data breakpoints to pin objects instead of using GC events to track object relocation, and to deprecate the GC events APIs. This change adds an API to pin an object, breaks the deadlock in thread suspension, and fails an attempt to QI the interface containing the API to enable GC events, ensuring that it would not be used. An update to VS would follow, to use pinning for managed data breakpoints.
Customer Impact
The .NET 5.0 release contained a potential deadlock issue when the debugger was attached. The deadlock was rare, but permanently deadlocked the target process
When VS breaks a process for managed debugging at an arbitrary time, if it happens during a very short window of time, the target process deadlocks and the only resolution is to kill the process
A workaround is to disable tiering and profiler-rejit-on-attach when debugging. This has to be done when starting the process, so it would not be usable when attaching to an already-running process with those features enabled.
Regression?
Yes, from 3.1, which used a different solution for preventing FuncEvals from deadlocking on the slot backpatching lock. That solution was problematic for VS debugger tests and had worse diagnostic experience in more situations since FuncEvals would not be allowed to run.
Testing
Induced the problem by triggering a GC and suspending the runtime for deleting call counts, and breaking the process from VS in the short timing window where this issue would occur
Verified that the runtime no longer deadlocks after this change
Verified that trying to add a managed data breakpoint to a patched runtime with an unpatched VS shows an error indicating that the runtime does not support managed data breakpoints
@chuckries smoke tested the fix, and things look good for VS w.r.t. the runtime: We are able to create pinned handles, avoid turning on GC events, and everything appears to work as expected. I think the PR is good to complete.
Risk
This is a minimal risk patch.
The patch removes an COM interface which indicates support for managed data breakpoints to older VS debuggers.
It adds a new COM interface which will allow new VS debuggers to support managed data breakpoints.
Then simplifies the debugger suspension logic to fix the deadlock.
Includes updated precompiled MIDL (with whitespace changes introduced by the MIDL compiler).
…otnet#44549)
* Stop providing IID_ICorDebugProcess10
Prevent older VS versions from setting managed data breakpoints.
* Simplify the thread collision logic to prevent deadlock
This is a simplification of dotnet#44539
as proposed by @kouvel
The .NET 5.0 release contained a potential deadlock issue when the debugger was attached. The deadlock was rare, but killed the target process
Testing
@kouvel@chuckries Please update any testing that has occurred, change this from a draft and add the servicing consider label.
Risk
This is a minimal risk patch. The patch removes an COM interface which indicates support for managed data breakpoints to older VS debuggers. It adds a new COM interface which will allow new VS debuggers to support managed data breakpoints.
danmoseley
changed the title
Port Debugger data breakpoint deadlock fixes to .NET 5.0
[release/5.0] Port Debugger data breakpoint deadlock fixes to .NET 5.0
Nov 17, 2020
Sure. I might have neglected to mention that a lot of this is machine generated core from the MIDL compiler. I left the whitespace as MIDL generated it. The change set looks substantially smaller with ignore whitespace.
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.
danmoseley
changed the title
jeffschwMSFT
added
the
Servicing-consider
sdmaclea
added
Servicing-approved
ghost
locked as resolved and limited conversation to collaborators
Backport of #44471 & #44549 to release/5.0
Customer Impact
Regression?
Testing
Risk
This is a minimal risk patch.
CC @tommcdon @noahfalk @kouvel @chuckries @jeffschwMSFT