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

[kouvel]

1. 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. This is currently not allowed because the order of operations conflicts with requirements to send GC events for managed data breakpoints to work correctly when suspending for a GC. Instead, the thread suspends for the debugger first, and after the runtime is resumed, continues suspending for GC.
2. At the same time, if the thread that is not suspended yet is in a forbid-suspend-for-debugger region, it cannot suspend for the debugger, which conflicts with the above scenario, but is currently necessary for the issue fixed by Don't suspend for debugger while holding the slot backpatching lock #40060
3. The current plan is to change managed data breakpoints implementation to pin objects instead of using GC events to track object relocation, and to deprecate the GC events APIs
4. With that, the requirement in # 1 goes away, so this change conditions the check to avoid suspending the runtime during a pending suspension for the debugger based on whether GC events are enabled

• Verified that the latest deadlock seen in Intermittent hang/deadlock in .net core 5.0 RC1 while debugging #42375 manifests only when a data breakpoint is set and not otherwise
• Combined with Add ICorDebugHeapValue4 -- CreatePinnedHandle #44471 and a VS update to use that to switch to the pinning mechanism, the latest deadlock issue seen in Intermittent hang/deadlock in .net core 5.0 RC1 while debugging #42375 should disappear completely

[kouvel]

CC @cshung @noahfalk @sdmaclea

[sdmaclea]

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?

[chuckries]

@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.

[kouvel]

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?

[sdmaclea]

I can draft the change. See #44549

[kouvel]

Closing in favor of #44549