Fix stepping issues with ReadyToRun and TieredCompilation (#66491)

* Fix stepping with ReadyToRun and TieredCompilation assemblies

Get the correct PCODE version for ReadyToRun methods when the StubManager
  is computing the address to step to.

* Update logging
Update GetJitInfoWorker to respect code versioning

Author: AaronRobinsonMSFT

src/coreclr/debug/ee/controller.cpp

@@ -1306,8 +1306,8 @@ bool DebuggerController::BindPatch(DebuggerControllerPatch *patch,
             return false;
         }
 
-        LOG((LF_CORDB,LL_INFO10000, "DC::BindPa: For startAddr 0x%x, got DJI "
-             "0x%x, from 0x%x size: 0x%x\n", startAddr, info, info->m_addrOfCode, info->m_sizeOfCode));
+        LOG((LF_CORDB,LL_INFO10000, "DC::BindPa: For startAddr 0x%p, got DJI "
+             "0x%p, from 0x%p size: 0x%x\n", startAddr, info, info->m_addrOfCode, info->m_sizeOfCode));
     }
 
     LOG((LF_CORDB, LL_INFO10000, "DC::BP:Trying to bind patch in %s::%s version %d\n",
@@ -1946,6 +1946,10 @@ BOOL DebuggerController::AddILPatch(AppDomain * pAppDomain, Module *module,
     BOOL fOk = FALSE;
 
     DebuggerMethodInfo *dmi = g_pDebugger->GetOrCreateMethodInfo(module, md); // throws
+    LOG((LF_CORDB,LL_INFO10000,"DC::AILP: dmi:0x%p, mdToken:0x%x, mdFilter:0x%p, "
+            "encVer:%zu, offset:0x%zx <- isIL:%d, Mod:0x%p\n",
+            dmi, md, pMethodDescFilter, encVersion, offset, offsetIsIL, module));
+
     if (dmi == NULL)
     {
         return false;
@@ -7173,7 +7177,7 @@ TP_RESULT DebuggerStepper::TriggerPatch(DebuggerControllerPatch *patch,
                 else
                 {
                     LOG((LF_CORDB, LL_INFO10000,
-                         "TSO for TRACE_MGR_PUSH case."));
+                         "TSO for TRACE_MGR_PUSH case. RetAddr: 0x%p\n", traceManagerRetAddr));
 
                     // We'd better have a valid return address.
                     _ASSERTE(traceManagerRetAddr != NULL);
@@ -7184,28 +7188,39 @@ TP_RESULT DebuggerStepper::TriggerPatch(DebuggerControllerPatch *patch,
                         DebuggerJitInfo *dji;
                         dji = g_pDebugger->GetJitInfoFromAddr((TADDR) traceManagerRetAddr);
 
-                        MethodDesc * mdNative = (dji == NULL) ?
-                            g_pEEInterface->GetNativeCodeMethodDesc(dac_cast<PCODE>(traceManagerRetAddr)) : dji->m_nativeCodeVersion.GetMethodDesc();
-                        _ASSERTE(mdNative != NULL);
+                        MethodDesc* mdNative = NULL;
+                        PCODE pcodeNative = NULL;
+                        if (dji != NULL)
+                        {
+                            mdNative = dji->m_nativeCodeVersion.GetMethodDesc();
+                            pcodeNative = dji->m_nativeCodeVersion.GetNativeCode();
+                        }
+                        else
+                        {
+                            // Find the method that the return is to.
+                            mdNative = g_pEEInterface->GetNativeCodeMethodDesc(dac_cast<PCODE>(traceManagerRetAddr));
+                            _ASSERTE(g_pEEInterface->GetFunctionAddress(mdNative) != NULL);
+                            pcodeNative = g_pEEInterface->GetFunctionAddress(mdNative);
+                        }
 
-                        // Find the method that the return is to.
-                        _ASSERTE(g_pEEInterface->GetFunctionAddress(mdNative) != NULL);
-                        SIZE_T offsetRet = dac_cast<TADDR>(traceManagerRetAddr -
-                            g_pEEInterface->GetFunctionAddress(mdNative));
+                        _ASSERTE(mdNative != NULL && pcodeNative != NULL);
+                        SIZE_T offsetRet = dac_cast<TADDR>(traceManagerRetAddr - pcodeNative);
+                        LOG((LF_CORDB, LL_INFO10000,
+                             "DS::TP: Before normally managed code AddPatch"
+                             " in %s::%s \n\tmd=0x%p, offset 0x%x, pcode=0x%p, dji=0x%p\n",
+                             mdNative->m_pszDebugClassName,
+                             mdNative->m_pszDebugMethodName,
+                             mdNative,
+                             offsetRet,
+                             pcodeNative,
+                             dji));
 
                         // Place the patch.
                         AddBindAndActivateNativeManagedPatch(mdNative,
                                  dji,
                                  offsetRet,
                                  LEAF_MOST_FRAME,
                                  NULL);
-
-                        LOG((LF_CORDB, LL_INFO10000,
-                             "DS::TP: normally managed code AddPatch"
-                             " in %s::%s, offset 0x%x\n",
-                             mdNative->m_pszDebugClassName,
-                             mdNative->m_pszDebugMethodName,
-                             offsetRet));
                     }
                     else
                     {

src/coreclr/debug/ee/debugger.cpp

@@ -2665,120 +2665,96 @@ DebuggerJitInfo *Debugger::GetJitInfo(MethodDesc *fd, const BYTE *pbAddr, Debugg
 // Internal worker to GetJitInfo. Doesn't validate parameters.
 DebuggerJitInfo *Debugger::GetJitInfoWorker(MethodDesc *fd, const BYTE *pbAddr, DebuggerMethodInfo **pMethInfo)
 {
+    CONTRACTL
+    {
+        THROWS;
+        GC_NOTRIGGER;
+        PRECONDITION(!g_pDebugger->HasDebuggerDataLock());
+    }
+    CONTRACTL_END;
 
     DebuggerMethodInfo *dmi = NULL;
     DebuggerJitInfo *dji = NULL;
 
+    if (pMethInfo)
+    {
+        *pMethInfo = NULL;
+    }
+
     // If we have a null MethodDesc - we're not going to get a jit-info. Do this check once at the top
     // rather than littered throughout the rest of this function.
     if (fd == NULL)
     {
-        LOG((LF_CORDB, LL_EVERYTHING, "Debugger::GetJitInfo, addr=0x%p - null fd - returning null\n", pbAddr));
+        LOG((LF_CORDB, LL_EVERYTHING, "D::GJIW: addr=0x%p - null fd - returning null\n", pbAddr));
         return NULL;
     }
-    else
-    {
-        CONSISTENCY_CHECK_MSGF(!fd->IsWrapperStub(), ("Can't get Jit-info for wrapper MDesc,'%s'", fd->m_pszDebugMethodName));
-    }
 
-    // The debugger doesn't track Lightweight-codegen methods b/c they have no metadata.
+    CONSISTENCY_CHECK_MSGF(!fd->IsWrapperStub(), ("Can't get Jit-info for wrapper MDesc,'%s'", fd->m_pszDebugMethodName));
+
+    // The debugger doesn't track dynamic methods b/c they have no metadata.
     if (fd->IsDynamicMethod())
     {
         return NULL;
     }
 
-
-    // initialize our out param
-    if (pMethInfo)
-    {
-        *pMethInfo = NULL;
-    }
-
-    LOG((LF_CORDB, LL_EVERYTHING, "Debugger::GetJitInfo called\n"));
-    //    CHECK_DJI_TABLE_DEBUGGER;
-
     // Find the DJI via the DMI
     //
     // One way to improve the perf, both in terms of memory usage, number of allocations
     // and lookup speeds would be to have the first JitInfo inline in the MethodInfo
     // struct.  After all, we never want to have a MethodInfo in the table without an
     // associated JitInfo, and this should bring us back very close to the old situation
     // in terms of perf.  But correctness comes first, and perf later...
-    //        CHECK_DMI_TABLE;
     dmi = GetOrCreateMethodInfo(fd->GetModule(), fd->GetMemberDef());
-
     if (dmi == NULL)
     {
         // If we can't create the DMI, we won't be able to create the DJI.
         return NULL;
     }
 
-    // TODO: Currently, this method does not handle code versioning properly (at least in some profiler scenarios), it may need
-    // to take pbAddr into account and lazily create a DJI for that particular version of the method.
-
-    // This may take the lock and lazily create an entry, so we do it up front.
-    dji = dmi->GetLatestJitInfo(fd);
-
-
-    DebuggerDataLockHolder debuggerDataLockHolder(this);
-
-    // Note the call to GetLatestJitInfo() will lazily create the first DJI if we don't already have one.
-    for (; dji != NULL; dji = dji->m_prevJitInfo)
+    if (pbAddr == NULL)
     {
-        if (PTR_TO_TADDR(dji->m_nativeCodeVersion.GetMethodDesc()) == PTR_HOST_TO_TADDR(fd))
-        {
-            break;
-        }
+        dji = dmi->GetLatestJitInfo(fd);
     }
-
-    LOG((LF_CORDB, LL_INFO1000, "D::GJI: for md:0x%p (%s::%s), got dmi:0x%p, dji:0x%p, latest dji:0x%p, latest fd:0x%p, prev dji:0x%p\n",
-        fd, fd->m_pszDebugClassName, fd->m_pszDebugMethodName,
-        dmi, dji, (dmi ? dmi->GetLatestJitInfo_NoCreate() : 0),
-        ((dmi && dmi->GetLatestJitInfo_NoCreate()) ? dmi->GetLatestJitInfo_NoCreate()->m_nativeCodeVersion.GetMethodDesc():0),
-        (dji?dji->m_prevJitInfo:0)));
-
-    if ((dji != NULL) && (pbAddr != NULL))
-    {
-        dji = dji->GetJitInfoByAddress(pbAddr);
-
-        // XXX Microsoft - dac doesn't support stub tracing
-        // so this just results in not-impl exceptions.
 #ifndef DACCESS_COMPILE
-        if (dji == NULL) //may have been given address of a thunk
+    else
+    {
+        PCODE startAddr = g_pEEInterface->GetNativeCodeStartAddress((PCODE)pbAddr);
+        if (startAddr == NULL)
         {
-            LOG((LF_CORDB,LL_INFO1000,"Couldn't find a DJI by address 0x%p, "
+            LOG((LF_CORDB,LL_INFO1000,"D::GJIW: Couldn't find a DJI by address 0x%p, "
                 "so it might be a stub or thunk\n", pbAddr));
-            TraceDestination trace;
 
-            g_pEEInterface->TraceStub((const BYTE *)pbAddr, &trace);
-
-            if ((trace.GetTraceType() == TRACE_MANAGED) && (pbAddr != (const BYTE *)trace.GetAddress()))
+            // if the address wasn't in jitted code we'll also check to see if it is a stub that leads to jitted code
+            TraceDestination trace;
+            (void)g_pEEInterface->TraceStub(pbAddr, &trace);
+            if(trace.GetTraceType() == TRACE_MANAGED && (PCODE)pbAddr != trace.GetAddress())
             {
-                LOG((LF_CORDB,LL_INFO1000,"Address thru thunk"
-                    ": 0x%p\n", trace.GetAddress()));
-                dji = GetJitInfo(fd, dac_cast<PTR_CBYTE>(trace.GetAddress()));
+                startAddr = trace.GetAddress();
+                LOG((LF_CORDB,LL_INFO1000,"D::GJIW: Address thru thunk: 0x%p\n", startAddr));
             }
 #ifdef LOGGING
             else
             {
-                _ASSERTE(trace.GetTraceType() != TRACE_UNJITTED_METHOD ||
-                    (fd == trace.GetMethodDesc()));
-                LOG((LF_CORDB,LL_INFO1000,"Address not thunked - "
+                _ASSERTE(trace.GetTraceType() != TRACE_UNJITTED_METHOD || (fd == trace.GetMethodDesc()));
+                LOG((LF_CORDB,LL_INFO1000,"D::GJIW: Address not thunked - "
                     "must be to unJITted method, or normal managed "
                     "method lacking a DJI!\n"));
             }
-#endif //LOGGING
+#endif // LOGGING
+        }
+
+        if (startAddr != NULL)
+        {
+            dji = dmi->FindOrCreateInitAndAddJitInfo(fd, startAddr);
         }
-#endif // #ifndef DACCESS_COMPILE
     }
+#endif // !DACCESS_COMPILE
 
     if (pMethInfo)
     {
         *pMethInfo = dmi;
     }
 
-    // DebuggerDataLockHolder out of scope - release implied
-
     return dji;
 }
 
@@ -2813,7 +2789,7 @@ DebuggerMethodInfo *Debugger::GetOrCreateMethodInfo(Module *pModule, mdMethodDef
     {
         info = CreateMethodInfo(pModule, token);
 
-        LOG((LF_CORDB, LL_INFO1000, "D::GOCMI: created DMI for mdToken:0x%x, dmi:0x%x\n",
+        LOG((LF_CORDB, LL_INFO1000, "D::GOCMI: created DMI for mdToken:0x%x, dmi:0x%p\n",
             token, info));
     }
 #endif // #ifndef DACCESS_COMPILE
@@ -3093,12 +3069,12 @@ CodeRegionInfo CodeRegionInfo::GetCodeRegionInfo(DebuggerJitInfo *dji, MethodDes
 
     if (dji && dji->m_addrOfCode)
     {
-        LOG((LF_CORDB, LL_EVERYTHING, "CRI::GCRI: simple case\n"));
+        LOG((LF_CORDB, LL_INFO10000, "CRI::GCRI: simple case: CodeRegionInfo* 0x%p\n", &dji->m_codeRegionInfo));
         return dji->m_codeRegionInfo;
     }
     else
     {
-        LOG((LF_CORDB, LL_EVERYTHING, "CRI::GCRI: more complex case\n"));
+        LOG((LF_CORDB, LL_INFO10000, "CRI::GCRI: more complex case\n"));
         CodeRegionInfo codeRegionInfo;
 
         // Use method desc from dji if present
@@ -3119,6 +3095,7 @@ CodeRegionInfo CodeRegionInfo::GetCodeRegionInfo(DebuggerJitInfo *dji, MethodDes
                      (addr == dac_cast<PTR_CORDB_ADDRESS_TYPE>(g_pEEInterface->GetFunctionAddress(md))));
         }
 
+        LOG((LF_CORDB, LL_INFO10000, "CRI::GCRI: Initializing CodeRegionInfo from 0x%p, md=0x%p\n", addr, md));
         if (addr)
         {
             PCODE pCode = PINSTRToPCODE(dac_cast<TADDR>(addr));
@@ -13961,7 +13938,7 @@ Debugger::InsertToMethodInfoList( DebuggerMethodInfo *dmi )
     }
     CONTRACTL_END;
 
-    LOG((LF_CORDB,LL_INFO10000,"D:IAHOL DMI: dmi:0x%08x\n", dmi));
+    LOG((LF_CORDB,LL_INFO10000,"D:IAHOL DMI: dmi:0x%p\n", dmi));
 
     HRESULT hr = S_OK;
 
@@ -13981,7 +13958,7 @@ Debugger::InsertToMethodInfoList( DebuggerMethodInfo *dmi )
 
     _ASSERTE((dmiPrev == NULL) || ((dmi->m_token == dmiPrev->m_token) && (dmi->m_module == dmiPrev->m_module)));
 
-    LOG((LF_CORDB,LL_INFO10000,"D:IAHOL: current head of dmi list:0x%08x\n",dmiPrev));
+    LOG((LF_CORDB,LL_INFO10000,"D:IAHOL: current head of dmi list:0x%p\n",dmiPrev));
 
     if (dmiPrev != NULL)
     {
@@ -14006,7 +13983,7 @@ Debugger::InsertToMethodInfoList( DebuggerMethodInfo *dmi )
     }
 #ifdef _DEBUG
     dmiPrev = m_pMethodInfos->GetMethodInfo(dmi->m_module, dmi->m_token);
-    LOG((LF_CORDB,LL_INFO10000,"D:IAHOL: new head of dmi list:0x%08x\n",
+    LOG((LF_CORDB,LL_INFO10000,"D:IAHOL: new head of dmi list:0x%p\n",
         dmiPrev));
 #endif //_DEBUG
 

src/coreclr/debug/ee/debugger.h

@@ -1604,10 +1604,6 @@ class DebuggerJitInfo
                               DWORD *which,
                               BOOL skipPrologs=FALSE);
 
-    // If a method has multiple copies of code (because of EnC or code-pitching),
-    // this returns the DJI corresponding to 'pbAddr'
-    DebuggerJitInfo *GetJitInfoByAddress(const BYTE *pbAddr );
-
     void Init(TADDR newAddress);
 
 #if defined(FEATURE_EH_FUNCLETS)

src/coreclr/debug/ee/frameinfo.cpp

@@ -601,13 +601,11 @@ DebuggerJitInfo * FrameInfo::GetJitInfoFromFrame() const
 
     DebuggerJitInfo *ji = NULL;
 
-    // @todo - we shouldn't need both a MD and an IP here.
     EX_TRY
     {
         _ASSERTE(this->md != NULL);
         ji = g_pDebugger->GetJitInfo(this->md, (const BYTE*)GetControlPC(&(this->registers)));
-        _ASSERTE(ji != NULL);
-        _ASSERTE(ji->m_nativeCodeVersion.GetMethodDesc() == this->md);
+        _ASSERTE(ji == NULL || ji->m_nativeCodeVersion.GetMethodDesc() == this->md);
     }
     EX_CATCH
     {