Skip to content

Drop support for IID_ICorDebugProcess10 and fix thread suspend logic - #44549

Merged
sdmaclea merged 2 commits into
dotnet:masterfrom
sdmaclea:createPinnedHandle
Nov 11, 2020
Merged

Drop support for IID_ICorDebugProcess10 and fix thread suspend logic#44549
sdmaclea merged 2 commits into
dotnet:masterfrom
sdmaclea:createPinnedHandle

Conversation

@sdmaclea

@sdmacleasdmaclea commented Nov 11, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

@sdmaclea
Stop providing IID_ICorDebugProcess10
7d04d12
Prevent older VS versions from setting managed data breakpoints.
@sdmaclea
Simplify the thread collision logic to prevent deadlock
367337c
This is a simplification of dotnet#44539
as proposed by @kouvel
@sdmacleasdmaclea added this to the 6.0.0 milestone Nov 11, 2020
@sdmacleasdmaclea self-assigned this Nov 11, 2020
@ghost

ghost commented Nov 11, 2020

Copy link
Copy Markdown

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Details

Issue meta data
Issue content:
Issue author:sdmaclea
Assignees:sdmaclea
Milestone:[object Object]

sdmaclea
sdmaclea commented Nov 11, 2020
View reviewed changes
Comment threadsrc/coreclr/src/vm/threadsuspend.cpp
//
|| (CORDebuggerAttached() &&
(g_pDebugInterface->ThreadsAtUnsafePlaces() || g_pDebugInterface->IsSynchronizing()))
|| (CORDebuggerAttached() && g_pDebugInterface->ThreadsAtUnsafePlaces())

@sdmacleasdmacleaNov 11, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a simplification of @kouvel's change as proposed in #44539 (comment)

@sdmacleasdmaclea mentioned this pull request Nov 11, 2020
Closed
noahfalk
noahfalk approved these changes Nov 11, 2020
View reviewed changes

@noahfalknoahfalk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM : )

@hoyosjs

hoyosjs commented Nov 11, 2020

Copy link
Copy Markdown
Member

@chuckries So going forward:

New versions of a debugger will just use the new mechanism for 5.0.1+ and notifications for 3.1 and below, where there is no risk of deadlocks?

On older versions 6.0+ will fail the QI so they just can't use DBP, 5.0.0 will potentially deadlock, 5.0.1 should probably also remove the QI to make it possible to just have the behavior in a good state for 5.0.x, and 3.1 work as usual.

Is this correct?

If so, this PR looks good to me

@sdmaclea

sdmaclea commented Nov 11, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@hoyosjs Yes the plan is to back port this and #44549 to .NET 5.0.x I didn't include the more general clean up to remove GC Event notifications in this PR to make it easier to get through servicing.

@kouvel

kouvel commented Nov 11, 2020

Copy link
Copy Markdown
Contributor

If an unpatched VS tries to add a data breakpoint to a patched runtime, would it fail?

@kouvel

kouvel commented Nov 11, 2020

Copy link
Copy Markdown
Contributor

Oh I see it would look like the patched runtime does not support DBs, think I got it now. LGTM too, thanks!

@sdmaclea
sdmaclea merged commit c8c0790 into dotnet:masterNov 11, 2020
sdmaclea added a commit to sdmaclea/runtime that referenced this pull request Nov 11, 2020
@sdmaclea
Drop support for IID_ICorDebugProcess10 and fix thread suspend logic (d…
bff12e4
…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
@sdmacleasdmaclea mentioned this pull request Nov 11, 2020
Merged
sdmaclea added a commit that referenced this pull request Nov 24, 2020
@sdmaclea
[release/5.0] Port Debugger data breakpoint deadlock fixes to .NET 5.0 (
ca4d6d1
#44563)
* Add ICorDebugHeapValue4 -- CreatePinnedHandle (#44471)
* Add ICorDebugHeapValue4
* Add EnableGCNotificationEvents deprecation comment
* Drop support for IID_ICorDebugProcess10 and fix thread suspend logic (#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 #44539
as proposed by @kouvel
@ghostghost locked as resolved and limited conversation to collaborators Dec 12, 2020
@sdmaclea
sdmaclea deleted the createPinnedHandle branch June 10, 2021 00:31
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@noahfalknoahfalknoahfalk approved these changes
@hoyosjshoyosjshoyosjs approved these changes
@kouvelkouvelAwaiting requested review from kouvel
+1 more reviewer
@chuckrieschuckrieschuckries approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@sdmacleasdmaclea

Labels

area-Diagnostics-coreclr

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@sdmaclea@hoyosjs@kouvel@noahfalk@chuckries