From a3dc83e91bbba4413f20fbaf3072c167a79224cc Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Fri, 9 Feb 2018 09:40:42 -0800 Subject: [PATCH] Revert "Debugger api to set a breakpoint on offset 0 of all methods (#15819)" This reverts commit 3d689d00843618105e735c5647e1cb64e721a333. --- src/debug/di/breakpoint.cpp | 12 +++--- src/debug/di/module.cpp | 39 +----------------- src/debug/di/rsfunction.cpp | 36 ----------------- src/debug/di/rspriv.h | 22 ++--------- src/debug/di/shimprocess.cpp | 8 ++-- src/debug/ee/controller.cpp | 31 +++++++++++++-- src/debug/ee/controller.h | 1 - src/debug/ee/debugger.cpp | 1 - src/inc/cordebug.idl | 18 +-------- src/pal/prebuilt/inc/cordebug.h | 87 ----------------------------------------- 10 files changed, 43 insertions(+), 212 deletions(-) diff --git a/src/debug/di/breakpoint.cpp b/src/debug/di/breakpoint.cpp index cc55060..e3e4fb0 100644 --- a/src/debug/di/breakpoint.cpp +++ b/src/debug/di/breakpoint.cpp @@ -57,11 +57,9 @@ HRESULT CordbBreakpoint::BaseIsActive(BOOL *pbActive) * ------------------------------------------------------------------------- */ CordbFunctionBreakpoint::CordbFunctionBreakpoint(CordbCode *code, - SIZE_T offset, - BOOL offsetIsIl) + SIZE_T offset) : CordbBreakpoint(code->GetProcess(), CBT_FUNCTION), - m_code(code), m_offset(offset), - m_offsetIsIl(offsetIsIl) + m_code(code), m_offset(offset) { // Remember the app domain we came from so that breakpoints can be // deactivated from within the ExitAppdomain callback. @@ -205,11 +203,11 @@ HRESULT CordbFunctionBreakpoint::Activate(BOOL fActivate) pEvent->BreakpointData.vmDomainFile = m_code->GetModule()->GetRuntimeDomainFile(); pEvent->BreakpointData.encVersion = m_code->GetVersion(); - BOOL codeIsIL = m_code->IsIL(); + BOOL fIsIL = m_code->IsIL(); - pEvent->BreakpointData.isIL = m_offsetIsIl; + pEvent->BreakpointData.isIL = fIsIL ? true : false; pEvent->BreakpointData.offset = m_offset; - if (codeIsIL) + if (fIsIL) { pEvent->BreakpointData.nativeCodeMethodDescToken = pEvent->BreakpointData.nativeCodeMethodDescToken.NullPtr(); } diff --git a/src/debug/di/module.cpp b/src/debug/di/module.cpp index b0ba054..5d1d3da 100644 --- a/src/debug/di/module.cpp +++ b/src/debug/di/module.cpp @@ -2975,9 +2975,8 @@ HRESULT CordbCode::CreateBreakpoint(ULONG32 offset, HRESULT hr; ULONG32 size = GetSize(); - BOOL offsetIsIl = IsIL(); LOG((LF_CORDB, LL_INFO10000, "CCode::CreateBreakpoint, offset=%d, size=%d, IsIl=%d, this=0x%p\n", - offset, size, offsetIsIl, this)); + offset, size, m_fIsIL, this)); // Make sure the offset is within range of the method. // If we're native code, then both offset & total code size are bytes of native code, @@ -2987,7 +2986,7 @@ HRESULT CordbCode::CreateBreakpoint(ULONG32 offset, return CORDBG_E_UNABLE_TO_SET_BREAKPOINT; } - CordbFunctionBreakpoint *bp = new (nothrow) CordbFunctionBreakpoint(this, offset, offsetIsIl); + CordbFunctionBreakpoint *bp = new (nothrow) CordbFunctionBreakpoint(this, offset); if (bp == NULL) return E_OUTOFMEMORY; @@ -3392,40 +3391,6 @@ mdSignature CordbILCode::GetLocalVarSigToken() return m_localVarSigToken; } -HRESULT CordbILCode::CreateNativeBreakpoint(ICorDebugFunctionBreakpoint **ppBreakpoint) -{ - FAIL_IF_NEUTERED(this); - VALIDATE_POINTER_TO_OBJECT(ppBreakpoint, ICorDebugFunctionBreakpoint **); - - HRESULT hr; - ULONG32 size = GetSize(); - LOG((LF_CORDB, LL_INFO10000, "CordbILCode::CreateNativeBreakpoint, size=%d, this=0x%p\n", - size, this)); - - ULONG32 offset = 0; - CordbFunctionBreakpoint *bp = new (nothrow) CordbFunctionBreakpoint(this, offset, FALSE); - - if (bp == NULL) - { - return E_OUTOFMEMORY; - } - - hr = bp->Activate(TRUE); - if (SUCCEEDED(hr)) - { - *ppBreakpoint = static_cast (bp); - bp->ExternalAddRef(); - return S_OK; - } - else - { - delete bp; - return hr; - } -} - - - CordbReJitILCode::CordbReJitILCode(CordbFunction *pFunction, SIZE_T encVersion, VMPTR_ILCodeVersionNode vmILCodeVersionNode) : CordbILCode(pFunction, TargetBuffer(), encVersion, mdSignatureNil, VmPtrToCookie(vmILCodeVersionNode)), m_cClauses(0), diff --git a/src/debug/di/rsfunction.cpp b/src/debug/di/rsfunction.cpp index 82ac875..bf3c49b 100644 --- a/src/debug/di/rsfunction.cpp +++ b/src/debug/di/rsfunction.cpp @@ -132,10 +132,6 @@ HRESULT CordbFunction::QueryInterface(REFIID id, void **pInterface) { *pInterface = static_cast(this); } - else if (id == IID_ICorDebugFunction4) - { - *pInterface = static_cast(this); - } else if (id == IID_IUnknown) { *pInterface = static_cast(static_cast(this)); @@ -574,38 +570,6 @@ HRESULT CordbFunction::GetActiveReJitRequestILCode(ICorDebugILCode **ppReJitedIL return hr; } -//----------------------------------------------------------------------------- -// CordbFunction::CreateNativeBreakpoint -// Public method for ICorDebugFunction4::CreateNativeBreakpoint. -// Sets a breakpoint at native offset 0 for all native code versions of a method. -// -// Parameters -// pnVersion - out parameter to hold the version number. -// -// Returns: -// S_OK on success. -//----------------------------------------------------------------------------- -HRESULT CordbFunction::CreateNativeBreakpoint(ICorDebugFunctionBreakpoint **ppBreakpoint) -{ - PUBLIC_API_ENTRY(this); - FAIL_IF_NEUTERED(this); - VALIDATE_POINTER_TO_OBJECT(ppBreakpoint, ICorDebugFunctionBreakpoint **); - ATT_REQUIRE_STOPPED_MAY_FAIL(GetProcess()); - - HRESULT hr = S_OK; - - RSExtSmartPtr pCode; - - hr = GetILCode(&pCode); - - if (SUCCEEDED(hr)) - { - hr = pCode->CreateNativeBreakpoint(ppBreakpoint); - } - - return hr; -} - // determine whether we have a native-only implementation // Arguments: // Input: none (we use information in various data members of this instance of CordbFunction: m_isNativeImpl, diff --git a/src/debug/di/rspriv.h b/src/debug/di/rspriv.h index 41a04f2..de821f2 100644 --- a/src/debug/di/rspriv.h +++ b/src/debug/di/rspriv.h @@ -5338,11 +5338,7 @@ const BOOL bILCode = TRUE; // B/C of generics, a single IL function may get jitted multiple times and // be associated w/ multiple native code blobs (CordbNativeCode). // -class CordbFunction : public CordbBase, - public ICorDebugFunction, - public ICorDebugFunction2, - public ICorDebugFunction3, - public ICorDebugFunction4 +class CordbFunction : public CordbBase, public ICorDebugFunction, public ICorDebugFunction2, public ICorDebugFunction3 { public: //----------------------------------------------------------- @@ -5401,11 +5397,6 @@ public: COM_METHOD GetActiveReJitRequestILCode(ICorDebugILCode **ppReJitedILCode); //----------------------------------------------------------- - // ICorDebugFunction4 - //----------------------------------------------------------- - COM_METHOD CreateNativeBreakpoint(ICorDebugFunctionBreakpoint **ppBreakpoint); - - //----------------------------------------------------------- // Internal members //----------------------------------------------------------- protected: @@ -5749,8 +5740,6 @@ public: HRESULT GetLocalVariableType(DWORD dwIndex, const Instantiation * pInst, CordbType ** ppResultType); mdSignature GetLocalVarSigToken(); - COM_METHOD CreateNativeBreakpoint(ICorDebugFunctionBreakpoint **ppBreakpoint); - private: // Read the actual bytes of IL code into the data member m_rgbCode. // Helper routine for GetCode @@ -5782,9 +5771,7 @@ protected: * rejitID. Thus it is 1:N with a given instantiation of CordbFunction. * ------------------------------------------------------------------------- */ -class CordbReJitILCode : public CordbILCode, - public ICorDebugILCode, - public ICorDebugILCode2 +class CordbReJitILCode : public CordbILCode, public ICorDebugILCode, public ICorDebugILCode2 { public: // Initialize a new CordbILCode instance @@ -5809,7 +5796,7 @@ public: //----------------------------------------------------------- COM_METHOD GetLocalVarSigToken(mdSignature *pmdSig); COM_METHOD GetInstrumentedILMap(ULONG32 cMap, ULONG32 *pcMap, COR_IL_MAP map[]); - + private: HRESULT Init(DacSharedReJitInfo* pSharedReJitInfo); @@ -7573,7 +7560,7 @@ class CordbFunctionBreakpoint : public CordbBreakpoint, public ICorDebugFunctionBreakpoint { public: - CordbFunctionBreakpoint(CordbCode *code, SIZE_T offset, BOOL offsetIsIl); + CordbFunctionBreakpoint(CordbCode *code, SIZE_T offset); ~CordbFunctionBreakpoint(); virtual void Neuter(); @@ -7634,7 +7621,6 @@ public: // leaked. RSExtSmartPtr m_code; SIZE_T m_offset; - BOOL m_offsetIsIl; }; /* ------------------------------------------------------------------------- * diff --git a/src/debug/di/shimprocess.cpp b/src/debug/di/shimprocess.cpp index 99967a2..46c35fd 100644 --- a/src/debug/di/shimprocess.cpp +++ b/src/debug/di/shimprocess.cpp @@ -365,12 +365,12 @@ DWORD WINAPI CallStopGoThreadProc(LPVOID parameter) // Calling Stop + Continue will synchronize the process and force any queued events to be called. // Stop is synchronous and will block until debuggee is synchronized. hr = pProc->Stop(INFINITE); - SIMPLIFYING_ASSUMPTION_SUCCEEDED(hr); + SIMPLIFYING_ASSUMPTION(SUCCEEDED(hr)); // Continue will resume the debuggee. If there are queued events (which we expect in this case) // then continue will drain the event queue instead of actually resuming the process. hr = pProc->Continue(FALSE); - SIMPLIFYING_ASSUMPTION_SUCCEEDED(hr); + SIMPLIFYING_ASSUMPTION(SUCCEEDED(hr)); // This thread just needs to trigger an event dispatch. Now that it's done that, it can exit. return 0; @@ -833,7 +833,7 @@ HRESULT ShimProcess::HandleWin32DebugEvent(const DEBUG_EVENT * pEvent) EX_CATCH_HRESULT(hrIgnore); // Dont' expect errors here (but could probably return it up to become an // unrecoverable error if necessary). We still want to call Continue thought. - SIMPLIFYING_ASSUMPTION_SUCCEEDED(hrIgnore); + SIMPLIFYING_ASSUMPTION(SUCCEEDED(hrIgnore)); // // Continue the debuggee if needed. @@ -873,7 +873,7 @@ HRESULT ShimProcess::HandleWin32DebugEvent(const DEBUG_EVENT * pEvent) { ::Sleep(500); hrIgnore = GetNativePipeline()->EnsureThreadsRunning(); - SIMPLIFYING_ASSUMPTION_SUCCEEDED(hrIgnore); + SIMPLIFYING_ASSUMPTION(SUCCEEDED(hrIgnore)); } } } diff --git a/src/debug/ee/controller.cpp b/src/debug/ee/controller.cpp index b6c20fd..55e9936 100644 --- a/src/debug/ee/controller.cpp +++ b/src/debug/ee/controller.cpp @@ -4946,17 +4946,40 @@ DebuggerBreakpoint::DebuggerBreakpoint(Module *module, SIZE_T ilEnCVersion, // must give the EnC version for non-native bps MethodDesc *nativeMethodDesc, // use only when m_native DebuggerJitInfo *nativeJITInfo, // optional when m_native, null otherwise - bool nativeCodeBindAllVersions, BOOL *pSucceed ) : DebuggerController(NULL, pAppDomain) { _ASSERTE(pSucceed != NULL); - _ASSERTE((native == (nativeMethodDesc != NULL)) || nativeCodeBindAllVersions); + _ASSERTE(native == (nativeMethodDesc != NULL)); _ASSERTE(native || nativeJITInfo == NULL); _ASSERTE(!nativeJITInfo || nativeJITInfo->m_jitComplete); // this is sent by the left-side, and it couldn't have got the code if the JIT wasn't complete - if (native && !nativeCodeBindAllVersions) + BOOL bindAcrossAllJittedInstances = !native; + MethodDesc* pGenericInstanceFilter = NULL; +#ifdef DEBUG + // Normally any breakpoint specified as a native offset only binds in one jitted instance of a method, however + // to better test the breakpoint binding logic in debug builds we allow the behavior to change. The test behavior + // binds native breakpoints in every code version of the same generic instance. Currently the only way to get more + // than one such version is to use tiered compilation, but even with only one version the code path is a little different. + // + // This covers the same code paths used to add a step-in breakpoint, because step-in needs to handle code version changes + // transparently but it is challenging to create a test case that ensures the code version will change exactly during the + // tiny window of time that the step-in breakpoint exists. + static ConfigDWORD config; + if(config.val(CLRConfig::INTERNAL_DbgNativeCodeBpBindsAcrossVersions)) + { + LOG((LF_CORDB, LL_INFO1000, "DB::DB Test hook COMPLUS_DbgNativeCodeBpBindsAcrossVersions is active\n")); + if (native && offset == 0 && nativeMethodDesc) + { + LOG((LF_CORDB, LL_INFO1000, "DB::DB Test hook modification: native breakpoint at offset 0 binding to all code versions\n")); + bindAcrossAllJittedInstances = TRUE; + pGenericInstanceFilter = nativeMethodDesc; + } + } +#endif + + if (!bindAcrossAllJittedInstances) { (*pSucceed) = AddBindAndActivateNativeManagedPatch(nativeMethodDesc, nativeJITInfo, offset, LEAF_MOST_FRAME, pAppDomain); return; @@ -4964,7 +4987,7 @@ DebuggerBreakpoint::DebuggerBreakpoint(Module *module, else { _ASSERTE(!native || offset == 0); - (*pSucceed) = AddILPatch(pAppDomain, module, md, NULL, ilEnCVersion, offset, !native); + (*pSucceed) = AddILPatch(pAppDomain, module, md, pGenericInstanceFilter, ilEnCVersion, offset, !native); } } diff --git a/src/debug/ee/controller.h b/src/debug/ee/controller.h index 16dd177..bac635e 100644 --- a/src/debug/ee/controller.h +++ b/src/debug/ee/controller.h @@ -1450,7 +1450,6 @@ public: SIZE_T ilEnCVersion, // must give the EnC version for non-native bps MethodDesc *nativeMethodDesc, // must be non-null when m_native, null otherwise DebuggerJitInfo *nativeJITInfo, // optional when m_native, null otherwise - bool nativeCodeBindAllVersions, BOOL *pSucceed ); diff --git a/src/debug/ee/debugger.cpp b/src/debug/ee/debugger.cpp index 94792da..23e5e54 100644 --- a/src/debug/ee/debugger.cpp +++ b/src/debug/ee/debugger.cpp @@ -10777,7 +10777,6 @@ bool Debugger::HandleIPCEvent(DebuggerIPCEvent * pEvent) pEvent->BreakpointData.encVersion, pMethodDesc, pDJI, - pEvent->BreakpointData.nativeCodeMethodDescToken == NULL, &fSuccess); TRACE_ALLOC(pDebuggerBP); diff --git a/src/inc/cordebug.idl b/src/inc/cordebug.idl index d556ebc..093b893 100644 --- a/src/inc/cordebug.idl +++ b/src/inc/cordebug.idl @@ -5427,23 +5427,6 @@ interface ICorDebugFunction3 : IUnknown }; /* -ICorDebugFunction4 is a logical extension to ICorDebugFunction. -*/ -[ - object, - local, - uuid(72965963-34fd-46e9-9434-b817fe6e7f43), - pointer_default(unique) -] -interface ICorDebugFunction4 : IUnknown -{ - /* - * Sets a breakpoint at offset 0 of any current or future jitted methods. - */ - HRESULT CreateNativeBreakpoint(ICorDebugFunctionBreakpoint **ppBreakpoint); -}; - -/* ICorDebugCode represents an IL or native code blob. For methods that take offsets, the units are the same as the units on the CordbCode object. @@ -5695,6 +5678,7 @@ interface ICorDebugILCode2 : IUnknown [out, size_is(cMap), length_is(*pcMap)] COR_IL_MAP map[]); } + /* ICorDebugClass represents a Class (mdTypeDef) in the IL image. For generic types, it represents the generic type definition (eg. List) not any of diff --git a/src/pal/prebuilt/inc/cordebug.h b/src/pal/prebuilt/inc/cordebug.h index 0935c96..3931040 100644 --- a/src/pal/prebuilt/inc/cordebug.h +++ b/src/pal/prebuilt/inc/cordebug.h @@ -577,13 +577,6 @@ typedef interface ICorDebugFunction3 ICorDebugFunction3; #endif /* __ICorDebugFunction3_FWD_DEFINED__ */ -#ifndef __ICorDebugFunction4_FWD_DEFINED__ -#define __ICorDebugFunction4_FWD_DEFINED__ -typedef interface ICorDebugFunction4 ICorDebugFunction4; - -#endif /* __ICorDebugFunction4_FWD_DEFINED__ */ - - #ifndef __ICorDebugCode_FWD_DEFINED__ #define __ICorDebugCode_FWD_DEFINED__ typedef interface ICorDebugCode ICorDebugCode; @@ -12091,86 +12084,6 @@ EXTERN_C const IID IID_ICorDebugFunction3; -#ifndef __ICorDebugFunction4_INTERFACE_DEFINED__ -#define __ICorDebugFunction4_INTERFACE_DEFINED__ - -/* interface ICorDebugFunction4 */ -/* [unique][uuid][local][object] */ - - -EXTERN_C const IID IID_ICorDebugFunction4; - -#if defined(__cplusplus) && !defined(CINTERFACE) - - MIDL_INTERFACE("72965963-34fd-46e9-9434-b817fe6e7f43") - ICorDebugFunction4 : public IUnknown - { - public: - virtual HRESULT STDMETHODCALLTYPE CreateNativeBreakpoint( - ICorDebugFunctionBreakpoint **ppBreakpoint) = 0; - - }; - - -#else /* C style interface */ - - typedef struct ICorDebugFunction4Vtbl - { - BEGIN_INTERFACE - - HRESULT ( STDMETHODCALLTYPE *QueryInterface )( - ICorDebugFunction4 * This, - /* [in] */ REFIID riid, - /* [annotation][iid_is][out] */ - _COM_Outptr_ void **ppvObject); - - ULONG ( STDMETHODCALLTYPE *AddRef )( - ICorDebugFunction4 * This); - - ULONG ( STDMETHODCALLTYPE *Release )( - ICorDebugFunction4 * This); - - HRESULT ( STDMETHODCALLTYPE *CreateNativeBreakpoint )( - ICorDebugFunction4 * This, - ICorDebugFunctionBreakpoint **ppBreakpoint); - - END_INTERFACE - } ICorDebugFunction4Vtbl; - - interface ICorDebugFunction4 - { - CONST_VTBL struct ICorDebugFunction4Vtbl *lpVtbl; - }; - - - -#ifdef COBJMACROS - - -#define ICorDebugFunction4_QueryInterface(This,riid,ppvObject) \ - ( (This)->lpVtbl -> QueryInterface(This,riid,ppvObject) ) - -#define ICorDebugFunction4_AddRef(This) \ - ( (This)->lpVtbl -> AddRef(This) ) - -#define ICorDebugFunction4_Release(This) \ - ( (This)->lpVtbl -> Release(This) ) - - -#define ICorDebugFunction4_CreateNativeBreakpoint(This,ppBreakpoint) \ - ( (This)->lpVtbl -> CreateNativeBreakpoint(This,ppBreakpoint) ) - -#endif /* COBJMACROS */ - - -#endif /* C style interface */ - - - - -#endif /* __ICorDebugFunction4_INTERFACE_DEFINED__ */ - - #endif /* __ICorDebugFunction3_INTERFACE_DEFINED__ */ -- 2.7.4