respond to debugger code review

dotnet · Oct 2, 2024 · c152b10 · c152b10
1 parent bcc0cf9
commit c152b10
Show file tree

Hide file tree

Showing 8 changed files with 70 additions and 60 deletions.
diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp
@@ -20,7 +20,6 @@
 
 #include "../../vm/methoditer.h"
 #include "../../vm/tailcallhelp.h"
-#include "../../vm/stubhelpers.h"
 
 const char *GetTType( TraceType tt);
 
@@ -2378,20 +2377,13 @@ bool DebuggerController::PatchTrace(TraceDestination *trace,
         return true;
 
     case TRACE_MGR_PUSH:
-        if (trace->GetAddress() == GetEEFuncEntryPoint(StubHelpers::MulticastDebuggerTraceHelper))
-        {
-            EnableMultiCastDelegate();
-        }
-        else
-        {
-            LOG((LF_CORDB, LL_INFO10000,
-             "Setting frame patch (TRACE_MGR_PUSH) at 0x%p(%p)\n",
-             trace->GetAddress(), fp.GetSPValue()));
-            dcp = AddAndActivateNativePatchForAddress((CORDB_ADDRESS_TYPE *)trace->GetAddress(),
-                        LEAF_MOST_FRAME, // But Mgr_push can't have fp affinity!
-                        TRUE,
-                        DPT_DEFAULT_TRACE_TYPE); // TRACE_OTHER
-        }
+        LOG((LF_CORDB, LL_INFO10000,
+            "Setting frame patch (TRACE_MGR_PUSH) at 0x%p(%p)\n",
+            trace->GetAddress(), fp.GetSPValue()));
+        dcp = AddAndActivateNativePatchForAddress((CORDB_ADDRESS_TYPE *)trace->GetAddress(),
+                    LEAF_MOST_FRAME, // But Mgr_push can't have fp affinity!
+                    TRUE,
+                    DPT_DEFAULT_TRACE_TYPE); // TRACE_OTHER
         // Now copy over the trace field since TriggerPatch will expect this
         // to be set for this case.
         if (dcp != NULL)
@@ -2401,6 +2393,10 @@ bool DebuggerController::PatchTrace(TraceDestination *trace,
 
         return true;
 
+    case TRACE_MULTICAST_DELEGATE_HELPER:
+        EnableMultiCastDelegate();
+        return true;
+
     case TRACE_OTHER:
         LOG((LF_CORDB, LL_INFO10000,
              "Can't set a trace patch for TRACE_OTHER...\n"));
@@ -3901,7 +3897,6 @@ void DebuggerController::EnableMultiCastDelegate()
     CONTRACTL_END;
 
     ControllerLockHolder chController;
-    Debugger::DebuggerDataLockHolder chInfo(g_pDebugger);
     if (!m_multicastDelegateHelper)
     {
         LOG((LF_CORDB, LL_INFO1000000, "DC::EnableMultiCastDel, this=%p, previously disabled\n", this));
@@ -3923,7 +3918,6 @@ void DebuggerController::DisableMultiCastDelegate()
     CONTRACTL_END;
 
     ControllerLockHolder chController;
-    Debugger::DebuggerDataLockHolder chInfo(g_pDebugger);
 
     if (m_multicastDelegateHelper)
     {
@@ -3937,7 +3931,7 @@ void DebuggerController::DisableMultiCastDelegate()
 }
 
 // Loop through controllers and dispatch TriggerMulticastDelegate
-void DebuggerController::DispatchMulticastDelegate(BYTE* pbDel, INT32 countDel)
+void DebuggerController::DispatchMulticastDelegate(DELEGATEREF pbDel, INT32 countDel)
 {
     LOG((LF_CORDB, LL_INFO10000, "DC::DispatchMulticastDelegate\n"));
 
@@ -3954,7 +3948,6 @@ void DebuggerController::DispatchMulticastDelegate(BYTE* pbDel, INT32 countDel)
             if ((p->GetThread() == NULL) || (p->GetThread() == pThread))
             {
                 p->TriggerMulticastDelegate(pbDel, countDel);
-                p->DisableMultiCastDelegate();
             }
         }
         p = p->m_next;
@@ -4104,9 +4097,9 @@ void DebuggerController::DispatchFuncEvalExit(Thread * thread)
 
 
 }
-void DebuggerController::TriggerMulticastDelegate(BYTE* pDel, INT32 delegateCount)
+void DebuggerController::TriggerMulticastDelegate(DELEGATEREF pDel, INT32 delegateCount)
 {
-    LOG((LF_CORDB, LL_INFO10000, "DC::TMD: in default TriggerMulticastDelegate\n"));
+    _ASSERTE(!"This code should be unreachable. If your controller enables MulticastDelegateHelper events,it should also override this callback to do something useful when the event arrives.");
 }
 
 
@@ -7638,30 +7631,30 @@ void DebuggerStepper::TriggerUnwind(Thread *thread,
     m_reason = unwindReason;
 }
 
-void DebuggerStepper::TriggerMulticastDelegate(BYTE* pDel, INT32 delegateCount)
+void DebuggerStepper::TriggerMulticastDelegate(DELEGATEREF pDel, INT32 delegateCount)
 {
     TraceDestination trace;
     FramePointer fp = LEAF_MOST_FRAME;
 
-    int totalDelegateCount = (int)*(size_t*)((BYTE*)pDel + DelegateObject::GetOffsetOfInvocationCount());
+    int totalDelegateCount = pDel->GetInvocationCount();
     if (delegateCount == totalDelegateCount)
     {
-        PCODE addr = NULL;
+        PCODE addr = (PCODE)NULL;
         trace.InitForOther(addr);
         LOG((LF_CORDB, LL_INFO10000, "DS::TMD this:0x%x, Fired all delegates\n", this));
     }
     else
     {
-        BYTE *pbDelInvocationList = *(BYTE **)((BYTE*)pDel + DelegateObject::GetOffsetOfInvocationList());
+        PTRARRAYREF pDelInvocationList = (PTRARRAYREF) pDel->GetInvocationList();
+        DELEGATEREF pCurrentInvokeDel = (DELEGATEREF) pDelInvocationList->GetAt(delegateCount);
 
-        BYTE* pbDel = *(BYTE**)( ((ArrayBase *)pbDelInvocationList)->GetDataPtr() +
-                            ((ArrayBase *)pbDelInvocationList)->GetComponentSize()*delegateCount);
-        _ASSERTE(pbDel);
-        StubLinkStubManager::TraceDelegateObject(pbDel, &trace);
+        StubLinkStubManager::TraceDelegateObject((BYTE*)OBJECTREFToObject(pCurrentInvokeDel), &trace);
     }
 
     g_pEEInterface->FollowTrace(&trace);
-    PatchTrace(&trace, fp, false); //Value of last bool only matters for the TRACE_UMNANAGED case, we are in the TRACE_MGR_PUSH case
+    //fStopInUnmanaged only matters for TRACE_UNMANAGED
+    PatchTrace(&trace, fp, /*fStopInUnmanaged*/false);
+    this->DisableMultiCastDelegate();
 }
 
 // Prepare for sending an event.

diff --git a/src/coreclr/debug/ee/controller.h b/src/coreclr/debug/ee/controller.h
@@ -1089,7 +1089,7 @@ class DebuggerController
     // pIP is the ip right after the prolog of the method we've entered.
     // fp is the frame pointer for that method.
     static void DispatchMethodEnter(void * pIP, FramePointer fp);
-    static void DispatchMulticastDelegate(BYTE* pbDel, INT32 countDel);
+    static void DispatchMulticastDelegate(DELEGATEREF pbDel, INT32 countDel);
 
 
     // Delete any patches that exist for a specific module and optionally a specific AppDomain.
@@ -1402,7 +1402,7 @@ class DebuggerController
                                     const BYTE * ip,
                                     FramePointer fp);
 
-    virtual void TriggerMulticastDelegate(BYTE* pDel, INT32 delegateCount);
+    virtual void TriggerMulticastDelegate(DELEGATEREF pDel, INT32 delegateCount);
 
     // Send the managed debug event.
     // This is called after TriggerPatch/TriggerSingleStep actually trigger.
@@ -1644,7 +1644,7 @@ class DebuggerStepper : public DebuggerController
 
 
     virtual void TriggerMethodEnter(Thread * thread, DebuggerJitInfo * dji, const BYTE * ip, FramePointer fp);
-    void TriggerMulticastDelegate(BYTE* pDel, INT32 delegateCount);
+    void TriggerMulticastDelegate(DELEGATEREF pDel, INT32 delegateCount);
 
     void ResetRange();
 

diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp
@@ -26,7 +26,7 @@
 #include "../../vm/dwreport.h"
 #include "../../vm/eepolicy.h"
 #include "../../vm/excep.h"
-#include "../../vm/stubhelpers.h"
+
 #if defined(FEATURE_DBGIPC_TRANSPORT_VM)
 #include "dbgtransportsession.h"
 #endif // FEATURE_DBGIPC_TRANSPORT_VM
@@ -16789,7 +16789,7 @@ BOOL Debugger::IsOutOfProcessSetContextEnabled()
 #endif // OUT_OF_PROCESS_SETTHREADCONTEXT
 #endif // DACCESS_COMPILE
 #ifndef DACCESS_COMPILE
-void Debugger::MulticastTraceNextStep(BYTE* pbDel, INT32 count)
+void Debugger::MulticastTraceNextStep(DELEGATEREF pbDel, INT32 count)
 {
     DebuggerController::DispatchMulticastDelegate(pbDel, count);
 }

diff --git a/src/coreclr/debug/ee/debugger.h b/src/coreclr/debug/ee/debugger.h
@@ -2614,7 +2614,7 @@ class Debugger : public DebugInterface
 
     HRESULT ReDaclEvents(PSECURITY_DESCRIPTOR securityDescriptor);
 #ifndef DACCESS_COMPILE
-    void MulticastTraceNextStep(BYTE* pbDel, INT32 count);
+    void MulticastTraceNextStep(DELEGATEREF pbDel, INT32 count);
 #endif
 
 #ifdef DACCESS_COMPILE

diff --git a/src/coreclr/vm/dbginterface.h b/src/coreclr/vm/dbginterface.h
@@ -411,7 +411,7 @@ class DebugInterface
 #ifndef DACCESS_COMPILE
     virtual HRESULT DeoptimizeMethod(Module* pModule, mdMethodDef methodDef) = 0;
     virtual HRESULT IsMethodDeoptimized(Module *pModule, mdMethodDef methodDef, BOOL *pResult) = 0;
-    virtual void MulticastTraceNextStep(BYTE* pbDel, INT32 count) = 0;
+    virtual void MulticastTraceNextStep(DELEGATEREF pbDel, INT32 count) = 0;
 #endif //DACCESS_COMPILE
 };
 

diff --git a/src/coreclr/vm/stubhelpers.cpp b/src/coreclr/vm/stubhelpers.cpp
@@ -740,8 +740,7 @@ FCIMPLEND
 FCIMPL2(void, StubHelpers::MulticastDebuggerTraceHelper, Object* element, INT32 count)
 {
     FCALL_CONTRACT;
-    FCUnique(0xa5);
-    g_pDebugger->MulticastTraceNextStep(reinterpret_cast<BYTE*>(element), count);
+    g_pDebugger->MulticastTraceNextStep((DELEGATEREF)ObjectToOBJECTREF(element), count);
 }
 FCIMPLEND
 

diff --git a/src/coreclr/vm/stubmgr.cpp b/src/coreclr/vm/stubmgr.cpp
@@ -19,14 +19,15 @@ const char *GetTType( TraceType tt)
 
     switch( tt )
     {
-        case TRACE_ENTRY_STUB:      return "TRACE_ENTRY_STUB";
-        case TRACE_STUB:            return "TRACE_STUB";
-        case TRACE_UNMANAGED:       return "TRACE_UNMANAGED";
-        case TRACE_MANAGED:         return "TRACE_MANAGED";
-        case TRACE_FRAME_PUSH:      return "TRACE_FRAME_PUSH";
-        case TRACE_MGR_PUSH:        return "TRACE_MGR_PUSH";
-        case TRACE_OTHER:           return "TRACE_OTHER";
-        case TRACE_UNJITTED_METHOD: return "TRACE_UNJITTED_METHOD";
+        case TRACE_ENTRY_STUB:                return "TRACE_ENTRY_STUB";
+        case TRACE_STUB:                      return "TRACE_STUB";
+        case TRACE_UNMANAGED:                 return "TRACE_UNMANAGED";
+        case TRACE_MANAGED:                   return "TRACE_MANAGED";
+        case TRACE_FRAME_PUSH:                return "TRACE_FRAME_PUSH";
+        case TRACE_MGR_PUSH:                  return "TRACE_MGR_PUSH";
+        case TRACE_OTHER:                     return "TRACE_OTHER";
+        case TRACE_UNJITTED_METHOD:           return "TRACE_UNJITTED_METHOD";
+        case TRACE_MULTICAST_DELEGATE_HELPER: return "TRACE_MULTICAST_DELEGATE_HELPER";
     }
     return "TRACE_REALLY_WACKED";
 }
@@ -117,6 +118,11 @@ const CHAR * TraceDestination::DbgToString(SString & buffer)
                 buffer.Printf("TRACE_MGR_PUSH(addr=%p, sm=%s)", GetAddress(), this->GetStubManager()->DbgGetName());
                 pValue = buffer.GetUTF8();
                 break;
+
+            case TRACE_MULTICAST_DELEGATE_HELPER:
+                buffer.Printf("TRACE_MULTICAST_DELEGATE_HELPER(addr=%p)", GetAddress());
+                pValue = buffer.GetUTF8();
+                break;
 
             case TRACE_OTHER:
                 pValue = "TRACE_OTHER";
@@ -1711,15 +1717,14 @@ BOOL ILStubManager::DoTraceStub(PCODE stubStartAddress,
     MethodDesc* pStubMD = ExecutionManager::GetCodeMethodDesc(stubStartAddress);
     if (pStubMD != NULL && pStubMD->AsDynamicMethodDesc()->IsMulticastStub())
     {
-        traceDestination = GetEEFuncEntryPoint(StubHelpers::MulticastDebuggerTraceHelper);
+        trace->InitForMulticastDelegateHelper(GetEEFuncEntryPoint(StubHelpers::MulticastDebuggerTraceHelper));
     }
     else
     {
         // This call is going out to unmanaged code, either through pinvoke or COM interop.
-        traceDestination = stubStartAddress;
+        trace->InitForManagerPush(stubStartAddress, this);
     }
 
-    trace->InitForManagerPush(traceDestination, this);
     LOG_TRACE_DESTINATION(trace, traceDestination, "ILStubManager::DoTraceStub");
 
     return TRUE;
@@ -1776,8 +1781,12 @@ BOOL ILStubManager::TraceManager(Thread *thread,
 
     // See code:ILStubCache.CreateNewMethodDesc for the code that sets flags on stub MDs
     PCODE target = (PCODE)NULL;
-
-    if (pStubMD->IsReverseStub())
+    if (pStubMD->IsMulticastStub())
+    {
+        _ASSERTE(!"We should never get here. Multicast Delegates should not invoke TraceManager.");
+        return FALSE;
+    }
+    else if (pStubMD->IsReverseStub())
     {
         if (pStubMD->IsStatic())
         {

diff --git a/src/coreclr/vm/stubmgr.h b/src/coreclr/vm/stubmgr.h
@@ -53,16 +53,17 @@
 // TraceType indicates what this 'target' is
 enum TraceType
 {
-    TRACE_ENTRY_STUB,               // Stub goes to an unmanaged entry stub
-    TRACE_STUB,                     // Stub goes to another stub
-    TRACE_UNMANAGED,                // Stub goes to unmanaged code
-    TRACE_MANAGED,                  // Stub goes to Jitted code
-    TRACE_UNJITTED_METHOD,          // Is the prestub, since there is no code, the address will actually be a MethodDesc*
+    TRACE_ENTRY_STUB,                // Stub goes to an unmanaged entry stub
+    TRACE_STUB,                      // Stub goes to another stub
+    TRACE_UNMANAGED,                 // Stub goes to unmanaged code
+    TRACE_MANAGED,                   // Stub goes to Jitted code
+    TRACE_UNJITTED_METHOD,           // Is the prestub, since there is no code, the address will actually be a MethodDesc*
 
-    TRACE_FRAME_PUSH,               // Don't know where stub goes, stop at address, and then ask the frame that is on the stack
-    TRACE_MGR_PUSH,                 // Don't know where stub goes, stop at address then call TraceManager() below to find out
+    TRACE_FRAME_PUSH,                // Don't know where stub goes, stop at address, and then ask the frame that is on the stack
+    TRACE_MGR_PUSH,                  // Don't know where stub goes, stop at address then call TraceManager() below to find out
+    TRACE_MULTICAST_DELEGATE_HELPER, // Stub goes to a multicast delegate helper
 
-    TRACE_OTHER                     // We are going somewhere you can't step into (eg. ee helper function)
+    TRACE_OTHER                      // We are going somewhere you can't step into (eg. ee helper function)
 };
 
 class StubManager;
@@ -147,6 +148,14 @@ class TraceDestination
         this->stubManager = NULL;
     }
 
+
+    void InitForMulticastDelegateHelper(PCODE addr)
+    {
+        this->type = TRACE_MULTICAST_DELEGATE_HELPER;
+        this->address = addr;
+        this->stubManager = NULL;
+    }
+
     // Nobody recognized the target address. We will not be able to step-in to it.
     // This is ok if the target just calls into mscorwks (such as an Fcall) because
     // there's no managed code to step in to, and we don't support debugging the CLR