Skip to content

Commit

Permalink
[release/5.0] Port Debugger data breakpoint deadlock fixes to .NET 5.0 (
Browse files Browse the repository at this point in the history 
#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
  • Loading branch information
@sdmaclea
sdmaclea authored Nov 24, 2020
1 parent 3700448 commit ca4d6d1
Show file tree
Hide file tree
Showing 9 changed files with 5,447 additions and 5,245 deletions.
78 changes: 70 additions & 8 deletions src/coreclr/src/debug/di/divalue.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,6 @@ HRESULT CordbValue::InternalCreateHandle(CorDebugHandleType handleType,

DebuggerIPCEvent event;
CordbProcess *process;
BOOL fStrong = FALSE;

// @dbgtodo- , as part of inspection, convert this path to throwing.
if (ppHandle == NULL)
Expand All @@ -431,13 +430,14 @@ HRESULT CordbValue::InternalCreateHandle(CorDebugHandleType handleType,

*ppHandle = NULL;

if (handleType == HANDLE_STRONG)
switch (handleType)
{
fStrong = TRUE;
}
else
{
_ASSERTE(handleType == HANDLE_WEAK_TRACK_RESURRECTION);
case HANDLE_STRONG:
case HANDLE_WEAK_TRACK_RESURRECTION:
case HANDLE_PINNED:
break;
default:
return E_INVALIDARG;
}


Expand All @@ -460,7 +460,7 @@ HRESULT CordbValue::InternalCreateHandle(CorDebugHandleType handleType,

CORDB_ADDRESS addr = GetValueHome() != NULL ? GetValueHome()->GetAddress() : NULL;
event.CreateHandle.objectToken = CORDB_ADDRESS_TO_PTR(addr);
event.CreateHandle.fStrong = fStrong;
event.CreateHandle.handleType = handleType;

// Note: two-way event here...
HRESULT hr = process->SendIPCEvent(&event, sizeof(DebuggerIPCEvent));
Expand Down Expand Up @@ -1827,6 +1827,10 @@ HRESULT CordbObjectValue::QueryInterface(REFIID id, void **pInterface)
{
*pInterface = static_cast<ICorDebugHeapValue3*>(this);
}
else if (id == IID_ICorDebugHeapValue4)
{
*pInterface = static_cast<ICorDebugHeapValue4*>(this);
}
else if ((id == IID_ICorDebugStringValue) &&
(m_info.objTypeData.elementType == ELEMENT_TYPE_STRING))
{
Expand Down Expand Up @@ -1963,6 +1967,21 @@ HRESULT CordbObjectValue::CreateHandle(
return CordbValue::InternalCreateHandle(handleType, ppHandle);
} // CreateHandle

/*
* Creates a pinned handle for this heap value.
*
* Not Implemented In-Proc.
*/
HRESULT CordbObjectValue::CreatePinnedHandle(
ICorDebugHandleValue ** ppHandle)
{
PUBLIC_API_ENTRY(this);
FAIL_IF_NEUTERED(this);
ATT_REQUIRE_STOPPED_MAY_FAIL(GetProcess());

return CordbValue::InternalCreateHandle(HANDLE_PINNED, ppHandle);
} // CreatePinnedHandle

// Get class information for this object
// Arguments:
// output: ppClass - ICDClass instance for this object
Expand Down Expand Up @@ -3325,6 +3344,10 @@ HRESULT CordbBoxValue::QueryInterface(REFIID id, void **pInterface)
{
*pInterface = static_cast<ICorDebugHeapValue3*>(this);
}
else if (id == IID_ICorDebugHeapValue4)
{
*pInterface = static_cast<ICorDebugHeapValue4*>(this);
}
else if (id == IID_IUnknown)
{
*pInterface = static_cast<IUnknown*>(static_cast<ICorDebugBoxValue*>(this));
Expand Down Expand Up @@ -3387,6 +3410,24 @@ HRESULT CordbBoxValue::CreateHandle(
return CordbValue::InternalCreateHandle(handleType, ppHandle);
} // CordbBoxValue::CreateHandle

// Creates a pinned handle for this heap value.
// Not Implemented In-Proc.
// Create a handle for a heap object.
// @todo: How to prevent this being called by non-heap object?
// Arguments:
// output: ppHandle - on success, the newly created handle
// Return Value: S_OK on success or E_INVALIDARG, E_OUTOFMEMORY, or CORDB_E_HELPER_MAY_DEADLOCK
HRESULT CordbBoxValue::CreatePinnedHandle(
ICorDebugHandleValue ** ppHandle)
{
PUBLIC_API_ENTRY(this);
FAIL_IF_NEUTERED(this);
ATT_REQUIRE_STOPPED_MAY_FAIL(GetProcess());

return CordbValue::InternalCreateHandle(HANDLE_PINNED, ppHandle);
} // CreatePinnedHandle


HRESULT CordbBoxValue::GetValue(void *pTo)
{
// Can't get a whole copy of a box.
Expand Down Expand Up @@ -3565,6 +3606,10 @@ HRESULT CordbArrayValue::QueryInterface(REFIID id, void **pInterface)
{
*pInterface = static_cast<ICorDebugHeapValue3*>(this);
}
else if (id == IID_ICorDebugHeapValue4)
{
*pInterface = static_cast<ICorDebugHeapValue4*>(this);
}
else if (id == IID_IUnknown)
{
*pInterface = static_cast<IUnknown*>(static_cast<ICorDebugArrayValue*>(this));
Expand Down Expand Up @@ -3888,6 +3933,23 @@ HRESULT CordbArrayValue::CreateHandle(
return CordbValue::InternalCreateHandle(handleType, ppHandle);
} // CordbArrayValue::CreateHandle

/*
* Creates a pinned handle for this heap value.
* Not Implemented In-Proc.
* Arguments:
* output: ppHandle - on success, the newly created handle
* Return Value: S_OK on success or E_INVALIDARG, E_OUTOFMEMORY, or CORDB_E_HELPER_MAY_DEADLOCK
*/
HRESULT CordbArrayValue::CreatePinnedHandle(
ICorDebugHandleValue ** ppHandle)
{
PUBLIC_API_ENTRY(this);
FAIL_IF_NEUTERED(this);
ATT_REQUIRE_STOPPED_MAY_FAIL(GetProcess());

return CordbValue::InternalCreateHandle(HANDLE_PINNED, ppHandle);
} // CreatePinnedHandle

// get a copy of the array
// Arguments
// output: pTo - pointer to a caller-allocated and managed buffer to hold the copy. The caller must guarantee
Expand Down
4 changes: 0 additions & 4 deletions src/coreclr/src/debug/di/process.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2169,10 +2169,6 @@ HRESULT CordbProcess::QueryInterface(REFIID id, void **pInterface)
{
*pInterface = static_cast<ICorDebugProcess8*>(this);
}
else if (id == IID_ICorDebugProcess10)
{
*pInterface = static_cast<ICorDebugProcess10*>(this);
}
else if (id == IID_ICorDebugProcess11)
{
*pInterface = static_cast<ICorDebugProcess11*>(this);
Expand Down
25 changes: 21 additions & 4 deletions src/coreclr/src/debug/di/rspriv.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2935,7 +2935,6 @@ class CordbProcess :
public ICorDebugProcess5,
public ICorDebugProcess7,
public ICorDebugProcess8,
public ICorDebugProcess10,
public ICorDebugProcess11,
public IDacDbiInterface::IAllocator,
public IDacDbiInterface::IMetaDataLookup,
Expand Down Expand Up @@ -3146,7 +3145,7 @@ class CordbProcess :
COM_METHOD EnableExceptionCallbacksOutsideOfMyCode(BOOL enableExceptionsOutsideOfJMC);

//-----------------------------------------------------------
// ICorDebugProcess10
// ICorDebugProcess10 (To be removed in .NET 6, in a separate cleanup PR)
//-----------------------------------------------------------
COM_METHOD EnableGCNotificationEvents(BOOL fEnable);

Expand Down Expand Up @@ -9168,6 +9167,7 @@ class CordbObjectValue : public CordbValue,
public ICorDebugValue3,
public ICorDebugHeapValue2,
public ICorDebugHeapValue3,
public ICorDebugHeapValue4,
public ICorDebugExceptionObjectValue,
public ICorDebugComObjectValue,
public ICorDebugDelegateObjectValue
Expand Down Expand Up @@ -9243,6 +9243,11 @@ class CordbObjectValue : public CordbValue,
COM_METHOD GetThreadOwningMonitorLock(ICorDebugThread **ppThread, DWORD *pAcquisitionCount);
COM_METHOD GetMonitorEventWaitList(ICorDebugThreadEnum **ppThreadEnum);

//-----------------------------------------------------------
// ICorDebugHeapValue4
//-----------------------------------------------------------
COM_METHOD CreatePinnedHandle(ICorDebugHandleValue ** ppHandle);

//-----------------------------------------------------------
// ICorDebugObjectValue
//-----------------------------------------------------------
Expand Down Expand Up @@ -9492,7 +9497,8 @@ class CordbBoxValue : public CordbValue,
public ICorDebugValue2,
public ICorDebugValue3,
public ICorDebugHeapValue2,
public ICorDebugHeapValue3
public ICorDebugHeapValue3,
public ICorDebugHeapValue4
{
public:
CordbBoxValue(CordbAppDomain * appdomain,
Expand Down Expand Up @@ -9582,6 +9588,11 @@ class CordbBoxValue : public CordbValue,
COM_METHOD GetThreadOwningMonitorLock(ICorDebugThread **ppThread, DWORD *pAcquisitionCount);
COM_METHOD GetMonitorEventWaitList(ICorDebugThreadEnum **ppThreadEnum);

//-----------------------------------------------------------
// ICorDebugHeapValue4
//-----------------------------------------------------------
COM_METHOD CreatePinnedHandle(ICorDebugHandleValue ** ppHandle);

//-----------------------------------------------------------
// ICorDebugGenericValue
//-----------------------------------------------------------
Expand Down Expand Up @@ -9620,7 +9631,8 @@ class CordbArrayValue : public CordbValue,
public ICorDebugValue2,
public ICorDebugValue3,
public ICorDebugHeapValue2,
public ICorDebugHeapValue3
public ICorDebugHeapValue3,
public ICorDebugHeapValue4
{
public:
CordbArrayValue(CordbAppDomain * appdomain,
Expand Down Expand Up @@ -9706,6 +9718,11 @@ class CordbArrayValue : public CordbValue,
COM_METHOD GetThreadOwningMonitorLock(ICorDebugThread **ppThread, DWORD *pAcquisitionCount);
COM_METHOD GetMonitorEventWaitList(ICorDebugThreadEnum **ppThreadEnum);

//-----------------------------------------------------------
// ICorDebugHeapValue4
//-----------------------------------------------------------
COM_METHOD CreatePinnedHandle(ICorDebugHandleValue ** ppHandle);

//-----------------------------------------------------------
// ICorDebugArrayValue
//-----------------------------------------------------------
Expand Down
30 changes: 20 additions & 10 deletions src/coreclr/src/debug/ee/debugger.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11369,7 +11369,7 @@ bool Debugger::HandleIPCEvent(DebuggerIPCEvent * pEvent)
Object * pObject = (Object*)pEvent->CreateHandle.objectToken;
OBJECTREF objref = ObjectToOBJECTREF(pObject);
AppDomain * pAppDomain = pEvent->vmAppDomain.GetRawPtr();
BOOL fStrong = pEvent->CreateHandle.fStrong;
CorDebugHandleType handleType = pEvent->CreateHandle.handleType;
OBJECTHANDLE objectHandle;

// This is a synchronous event (reply required)
Expand All @@ -11385,17 +11385,27 @@ bool Debugger::HandleIPCEvent(DebuggerIPCEvent * pEvent)

if (SUCCEEDED(pEvent->hr))
{
if (fStrong == TRUE)
{
// create strong handle
objectHandle = pAppDomain->CreateStrongHandle(objref);
}
else
{
switch (handleType)
{
case HANDLE_STRONG:
// create strong handle
objectHandle = pAppDomain->CreateStrongHandle(objref);
break;
case HANDLE_WEAK_TRACK_RESURRECTION:
// create the weak long handle
objectHandle = pAppDomain->CreateLongWeakHandle(objref);
}
pEvent->CreateHandleResult.vmObjectHandle.SetRawPtr(objectHandle);
break;
case HANDLE_PINNED:
// create pinning handle
objectHandle = pAppDomain->CreatePinningHandle(objref);
break;
default:
pEvent->hr = E_INVALIDARG;
}
}
if (SUCCEEDED(pEvent->hr))
{
pEvent->CreateHandleResult.vmObjectHandle.SetRawPtr(objectHandle);
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/src/debug/inc/dbgipcevents.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2297,8 +2297,8 @@ struct MSLAYOUT DebuggerIPCEvent

struct MSLAYOUT
{
void *objectToken;
BOOL fStrong;
void *objectToken;
CorDebugHandleType handleType;
} CreateHandle;

struct MSLAYOUT
Expand Down
33 changes: 30 additions & 3 deletions src/coreclr/src/inc/cordebug.idl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ interface ICorDebugReferenceValue;
interface ICorDebugHeapValue;
interface ICorDebugHeapValue2;
interface ICorDebugHeapValue3;
interface ICorDebugHeapValue4;
interface ICorDebugHandleValue;
interface ICorDebugObjectValue;
interface ICorDebugStringValue;
Expand Down Expand Up @@ -1603,13 +1604,15 @@ typedef enum CorDebugCreateProcessFlags


/* ICorDebugHeapValue::CreateHandle takes a handle flavor.
* A strong handle will keep an object alive while a weak track resurrection
* will not.
* - A strong handle will keep an object alive while allowing GC relocation
* - A weak handle will not keep an object alive
* - A pinned handle will keep an object alive and disallow GC relocation
*/
typedef enum CorDebugHandleType
{
HANDLE_STRONG = 1,
HANDLE_WEAK_TRACK_RESURRECTION = 2
HANDLE_WEAK_TRACK_RESURRECTION = 2,
HANDLE_PINNED = 3
} CorDebugHandleType;

#pragma warning(push)
Expand Down Expand Up @@ -3304,6 +3307,9 @@ interface ICorDebugProcess10 : IUnknown
// Enable or disable the GC notification events. The GC notification events are turned off by default
// They will be delivered through ICorDebugManagedCallback4
//
// This interface is deprecated. The EnableGCNotificationEvents(true) occasionally deadlocked debug sessions
// in .NET Core 5.0 and later. Please use the IID_ICorDebugHeapValue4 to pin an object and prevent its relocation
//
// Parameters
// fEnable - true to enable the events, false to disable
//
Expand Down Expand Up @@ -6472,6 +6478,27 @@ interface ICorDebugHeapValue3 : IUnknown
HRESULT GetMonitorEventWaitList([out] ICorDebugThreadEnum **ppThreadEnum);
};

/*
* ICorDebugHeapValue4
*/

[
object,
local,
uuid(B35DD495-A555-463B-9BE9-C55338486BB8),
pointer_default(unique)
]
interface ICorDebugHeapValue4 : IUnknown
{

/*
* Creates a handle of the given type for this heap value.
*
*/
HRESULT CreatePinnedHandle([out] ICorDebugHandleValue ** ppHandle);

};

/*
* ICorDebugObjectValue is a subclass of ICorDebugValue which applies to
* values which contain an object.
Expand Down
18 changes: 11 additions & 7 deletions src/coreclr/src/pal/prebuilt/idl/cordebug_i.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,11 @@


/* File created by MIDL compiler version 8.01.0622 */
/* at Mon Jan 18 19:14:07 2038
*/
/* Compiler settings for runtime/src/coreclr/src/inc/cordebug.idl:
Oicf, W1, Zp8, env=Win32 (32b run), target_arch=X86 8.01.0622
/* Compiler settings for cordebug.idl:
Oicf, W1, Zp8, env=Win32 (32b run), target_arch=X86 8.01.0622
protocol : dce , ms_ext, c_ext, robust
error checks: allocation ref bounds_check enum stub_data
VC __declspec() decoration level:
error checks: allocation ref bounds_check enum stub_data
VC __declspec() decoration level:
__declspec(uuid()), __declspec(selectany), __declspec(novtable)
DECLSPEC_UUID(), MIDL_INTERFACE()
*/
Expand All @@ -23,7 +21,7 @@

#ifdef __cplusplus
extern "C"{
#endif
#endif


#include <rpc.h>
Expand Down Expand Up @@ -367,6 +365,9 @@ MIDL_DEFINE_GUID(IID, IID_ICorDebugHeapValue2,0xE3AC4D6C,0x9CB7,0x43e6,0x96,0xCC
MIDL_DEFINE_GUID(IID, IID_ICorDebugHeapValue3,0xA69ACAD8,0x2374,0x46e9,0x9F,0xF8,0xB1,0xF1,0x41,0x20,0xD2,0x96);


MIDL_DEFINE_GUID(IID, IID_ICorDebugHeapValue4,0xB35DD495,0xA555,0x463B,0x9B,0xE9,0xC5,0x53,0x38,0x48,0x6B,0xB8);


MIDL_DEFINE_GUID(IID, IID_ICorDebugObjectValue,0x18AD3D6E,0xB7D2,0x11d2,0xBD,0x04,0x00,0x00,0xF8,0x08,0x49,0xBD);


Expand Down Expand Up @@ -479,3 +480,6 @@ MIDL_DEFINE_GUID(CLSID, CLSID_EmbeddedCLRCorDebug,0x211f1254,0xbc7e,0x4af5,0xb9,
#ifdef __cplusplus
}
#endif



Loading

0 comments on commit ca4d6d1

Please sign in to comment.