From bc0eda3d29e6c67bd929cb9ba5e77d0f34b1b9e6 Mon Sep 17 00:00:00 2001 From: Mikelle Date: Wed, 16 Aug 2023 13:48:23 -0700 Subject: [PATCH] Revert "Put HasNativeCodeReJITAware into GetFunctionAddress (#90049)" This reverts commit eacb32e400b3f9d3522bed193bd534d02e8b170a. Need to investigate changes because they caused a test failure. --- src/coreclr/debug/daccess/dacimpl.h | 11 ++++++ src/coreclr/debug/daccess/task.cpp | 4 +- src/coreclr/debug/di/breakpoint.cpp | 2 - src/coreclr/debug/ee/controller.cpp | 24 ++++++++++-- src/coreclr/debug/ee/debugger.cpp | 56 +++++++++++++++++++++------ src/coreclr/debug/ee/debugger.h | 4 +- src/coreclr/debug/ee/functioninfo.cpp | 12 +++++- src/coreclr/debug/inc/dbgipcevents.h | 1 - src/coreclr/vm/dbginterface.h | 7 +++- src/coreclr/vm/eedbginterfaceimpl.cpp | 1 + src/coreclr/vm/encee.cpp | 4 +- src/coreclr/vm/method.cpp | 19 ++++----- src/coreclr/vm/method.hpp | 8 ++-- 13 files changed, 113 insertions(+), 40 deletions(-) diff --git a/src/coreclr/debug/daccess/dacimpl.h b/src/coreclr/debug/daccess/dacimpl.h index 8de684a5dae9a..ddf61370f416e 100644 --- a/src/coreclr/debug/daccess/dacimpl.h +++ b/src/coreclr/debug/daccess/dacimpl.h @@ -1253,6 +1253,17 @@ class ClrDataAccess /* [out] */ union STUB_BUF* outBuffer, /* [out] */ ULONG32* outFlags); + DebuggerJitInfo* GetDebuggerJitInfo(MethodDesc* methodDesc, + TADDR addr) + { + if (g_pDebugger) + { + return g_pDebugger->GetJitInfo(methodDesc, (PBYTE)addr, NULL); + } + + return NULL; + } + HRESULT GetMethodExtents(MethodDesc* methodDesc, METH_EXTENTS** extents); HRESULT GetMethodVarInfo(MethodDesc* methodDesc, diff --git a/src/coreclr/debug/daccess/task.cpp b/src/coreclr/debug/daccess/task.cpp index ddbf251b7b982..9e428e81adef2 100644 --- a/src/coreclr/debug/daccess/task.cpp +++ b/src/coreclr/debug/daccess/task.cpp @@ -5225,7 +5225,7 @@ EnumMethodInstances::Next(ClrDataAccess* dac, } } - if (!m_methodIter.Current()->HasNativeCodeAnyVersion()) + if (!m_methodIter.Current()->HasNativeCodeReJITAware()) { goto NextMethod; } @@ -5243,7 +5243,7 @@ EnumMethodInstances::CdStart(MethodDesc* methodDesc, CLRDATA_ENUM* handle) { if (!methodDesc->HasClassOrMethodInstantiation() && - !(methodDesc->HasNativeCodeAnyVersion())) + !methodDesc->HasNativeCodeReJITAware()) { *handle = 0; return S_FALSE; diff --git a/src/coreclr/debug/di/breakpoint.cpp b/src/coreclr/debug/di/breakpoint.cpp index 568d7fc9fc66a..ad45df5c618ac 100644 --- a/src/coreclr/debug/di/breakpoint.cpp +++ b/src/coreclr/debug/di/breakpoint.cpp @@ -211,13 +211,11 @@ HRESULT CordbFunctionBreakpoint::Activate(BOOL fActivate) if (codeIsIL) { pEvent->BreakpointData.nativeCodeMethodDescToken = pEvent->BreakpointData.nativeCodeMethodDescToken.NullPtr(); - pEvent->BreakpointData.codeStartAddress = 0; } else { pEvent->BreakpointData.nativeCodeMethodDescToken = (m_code.GetValue()->AsNativeCode())->GetVMNativeCodeMethodDescToken().ToLsPtr(); - pEvent->BreakpointData.codeStartAddress = (m_code.GetValue()->AsNativeCode())->GetAddress(); } // Note: we're sending a two-way event, so it blocks here diff --git a/src/coreclr/debug/ee/controller.cpp b/src/coreclr/debug/ee/controller.cpp index 58e63ab399db2..7dd186b4113d4 100644 --- a/src/coreclr/debug/ee/controller.cpp +++ b/src/coreclr/debug/ee/controller.cpp @@ -1247,8 +1247,26 @@ bool DebuggerController::BindPatch(DebuggerControllerPatch *patch, startAddr = (CORDB_ADDRESS_TYPE *) CORDB_ADDRESS_TO_PTR(patch->GetDJI()->m_addrOfCode); _ASSERTE(startAddr != NULL); } - //We should never be calling this function with both a NULL startAddr and a DJI that doesn't have code. - _ASSERTE(startAddr != NULL); + if (startAddr == NULL) + { + // Should not be trying to place patches on MethodDecs's for stubs. + // These stubs will never get jitted. + CONSISTENCY_CHECK_MSGF(!pMD->IsWrapperStub(), ("Can't place patch at stub md %p, %s::%s", + pMD, pMD->m_pszDebugClassName, pMD->m_pszDebugMethodName)); + + startAddr = (CORDB_ADDRESS_TYPE *)g_pEEInterface->GetFunctionAddress(pMD); + // + // Code is not available yet to patch. The prestub should + // notify us when it is executed. + // + if (startAddr == NULL) + { + LOG((LF_CORDB, LL_INFO10000, + "DC::BP: Patch at 0x%zx not bindable yet.\n", patch->offset)); + + return false; + } + } } _ASSERTE(!g_pEEInterface->IsStub((const BYTE *)startAddr)); @@ -8638,7 +8656,7 @@ bool DebuggerFuncEvalComplete::SendEvent(Thread *thread, bool fIpChanged) // DebuggerEnCBreakpoint constructor - creates and activates a new EnC breakpoint // // Arguments: -// offset - IL offset in the function to place the patch +// offset - native offset in the function to place the patch // jitInfo - identifies the function in which the breakpoint is being placed // fTriggerType - breakpoint type: either REMAP_PENDING or REMAP_COMPLETE // pAppDomain - the breakpoint applies to the specified AppDomain only diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index d58a987244a8e..a44a9e235f36c 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -2841,8 +2841,6 @@ HRESULT Debugger::GetILToNativeMapping(PCODE pNativeCodeStartAddress, ULONG32 cM } CONTRACTL_END; - _ASSERTE(pNativeCodeStartAddress != NULL); - #ifdef PROFILING_SUPPORTED // At this point, we're pulling in the debugger. if (!HasLazyData()) @@ -3009,7 +3007,6 @@ HRESULT Debugger::GetILToNativeMappingIntoArrays( _ASSERTE(pcMap != NULL); _ASSERTE(prguiILOffset != NULL); _ASSERTE(prguiNativeOffset != NULL); - _ASSERTE(pNativeCodeStartAddress != NULL); // Any caller of GetILToNativeMappingIntoArrays had better call // InitializeLazyDataIfNecessary first! @@ -5414,6 +5411,28 @@ void Debugger::ReleaseAllRuntimeThreads(AppDomain *pAppDomain) g_pEEInterface->ResumeFromDebug(pAppDomain); } +// Given a method, get's its EnC version number. 1 if the method is not EnCed. +// Note that MethodDescs are reused between versions so this will give us +// the most recent EnC number. +int Debugger::GetMethodEncNumber(MethodDesc * pMethod) +{ + CONTRACTL + { + THROWS; + GC_NOTRIGGER; + } + CONTRACTL_END; + + DebuggerJitInfo * dji = GetLatestJitInfoFromMethodDesc(pMethod); + if (dji == NULL) + { + // If there's no DJI, couldn't have been EnCed. + return 1; + } + return (int) dji->m_encVersion; +} + + bool Debugger::IsJMCMethod(Module* pModule, mdMethodDef tkMethod) { CONTRACTL @@ -6200,6 +6219,25 @@ void Debugger::LockAndSendEnCRemapCompleteEvent(MethodDesc *pMD) Thread *thread = g_pEEInterface->GetThread(); // Note that the debugger lock is reentrant, so we may or may not hold it already. SENDIPCEVENT_BEGIN(this, thread); + + EX_TRY + { + // Ensure the DJI for the latest version of this method has been pre-created. + // It's not clear whether this is necessary or not, but it shouldn't hurt since + // we're going to need to create it anyway since we'll be debugging inside it. + DebuggerJitInfo *dji = g_pDebugger->GetLatestJitInfoFromMethodDesc(pMD); + (void)dji; //prevent "unused variable" error from GCC + _ASSERTE( dji != NULL ); + } + EX_CATCH + { + // GetLatestJitInfo could throw on OOM, but the debugger isn't resiliant to OOM. + // I'm not aware of any other legitimate reason why it may throw, so we'll ASSERT + // if it fails. + _ASSERTE(!"Unexpected exception from Debugger::GetLatestJitInfoFromMethodDesc on EnC remap complete"); + } + EX_END_CATCH(RethrowTerminalExceptions); + // Send an EnC remap complete event to the Right Side. DebuggerIPCEvent* ipce = m_pRCThread->GetIPCEventSendBuffer(); InitIPCEvent(ipce, @@ -7827,7 +7865,6 @@ void Debugger::FirstChanceManagedExceptionCatcherFound(Thread *pThread, // Implements DebugInterface // Call by EE/exception. Must be on managed thread _ASSERTE(GetThreadNULLOk() != NULL); - _ASSERTE(pMethodAddr != NULL); // Quick check. if (!CORDebuggerAttached()) @@ -10461,7 +10498,7 @@ bool Debugger::HandleIPCEvent(DebuggerIPCEvent * pEvent) DebuggerJitInfo * pDJI = NULL; if ((pMethodDesc != NULL) && (pDMI != NULL)) { - pDJI = pDMI->FindOrCreateInitAndAddJitInfo(pMethodDesc, PINSTRToPCODE(dac_cast(pEvent->BreakpointData.codeStartAddress))); + pDJI = pDMI->FindOrCreateInitAndAddJitInfo(pMethodDesc, NULL /* startAddr */); } { @@ -12588,7 +12625,7 @@ DWORD Debugger::GetThreadIdHelper(Thread *pThread) // does not own the memory provided via vars outparameter. //----------------------------------------------------------------------------- void Debugger::GetVarInfo(MethodDesc * fd, // [IN] method of interest - CORDB_ADDRESS nativeCodeAddress, // [IN] which edit version + void *DebuggerVersionToken, // [IN] which edit version SIZE_T * cVars, // [OUT] size of 'vars' const ICorDebugInfo::NativeVarInfo **vars // [OUT] map telling where local vars are stored ) @@ -12600,7 +12637,7 @@ void Debugger::GetVarInfo(MethodDesc * fd, // [IN] method of interest } CONTRACTL_END; - DebuggerJitInfo * ji = g_pDebugger->GetJitInfo(fd, (const BYTE *)nativeCodeAddress); + DebuggerJitInfo * ji = (DebuggerJitInfo *)DebuggerVersionToken; // If we didn't supply a DJI, then we're asking for the most recent version. if (ji == NULL) @@ -12924,11 +12961,6 @@ HRESULT Debugger::UpdateFunction(MethodDesc* pMD, SIZE_T encVersion) // For each offset in the IL->Native map, set a new EnC breakpoint on the // ones that we know could be remap points. - - // Depending on which DJI was picked, the code might compute different IL offsets. The JIT may not guarantee it produces - // the same set of sequence points for every generic instantiation. - // Inside ENCSequencePointHelper there is logic that skips IL offsets that map to the same native offset. - // Its possible that one version of the code maps two IL offsets to the same native offset but another version of the code maps them to different offsets. PTR_DebuggerILToNativeMap seqMap = pJitInfo->GetSequenceMap(); for (unsigned int i = 0; i < pJitInfo->GetSequenceMapCount(); i++) { diff --git a/src/coreclr/debug/ee/debugger.h b/src/coreclr/debug/ee/debugger.h index 2c2440ddaf697..26edd26a96140 100644 --- a/src/coreclr/debug/ee/debugger.h +++ b/src/coreclr/debug/ee/debugger.h @@ -1933,6 +1933,8 @@ class Debugger : public DebugInterface bool IsJMCMethod(Module* pModule, mdMethodDef tkMethod); + int GetMethodEncNumber(MethodDesc * pMethod); + bool FirstChanceManagedException(Thread *pThread, SIZE_T currentIP, SIZE_T currentSP); @@ -1978,7 +1980,7 @@ class Debugger : public DebugInterface #endif // EnC_SUPPORTED void GetVarInfo(MethodDesc * fd, // [IN] method of interest - CORDB_ADDRESS nativeCodeAddress, // [IN] which edit version + void *DebuggerVersionToken, // [IN] which edit version SIZE_T * cVars, // [OUT] size of 'vars' const ICorDebugInfo::NativeVarInfo **vars // [OUT] map telling where local vars are stored ); diff --git a/src/coreclr/debug/ee/functioninfo.cpp b/src/coreclr/debug/ee/functioninfo.cpp index 19910c6429a9c..76d4be3ab232f 100644 --- a/src/coreclr/debug/ee/functioninfo.cpp +++ b/src/coreclr/debug/ee/functioninfo.cpp @@ -1565,7 +1565,9 @@ DebuggerJitInfo *DebuggerMethodInfo::FindOrCreateInitAndAddJitInfo(MethodDesc* f GC_NOTRIGGER; } CONTRACTL_END; + _ASSERTE(fd != NULL); + // The debugger doesn't track Lightweight-codegen methods b/c they have no metadata. if (fd->IsDynamicMethod()) { @@ -1574,8 +1576,16 @@ DebuggerJitInfo *DebuggerMethodInfo::FindOrCreateInitAndAddJitInfo(MethodDesc* f if (startAddr == NULL) { + // This will grab the start address for the current code version. startAddr = g_pEEInterface->GetFunctionAddress(fd); - _ASSERTE(startAddr != NULL); + if (startAddr == NULL) + { + startAddr = fd->GetNativeCodeReJITAware(); + if (startAddr == NULL) + { + return NULL; + } + } } else { diff --git a/src/coreclr/debug/inc/dbgipcevents.h b/src/coreclr/debug/inc/dbgipcevents.h index e9643e50f480a..9fe1afd31a54b 100644 --- a/src/coreclr/debug/inc/dbgipcevents.h +++ b/src/coreclr/debug/inc/dbgipcevents.h @@ -2011,7 +2011,6 @@ struct MSLAYOUT DebuggerIPCEvent SIZE_T offset; SIZE_T encVersion; LSPTR_METHODDESC nativeCodeMethodDescToken; // points to the MethodDesc if !isIL - CORDB_ADDRESS codeStartAddress; } BreakpointData; struct MSLAYOUT diff --git a/src/coreclr/vm/dbginterface.h b/src/coreclr/vm/dbginterface.h index 85b9785bccbb9..daa57d25c86cf 100644 --- a/src/coreclr/vm/dbginterface.h +++ b/src/coreclr/vm/dbginterface.h @@ -203,7 +203,7 @@ class DebugInterface // Get debugger variable information for a specific version of a method virtual void GetVarInfo(MethodDesc * fd, // [IN] method of interest - CORDB_ADDRESS nativeCodeAddress, // [IN] which edit version + void *DebuggerVersionToken, // [IN] which edit version SIZE_T * cVars, // [OUT] size of 'vars' const ICorDebugInfo::NativeVarInfo **vars // [OUT] map telling where local vars are stored ) = 0; @@ -262,6 +262,11 @@ class DebugInterface virtual bool IsJMCMethod(Module* pModule, mdMethodDef tkMethod) = 0; + // Given a method, get's its EnC version number. 1 if the method is not EnCed. + // Note that MethodDescs are reused between versions so this will give us + // the most recent EnC number. + virtual int GetMethodEncNumber(MethodDesc * pMethod) = 0; + virtual void SendLogSwitchSetting (int iLevel, int iReason, _In_z_ LPCWSTR pLogSwitchName, diff --git a/src/coreclr/vm/eedbginterfaceimpl.cpp b/src/coreclr/vm/eedbginterfaceimpl.cpp index 352a534d5c1a8..792c608918a61 100644 --- a/src/coreclr/vm/eedbginterfaceimpl.cpp +++ b/src/coreclr/vm/eedbginterfaceimpl.cpp @@ -630,6 +630,7 @@ PCODE EEDbgInterfaceImpl::GetFunctionAddress(MethodDesc *pFD) SUPPORTS_DAC; } CONTRACTL_END; + return pFD->GetNativeCode(); } diff --git a/src/coreclr/vm/encee.cpp b/src/coreclr/vm/encee.cpp index 3339462ad7fe7..1dcfb8bf091f4 100644 --- a/src/coreclr/vm/encee.cpp +++ b/src/coreclr/vm/encee.cpp @@ -806,8 +806,8 @@ NOINLINE void EditAndContinueModule::FixContextAndResume( // Get the var info which the codemanager will use for updating // enregistered variables correctly, or variables whose lifetimes differ // at the update point - g_pDebugInterface->GetVarInfo(pMD, oldCodeInfo.GetCodeAddress(), &oldVarInfoCount, &pOldVarInfo); - g_pDebugInterface->GetVarInfo(pMD, newCodeInfo.GetCodeAddress(), &newVarInfoCount, &pNewVarInfo); + g_pDebugInterface->GetVarInfo(pMD, oldDebuggerFuncHandle, &oldVarInfoCount, &pOldVarInfo); + g_pDebugInterface->GetVarInfo(pMD, NULL, &newVarInfoCount, &pNewVarInfo); #ifdef TARGET_X86 // save the frame pointer as FixContextForEnC might step on it. diff --git a/src/coreclr/vm/method.cpp b/src/coreclr/vm/method.cpp index 29910d6cb4c1c..62b24e3dc091c 100644 --- a/src/coreclr/vm/method.cpp +++ b/src/coreclr/vm/method.cpp @@ -913,6 +913,7 @@ PCODE MethodDesc::GetNativeCode() WRAPPER_NO_CONTRACT; SUPPORTS_DAC; _ASSERTE(!IsDefaultInterfaceMethod() || HasNativeCodeSlot()); + if (HasNativeCodeSlot()) { // When profiler is enabled, profiler may ask to rejit a code even though we @@ -934,7 +935,7 @@ PCODE MethodDesc::GetNativeCode() return GetStableEntryPoint(); } -PCODE MethodDesc::GetNativeCodeAnyVersion() +PCODE MethodDesc::GetNativeCodeReJITAware() { WRAPPER_NO_CONTRACT; SUPPORTS_DAC; @@ -945,23 +946,19 @@ PCODE MethodDesc::GetNativeCodeAnyVersion() return pDefaultCode; } - else { CodeVersionManager *pCodeVersionManager = GetCodeVersionManager(); CodeVersionManager::LockHolder codeVersioningLockHolder; - ILCodeVersionCollection ilVersionCollection = pCodeVersionManager->GetILCodeVersions(PTR_MethodDesc(this)); - for (ILCodeVersionIterator curIL = ilVersionCollection.Begin(), endIL = ilVersionCollection.End(); curIL != endIL; curIL++) + ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(PTR_MethodDesc(this)); + if (!ilVersion.IsDefaultVersion()) { - NativeCodeVersionCollection nativeCollection = curIL->GetNativeCodeVersions(PTR_MethodDesc(this)); - for (NativeCodeVersionIterator curNative = nativeCollection.Begin(), endNative = nativeCollection.End(); curNative != endNative; curNative++) + NativeCodeVersion activeNativeCodeVersion = ilVersion.GetActiveNativeCodeVersion(PTR_MethodDesc(this)); + if (!activeNativeCodeVersion.IsNull()) { - PCODE native = curNative->GetNativeCode(); - if(native != NULL) - { - return native; - } + return activeNativeCodeVersion.GetNativeCode(); } } + return NULL; } } diff --git a/src/coreclr/vm/method.hpp b/src/coreclr/vm/method.hpp index aaecfb20779f3..4b34045b57671 100644 --- a/src/coreclr/vm/method.hpp +++ b/src/coreclr/vm/method.hpp @@ -1373,11 +1373,11 @@ class MethodDesc } // Perf warning: takes the CodeVersionManagerLock on every call - BOOL HasNativeCodeAnyVersion() + BOOL HasNativeCodeReJITAware() { LIMITED_METHOD_DAC_CONTRACT; - return GetNativeCodeAnyVersion() != NULL; + return GetNativeCodeReJITAware() != NULL; } BOOL SetNativeCodeInterlocked(PCODE addr, PCODE pExpected = NULL); @@ -1437,9 +1437,9 @@ class MethodDesc PCODE GetNativeCode(); // Returns GetNativeCode() if it exists, but also checks to see if there - // is a non-default code version that is populated with a code body and returns that. + // is a non-default IL code version and returns that. // Perf warning: takes the CodeVersionManagerLock on every call - PCODE GetNativeCodeAnyVersion(); + PCODE GetNativeCodeReJITAware(); #if defined(FEATURE_JIT_PITCHING) bool IsPitchable();