Debugger api to set a breakpoint on offset 0 of all methods (#15819)

* Debugger api to set a breakpoint on offset 0 of all methods

* code review feedback

* more code review feedback

* respect IsIl in CorDbFunctionBreakoint()

* change SIMPLIFYING_ASSERT to SIMPLIFYING_ASSUMPTION_SUCCEEDED for hrs, it outputs a much better diagnostic message

8 files changed, 108 insertions, 42 deletions

diff --git a/src/debug/di/breakpoint.cpp b/src/debug/di/breakpoint.cpp
index e3e4fb04a5..cc55060fa8 100644
--- a/src/debug/di/breakpoint.cpp
+++ b/src/debug/di/breakpoint.cpp
@@ -57,9 +57,11 @@ HRESULT CordbBreakpoint::BaseIsActive(BOOL *pbActive)
  * ------------------------------------------------------------------------- */
 
 CordbFunctionBreakpoint::CordbFunctionBreakpoint(CordbCode *code,
-                                                 SIZE_T offset)
+                                                 SIZE_T offset,
+                                                 BOOL offsetIsIl)
   : CordbBreakpoint(code->GetProcess(), CBT_FUNCTION), 
-  m_code(code), m_offset(offset)
+  m_code(code), m_offset(offset),
+  m_offsetIsIl(offsetIsIl)
 {
     // Remember the app domain we came from so that breakpoints can be
     // deactivated from within the ExitAppdomain callback.
@@ -203,11 +205,11 @@ HRESULT CordbFunctionBreakpoint::Activate(BOOL fActivate)
         pEvent->BreakpointData.vmDomainFile = m_code->GetModule()->GetRuntimeDomainFile();
         pEvent->BreakpointData.encVersion = m_code->GetVersion();
 
-        BOOL fIsIL = m_code->IsIL();
+        BOOL codeIsIL = m_code->IsIL();
 
-        pEvent->BreakpointData.isIL = fIsIL ? true : false;
+        pEvent->BreakpointData.isIL = m_offsetIsIl;
         pEvent->BreakpointData.offset = m_offset;
-        if (fIsIL)
+        if (codeIsIL)
         {
             pEvent->BreakpointData.nativeCodeMethodDescToken = pEvent->BreakpointData.nativeCodeMethodDescToken.NullPtr();
         }
diff --git a/src/debug/di/module.cpp b/src/debug/di/module.cpp
index 5d1d3da427..b0ba054a87 100644
--- a/src/debug/di/module.cpp
+++ b/src/debug/di/module.cpp
@@ -2975,8 +2975,9 @@ 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, m_fIsIL, this));
+        offset, size, offsetIsIl, 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,
@@ -2986,7 +2987,7 @@ HRESULT CordbCode::CreateBreakpoint(ULONG32 offset,
         return CORDBG_E_UNABLE_TO_SET_BREAKPOINT;
     }
 
-    CordbFunctionBreakpoint *bp = new (nothrow) CordbFunctionBreakpoint(this, offset);
+    CordbFunctionBreakpoint *bp = new (nothrow) CordbFunctionBreakpoint(this, offset, offsetIsIl);
 
     if (bp == NULL)
         return E_OUTOFMEMORY;
@@ -3391,6 +3392,40 @@ 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<ICorDebugFunctionBreakpoint*> (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 bf3c49bb98..82ac87530d 100644
--- a/src/debug/di/rsfunction.cpp
+++ b/src/debug/di/rsfunction.cpp
@@ -132,6 +132,10 @@ HRESULT CordbFunction::QueryInterface(REFIID id, void **pInterface)
     {
         *pInterface = static_cast<ICorDebugFunction3*>(this);
     }
+    else if (id == IID_ICorDebugFunction4)
+    {
+        *pInterface = static_cast<ICorDebugFunction4*>(this);
+    }
     else if (id == IID_IUnknown)
     {
         *pInterface = static_cast<IUnknown*>(static_cast<ICorDebugFunction*>(this));
@@ -570,6 +574,38 @@ 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<CordbILCode> 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 de821f26a6..41a04f284e 100644
--- a/src/debug/di/rspriv.h
+++ b/src/debug/di/rspriv.h
@@ -5338,7 +5338,11 @@ 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
+class CordbFunction : public CordbBase, 
+                      public ICorDebugFunction, 
+                      public ICorDebugFunction2, 
+                      public ICorDebugFunction3, 
+                      public ICorDebugFunction4
 {
 public:
     //-----------------------------------------------------------
@@ -5397,6 +5401,11 @@ public:
     COM_METHOD GetActiveReJitRequestILCode(ICorDebugILCode **ppReJitedILCode);
 
     //-----------------------------------------------------------
+    // ICorDebugFunction4
+    //-----------------------------------------------------------
+    COM_METHOD CreateNativeBreakpoint(ICorDebugFunctionBreakpoint **ppBreakpoint);
+
+    //-----------------------------------------------------------
     // Internal members
     //-----------------------------------------------------------
 protected:
@@ -5740,6 +5749,8 @@ 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
@@ -5771,7 +5782,9 @@ 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
@@ -5796,7 +5809,7 @@ public:
     //-----------------------------------------------------------
     COM_METHOD GetLocalVarSigToken(mdSignature *pmdSig);
     COM_METHOD GetInstrumentedILMap(ULONG32 cMap, ULONG32 *pcMap, COR_IL_MAP map[]);
-
+    
 private:
     HRESULT Init(DacSharedReJitInfo* pSharedReJitInfo);
 
@@ -7560,7 +7573,7 @@ class CordbFunctionBreakpoint : public CordbBreakpoint,
                                 public ICorDebugFunctionBreakpoint
 {
 public:
-    CordbFunctionBreakpoint(CordbCode *code, SIZE_T offset);
+    CordbFunctionBreakpoint(CordbCode *code, SIZE_T offset, BOOL offsetIsIl);
     ~CordbFunctionBreakpoint();
 
     virtual void Neuter();
@@ -7621,6 +7634,7 @@ public:
     // leaked.
     RSExtSmartPtr<CordbCode> m_code;
     SIZE_T          m_offset;
+    BOOL            m_offsetIsIl;
 };
 
 /* ------------------------------------------------------------------------- *
diff --git a/src/debug/di/shimprocess.cpp b/src/debug/di/shimprocess.cpp
index 46c35fdc2b..99967a2af9 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 55e9936866..b6c20fd4d6 100644
--- a/src/debug/ee/controller.cpp
+++ b/src/debug/ee/controller.cpp
@@ -4946,40 +4946,17 @@ 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));
+    _ASSERTE((native == (nativeMethodDesc != NULL)) || nativeCodeBindAllVersions);
     _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
 
-    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)
+    if (native && !nativeCodeBindAllVersions)
     {
         (*pSucceed) = AddBindAndActivateNativeManagedPatch(nativeMethodDesc, nativeJITInfo, offset, LEAF_MOST_FRAME, pAppDomain);
         return;
@@ -4987,7 +4964,7 @@ DebuggerBreakpoint::DebuggerBreakpoint(Module *module,
     else
     {
         _ASSERTE(!native || offset == 0);
-        (*pSucceed) = AddILPatch(pAppDomain, module, md, pGenericInstanceFilter, ilEnCVersion, offset, !native);
+        (*pSucceed) = AddILPatch(pAppDomain, module, md, NULL, ilEnCVersion, offset, !native);
     }
 }
 
diff --git a/src/debug/ee/controller.h b/src/debug/ee/controller.h
index bac635e2f7..16dd1772fd 100644
--- a/src/debug/ee/controller.h
+++ b/src/debug/ee/controller.h
@@ -1450,6 +1450,7 @@ 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 23e5e54df4..94792dac2c 100644
--- a/src/debug/ee/debugger.cpp
+++ b/src/debug/ee/debugger.cpp
@@ -10777,6 +10777,7 @@ bool Debugger::HandleIPCEvent(DebuggerIPCEvent * pEvent)
                                                                             pEvent->BreakpointData.encVersion,
                                                                             pMethodDesc,
                                                                             pDJI,
+                                                                            pEvent->BreakpointData.nativeCodeMethodDescToken == NULL,
                                                                             &fSuccess);
 
                 TRACE_ALLOC(pDebuggerBP);