Skip to content
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.

Sign up for GitHub

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

Allow suspending the runtime while suspension for the debugger is pending in some cases #44539

Closed
wants to merge 1 commit into from
Closed

Allow suspending the runtime while suspension for the debugger is pending in some cases #44539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/coreclr/src/debug/ee/debugger.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2187,6 +2187,13 @@ class Debugger : public DebugInterface
unsigned int errorLine,
bool exitThread);

virtual BOOL AreGarbageCollectionEventsEnabledForNextSuspension(void)
{
LIMITED_METHOD_CONTRACT;

return m_isGarbageCollectionEventsEnabledLatch;
}

virtual BOOL IsSynchronizing(void)
{
LIMITED_METHOD_CONTRACT;
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/src/vm/dbginterface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,7 @@ class DebugInterface
virtual void SuspendForGarbageCollectionStarted() = 0;
virtual void SuspendForGarbageCollectionCompleted() = 0;
virtual void ResumeForGarbageCollectionStarted() = 0;
virtual BOOL AreGarbageCollectionEventsEnabledForNextSuspension() = 0;
#endif
virtual BOOL IsSynchronizing() = 0;
};
Expand Down
23 changes: 17 additions & 6 deletions src/coreclr/src/vm/threadsuspend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5946,13 +5946,24 @@ void ThreadSuspend::SuspendEE(SUSPEND_REASON reason)
//
// When the debugger is synchronizing, trying to perform a GC could deadlock. The GC has the
// threadstore lock and synchronization cannot complete until the debugger can get the
// threadstore lock. However the GC can not complete until it sends the BeforeGarbageCollection
// event, and the event can not be sent until the debugger is synchronized. In order to break
// this deadlock cycle the GC must give up the threadstore lock, allow the debugger to synchronize,
// then try again.
// threadstore lock. However, if GC events are enabled, the GC can not complete until it sends the
// BeforeGarbageCollection event, and the event can not be sent until the debugger is synchronized. In order to
// break this deadlock cycle the GC must give up the threadstore lock, allow the debugger to synchronize,
// then try again. At the same time, currently a thread in a forbid-suspend-for-debugger region cannot suspend for
// the debugger at this point. To break both deadlocks, sending GC events would be deprecated and typically
// AreGarbageCollectionEventsEnabledForNextSuspension() would be false.
//
|| (CORDebuggerAttached() &&
(g_pDebugInterface->ThreadsAtUnsafePlaces() || g_pDebugInterface->IsSynchronizing()))
||
(
CORDebuggerAttached() &&
(
g_pDebugInterface->ThreadsAtUnsafePlaces() ||
(
g_pDebugInterface->AreGarbageCollectionEventsEnabledForNextSuspension() &&
Copy link
Contributor

@sdmaclea sdmaclea Nov 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is adding complexity to preserve data breakpoints in VS versions without our pending fix. However for these versions the deadlock would still exist.

I wonder if the right fix is to simply never allow EnableGCNotification(true). We could do that by returning an error when when this is set to true. (Or by removing the interface)

The benefit would be that the deadlock would be fixed completely (at the sake of broken data breakpoints on non-updated VS versions). I guess to me that feels like a better solution. I would prefer a hard failure on data breakpoints, than a intermittent deadlock.

@chuckries @gregg-miskelly opinions? ramifications?

Copy link
Contributor

@chuckries chuckries Nov 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@sdmaclea @gregg-miskelly I don't have a problem with deprecating EnableGCNotifications entirely, however there is a problem in VS: It appears we never actually check the HRESULT from EnableGCNotificationEvents(...). This means that for existing version of VS, a failed return won't actually fail the data bp code path, and the thread contexts will still get updated to set the hardware data bp. Without the GC events actually enabled, these data bp's would be enabled across GC's and almost certainly cause major issues.

Data BP's could be "gracefully" disabled by failing the QI for ICorDebugProcess10. This would make VS think that the target runtime simply doesn't support data bp's. It appears that EnableGCNotificationEvents(...) is the only API available on ICorDebugProcess10, so I think this might be a valid approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

The deadlock introduced by #40060 in adding the forbid-suspend-for-debugger region is only applicable to .NET 5, for 3.1 a different solution was used and that deadlock is not applicable there. So after this change, that deadlock could still occur in .NET 5 (but not 3.1) when using a VS version that uses GC events.

Not sure I followed the above completely. If the target runtime supports data BPs, is there a suggestion for a change?

Copy link
Contributor

@sdmaclea sdmaclea Nov 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I can draft the change. See #44549

g_pDebugInterface->IsSynchronizing()
)
)
)
#endif // DEBUGGING_SUPPORTED
)
{
Expand Down