From 5e560d9ee5d1576aa8b085b54af5f55de83ebe03 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 10 Sep 2024 17:30:58 -0700 Subject: [PATCH] Remove helper method frame from Profiler hooks (#107152) Remove helper method frame and update the CONTRACTs to suit the new behavior. This WILL change the profiler behavior in that a set of apis in the Profiler api surface will not fail immediately on any attempt to use them. However, the set of apis which change are I believe not safe to call within a ProfilerEnter/Leave hook as an actual triggered GC will cause potential memory corruption if the arguments/return value of the function have any GC pointers. --- src/coreclr/debug/daccess/dacdbiimpl.cpp | 34 +-------- src/coreclr/debug/daccess/request.cpp | 25 +++++-- src/coreclr/debug/daccess/task.cpp | 31 +++------ src/coreclr/debug/ee/debugger.cpp | 2 +- src/coreclr/debug/ee/functioninfo.cpp | 16 +++-- src/coreclr/inc/daccess.h | 6 ++ src/coreclr/vm/appdomain.cpp | 89 +++++------------------- src/coreclr/vm/appdomain.hpp | 10 +-- src/coreclr/vm/ceeload.inl | 1 - src/coreclr/vm/codeversion.cpp | 26 +++---- src/coreclr/vm/codeversion.h | 34 ++++----- src/coreclr/vm/debuginfostore.cpp | 2 +- src/coreclr/vm/multicorejitplayer.cpp | 2 +- src/coreclr/vm/proftoeeinterfaceimpl.cpp | 80 ++++++++------------- src/coreclr/vm/rejit.cpp | 45 ++---------- src/coreclr/vm/rejit.h | 1 - 16 files changed, 131 insertions(+), 273 deletions(-) diff --git a/src/coreclr/debug/daccess/dacdbiimpl.cpp b/src/coreclr/debug/daccess/dacdbiimpl.cpp index 223d675df254a..77ed5df2327cc 100644 --- a/src/coreclr/debug/daccess/dacdbiimpl.cpp +++ b/src/coreclr/debug/daccess/dacdbiimpl.cpp @@ -649,39 +649,7 @@ void DacDbiInterfaceImpl::GetAppDomainFullName( AppDomain * pAppDomain = vmAppDomain.GetDacPtr(); // Get the AppDomain name from the VM without changing anything - // We might be able to simplify this, eg. by returning an SString. - bool fIsUtf8; - PVOID pRawName = pAppDomain->GetFriendlyNameNoSet(&fIsUtf8); - - if (!pRawName) - { - ThrowHR(E_NOINTERFACE); - } - - HRESULT hrStatus = S_OK; - if (fIsUtf8) - { - // we have to allocate a temporary string - // we could avoid this by adding a version of IStringHolder::AssignCopy that takes a UTF8 string - // We should also probably check to see when fIsUtf8 is ever true (it looks like it should normally be false). - ULONG32 dwNameLen = 0; - hrStatus = ConvertUtf8((LPCUTF8)pRawName, 0, &dwNameLen, NULL); - if (SUCCEEDED( hrStatus )) - { - NewArrayHolder pwszName(new WCHAR[dwNameLen]); - hrStatus = ConvertUtf8((LPCUTF8)pRawName, dwNameLen, &dwNameLen, pwszName ); - IfFailThrow(hrStatus); - - hrStatus = pStrName->AssignCopy(pwszName); - } - } - else - { - hrStatus = pStrName->AssignCopy(static_cast(pRawName)); - } - - // Very important that this either sets pStrName or Throws. - IfFailThrow(hrStatus); + IfFailThrow(pStrName->AssignCopy(pAppDomain->GetFriendlyName())); } //- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - diff --git a/src/coreclr/debug/daccess/request.cpp b/src/coreclr/debug/daccess/request.cpp index 5e73d02325112..1b886e300471f 100644 --- a/src/coreclr/debug/daccess/request.cpp +++ b/src/coreclr/debug/daccess/request.cpp @@ -3040,24 +3040,39 @@ ClrDataAccess::GetAppDomainName(CLRDATA_ADDRESS addr, unsigned int count, _Inout // SystemDomain doesn't have this field. if (pNeeded) *pNeeded = 1; - if (name) + if (name && count > 0) name[0] = 0; } else { PTR_AppDomain pAppDomain = PTR_AppDomain(TO_TADDR(addr)); - if (!pAppDomain->m_friendlyName.IsEmpty()) + + size_t countAsSizeT = count; + if (pAppDomain->m_friendlyName.IsValid()) { - if (!pAppDomain->m_friendlyName.DacGetUnicode(count, name, pNeeded)) + LPCWSTR friendlyName = (LPCWSTR)pAppDomain->m_friendlyName; + size_t friendlyNameLen = u16_strlen(friendlyName); + + if (pNeeded) { - hr = E_FAIL; + *pNeeded = (unsigned int)(friendlyNameLen + 1); + } + + if (name && count > 0) + { + if (countAsSizeT > (friendlyNameLen + 1)) + { + countAsSizeT = friendlyNameLen + 1; + } + memcpy(name, friendlyName, countAsSizeT * sizeof(WCHAR)); + name[countAsSizeT - 1] = 0; } } else { if (pNeeded) *pNeeded = 1; - if (name) + if (name && count > 0) name[0] = 0; hr = S_OK; diff --git a/src/coreclr/debug/daccess/task.cpp b/src/coreclr/debug/daccess/task.cpp index ca597485584ba..085c0e551ae8d 100644 --- a/src/coreclr/debug/daccess/task.cpp +++ b/src/coreclr/debug/daccess/task.cpp @@ -713,30 +713,21 @@ ClrDataAppDomain::GetName( EX_TRY { - bool isUtf8; - PVOID rawName = m_appDomain->GetFriendlyNameNoSet(&isUtf8); + LPCWSTR rawName = m_appDomain->GetFriendlyName(); if (rawName) { - if (isUtf8) - { - status = ConvertUtf8((LPCUTF8)rawName, - bufLen, nameLen, name); - } - else + status = StringCchCopy(name, bufLen, rawName) == S_OK ? + S_OK : S_FALSE; + if (nameLen) { - status = StringCchCopy(name, bufLen, (PCWSTR)rawName) == S_OK ? - S_OK : S_FALSE; - if (nameLen) + size_t cchName = u16_strlen(rawName) + 1; + if (FitsIn(cchName)) { - size_t cchName = u16_strlen((PCWSTR)rawName) + 1; - if (FitsIn(cchName)) - { - *nameLen = (ULONG32) cchName; - } - else - { - status = COR_E_OVERFLOW; - } + *nameLen = (ULONG32) cchName; + } + else + { + status = COR_E_OVERFLOW; } } } diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index f2682725f0fd3..0e9d93f26c89a 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -2832,7 +2832,7 @@ HRESULT Debugger::GetILToNativeMapping(PCODE pNativeCodeStartAddress, ULONG32 cM CONTRACTL { THROWS; - GC_TRIGGERS_FROM_GETJITINFO; + GC_NOTRIGGER; } CONTRACTL_END; diff --git a/src/coreclr/debug/ee/functioninfo.cpp b/src/coreclr/debug/ee/functioninfo.cpp index 96c50bfa84afb..d1765d176fb39 100644 --- a/src/coreclr/debug/ee/functioninfo.cpp +++ b/src/coreclr/debug/ee/functioninfo.cpp @@ -926,8 +926,6 @@ void DebuggerJitInfo::LazyInitBounds() LOG((LF_CORDB,LL_EVERYTHING, "DJI::LazyInitBounds: this=%p GetBoundariesAndVars success=%s\n", this, (fSuccess ? "true" : "false"))); - // SetBoundaries uses the CodeVersionManager, need to take it now for lock ordering reasons - CodeVersionManager::LockHolder codeVersioningLockHolder; Debugger::DebuggerDataLockHolder debuggerDataLockHolder(g_pDebugger); if (!m_fAttemptInit) @@ -1053,15 +1051,22 @@ void DebuggerJitInfo::SetBoundaries(ULONG32 cMap, ICorDebugInfo::OffsetMapping * // Pick a unique initial value (-10) so that the 1st doesn't accidentally match. int ilPrevOld = -10; - _ASSERTE(CodeVersionManager::IsLockOwnedByCurrentThread()); - InstrumentedILOffsetMapping mapping; ILCodeVersion ilVersion = m_nativeCodeVersion.GetILCodeVersion(); if (!ilVersion.IsDefaultVersion()) { // Did the current rejit provide a map? - const InstrumentedILOffsetMapping *pReJitMap = ilVersion.GetInstrumentedILMap(); + const InstrumentedILOffsetMapping *pReJitMap = NULL; + if (ilVersion.GetRejitState() == ILCodeVersion::kStateActive) + { + pReJitMap = ilVersion.GetInstrumentedILMap(); + } + else + { + _ASSERTE(!"Unexpected rejit state, should be active as there exists a native code version for this IL code version"); + } + if (pReJitMap != NULL) { mapping = *pReJitMap; @@ -1607,7 +1612,6 @@ DebuggerJitInfo *DebuggerMethodInfo::FindOrCreateInitAndAddJitInfo(MethodDesc* f if (fd->IsVersionable()) { CodeVersionManager *pCodeVersionManager = fd->GetCodeVersionManager(); - CodeVersionManager::LockHolder codeVersioningLockHolder; nativeCodeVersion = pCodeVersionManager->GetNativeCodeVersion(fd, startAddr); if (nativeCodeVersion.IsNull()) { diff --git a/src/coreclr/inc/daccess.h b/src/coreclr/inc/daccess.h index 74aeed1c9bb06..3c6a1b85a3511 100644 --- a/src/coreclr/inc/daccess.h +++ b/src/coreclr/inc/daccess.h @@ -2469,6 +2469,12 @@ typedef DPTR(T_RUNTIME_FUNCTION) PTR_RUNTIME_FUNCTION; #endif #endif +#ifdef DACCESS_COMPILE +#define DAC_IGNORE(x) +#else +#define DAC_IGNORE(x) x +#endif // DACCESS_COMPILE + //---------------------------------------------------------------------------- // // A PCODE is a valid PC/IP value -- a pointer to an instruction, possibly including some processor mode bits. diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index 287823cac17f6..157f32ebbd437 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -1610,6 +1610,7 @@ AppDomain::AppDomain() , m_hndMissing{NULL} #endif //FEATURE_COMINTEROP , m_pDelayedLoaderAllocatorUnloadList{NULL} + , m_friendlyName{NULL} , m_pRootAssembly{NULL} , m_dwFlags{0} , m_cRef{1} @@ -2803,7 +2804,7 @@ DomainAssembly * AppDomain::FindAssembly(PEAssembly * pPEAssembly, FindAssemblyO return NULL; } -void AppDomain::SetFriendlyName(LPCWSTR pwzFriendlyName, BOOL fDebuggerCares/*=TRUE*/) +void AppDomain::SetFriendlyName(LPCWSTR pwzFriendlyName) { CONTRACTL { @@ -2821,29 +2822,15 @@ void AppDomain::SetFriendlyName(LPCWSTR pwzFriendlyName, BOOL fDebuggerCares/*=T if (pwzFriendlyName) tmpFriendlyName.Set(pwzFriendlyName); else - { - // If there is an assembly, try to get the name from it. - // If no assembly, but if it's the DefaultDomain, then give it a name - - if (m_pRootAssembly) - { - tmpFriendlyName.SetUTF8(m_pRootAssembly->GetSimpleName()); - - SString::Iterator i = tmpFriendlyName.End(); - if (tmpFriendlyName.FindBack(i, '.')) - tmpFriendlyName.Truncate(i); - } - else - { - tmpFriendlyName.Set(DEFAULT_DOMAIN_FRIENDLY_NAME); - } - } + tmpFriendlyName.Set(DEFAULT_DOMAIN_FRIENDLY_NAME); tmpFriendlyName.Normalize(); + // This happens at most twice in a process, so don't worry about freeing the old one. + LPWSTR newFriendlyName = new WCHAR[tmpFriendlyName.GetCount() + 1]; + u16_strcpy_s(newFriendlyName, tmpFriendlyName.GetCount() + 1, tmpFriendlyName.GetUnicode()); - m_friendlyName = tmpFriendlyName; - m_friendlyName.Normalize(); + m_friendlyName = newFriendlyName; if(g_pDebugInterface) { @@ -2851,42 +2838,32 @@ void AppDomain::SetFriendlyName(LPCWSTR pwzFriendlyName, BOOL fDebuggerCares/*=T if (SUCCEEDED(g_pDebugInterface->UpdateAppDomainEntryInIPC(this))) { // inform the attached debugger that the name of this appdomain has changed. - if (IsDebuggerAttached() && fDebuggerCares) + if (IsDebuggerAttached()) g_pDebugInterface->NameChangeEvent(this, NULL); } } } +#endif // !DACCESS_COMPILE -LPCWSTR AppDomain::GetFriendlyName(BOOL fDebuggerCares/*=TRUE*/) +LPCWSTR AppDomain::GetFriendlyName() { CONTRACT (LPCWSTR) { - THROWS; - if (GetThreadNULLOk()) {GC_TRIGGERS;} else {DISABLED(GC_NOTRIGGER);} + NOTHROW; + GC_NOTRIGGER; MODE_ANY; POSTCONDITION(CheckPointer(RETVAL, NULL_OK)); INJECT_FAULT(COMPlusThrowOM();); } CONTRACT_END; - if (m_friendlyName.IsEmpty()) - SetFriendlyName(NULL, fDebuggerCares); + if (m_friendlyName == NULL) + RETURN DEFAULT_DOMAIN_FRIENDLY_NAME; - RETURN m_friendlyName; + RETURN (LPCWSTR)m_friendlyName; } -LPCWSTR AppDomain::GetFriendlyNameForLogging() -{ - CONTRACT(LPCWSTR) - { - NOTHROW; - GC_NOTRIGGER; - MODE_ANY; - POSTCONDITION(CheckPointer(RETVAL,NULL_OK)); - } - CONTRACT_END; - RETURN (m_friendlyName.IsEmpty() ?W(""):(LPCWSTR)m_friendlyName); -} +#ifndef DACCESS_COMPILE LPCWSTR AppDomain::GetFriendlyNameForDebugger() { @@ -2900,7 +2877,7 @@ LPCWSTR AppDomain::GetFriendlyNameForDebugger() CONTRACT_END; - if (m_friendlyName.IsEmpty()) + if (m_friendlyName == NULL) { BOOL fSuccess = FALSE; @@ -2928,36 +2905,6 @@ LPCWSTR AppDomain::GetFriendlyNameForDebugger() #endif // !DACCESS_COMPILE -#ifdef DACCESS_COMPILE - -PVOID AppDomain::GetFriendlyNameNoSet(bool* isUtf8) -{ - SUPPORTS_DAC; - - if (!m_friendlyName.IsEmpty()) - { - *isUtf8 = false; - return m_friendlyName.DacGetRawContent(); - } - else if (m_pRootAssembly) - { - *isUtf8 = true; - return (PVOID)m_pRootAssembly->GetSimpleName(); - } - else if (dac_cast(this) == - dac_cast(SystemDomain::System()->DefaultDomain())) - { - *isUtf8 = false; - return (PVOID)DEFAULT_DOMAIN_FRIENDLY_NAME; - } - else - { - return NULL; - } -} - -#endif // DACCESS_COMPILE - #ifndef DACCESS_COMPILE BOOL AppDomain::AddFileToCache(AssemblySpec* pSpec, PEAssembly * pPEAssembly) @@ -4480,7 +4427,7 @@ AppDomain::EnumMemoryRegions(CLRDataEnumMemoryFlags flags, bool enumThis) // We don't need AppDomain name in triage dumps. if (flags != CLRDATA_ENUM_MEM_TRIAGE) { - m_friendlyName.EnumMemoryRegions(flags); + m_friendlyName.EnumMem(); } if (flags == CLRDATA_ENUM_MEM_HEAP2) diff --git a/src/coreclr/vm/appdomain.hpp b/src/coreclr/vm/appdomain.hpp index ef026a3704de7..5f8ad63a07113 100644 --- a/src/coreclr/vm/appdomain.hpp +++ b/src/coreclr/vm/appdomain.hpp @@ -1207,13 +1207,9 @@ class AppDomain : public BaseDomain ULONG Release(void) DAC_EMPTY_RET(0); //**************************************************************************************** - LPCWSTR GetFriendlyName(BOOL fDebuggerCares = TRUE); + LPCWSTR GetFriendlyName(); LPCWSTR GetFriendlyNameForDebugger(); - LPCWSTR GetFriendlyNameForLogging(); -#ifdef DACCESS_COMPILE - PVOID GetFriendlyNameNoSet(bool* isUtf8); -#endif - void SetFriendlyName(LPCWSTR pwzFriendlyName, BOOL fDebuggerCares = TRUE); + void SetFriendlyName(LPCWSTR pwzFriendlyName); PEAssembly * BindAssemblySpec( AssemblySpec *pSpec, @@ -1522,7 +1518,7 @@ class AppDomain : public BaseDomain SPTR_DECL(AppDomain, m_pTheAppDomain); private: - SString m_friendlyName; + PTR_CWSTR m_friendlyName; PTR_Assembly m_pRootAssembly; // General purpose flags. diff --git a/src/coreclr/vm/ceeload.inl b/src/coreclr/vm/ceeload.inl index 1fa6588d99101..06a5001c23b22 100644 --- a/src/coreclr/vm/ceeload.inl +++ b/src/coreclr/vm/ceeload.inl @@ -285,7 +285,6 @@ inline PTR_ILCodeVersioningState Module::LookupILCodeVersioningState(mdMethodDef } CONTRACTL_END - _ASSERTE(CodeVersionManager::IsLockOwnedByCurrentThread()); _ASSERTE(TypeFromToken(token) == mdtMethodDef); return m_ILCodeVersioningStateMap.GetElement(RidFromToken(token)); } diff --git a/src/coreclr/vm/codeversion.cpp b/src/coreclr/vm/codeversion.cpp index 846168cf5cdf4..04de0dc003be7 100644 --- a/src/coreclr/vm/codeversion.cpp +++ b/src/coreclr/vm/codeversion.cpp @@ -94,12 +94,6 @@ ReJITID NativeCodeVersionNode::GetILVersionId() const ILCodeVersion NativeCodeVersionNode::GetILCodeVersion() const { LIMITED_METHOD_DAC_CONTRACT; -#ifdef DEBUG - if (GetILVersionId() != 0) - { - _ASSERTE(CodeVersionManager::IsLockOwnedByCurrentThread()); - } -#endif PTR_MethodDesc pMD = GetMethodDesc(); return pMD->GetCodeVersionManager()->GetILCodeVersion(pMD, GetILVersionId()); } @@ -169,7 +163,7 @@ void NativeCodeVersionNode::SetOptimizationTier(NativeCodeVersion::OptimizationT #ifdef FEATURE_ON_STACK_REPLACEMENT -PatchpointInfo* NativeCodeVersionNode::GetOSRInfo(unsigned * ilOffset) +PatchpointInfo* NativeCodeVersionNode::GetOSRInfo(unsigned * ilOffset) const { LIMITED_METHOD_DAC_CONTRACT; *ilOffset = m_ilOffset; @@ -622,14 +616,12 @@ DWORD ILCodeVersionNode::GetJitFlags() const const InstrumentedILOffsetMapping* ILCodeVersionNode::GetInstrumentedILMap() const { LIMITED_METHOD_DAC_CONTRACT; - _ASSERTE(CodeVersionManager::IsLockOwnedByCurrentThread()); return &m_instrumentedILMap; } PTR_ILCodeVersionNode ILCodeVersionNode::GetNextILVersionNode() const { LIMITED_METHOD_DAC_CONTRACT; - _ASSERTE(CodeVersionManager::IsLockOwnedByCurrentThread()); return m_pNextILVersionNode; } @@ -812,6 +804,8 @@ NativeCodeVersionCollection ILCodeVersion::GetNativeCodeVersions(PTR_MethodDesc NativeCodeVersion ILCodeVersion::GetActiveNativeCodeVersion(PTR_MethodDesc pClosedMethodDesc) const { LIMITED_METHOD_DAC_CONTRACT; + _ASSERTE(CodeVersionManager::IsLockOwnedByCurrentThread()); + NativeCodeVersionCollection versions = GetNativeCodeVersions(pClosedMethodDesc); for (NativeCodeVersionIterator cur = versions.Begin(), end = versions.End(); cur != end; cur++) { @@ -1019,6 +1013,8 @@ HRESULT ILCodeVersion::AddNativeCodeVersion( HRESULT ILCodeVersion::GetOrCreateActiveNativeCodeVersion(MethodDesc* pClosedMethodDesc, NativeCodeVersion* pActiveNativeCodeVersion) { LIMITED_METHOD_CONTRACT; + _ASSERTE(CodeVersionManager::IsLockOwnedByCurrentThread()); + HRESULT hr = S_OK; NativeCodeVersion activeNativeChild = GetActiveNativeCodeVersion(pClosedMethodDesc); if (activeNativeChild.IsNull()) @@ -1143,7 +1139,6 @@ void ILCodeVersionIterator::Next() } if (m_stage == IterationStage::ImplicitCodeVersion) { - _ASSERTE(CodeVersionManager::IsLockOwnedByCurrentThread()); CodeVersionManager* pCodeVersionManager = m_pCollection->m_pModule->GetCodeVersionManager(); PTR_ILCodeVersioningState pILCodeVersioningState = pCodeVersionManager->GetILCodeVersioningState(m_pCollection->m_pModule, m_pCollection->m_methodDef); if (pILCodeVersioningState != NULL) @@ -1235,7 +1230,7 @@ void MethodDescVersioningState::LinkNativeCodeVersionNode(NativeCodeVersionNode* { LIMITED_METHOD_CONTRACT; pNativeCodeVersionNode->m_pNextMethodDescSibling = m_pFirstVersionNode; - m_pFirstVersionNode = pNativeCodeVersionNode; + VolatileStore(&m_pFirstVersionNode, pNativeCodeVersionNode); } #endif @@ -1297,8 +1292,9 @@ void ILCodeVersioningState::SetActiveVersion(ILCodeVersion ilActiveCodeVersion) void ILCodeVersioningState::LinkILCodeVersionNode(ILCodeVersionNode* pILCodeVersionNode) { LIMITED_METHOD_CONTRACT; + _ASSERTE(CodeVersionManager::IsLockOwnedByCurrentThread()); pILCodeVersionNode->SetNextILVersionNode(m_pFirstVersionNode); - m_pFirstVersionNode = pILCodeVersionNode; + VolatileStore(&m_pFirstVersionNode, pILCodeVersionNode); } #endif @@ -1309,6 +1305,7 @@ bool CodeVersionManager::s_initialNativeCodeVersionMayNotBeTheDefaultNativeCodeV PTR_ILCodeVersioningState CodeVersionManager::GetILCodeVersioningState(PTR_Module pModule, mdMethodDef methodDef) const { LIMITED_METHOD_DAC_CONTRACT; + // Safe without any locks, because this uses a LookupMap, which is safe for concurrent reads of pre-initialized data return pModule->LookupILCodeVersioningState(methodDef); } @@ -1408,14 +1405,12 @@ BOOL CodeVersionManager::HasNonDefaultILVersions() ILCodeVersionCollection CodeVersionManager::GetILCodeVersions(PTR_MethodDesc pMethod) { LIMITED_METHOD_DAC_CONTRACT; - _ASSERTE(IsLockOwnedByCurrentThread()); return GetILCodeVersions(dac_cast(pMethod->GetModule()), pMethod->GetMemberDef()); } ILCodeVersionCollection CodeVersionManager::GetILCodeVersions(PTR_Module pModule, mdMethodDef methodDef) { LIMITED_METHOD_DAC_CONTRACT; - _ASSERTE(IsLockOwnedByCurrentThread()); return ILCodeVersionCollection(pModule, methodDef); } @@ -1444,7 +1439,6 @@ ILCodeVersion CodeVersionManager::GetActiveILCodeVersion(PTR_Module pModule, mdM ILCodeVersion CodeVersionManager::GetILCodeVersion(PTR_MethodDesc pMethod, ReJITID rejitId) { LIMITED_METHOD_DAC_CONTRACT; - _ASSERTE(IsLockOwnedByCurrentThread()); #ifdef FEATURE_REJIT ILCodeVersionCollection collection = GetILCodeVersions(pMethod); @@ -1465,14 +1459,12 @@ ILCodeVersion CodeVersionManager::GetILCodeVersion(PTR_MethodDesc pMethod, ReJIT NativeCodeVersionCollection CodeVersionManager::GetNativeCodeVersions(PTR_MethodDesc pMethod) const { LIMITED_METHOD_DAC_CONTRACT; - _ASSERTE(IsLockOwnedByCurrentThread()); return NativeCodeVersionCollection(pMethod, ILCodeVersion()); } NativeCodeVersion CodeVersionManager::GetNativeCodeVersion(PTR_MethodDesc pMethod, PCODE codeStartAddress) const { LIMITED_METHOD_DAC_CONTRACT; - _ASSERTE(IsLockOwnedByCurrentThread()); NativeCodeVersionCollection nativeCodeVersions = GetNativeCodeVersions(pMethod); for (NativeCodeVersionIterator cur = nativeCodeVersions.Begin(), end = nativeCodeVersions.End(); cur != end; cur++) diff --git a/src/coreclr/vm/codeversion.h b/src/coreclr/vm/codeversion.h index bd778bd47d7bf..faf0157883686 100644 --- a/src/coreclr/vm/codeversion.h +++ b/src/coreclr/vm/codeversion.h @@ -263,11 +263,11 @@ class NativeCodeVersionNode PatchpointInfo* patchpointInfo, unsigned ilOffset); #endif - PTR_MethodDesc GetMethodDesc() const; - NativeCodeVersionId GetVersionId() const; - PCODE GetNativeCode() const; - ReJITID GetILVersionId() const; - ILCodeVersion GetILCodeVersion() const; + PTR_MethodDesc GetMethodDesc() const; // Can be called without any locks + NativeCodeVersionId GetVersionId() const; // Can be called without any locks + PCODE GetNativeCode() const; // Can be called without any locks, but result may be stale if it wasn't already set + ReJITID GetILVersionId() const; // Can be called without any locks + ILCodeVersion GetILCodeVersion() const;// Can be called without any locks BOOL IsActiveChildVersion() const; #ifndef DACCESS_COMPILE BOOL SetNativeCodeInterlocked(PCODE pCode, PCODE pExpected); @@ -287,28 +287,28 @@ class NativeCodeVersionNode #endif #ifdef FEATURE_ON_STACK_REPLACEMENT - PatchpointInfo * GetOSRInfo(unsigned * ilOffset); + PatchpointInfo * GetOSRInfo(unsigned * ilOffset) const; #endif private: //union - could save a little memory? //{ PCODE m_pNativeCode; - PTR_MethodDesc m_pMethodDesc; + DAC_IGNORE(const) PTR_MethodDesc m_pMethodDesc; //}; - ReJITID m_parentId; - PTR_NativeCodeVersionNode m_pNextMethodDescSibling; - NativeCodeVersionId m_id; + DAC_IGNORE(const) ReJITID m_parentId; + PTR_NativeCodeVersionNode m_pNextMethodDescSibling; // Never modified after being added to the linked list + DAC_IGNORE(const) NativeCodeVersionId m_id; #ifdef FEATURE_TIERED_COMPILATION - NativeCodeVersion::OptimizationTier m_optTier; + NativeCodeVersion::OptimizationTier m_optTier; // Set in constructor, but as the JIT runs it may upgrade the optimization tier #endif #ifdef HAVE_GCCOVER PTR_GCCoverageInfo m_gcCover; #endif #ifdef FEATURE_ON_STACK_REPLACEMENT - PTR_PatchpointInfo m_patchpointInfo; - unsigned m_ilOffset; + DAC_IGNORE(const) PTR_PatchpointInfo m_patchpointInfo; + DAC_IGNORE(const) unsigned m_ilOffset; #endif enum NativeCodeVersionNodeFlags @@ -388,10 +388,10 @@ class ILCodeVersionNode #endif private: - PTR_Module m_pModule; - mdMethodDef m_methodDef; - ReJITID m_rejitId; - PTR_ILCodeVersionNode m_pNextILVersionNode; + const PTR_Module m_pModule; + const mdMethodDef m_methodDef; + const ReJITID m_rejitId; + PTR_ILCodeVersionNode m_pNextILVersionNode; // Never modified after being added to the linked list Volatile m_rejitState; VolatilePtr m_pIL; Volatile m_jitFlags; diff --git a/src/coreclr/vm/debuginfostore.cpp b/src/coreclr/vm/debuginfostore.cpp index 8d8b05a676260..789693f33b68a 100644 --- a/src/coreclr/vm/debuginfostore.cpp +++ b/src/coreclr/vm/debuginfostore.cpp @@ -985,7 +985,7 @@ BOOL DebugInfoManager::GetBoundariesAndVars( CONTRACTL { THROWS; - WRAPPER(GC_TRIGGERS); // depends on fpNew + GC_NOTRIGGER; SUPPORTS_DAC; } CONTRACTL_END; diff --git a/src/coreclr/vm/multicorejitplayer.cpp b/src/coreclr/vm/multicorejitplayer.cpp index c49d9eac3d9c9..76659b255229e 100644 --- a/src/coreclr/vm/multicorejitplayer.cpp +++ b/src/coreclr/vm/multicorejitplayer.cpp @@ -1132,7 +1132,7 @@ HRESULT MulticoreJitProfilePlayer::PlayProfile() MulticoreJitTrace(("PlayProfile %d bytes in (%s)", nSize, - GetAppDomain()->GetFriendlyNameForLogging())); + GetAppDomain()->GetFriendlyName())); while ((SUCCEEDED(hr)) && (nSize > sizeof(unsigned))) { diff --git a/src/coreclr/vm/proftoeeinterfaceimpl.cpp b/src/coreclr/vm/proftoeeinterfaceimpl.cpp index 614a6036fdf8a..f84f30f4fde28 100644 --- a/src/coreclr/vm/proftoeeinterfaceimpl.cpp +++ b/src/coreclr/vm/proftoeeinterfaceimpl.cpp @@ -2048,9 +2048,8 @@ HRESULT ProfToEEInterfaceImpl::GetFunctionFromIP2(LPCBYTE ip, FunctionID * pFunc // Yay! NOTHROW; - // Grabbing the rejitid requires entering the rejit manager's hash table & lock, - // which can switch us to preemptive mode and trigger GCs - GC_TRIGGERS; + // Yay! + GC_NOTRIGGER; // Yay! MODE_ANY; @@ -2068,7 +2067,7 @@ HRESULT ProfToEEInterfaceImpl::GetFunctionFromIP2(LPCBYTE ip, FunctionID * pFunc PERMANENT_CONTRACT_VIOLATION(TakesLockViolation, ReasonProfilerAsyncCannotRetakeLock); PROFILER_TO_CLR_ENTRYPOINT_SYNC_EX( - kP2EEAllowableAfterAttach | kP2EETriggers, + kP2EEAllowableAfterAttach, (LF_CORPROF, LL_INFO1000, "**PROF: GetFunctionFromIP2 0x%p.\n", @@ -2500,9 +2499,8 @@ HRESULT ProfToEEInterfaceImpl::GetCodeInfo3(FunctionID functionId, // Yay! NOTHROW; - // We need to access the rejitmanager, which means taking locks, which means we - // may trigger a GC - GC_TRIGGERS; + // Yay! + GC_NOTRIGGER; // Yay! MODE_ANY; @@ -2510,10 +2508,10 @@ HRESULT ProfToEEInterfaceImpl::GetCodeInfo3(FunctionID functionId, // Yay! EE_THREAD_NOT_REQUIRED; - // We need to access the rejitmanager, which means taking locks + // We need to take the ExecutionManager reader lock to find the + // appropriate jit manager. CAN_TAKE_LOCK; - PRECONDITION(CheckPointer(pcCodeInfos, NULL_OK)); PRECONDITION(CheckPointer(codeInfos, NULL_OK)); } @@ -2523,7 +2521,7 @@ HRESULT ProfToEEInterfaceImpl::GetCodeInfo3(FunctionID functionId, PERMANENT_CONTRACT_VIOLATION(TakesLockViolation, ReasonProfilerAsyncCannotRetakeLock); PROFILER_TO_CLR_ENTRYPOINT_SYNC_EX( - kP2EEAllowableAfterAttach | kP2EETriggers, + kP2EEAllowableAfterAttach, (LF_CORPROF, LL_INFO1000, "**PROF: GetCodeInfo3 0x%p 0x%p.\n", @@ -2541,8 +2539,6 @@ HRESULT ProfToEEInterfaceImpl::GetCodeInfo3(FunctionID functionId, PCODE pCodeStart = (PCODE)NULL; CodeVersionManager* pCodeVersionManager = pMethodDesc->GetCodeVersionManager(); { - CodeVersionManager::LockHolder codeVersioningLockHolder; - ILCodeVersion ilCodeVersion = pCodeVersionManager->GetILCodeVersion(pMethodDesc, reJitId); NativeCodeVersionCollection nativeCodeVersions = ilCodeVersion.GetNativeCodeVersions(pMethodDesc); @@ -4928,8 +4924,6 @@ HRESULT ProfToEEInterfaceImpl::GetILToNativeMapping2(FunctionID functionId, CodeVersionManager *pCodeVersionManager = pMD->GetCodeVersionManager(); ILCodeVersion ilCodeVersion = NULL; { - CodeVersionManager::LockHolder codeVersioningLockHolder; - pCodeVersionManager->GetILCodeVersion(pMD, reJitId); NativeCodeVersionCollection nativeCodeVersions = ilCodeVersion.GetNativeCodeVersions(pMD); @@ -5455,20 +5449,20 @@ HRESULT ProfToEEInterfaceImpl::GetAppDomainInfo(AppDomainID appDomainId, // Yay! NOTHROW; - // AppDomain::GetFriendlyNameForDebugger triggers - GC_TRIGGERS; + // Yay! + GC_NOTRIGGER; // Yay! MODE_ANY; - // AppDomain::GetFriendlyNameForDebugger takes a lock - CAN_TAKE_LOCK; + // Yay! + CANNOT_TAKE_LOCK; } CONTRACTL_END; PROFILER_TO_CLR_ENTRYPOINT_SYNC_EX( - kP2EEAllowableAfterAttach | kP2EETriggers, + kP2EEAllowableAfterAttach, (LF_CORPROF, LL_INFO1000, "**PROF: GetAppDomainInfo 0x%p.\n", @@ -5493,7 +5487,7 @@ HRESULT ProfToEEInterfaceImpl::GetAppDomainInfo(AppDomainID appDomainId, if (appDomainId == (AppDomainID)SystemDomain::System()) szFriendlyName = g_pwBaseLibrary; else - szFriendlyName = ((AppDomain*)appDomainId)->GetFriendlyNameForDebugger(); + szFriendlyName = ((AppDomain*)appDomainId)->GetFriendlyName(); if (szFriendlyName != NULL) { @@ -6219,13 +6213,11 @@ HRESULT ProfToEEInterfaceImpl::GetFunctionFromIP3(LPCBYTE ip, FunctionID * pFunc { NOTHROW; - // Grabbing the rejitid requires entering the rejit manager's hash table & lock, - // which can switch us to preemptive mode and trigger GCs - GC_TRIGGERS; + GC_NOTRIGGER; MODE_ANY; EE_THREAD_NOT_REQUIRED; - // Grabbing the rejitid requires entering the rejit manager's hash table & lock, + // Calling GetFunctionFromIPInternal may take a reader lock CAN_TAKE_LOCK; } @@ -6235,7 +6227,7 @@ HRESULT ProfToEEInterfaceImpl::GetFunctionFromIP3(LPCBYTE ip, FunctionID * pFunc PERMANENT_CONTRACT_VIOLATION(TakesLockViolation, ReasonProfilerAsyncCannotRetakeLock); PROFILER_TO_CLR_ENTRYPOINT_SYNC_EX( - kP2EEAllowableAfterAttach | kP2EETriggers, + kP2EEAllowableAfterAttach, (LF_CORPROF, LL_INFO1000, "**PROF: GetFunctionFromIP3 0x%p.\n", @@ -6596,7 +6588,7 @@ HRESULT ProfToEEInterfaceImpl::GetCodeInfo4(UINT_PTR pNativeCodeStartAddress, CONTRACTL { NOTHROW; - GC_TRIGGERS; + GC_NOTRIGGER; MODE_ANY; EE_THREAD_NOT_REQUIRED; CAN_TAKE_LOCK; @@ -6608,7 +6600,7 @@ HRESULT ProfToEEInterfaceImpl::GetCodeInfo4(UINT_PTR pNativeCodeStartAddress, CONTRACTL_END; PROFILER_TO_CLR_ENTRYPOINT_SYNC_EX( - kP2EEAllowableAfterAttach | kP2EETriggers, + kP2EEAllowableAfterAttach, (LF_CORPROF, LL_INFO1000, "**PROF: GetCodeInfo4 0x%p.\n", @@ -10754,6 +10746,7 @@ GCX_COOP_THREAD_EXISTS(GET_THREAD()); HCIMPL_PROLOG(ProfileEnter) { FCALL_CONTRACT; + FC_GC_POLL_NOT_NEEDED(); // we pulse GC mode, so we are doing a poll if (GetThreadNULLOk() == NULL) { @@ -10793,13 +10786,10 @@ HCIMPL_PROLOG(ProfileEnter) _ASSERTE(GetThread()->PreemptiveGCDisabled()); _ASSERTE(platformSpecificHandle != NULL); - // Set up a frame - HELPER_METHOD_FRAME_BEGIN_ATTRIB_NOPOLL(Frame::FRAME_ATTR_CAPTURE_DEPTH_2); - - // Our contract is FCALL_CONTRACT, which is considered triggers if you set up a - // frame, like we're about to do. + // This callback is called from the prolog of a method, without a valid ability to suspend the runtime/take a GC. + // This means that we cannot trigger a GC. SetCallbackStateFlagsHolder csf( - COR_PRF_CALLBACKSTATE_INCALLBACK | COR_PRF_CALLBACKSTATE_IN_TRIGGERS_SCOPE); + COR_PRF_CALLBACKSTATE_INCALLBACK); COR_PRF_ELT_INFO_INTERNAL eltInfo; eltInfo.platformSpecificHandle = platformSpecificHandle; @@ -10926,8 +10916,6 @@ HCIMPL_PROLOG(ProfileEnter) LExit: ; - HELPER_METHOD_FRAME_END(); // Un-link the frame - #endif // PROFILING_SUPPORTED } HCIMPLEND @@ -10967,13 +10955,10 @@ HCIMPL_PROLOG(ProfileLeave) _ASSERTE(GetThread()->PreemptiveGCDisabled()); _ASSERTE(platformSpecificHandle != NULL); - // Set up a frame - HELPER_METHOD_FRAME_BEGIN_ATTRIB_NOPOLL(Frame::FRAME_ATTR_CAPTURE_DEPTH_2); - - // Our contract is FCALL_CONTRACT, which is considered triggers if you set up a - // frame, like we're about to do. + // This callback is called from the epilog of a method, without a valid ability to suspend the runtime/take a GC. + // This means that we cannot trigger a GC. SetCallbackStateFlagsHolder csf( - COR_PRF_CALLBACKSTATE_INCALLBACK | COR_PRF_CALLBACKSTATE_IN_TRIGGERS_SCOPE); + COR_PRF_CALLBACKSTATE_INCALLBACK); COR_PRF_ELT_INFO_INTERNAL eltInfo; eltInfo.platformSpecificHandle = platformSpecificHandle; @@ -11060,8 +11045,6 @@ HCIMPL_PROLOG(ProfileLeave) ; - HELPER_METHOD_FRAME_END(); // Un-link the frame - #endif // PROFILING_SUPPORTED } HCIMPLEND @@ -11099,13 +11082,10 @@ HCIMPL2(EXTERN_C void, ProfileTailcall, UINT_PTR clientData, void * platformSpec _ASSERTE(GetThread()->PreemptiveGCDisabled()); _ASSERTE(platformSpecificHandle != NULL); - // Set up a frame - HELPER_METHOD_FRAME_BEGIN_ATTRIB_NOPOLL(Frame::FRAME_ATTR_CAPTURE_DEPTH_2); - - // Our contract is FCALL_CONTRACT, which is considered triggers if you set up a - // frame, like we're about to do. + // This callback is called from the epilog of a method, without a valid ability to suspend the runtime/take a GC. + // This means that we cannot trigger a GC. SetCallbackStateFlagsHolder csf( - COR_PRF_CALLBACKSTATE_INCALLBACK | COR_PRF_CALLBACKSTATE_IN_TRIGGERS_SCOPE); + COR_PRF_CALLBACKSTATE_INCALLBACK); COR_PRF_ELT_INFO_INTERNAL eltInfo; eltInfo.platformSpecificHandle = platformSpecificHandle; @@ -11188,8 +11168,6 @@ HCIMPL2(EXTERN_C void, ProfileTailcall, UINT_PTR clientData, void * platformSpec ; - HELPER_METHOD_FRAME_END(); // Un-link the frame - #endif // PROFILING_SUPPORTED } HCIMPLEND diff --git a/src/coreclr/vm/rejit.cpp b/src/coreclr/vm/rejit.cpp index aa07730259f32..01d03666546ef 100644 --- a/src/coreclr/vm/rejit.cpp +++ b/src/coreclr/vm/rejit.cpp @@ -1123,53 +1123,21 @@ ReJITID ReJitManager::GetReJitId(PTR_MethodDesc pMD, PCODE pCodeStart) CONTRACTL { NOTHROW; - CAN_TAKE_LOCK; - GC_TRIGGERS; + CANNOT_TAKE_LOCK; + GC_NOTRIGGER; PRECONDITION(CheckPointer(pMD)); PRECONDITION(pCodeStart != NULL); } CONTRACTL_END; - // Fast-path: If the rejit map is empty, no need to look up anything. Do this outside - // of a lock to impact our caller (the prestub worker) as little as possible. If the - // map is nonempty, we'll acquire the lock at that point and do the lookup for real. + // Fast-path: If the rejit map is empty, no need to look up anything. CodeVersionManager* pCodeVersionManager = pMD->GetCodeVersionManager(); if (!pCodeVersionManager->HasNonDefaultILVersions()) { return 0; } - CodeVersionManager::LockHolder codeVersioningLockHolder; - return ReJitManager::GetReJitIdNoLock(pMD, pCodeStart); -} - -//--------------------------------------------------------------------------------------- -// -// See comment above code:ReJitManager::GetReJitId for main details of what this does. -// -// This function is basically the same as GetReJitId, except caller is expected to take -// the ReJitManager lock directly (via ReJitManager::TableLockHolder). This exists so -// that ETW can explicitly take the triggering ReJitManager lock up front, and in the -// proper order, to avoid lock leveling issues, and triggering issues with other locks it -// takes that are CRST_UNSAFE_ANYMODE -// - -ReJITID ReJitManager::GetReJitIdNoLock(PTR_MethodDesc pMD, PCODE pCodeStart) -{ - CONTRACTL - { - NOTHROW; - CANNOT_TAKE_LOCK; - GC_NOTRIGGER; - PRECONDITION(CheckPointer(pMD)); - PRECONDITION(pCodeStart != NULL); - } - CONTRACTL_END; - - // Caller must ensure this lock is taken! - _ASSERTE(CodeVersionManager::IsLockOwnedByCurrentThread()); - - NativeCodeVersion nativeCodeVersion = pMD->GetCodeVersionManager()->GetNativeCodeVersion(pMD, pCodeStart); + NativeCodeVersion nativeCodeVersion = pCodeVersionManager->GetNativeCodeVersion(pMD, pCodeStart); if (nativeCodeVersion.IsNull()) { return 0; @@ -1269,11 +1237,6 @@ ReJITID ReJitManager::GetReJitId(PTR_MethodDesc pMD, PCODE pCodeStart) return 0; } -ReJITID ReJitManager::GetReJitIdNoLock(PTR_MethodDesc pMD, PCODE pCodeStart) -{ - return 0; -} - HRESULT ReJitManager::GetReJITIDs(PTR_MethodDesc pMD, ULONG cReJitIds, ULONG * pcReJitIds, ReJITID reJitIds[]) { return E_NOTIMPL; diff --git a/src/coreclr/vm/rejit.h b/src/coreclr/vm/rejit.h index c54c567e592a7..a994f9d3455b5 100644 --- a/src/coreclr/vm/rejit.h +++ b/src/coreclr/vm/rejit.h @@ -114,7 +114,6 @@ class ReJitManager static CORJIT_FLAGS JitFlagsFromProfCodegenFlags(DWORD dwCodegenFlags); static ReJITID GetReJitId(PTR_MethodDesc pMD, PCODE pCodeStart); - static ReJITID GetReJitIdNoLock(PTR_MethodDesc pMD, PCODE pCodeStart); static HRESULT GetReJITIDs(PTR_MethodDesc pMD, ULONG cReJitIds, ULONG * pcReJitIds, ReJITID reJitIds[]); #ifdef FEATURE_REJIT