From 261a79c7803f066cd73857ba698cee114ebe43b6 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 8 Jul 2026 12:24:37 -0700 Subject: [PATCH 1/7] Reconcile ReleaseHolder, ComHolderPreemp, and ComHolderAnyMode Converge the three COM-interface RAII holders onto the LifetimeHolder pattern: - Retarget ReleaseHolder to LifetimeHolder>, adopting ComHolderPreemp's preemptive-mode release semantics (the VM default). The Free() contract is MODE_PREEMPTIVE, gated on ENABLE_CONTRACTS_IMPL so non-VM consumers (utilcode, md, DAC, createdump) use a static-contract annotation. - Remove ComHolderPreemp entirely; ReleaseHolder replaces all uses. - Rename ComHolderAnyMode to ReleaseHolderAnyMode. - Relax both holders' static_assert from is_base_of to a duck-typed "T has a Release() member" check (holder_detail::HasReleaseMethod), so ReleaseHolder can hold non-IUnknown types that expose Release(). Migrate all call sites off the legacy SpecializedWrapper API onto the LifetimeHolder API (Extract->Detach, Clear->Free, GetValue/IsNull/Assign/SuppressRelease removed). Read-after-suppress sites are restructured to detach into a raw pointer to preserve exception-safety and refcount behavior. BindAssemblySpec is refactored so the CoreLib singleton borrow no longer relies on SuppressRelease. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/binder/assembly.cpp | 2 +- src/coreclr/binder/assemblybindercommon.cpp | 8 +- src/coreclr/binder/customassemblybinder.cpp | 4 +- src/coreclr/binder/defaultassemblybinder.cpp | 10 +-- src/coreclr/debug/createdump/crashinfo.cpp | 4 +- .../debug/createdump/createdumpunix.cpp | 2 +- src/coreclr/debug/createdump/threadinfo.cpp | 2 +- src/coreclr/debug/daccess/cdac.h | 4 +- src/coreclr/debug/daccess/daccess.cpp | 2 +- src/coreclr/debug/daccess/dacdbiimpl.cpp | 2 +- src/coreclr/debug/daccess/enummem.cpp | 34 ++++----- src/coreclr/debug/ee/debugger.cpp | 2 +- src/coreclr/dlls/mscoree/exports.cpp | 3 +- src/coreclr/inc/holder.h | 74 +++++++++++-------- src/coreclr/md/enc/mdinternalrw.cpp | 2 +- src/coreclr/vm/appdomain.cpp | 45 ++++------- src/coreclr/vm/appdomain.hpp | 2 +- src/coreclr/vm/assembly.cpp | 14 ++-- src/coreclr/vm/assemblynative.cpp | 2 +- src/coreclr/vm/ceeload.cpp | 12 +-- src/coreclr/vm/clrex.cpp | 2 +- src/coreclr/vm/comcache.cpp | 12 +-- src/coreclr/vm/comcallablewrapper.cpp | 41 +++++----- src/coreclr/vm/comconnectionpoints.cpp | 2 +- src/coreclr/vm/commodule.cpp | 10 +-- src/coreclr/vm/coreassemblyspec.cpp | 2 +- src/coreclr/vm/crossloaderallocatorhash.inl | 6 +- src/coreclr/vm/dispparammarshaler.cpp | 4 +- src/coreclr/vm/dllimport.cpp | 2 +- src/coreclr/vm/eetoprofinterfaceimpl.cpp | 33 +++------ src/coreclr/vm/encee.cpp | 6 +- src/coreclr/vm/interopconverter.cpp | 12 +-- src/coreclr/vm/interoputil.cpp | 14 ++-- src/coreclr/vm/marshalnative.cpp | 4 +- src/coreclr/vm/nativeimage.cpp | 6 +- src/coreclr/vm/olecontexthelpers.cpp | 4 +- src/coreclr/vm/olevariant.cpp | 6 +- src/coreclr/vm/peassembly.cpp | 2 +- src/coreclr/vm/peimage.cpp | 2 +- src/coreclr/vm/peimagelayout.cpp | 4 +- src/coreclr/vm/rejit.cpp | 2 +- src/coreclr/vm/runtimecallablewrapper.cpp | 47 ++++++------ src/coreclr/vm/stdinterfaces.cpp | 30 ++++---- src/coreclr/vm/stubhelpers.cpp | 2 +- src/coreclr/vm/stubmgr.cpp | 6 +- src/coreclr/vm/weakreferencenative.cpp | 8 +- src/coreclr/vm/wrappers.h | 41 ++-------- 47 files changed, 248 insertions(+), 292 deletions(-) diff --git a/src/coreclr/binder/assembly.cpp b/src/coreclr/binder/assembly.cpp index 3309c0d17f9358..809f541eed2f67 100644 --- a/src/coreclr/binder/assembly.cpp +++ b/src/coreclr/binder/assembly.cpp @@ -57,7 +57,7 @@ namespace BINDER_SPACE m_pPEImage = pPEImage; // Now take ownership of assembly name - m_pAssemblyName = pAssemblyName.Extract(); + m_pAssemblyName = pAssemblyName.Detach(); Exit: return hr; diff --git a/src/coreclr/binder/assemblybindercommon.cpp b/src/coreclr/binder/assemblybindercommon.cpp index 7316f981d67d6a..ca38b249d122e8 100644 --- a/src/coreclr/binder/assemblybindercommon.cpp +++ b/src/coreclr/binder/assemblybindercommon.cpp @@ -329,7 +329,7 @@ namespace BINDER_SPACE IF_FAIL_GO(hr); - *ppSystemAssembly = pSystemAssembly.Extract(); + *ppSystemAssembly = pSystemAssembly.Detach(); Exit: return hr; @@ -382,7 +382,7 @@ namespace BINDER_SPACE probeExtensionResult)); BinderTracing::PathProbed(sCoreLibSatellite, pathSource, hr); - *ppSystemAssembly = pSystemAssembly.Extract(); + *ppSystemAssembly = pSystemAssembly.Detach(); Exit: return hr; @@ -790,7 +790,7 @@ namespace BINDER_SPACE } // Set any found assembly. It is up to the caller to check the returned HRESULT for errors due to validation - *ppAssembly = pAssembly.Extract(); + *ppAssembly = pAssembly.Detach(); if (FAILED(hr)) return hr; @@ -1030,7 +1030,7 @@ namespace BINDER_SPACE } // We're done - *ppAssembly = pAssembly.Extract(); + *ppAssembly = pAssembly.Detach(); Exit: diff --git a/src/coreclr/binder/customassemblybinder.cpp b/src/coreclr/binder/customassemblybinder.cpp index 4829b19ff390b2..f828bed5669880 100644 --- a/src/coreclr/binder/customassemblybinder.cpp +++ b/src/coreclr/binder/customassemblybinder.cpp @@ -98,7 +98,7 @@ HRESULT CustomAssemblyBinder::BindUsingAssemblyName(BINDER_SPACE::AssemblyName* // For TPA assemblies that were bound, DefaultBinder // would have already set the binder reference for the assembly, so we just need to // extract the reference now. - *ppAssembly = pCoreCLRFoundAssembly.Extract(); + *ppAssembly = pCoreCLRFoundAssembly.Detach(); Exit:; @@ -139,7 +139,7 @@ HRESULT CustomAssemblyBinder::BindUsingPEImage( /* in */ PEImage *pPEImage, { _ASSERTE(pCoreCLRFoundAssembly != NULL); pCoreCLRFoundAssembly->SetBinder(this); - *ppAssembly = pCoreCLRFoundAssembly.Extract(); + *ppAssembly = pCoreCLRFoundAssembly.Detach(); } Exit:; } diff --git a/src/coreclr/binder/defaultassemblybinder.cpp b/src/coreclr/binder/defaultassemblybinder.cpp index 0504997a91b354..6ef624e8f2a0d7 100644 --- a/src/coreclr/binder/defaultassemblybinder.cpp +++ b/src/coreclr/binder/defaultassemblybinder.cpp @@ -111,7 +111,7 @@ HRESULT DefaultAssemblyBinder::BindUsingAssemblyName(BINDER_SPACE::AssemblyName IF_FAIL_GO(hr); - *ppAssembly = pCoreCLRFoundAssembly.Extract(); + *ppAssembly = pCoreCLRFoundAssembly.Detach(); Exit:; @@ -162,7 +162,7 @@ HRESULT DefaultAssemblyBinder::BindUsingPEImage( /* in */ PEImage *pPEImage, { if (pCoreCLRFoundAssembly->GetIsInTPA()) { - *ppAssembly = pCoreCLRFoundAssembly.Extract(); + *ppAssembly = pCoreCLRFoundAssembly.Detach(); goto Exit; } } @@ -172,7 +172,7 @@ HRESULT DefaultAssemblyBinder::BindUsingPEImage( /* in */ PEImage *pPEImage, // Return the existing assembly so the caller can provide an informative error message. if (ppExistingAssemblyOnConflict != nullptr) { - *ppExistingAssemblyOnConflict = pExistingAssembly.Extract(); + *ppExistingAssemblyOnConflict = pExistingAssembly.Detach(); } goto Exit; } @@ -184,7 +184,7 @@ HRESULT DefaultAssemblyBinder::BindUsingPEImage( /* in */ PEImage *pPEImage, { _ASSERTE(pCoreCLRFoundAssembly != NULL); pCoreCLRFoundAssembly->SetBinder(this); - *ppAssembly = pCoreCLRFoundAssembly.Extract(); + *ppAssembly = pCoreCLRFoundAssembly.Detach(); } Exit:; } @@ -221,7 +221,7 @@ HRESULT DefaultAssemblyBinder::BindToSystem(BINDER_SPACE::Assembly** ppSystemAss if (SUCCEEDED(hr)) { _ASSERTE(pAsm != NULL); - *ppSystemAssembly = pAsm.Extract(); + *ppSystemAssembly = pAsm.Detach(); (*ppSystemAssembly)->SetBinder(this); } diff --git a/src/coreclr/debug/createdump/crashinfo.cpp b/src/coreclr/debug/createdump/crashinfo.cpp index a34b5924ee741b..a25265e8d2755a 100644 --- a/src/coreclr/debug/createdump/crashinfo.cpp +++ b/src/coreclr/debug/createdump/crashinfo.cpp @@ -303,7 +303,7 @@ CrashInfo::InitializeDAC(DumpType dumpType) printf_error("InitializeDAC: coreclr not found; not using DAC\n"); return true; } - ReleaseHolder dataTarget = new DumpDataTarget(*this); + ReleaseHolder dataTarget{ new DumpDataTarget(*this) }; PFN_CLRDataCreateInstance pfnCLRDataCreateInstance = nullptr; PFN_DLLMAIN pfnDllMain = nullptr; bool result = false; @@ -483,7 +483,7 @@ CrashInfo::UnwindAllThreads() if (m_appModel != AppModelType::NativeAOT) { TRACE("UnwindAllThreads: STARTED (%d)\n", m_dataTargetPagesAdded); - ReleaseHolder pSos = nullptr; + ReleaseHolder pSos; if (m_pClrDataProcess != nullptr) { m_pClrDataProcess->QueryInterface(__uuidof(ISOSDacInterface), (void**)&pSos); } diff --git a/src/coreclr/debug/createdump/createdumpunix.cpp b/src/coreclr/debug/createdump/createdumpunix.cpp index ac9ba3043e35d9..6fdc334b5ed2a1 100644 --- a/src/coreclr/debug/createdump/createdumpunix.cpp +++ b/src/coreclr/debug/createdump/createdumpunix.cpp @@ -14,7 +14,7 @@ long g_pageSize = 0; bool CreateDump(const CreateDumpOptions& options) { - ReleaseHolder crashInfo = new CrashInfo(options); + ReleaseHolder crashInfo{ new CrashInfo(options) }; DumpWriter dumpWriter(*crashInfo); std::string dumpPath; bool result = false; diff --git a/src/coreclr/debug/createdump/threadinfo.cpp b/src/coreclr/debug/createdump/threadinfo.cpp index 26feb8c2f17607..b4411e5f23c68f 100644 --- a/src/coreclr/debug/createdump/threadinfo.cpp +++ b/src/coreclr/debug/createdump/threadinfo.cpp @@ -299,7 +299,7 @@ ThreadInfo::GatherStackFrames(CONTEXT* pContext, IXCLRDataStackWalk* pStackwalk) } // Add managed stack frame for the crash info notes - StackFrame frame(moduleAddress, ip, sp, pMethod.Extract(), nativeOffset, token, ilOffset); + StackFrame frame(moduleAddress, ip, sp, pMethod.Detach(), nativeOffset, token, ilOffset); AddStackFrame(frame); } diff --git a/src/coreclr/debug/daccess/cdac.h b/src/coreclr/debug/daccess/cdac.h index e757336475e5da..93dd68d69908ee 100644 --- a/src/coreclr/debug/daccess/cdac.h +++ b/src/coreclr/debug/daccess/cdac.h @@ -18,7 +18,7 @@ class CDAC final CDAC(CDAC&& other) : m_module{ other.m_module } , m_cdac_handle{ other.m_cdac_handle } - , m_target{ other.m_target.Extract() } + , m_target{ other.m_target.Detach() } , m_legacyImpl{ other.m_legacyImpl } { other.m_module = NULL; @@ -31,7 +31,7 @@ class CDAC final { m_module = other.m_module; m_cdac_handle = other.m_cdac_handle; - m_target = other.m_target.Extract(); + m_target = other.m_target.Detach(); m_legacyImpl = other.m_legacyImpl; other.m_module = NULL; diff --git a/src/coreclr/debug/daccess/daccess.cpp b/src/coreclr/debug/daccess/daccess.cpp index df4f4defa29c6d..632677720b11d7 100644 --- a/src/coreclr/debug/daccess/daccess.cpp +++ b/src/coreclr/debug/daccess/daccess.cpp @@ -6541,7 +6541,7 @@ CLRDataCreateInstance(REFIID iid, #endif // TODO: [cdac] Remove when cDAC deploys with SOS - https://github.com/dotnet/runtime/issues/108720 - ReleaseHolder cdacInterface = nullptr; + ReleaseHolder cdacInterface; #ifdef CAN_USE_CDAC CLRConfigNoCache enable = CLRConfigNoCache::Get("ENABLE_CDAC"); if (enable.IsSet()) diff --git a/src/coreclr/debug/daccess/dacdbiimpl.cpp b/src/coreclr/debug/daccess/dacdbiimpl.cpp index 9b44cbd886a896..fe2451c812b642 100644 --- a/src/coreclr/debug/daccess/dacdbiimpl.cpp +++ b/src/coreclr/debug/daccess/dacdbiimpl.cpp @@ -304,7 +304,7 @@ DacDbiInterfaceInstance( cdac = CDAC::Create(contractDescriptorAddr, pDac->m_pTarget, legacyImpl); if (cdac.IsValid()) { - ReleaseHolder cdacInterface = nullptr; + ReleaseHolder cdacInterface; cdac.CreateDacDbiInterface(&cdacInterface); if (cdacInterface != nullptr) { diff --git a/src/coreclr/debug/daccess/enummem.cpp b/src/coreclr/debug/daccess/enummem.cpp index f23ad172e3f2be..94eeac8d7ad860 100644 --- a/src/coreclr/debug/daccess/enummem.cpp +++ b/src/coreclr/debug/daccess/enummem.cpp @@ -880,13 +880,13 @@ HRESULT ClrDataAccess::EnumMemWalkStackHelper(CLRDataEnumMemoryFlags flags, if (SUCCEEDED(pMethod->GetTypeInstance(&pTypeInstance)) && (pTypeInstance != NULL)) { - pTypeInstance.Clear(); + pTypeInstance.Free(); } if(SUCCEEDED(pMethod->GetDefinition(&pMethodDefinition)) && (pMethodDefinition != NULL)) { - pMethodDesc = ((ClrDataMethodDefinition *)pMethodDefinition.GetValue())->GetMethodDesc(); + pMethodDesc = ((ClrDataMethodDefinition *)(IXCLRDataMethodDefinition*)pMethodDefinition)->GetMethodDesc(); if (pMethodDesc) { @@ -998,11 +998,11 @@ HRESULT ClrDataAccess::EnumMemWalkStackHelper(CLRDataEnumMemoryFlags flags, } #endif // USE_GC_INFO_DECODER } - pMethodDefinition.Clear(); + pMethodDefinition.Free(); } - pMethod.Clear(); + pMethod.Free(); } - pFrame.Clear(); + pFrame.Free(); } previousSP = currentSP; @@ -1144,7 +1144,7 @@ HRESULT ClrDataAccess::EnumMemDumpAllThreadsStack(CLRDataEnumMemoryFlags flags) EX_TRY { // get Thread * - pThread = ((ClrDataTask *)pIXCLRDataTask.GetValue())->GetThread(); + pThread = ((ClrDataTask*)(IXCLRDataTask*)pIXCLRDataTask)->GetThread(); // dump the exception object DumpManagedExcepObject(flags, pThread->LastThrownObject()); @@ -1156,7 +1156,7 @@ HRESULT ClrDataAccess::EnumMemDumpAllThreadsStack(CLRDataEnumMemoryFlags flags) EX_TRY { // touch the throwable in exception state - PTR_UNCHECKED_OBJECTREF throwRef(((ClrDataExceptionState *)pExcepState.GetValue())->m_throwable); + PTR_UNCHECKED_OBJECTREF throwRef(((ClrDataExceptionState*)(IXCLRDataExceptionState*)pExcepState)->m_throwable); // If we've already attempted enumeration for this exception, it's time to quit. if (!exceptionTrackingInner.AddNewAddressOnly(throwRef.GetAddr())) @@ -1180,7 +1180,7 @@ HRESULT ClrDataAccess::EnumMemDumpAllThreadsStack(CLRDataEnumMemoryFlags flags) EX_CATCH_RETHROW_ONLY_COR_E_OPERATIONCANCELLED // get next thread - pIXCLRDataTask.Clear(); + pIXCLRDataTask.Free(); status = EnumTask(&handle, &pIXCLRDataTask); } EndEnumTasks(handle); @@ -1222,7 +1222,7 @@ HRESULT ClrDataAccess::EnumMemDumpAllThreadsStack(CLRDataEnumMemoryFlags flags) EX_TRY { // get Thread * - pThread = ((ClrDataTask *)pIXCLRDataTask.GetValue())->GetThread(); + pThread = ((ClrDataTask*)(IXCLRDataTask*)pIXCLRDataTask)->GetThread(); // Write out the Thread instance DacEnumHostDPtrMem(pThread); @@ -1248,7 +1248,7 @@ HRESULT ClrDataAccess::EnumMemDumpAllThreadsStack(CLRDataEnumMemoryFlags flags) if (status == S_OK && pStackWalk != NULL) { status = EnumMemWalkStackHelper(flags, pStackWalk, pThread); - pStackWalk.Clear(); + pStackWalk.Free(); } // Now probe into the exception info @@ -1258,7 +1258,7 @@ HRESULT ClrDataAccess::EnumMemDumpAllThreadsStack(CLRDataEnumMemoryFlags flags) EX_TRY { // touch the throwable in exception state - PTR_UNCHECKED_OBJECTREF throwRef(((ClrDataExceptionState *)pExcepState.GetValue())->m_throwable); + PTR_UNCHECKED_OBJECTREF throwRef(((ClrDataExceptionState*)(IXCLRDataExceptionState*)pExcepState)->m_throwable); // If we've already attempted enumeration for this exception, it's time to quit. if (!exceptionTracking.AddNewAddressOnly(throwRef.GetAddr())) @@ -1278,7 +1278,7 @@ HRESULT ClrDataAccess::EnumMemDumpAllThreadsStack(CLRDataEnumMemoryFlags flags) ReleaseHolder pTypeInstance(NULL); // Make sure that we can get back a TypeInstance during inspection status = pValue->GetType(&pTypeInstance); - pValue.Clear(); + pValue.Free(); } // If Exception state has a new context, we will walk with the stashed context as well. @@ -1290,7 +1290,7 @@ HRESULT ClrDataAccess::EnumMemDumpAllThreadsStack(CLRDataEnumMemoryFlags flags) // to walk the stack correctly here. Anyway, we try to catch exception thrown // by bad stack walk in EnumMemWalkStackHelper. // - PTR_CONTEXT pContext = ((ClrDataExceptionState*)pExcepState.GetValue())->GetCurrentContextRecord(); + PTR_CONTEXT pContext = ((ClrDataExceptionState*)(IXCLRDataExceptionState*)pExcepState)->GetCurrentContextRecord(); if (pContext != NULL) { T_CONTEXT newContext; @@ -1306,7 +1306,7 @@ HRESULT ClrDataAccess::EnumMemDumpAllThreadsStack(CLRDataEnumMemoryFlags flags) { status = EnumMemWalkStackHelper(flags, pStackWalk, pThread); } - pStackWalk.Clear(); + pStackWalk.Free(); } } } @@ -1324,7 +1324,7 @@ HRESULT ClrDataAccess::EnumMemDumpAllThreadsStack(CLRDataEnumMemoryFlags flags) EX_CATCH_RETHROW_ONLY_COR_E_OPERATIONCANCELLED // get next thread - pIXCLRDataTask.Clear(); + pIXCLRDataTask.Free(); status = EnumTask(&handle, &pIXCLRDataTask); } EndEnumTasks(handle); @@ -1381,7 +1381,7 @@ HRESULT ClrDataAccess::EnumMemStowedException(CLRDataEnumMemoryFlags flags) } EX_TRY { - if (((ClrDataTask *)pIXCLRDataTask.GetValue())->GetThread()->GetOSThreadId() == exThreadID) + if (((ClrDataTask*)(IXCLRDataTask*)pIXCLRDataTask)->GetThread()->GetOSThreadId() == exThreadID) { // found the thread foundThread = TRUE; @@ -1391,7 +1391,7 @@ HRESULT ClrDataAccess::EnumMemStowedException(CLRDataEnumMemoryFlags flags) EX_CATCH_RETHROW_ONLY_COR_E_OPERATIONCANCELLED // get next thread - pIXCLRDataTask.Clear(); + pIXCLRDataTask.Free(); status = EnumTask(&handle, &pIXCLRDataTask); } EndEnumTasks(handle); diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index 93369aa1357008..de30ac0c73b973 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -3027,7 +3027,7 @@ void Debugger::getBoundariesHelper(MethodDesc * md, (void)pModule; //prevent "unused variable" error from GCC _ASSERTE(pModule != NULL); - ComHolderPreemp pReader(pModule->GetISymUnmanagedReader()); + ReleaseHolder pReader(pModule->GetISymUnmanagedReader()); // If we got a reader, use it. if (pReader != NULL) diff --git a/src/coreclr/dlls/mscoree/exports.cpp b/src/coreclr/dlls/mscoree/exports.cpp index 30f1846c848c79..c52919fd95ce83 100644 --- a/src/coreclr/dlls/mscoree/exports.cpp +++ b/src/coreclr/dlls/mscoree/exports.cpp @@ -395,8 +395,7 @@ int coreclr_initialize( if (SUCCEEDED(hr)) { - host.SuppressRelease(); - *hostHandle = host; + *hostHandle = host.Detach(); #ifdef FEATURE_GDBJIT HRESULT createDelegateResult; createDelegateResult = coreclr_create_delegate(*hostHandle, diff --git a/src/coreclr/inc/holder.h b/src/coreclr/inc/holder.h index 5949fde1ab55ab..286d9ae6451372 100644 --- a/src/coreclr/inc/holder.h +++ b/src/coreclr/inc/holder.h @@ -813,37 +813,6 @@ class SpecializedWrapper : public Wrapper<_TYPE*, DoNothing<_TYPE*>, _RELEASEF, INDEBUG_AND_WINDOWS_FOR_HOLDERS(_TYPE ** m_pvalue;) }; -//----------------------------------------------------------------------------- -// NOTE: THIS IS UNSAFE TO USE IN THE VM for interop COM objects!! -// WE DO NOT CORRECTLY CHANGE TO PREEMPTIVE MODE BEFORE CALLING RELEASE!! -// USE ComHolderAnyMode -// -// ReleaseHolder : COM Interface holder for use outside the VM (or on well known instances -// which do not need preemptive Release) -// -// Usage example: -// -// { -// ReleaseHolder foo; -// hr = FunctionToGetRefOfFoo(&foo); -// // Note ComHolder doesn't call AddRef - it assumes you already have a ref (if non-0). -// } // foo->Release() on out of scope (WITHOUT RESPECT FOR GC MODE!!) -// -//----------------------------------------------------------------------------- - -template -FORCEINLINE void DoTheRelease(TYPE *value) -{ - STATIC_CONTRACT_WRAPPER; - if (value) - { - value->Release(); - } -} - -template -using ReleaseHolder = SpecializedWrapper<_TYPE, DoTheRelease<_TYPE>>; - //----------------------------------------------------------------------------- // NewHolder : New'ed memory holder // @@ -1022,6 +991,47 @@ class LifetimeHolder final } }; +// Detects whether a type exposes a callable Release() member. Used by the COM +// interface holder traits, which invoke Release() to relinquish their reference. +namespace HolderDetail +{ + template + struct HasReleaseMethod : std::false_type {}; + + template + struct HasReleaseMethod().Release())>> : std::true_type {}; +} + +template +struct ReleaseHolderTraits final +{ + static_assert( + HolderDetail::HasReleaseMethod::value, + "TYPE must have a Release() member"); + + using Type = TYPE*; + static constexpr Type Default() { return NULL; } + static void Free(Type value) + { +#ifdef ENABLE_CONTRACTS_IMPL + CONTRACTL + { + NOTHROW; + GC_TRIGGERS; + MODE_PREEMPTIVE; + } CONTRACTL_END; +#else + STATIC_CONTRACT_NOTHROW; + STATIC_CONTRACT_MODE_PREEMPTIVE; +#endif + if (value != NULL) + value->Release(); + } +}; + +template +using ReleaseHolder = LifetimeHolder>; + //----------------------------------------------------------------------------- // Wrap win32 functions using HANDLE //----------------------------------------------------------------------------- @@ -1382,7 +1392,7 @@ namespace clr { STATIC_CONTRACT_LIMITED_METHOD; //@TODO: Would be good to add runtime validation that the return value is used. - return SafeAddRef(pItf.GetValue()); + return SafeAddRef((ItfT*)pItf); } namespace detail diff --git a/src/coreclr/md/enc/mdinternalrw.cpp b/src/coreclr/md/enc/mdinternalrw.cpp index 00ff80614892fa..b4e658942bb77a 100644 --- a/src/coreclr/md/enc/mdinternalrw.cpp +++ b/src/coreclr/md/enc/mdinternalrw.cpp @@ -196,7 +196,7 @@ STDAPI GetMDInternalInterfaceFromPublic( if (riid != IID_IMDInternalImport || pIUnkPublic == NULL || ppIUnkInternal == NULL) IfFailGo(E_INVALIDARG); - IfFailGo( pIUnkPublic->QueryInterface(IID_IGetIMDInternalImport, &pGetIMDInternalImport)); + IfFailGo( pIUnkPublic->QueryInterface(IID_IGetIMDInternalImport, (void**)&pGetIMDInternalImport)); IfFailGo( pGetIMDInternalImport->GetIMDInternalImport((IMDInternalImport **)ppIUnkInternal)); ErrExit: diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index db7d66a6b0326c..12956082ebc49d 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -3069,33 +3069,20 @@ void AppDomain::GetParentAssemblyChain(Assembly *pStartAssembly, SString &chain, } } -PEAssembly* AppDomain::FindCachedFile(AssemblySpec* pSpec, BOOL fThrow /*=TRUE*/) +PEAssembly* AppDomain::FindCachedFile(AssemblySpec* pSpec) { - CONTRACTL - { - if (fThrow) { - GC_TRIGGERS; - THROWS; - } - else { - GC_NOTRIGGER; - NOTHROW; - } - MODE_ANY; - } - CONTRACTL_END; + STANDARD_VM_CONTRACT; + _ASSERTE(pSpec != NULL); // Check to see if this fits our rather loose idea of a reference to CoreLib. // If so, don't use fusion to bind it - do it ourselves. - if (fThrow && pSpec->IsCoreLib()) + if (pSpec->IsCoreLib()) { CONSISTENCY_CHECK(SystemDomain::System()->SystemAssembly() != NULL); - PEAssembly * pPEAssembly = SystemDomain::System()->SystemPEAssembly(); - pPEAssembly->AddRef(); - return pPEAssembly; + return SystemDomain::System()->SystemPEAssembly(); } - return m_AssemblyCache.LookupFile(pSpec, fThrow); + return m_AssemblyCache.LookupFile(pSpec, /* fThrow */ TRUE); } @@ -3161,7 +3148,8 @@ PEAssembly * AppDomain::BindAssemblySpec( BinderTracing::AssemblyBindOperation bindOperation(pSpec); HRESULT hrBindResult = S_OK; - PEAssemblyHolder result; + // Retain the lifetime of the non-cached PEAssembly until the caller has a chance to add it to the cache. + PEAssemblyHolder nonCachedLifetime; StackSString bindDiagnosticInfo; bool isCached = false; @@ -3170,7 +3158,7 @@ PEAssembly * AppDomain::BindAssemblySpec( isCached = IsCached(pSpec); if (!isCached) { - + PEAssembly* result = NULL; { ReleaseHolder boundAssembly; hrBindResult = pSpec->Bind(this, &boundAssembly, &bindDiagnosticInfo); @@ -3181,12 +3169,12 @@ PEAssembly * AppDomain::BindAssemblySpec( { // Avoid rebinding to another copy of CoreLib result = SystemDomain::SystemPEAssembly(); - result.SuppressRelease(); // Didn't get a refcount } else { // IsSystem on the PEAssembly should be false, even for CoreLib satellites result = PEAssembly::Open(boundAssembly); + nonCachedLifetime = result; } // Setup the reference to the binder, which performed the bind, into the AssemblySpec @@ -3312,16 +3300,15 @@ PEAssembly * AppDomain::BindAssemblySpec( EX_END_CATCH // Now, if it's a cacheable bind we need to re-fetch the result from the cache, as we may have been racing with another - // thread to store our result. Note that we may throw from here, if there is a cached exception. - // This will release the refcount of the current result holder (if any), and will replace - // it with a non-addref'ed result - result = FindCachedFile(pSpec); - + // thread to store our result. Note that we may throw from here, if there is a cached exception. + // Note the non-cached result holder above may be released (if any). + // Returned cached files are not AddRef'd, so we call AddRef inorder to retain it. + PEAssemblyHolder result{ FindCachedFile(pSpec) }; if (result != NULL) result->AddRef(); - bindOperation.SetResult(result.GetValue(), isCached); - return result.Extract(); + bindOperation.SetResult(result, isCached); + return result.Detach(); } // AppDomain::BindAssemblySpec diff --git a/src/coreclr/vm/appdomain.hpp b/src/coreclr/vm/appdomain.hpp index b005646a79db6b..655f295fa7055f 100644 --- a/src/coreclr/vm/appdomain.hpp +++ b/src/coreclr/vm/appdomain.hpp @@ -1115,7 +1115,7 @@ class AppDomain final void GetParentAssemblyChain(Assembly *pStartAssembly, SString &chain, int maxDepth); private: - PEAssembly* FindCachedFile(AssemblySpec* pSpec, BOOL fThrow = TRUE); + PEAssembly* FindCachedFile(AssemblySpec* pSpec); BOOL IsCached(AssemblySpec *pSpec); #endif // DACCESS_COMPILE diff --git a/src/coreclr/vm/assembly.cpp b/src/coreclr/vm/assembly.cpp index 35bbfbea990a24..48f5e5dd1788d2 100644 --- a/src/coreclr/vm/assembly.cpp +++ b/src/coreclr/vm/assembly.cpp @@ -73,7 +73,7 @@ namespace } CONTRACT_END; - ComHolderAnyMode pDispenser; + ReleaseHolderAnyMode pDispenser; // Get the Dispenser interface. CreateMetaDataDispenser(IID_IMetaDataDispenserEx, (void**)&pDispenser); @@ -407,7 +407,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar // add such a reference. Also because the referenced assembly if dynamic strong name, it may // not be ready to be hashed! - ComHolderAnyMode pAssemblyEmit; + ReleaseHolderAnyMode pAssemblyEmit; DefineEmitScope( IID_IMetaDataAssemblyEmit, (void**)&pAssemblyEmit); @@ -902,14 +902,14 @@ void Assembly::CacheFriendAssemblyInfo() if (m_pFriendAssemblyDescriptor == NULL) { - ReleaseHolder pFriendAssemblies = FriendAssemblyDescriptor::CreateFriendAssemblyDescriptor(this->GetPEAssembly()); + ReleaseHolder pFriendAssemblies{ FriendAssemblyDescriptor::CreateFriendAssemblyDescriptor(this->GetPEAssembly()) }; _ASSERTE(pFriendAssemblies != NULL); CrstHolder friendDescriptorLock(&g_friendAssembliesCrst); if (m_pFriendAssemblyDescriptor == NULL) { - m_pFriendAssemblyDescriptor = pFriendAssemblies.Extract(); + m_pFriendAssemblyDescriptor = pFriendAssemblies.Detach(); } } } // void Assembly::CacheFriendAssemblyInfo() @@ -937,7 +937,7 @@ void Assembly::UpdateCachedFriendAssemblyInfo() while (true) { - ReleaseHolder pFriendAssemblies = FriendAssemblyDescriptor::CreateFriendAssemblyDescriptor(this->GetPEAssembly()); + ReleaseHolder pFriendAssemblies{ FriendAssemblyDescriptor::CreateFriendAssemblyDescriptor(this->GetPEAssembly()) }; FriendAssemblyDescriptor* pFriendAssemblyDescriptorNextLoop = NULL; { @@ -948,7 +948,7 @@ void Assembly::UpdateCachedFriendAssemblyInfo() if (m_pFriendAssemblyDescriptor != NULL) m_pFriendAssemblyDescriptor->Release(); - m_pFriendAssemblyDescriptor = pFriendAssemblies.Extract(); + m_pFriendAssemblyDescriptor = pFriendAssemblies.Detach(); return; } else @@ -2513,7 +2513,7 @@ ReleaseHolder FriendAssemblyDescriptor::CreateFriendAs } CONTRACTL_END - ReleaseHolder pFriendAssemblies = new FriendAssemblyDescriptor; + ReleaseHolder pFriendAssemblies{ new FriendAssemblyDescriptor }; // We're going to do this twice, once for InternalsVisibleTo and once for IgnoresAccessChecks IMDInternalImport* pImport = pAssembly->GetMDImport(); diff --git a/src/coreclr/vm/assemblynative.cpp b/src/coreclr/vm/assemblynative.cpp index 97ee4dfc6dfbab..161671b4ec5ec5 100644 --- a/src/coreclr/vm/assemblynative.cpp +++ b/src/coreclr/vm/assemblynative.cpp @@ -191,7 +191,7 @@ Assembly* AssemblyNative::LoadFromPEImage(AssemblyBinder* pBinder, PEImage *pIma } PEAssemblyHolder pPEAssembly(PEAssembly::Open(pAssembly->GetPEImage(), pAssembly)); - bindOperation.SetResult(pPEAssembly.GetValue()); + bindOperation.SetResult(pPEAssembly); RETURN pCurDomain->LoadAssembly(&spec, pPEAssembly, FILE_LOADED); } diff --git a/src/coreclr/vm/ceeload.cpp b/src/coreclr/vm/ceeload.cpp index e8e66f853857a8..c279eb4c7861d8 100644 --- a/src/coreclr/vm/ceeload.cpp +++ b/src/coreclr/vm/ceeload.cpp @@ -1756,7 +1756,7 @@ ISymUnmanagedReader *Module::GetISymUnmanagedReader(void) // where right now... HRESULT hr = S_OK; - ComHolderPreemp pBinder; + ReleaseHolder pBinder; if (g_pDebugInterface == NULL) { @@ -1784,11 +1784,11 @@ ISymUnmanagedReader *Module::GetISymUnmanagedReader(void) // hard disk for files. ErrorModeHolder errorMode{}; - ComHolderPreemp pReader; + ReleaseHolder pReader; if (fInMemorySymbols) { - ComHolderPreemp pIStream( NULL ); + ReleaseHolder pIStream; // If debug stream is already specified, don't bother to go through fusion // This is the common case for case 2 (hosted modules) and case 3 (Ref.Emit). @@ -1823,7 +1823,7 @@ ISymUnmanagedReader *Module::GetISymUnmanagedReader(void) // Call Fusion to ensure that any PDB's are shadow copied before // trying to get a symbol reader. This has to be done once per // Assembly. - ReleaseHolder pUnk = NULL; + ReleaseHolder pUnk; hr = GetReadablePublicMetaDataInterface(ofReadOnly, IID_IMetaDataImport, &pUnk); if (SUCCEEDED(hr)) hr = pBinder->GetReaderForFile(pUnk, path, NULL, &pReader); @@ -1890,7 +1890,7 @@ void Module::SetSymbolBytes(LPCBYTE pbSyms, DWORD cbSyms) STANDARD_VM_CONTRACT; // Create a IStream from the memory for the syms. - ComHolderPreemp pStream(new CGrowableStream()); + ReleaseHolder pStream(new CGrowableStream()); // Do not need to AddRef the CGrowableStream because the constructor set it to 1 // ref count already. The Module will keep a copy for its own use. @@ -2463,7 +2463,7 @@ Assembly * Module::LoadAssemblyImpl(mdAssemblyRef kAssemblyRef) } { - PEAssemblyHolder pPEAssembly = GetPEAssembly()->LoadAssembly(kAssemblyRef); + PEAssemblyHolder pPEAssembly{ GetPEAssembly()->LoadAssembly(kAssemblyRef) }; AssemblySpec spec; spec.InitializeSpec(kAssemblyRef, GetMDImport(), GetAssembly()); // Set the binding context in the AssemblySpec if one is available. This can happen if the LoadAssembly ended up diff --git a/src/coreclr/vm/clrex.cpp b/src/coreclr/vm/clrex.cpp index e885e4898e9a9d..700532f2ac4587 100644 --- a/src/coreclr/vm/clrex.cpp +++ b/src/coreclr/vm/clrex.cpp @@ -686,7 +686,7 @@ OBJECTREF CLRException::GetThrowableFromException(Exception *pException) else { #ifdef FEATURE_COMINTEROP - ComHolderAnyMode pErrInfo(pException->GetErrorInfo()); + ReleaseHolderAnyMode pErrInfo(pException->GetErrorInfo()); if (pErrInfo != NULL) { diff --git a/src/coreclr/vm/comcache.cpp b/src/coreclr/vm/comcache.cpp index 32ac17778082f3..24ff0bb3dd1194 100644 --- a/src/coreclr/vm/comcache.cpp +++ b/src/coreclr/vm/comcache.cpp @@ -81,7 +81,7 @@ static IErrorInfo *CheckForFuncEvalAbortNoThrow(HRESULT hr) if (SafeQueryInterface(pErrorInfo, IID_IFuncEvalAbort, &pUnk) == S_OK) { // QI succeeded, this is a func eval abort - return pErrorInfo.Extract(); + return pErrorInfo.Detach(); } else { @@ -899,7 +899,7 @@ IUnknown* IUnkEntry::UnmarshalIUnknownForCurrContextHelper() HRESULT hrCDH = S_OK; IUnknown * pUnk = NULL; - ComHolderAnyMode spStream; + ReleaseHolderAnyMode spStream; CheckValidIUnkEntry(); @@ -1020,7 +1020,7 @@ bool IUnkEntry::IsComponentFreeThreaded(IUnknown *pUnk) CONTRACTL_END; // First see if the object implements the IAgileObject marker interface - ComHolderPreemp pAgileObject; + ReleaseHolder pAgileObject; HRESULT hr = SafeQueryInterfacePreemp(pUnk, IID_IAgileObject, (IUnknown**)&pAgileObject); LogInteropQI(pUnk, IID_IAgileObject, hr, "IUnkEntry::IsComponentFreeThreaded: QI for IAgileObject"); @@ -1030,7 +1030,7 @@ bool IUnkEntry::IsComponentFreeThreaded(IUnknown *pUnk) } else { - ComHolderPreemp pMarshal; + ReleaseHolder pMarshal; // If not, then we can try to determine if the component aggregates the FTM via IMarshal. hr = SafeQueryInterfacePreemp(pUnk, IID_IMarshal, (IUnknown **)&pMarshal); @@ -1321,7 +1321,7 @@ HRESULT CtxEntry::EnterContext(PFNCTXCALLBACK pCallbackFunc, LPVOID pData) CallbackInfo.m_UserCallbackHR = E_FAIL; // Retrieve the IContextCallback interface from the IObjectContext. - ComHolderPreemp pCallback; + ReleaseHolder pCallback; hr = SafeQueryInterfacePreemp(m_pObjCtx, IID_IContextCallback, (IUnknown**)&pCallback); LogInteropQI(m_pObjCtx, IID_IContextCallback, hr, "QI for IID_IContextCallback"); _ASSERTE(SUCCEEDED(hr) && pCallback); @@ -1346,7 +1346,7 @@ HRESULT CtxEntry::EnterContext(PFNCTXCALLBACK pCallbackFunc, LPVOID pData) { // If the transition failed because of an aborted func eval, simply propagate // the HRESULT/IErrorInfo back to the caller as we cannot throw here. - ComHolderPreemp pErrorInfo{ CheckForFuncEvalAbortNoThrow(hr) }; + ReleaseHolder pErrorInfo{ CheckForFuncEvalAbortNoThrow(hr) }; if (pErrorInfo != NULL) { LOG((LF_INTEROP, LL_INFO100, "Entering into context 0x08X has failed since the debugger is blocking it\n", m_pCtxCookie)); diff --git a/src/coreclr/vm/comcallablewrapper.cpp b/src/coreclr/vm/comcallablewrapper.cpp index 91e19a289925b6..f5b1e9637e1ba5 100644 --- a/src/coreclr/vm/comcallablewrapper.cpp +++ b/src/coreclr/vm/comcallablewrapper.cpp @@ -710,7 +710,7 @@ BOOL SimpleComCallWrapper::CustomQIRespondsToIMarshal() { DWORD newFlags = enum_CustomQIRespondsToIMarshal_Inited; - ComHolderAnyMode pUnk; + ReleaseHolderAnyMode pUnk; if (GetComIPFromCCW_HandleCustomQI(GetMainWrapper(), IID_IMarshal, NULL, &pUnk)) { newFlags |= enum_CustomQIRespondsToIMarshal; @@ -2209,7 +2209,7 @@ static IUnknown * GetComIPFromCCW_HandleExtendsCOMObject( SyncBlock* pBlock = pWrap->GetSyncBlock(); _ASSERTE(pBlock); - ComHolderAnyMode pUnk; + ReleaseHolderAnyMode pUnk; RCWHolder pRCW(GetThread()); RCWPROTECT_BEGIN(pRCW, pBlock); @@ -2588,7 +2588,7 @@ IDispatch* ComCallWrapper::GetIDispatchIP() if ((DefItfType == DefaultInterfaceType_AutoDual) || (DefItfType == DefaultInterfaceType_AutoDispatch)) { // Make sure we release the BasicIP we're about to get. - ComHolderAnyMode pBasic{ GetBasicIP() }; + ReleaseHolderAnyMode pBasic{ GetBasicIP() }; ComMethodTable* pCMT = ComMethodTable::ComMethodTableFromIP(pBasic); pCMT->CheckParentComVisibility(); } @@ -2636,7 +2636,7 @@ IDispatch* ComCallWrapper::GetIDispatchIP() SyncBlock* pBlock = GetSyncBlock(); _ASSERTE(pBlock); - ComHolderAnyMode pDisp; + ReleaseHolderAnyMode pDisp; RCWHolder pRCW(GetThread()); RCWPROTECT_BEGIN(pRCW, pBlock); @@ -4365,14 +4365,13 @@ ComCallWrapperTemplate* ComCallWrapperTemplate::CreateTemplate(TypeHandle thClas // Check to see if another thread has already set up the template. { // Move this inside the scope so it is destroyed before its memory is. - ComCallWrapperTemplateHolder pTemplate = NULL; + ComCallWrapperTemplateHolder pTemplate; pTemplate = thClass.GetComCallWrapperTemplate(); if (pTemplate) { - pTemplate.SuppressRelease(); - RETURN pTemplate; + RETURN pTemplate.Detach(); } // Allocate the template. @@ -4423,24 +4422,24 @@ ComCallWrapperTemplate* ComCallWrapperTemplate::CreateTemplate(TypeHandle thClas pTemplate = thClass.GetComCallWrapperTemplate(); _ASSERTE(pTemplate != NULL); - pTemplate.SuppressRelease(); - RETURN pTemplate; + RETURN pTemplate.Detach(); } - pTemplate.SuppressRelease(); + // The class now owns the template refcount; take a non-owning pointer for the return path. + ComCallWrapperTemplate* pRetTemplate = pTemplate.Detach(); #ifdef PROFILING_SUPPORTED // Notify profiler of the CCW, so it can avoid double-counting. - if (pTemplate->SupportsIClassX()) + if (pRetTemplate->SupportsIClassX()) { BEGIN_PROFILER_CALLBACK(CORProfilerTrackCCW()); // When under the profiler, we'll eagerly generate the IClassX CMT. - pTemplate->GetClassComMT(); + pRetTemplate->GetClassComMT(); IID IClassXIID = GUID_NULL; - SLOT *pComVtable = (SLOT *)(pTemplate->m_pClassComMT + 1); + SLOT *pComVtable = (SLOT *)(pRetTemplate->m_pClassComMT + 1); // If the class is visible from COM, then give out the IClassX IID. - if (pTemplate->m_pClassComMT->IsComVisible()) + if (pRetTemplate->m_pClassComMT->IsComVisible()) GenerateClassItfGuid(thClass, &IClassXIID); #if defined(_DEBUG) @@ -4456,12 +4455,12 @@ ComCallWrapperTemplate* ComCallWrapperTemplate::CreateTemplate(TypeHandle thClas #endif (&g_profControlBlock)->COMClassicVTableCreated( (ClassID) thClass.AsPtr(), IClassXIID, pComVtable, - pTemplate->m_pClassComMT->m_cbSlots + - ComMethodTable::GetNumExtraSlots(pTemplate->m_pClassComMT->GetInterfaceType())); + pRetTemplate->m_pClassComMT->m_cbSlots + + ComMethodTable::GetNumExtraSlots(pRetTemplate->m_pClassComMT->GetInterfaceType())); END_PROFILER_CALLBACK(); } #endif // PROFILING_SUPPORTED - RETURN pTemplate; + RETURN pRetTemplate; } } @@ -4488,11 +4487,10 @@ ComCallWrapperTemplate *ComCallWrapperTemplate::CreateTemplateForInterface(Metho unsigned numInterfaces = 1; // Allocate the template. - ComCallWrapperTemplateHolder pTemplate = pItfMT->GetComCallWrapperTemplate(); + ComCallWrapperTemplateHolder pTemplate{ pItfMT->GetComCallWrapperTemplate() }; if (pTemplate) { - pTemplate.SuppressRelease(); - RETURN pTemplate; + RETURN pTemplate.Detach(); } pTemplate = (ComCallWrapperTemplate *)new BYTE[sizeof(ComCallWrapperTemplate) + numInterfaces * sizeof(SLOT)]; @@ -4523,8 +4521,7 @@ ComCallWrapperTemplate *ComCallWrapperTemplate::CreateTemplateForInterface(Metho _ASSERTE(pTemplate != NULL); } - pTemplate.SuppressRelease(); - RETURN pTemplate; + RETURN pTemplate.Detach(); } void ComCallWrapperTemplate::DetermineComVisibility() diff --git a/src/coreclr/vm/comconnectionpoints.cpp b/src/coreclr/vm/comconnectionpoints.cpp index ec13d9199f6f66..05d078f3c3749a 100644 --- a/src/coreclr/vm/comconnectionpoints.cpp +++ b/src/coreclr/vm/comconnectionpoints.cpp @@ -313,7 +313,7 @@ void ConnectionPoint::AdviseWorker(IUnknown *pUnk, DWORD *pdwCookie) } CONTRACTL_END; - ComHolderAnyMode pEventItf; + ReleaseHolderAnyMode pEventItf; HRESULT hr; // Make sure we have a pointer to the interface and not to another IUnknown. diff --git a/src/coreclr/vm/commodule.cpp b/src/coreclr/vm/commodule.cpp index cc2308fc085f09..40ee2900fd0759 100644 --- a/src/coreclr/vm/commodule.cpp +++ b/src/coreclr/vm/commodule.cpp @@ -85,7 +85,7 @@ extern "C" mdTypeRef QCALLTYPE ModuleBuilder_GetTypeRef(QCall::ModuleHandle pMod { // reference to top level type - ComHolderPreemp pAssemblyEmit; + ReleaseHolder pAssemblyEmit; // Generate AssemblyRef IfFailThrow( pEmit->QueryInterface(IID_IMetaDataAssemblyEmit, (void **) &pAssemblyEmit) ); @@ -181,7 +181,7 @@ namespace mdToken rs; // resolution scope DWORD dwFlags; - ComHolderAnyMode pImport; + ReleaseHolderAnyMode pImport; IfFailThrow( pEmit->QueryInterface(IID_IMetaDataImport, (void **)&pImport) ); IfFailThrow( pImport->GetTypeDefProps(td, szTypeDef, MAX_CLASSNAME_LENGTH, NULL, &dwFlags, NULL) ); if ( IsTdNested(dwFlags) ) @@ -244,7 +244,7 @@ extern "C" INT32 QCALLTYPE ModuleBuilder_GetMemberRef(QCall::ModuleHandle pModul COMPlusThrow(kNotSupportedException, W("NotSupported_CollectibleBoundNonCollectible")); } - ComHolderPreemp pAssemblyEmit; + ReleaseHolder pAssemblyEmit; IfFailThrow( pRefingAssembly->GetModule()->GetEmitter()->QueryInterface(IID_IMetaDataAssemblyEmit, (void **) &pAssemblyEmit) ); CQuickBytes qbNewSig; @@ -321,7 +321,7 @@ extern "C" INT32 QCALLTYPE ModuleBuilder_GetMemberRefOfMethodInfo(QCall::ModuleH Assembly * pRefedAssembly = pMeth->GetModule()->GetAssembly(); Assembly * pRefingAssembly = pModule->GetAssembly(); - ComHolderPreemp pAssemblyEmit; + ReleaseHolder pAssemblyEmit; IfFailThrow( pRefingAssembly->GetModule()->GetEmitter()->QueryInterface(IID_IMetaDataAssemblyEmit, (void **) &pAssemblyEmit) ); CQuickBytes qbNewSig; @@ -407,7 +407,7 @@ extern "C" mdMemberRef QCALLTYPE ModuleBuilder_GetMemberRefOfFieldInfo(QCall::Mo else COMPlusThrow(kNotSupportedException, W("NotSupported_CollectibleBoundNonCollectible")); } - ComHolderPreemp pAssemblyEmit; + ReleaseHolder pAssemblyEmit; IfFailThrow( pRefingAssembly->GetModule()->GetEmitter()->QueryInterface(IID_IMetaDataAssemblyEmit, (void **) &pAssemblyEmit) ); // Translate the field signature this scope diff --git a/src/coreclr/vm/coreassemblyspec.cpp b/src/coreclr/vm/coreassemblyspec.cpp index b3677a8460ab4e..1962821fb7bd36 100644 --- a/src/coreclr/vm/coreassemblyspec.cpp +++ b/src/coreclr/vm/coreassemblyspec.cpp @@ -68,7 +68,7 @@ HRESULT AssemblySpec::Bind(AppDomain *pAppDomain, BINDER_SPACE::Assembly** ppAs if (SUCCEEDED(hr)) { _ASSERTE(pPrivAsm != nullptr); - *ppAssembly = pPrivAsm.Extract(); + *ppAssembly = pPrivAsm.Detach(); } return hr; diff --git a/src/coreclr/vm/crossloaderallocatorhash.inl b/src/coreclr/vm/crossloaderallocatorhash.inl index 1dc179bf051e2c..232492b33643de 100644 --- a/src/coreclr/vm/crossloaderallocatorhash.inl +++ b/src/coreclr/vm/crossloaderallocatorhash.inl @@ -831,12 +831,12 @@ CrossLoaderAllocatorHash::GetDependentTrackerForLoaderAllocator(LoaderAl } NewHolder laDependentKeyToValuesHashHolder = new LADependentKeyToValuesHash(); - ReleaseHolder dependentTrackerHolder = - new LAHashDependentHashTracker(pLoaderAllocator, laDependentKeyToValuesHashHolder); + ReleaseHolder dependentTrackerHolder{ + new LAHashDependentHashTracker(pLoaderAllocator, laDependentKeyToValuesHashHolder) }; laDependentKeyToValuesHashHolder.SuppressRelease(); dependentTrackerHash.Add(dependentTrackerHolder); - return dependentTrackerHolder.Extract(); + return dependentTrackerHolder.Detach(); } #endif // !DACCESS_COMPILE diff --git a/src/coreclr/vm/dispparammarshaler.cpp b/src/coreclr/vm/dispparammarshaler.cpp index 8e61e12d489105..88a123a596db3f 100644 --- a/src/coreclr/vm/dispparammarshaler.cpp +++ b/src/coreclr/vm/dispparammarshaler.cpp @@ -589,8 +589,8 @@ void DispParamCustomMarshaler::MarshalManagedToNative(OBJECTREF *pSrcObj, VARIAN } CONTRACTL_END; - ComHolderAnyMode pUnk; - ComHolderAnyMode pDisp; + ReleaseHolderAnyMode pUnk; + ReleaseHolderAnyMode pDisp; // Convert the object using the custom marshaler. SafeVariantClear(pDestVar); diff --git a/src/coreclr/vm/dllimport.cpp b/src/coreclr/vm/dllimport.cpp index d861f83a073e65..14ea54d1cf9925 100644 --- a/src/coreclr/vm/dllimport.cpp +++ b/src/coreclr/vm/dllimport.cpp @@ -5308,7 +5308,7 @@ namespace { pStubMD = ilStubCreatorHelper.GetStubMD(); - pEntry.Assign(ListLockEntry::Find(pILStubLock, pStubMD, "il stub gen lock")); + pEntry = ListLockEntry::Find(pILStubLock, pStubMD, "il stub gen lock"); pEntryLock.Assign(pEntry, FALSE); // We have the entry lock we need to use, so we can release the global lock. diff --git a/src/coreclr/vm/eetoprofinterfaceimpl.cpp b/src/coreclr/vm/eetoprofinterfaceimpl.cpp index 5de81da403d444..1230467d24674d 100644 --- a/src/coreclr/vm/eetoprofinterfaceimpl.cpp +++ b/src/coreclr/vm/eetoprofinterfaceimpl.cpp @@ -371,8 +371,7 @@ static HRESULT CoCreateProfiler( } // Ok, safe to transfer ownership to caller's [out] param - *ppCallback = pCallback2FromQI.Extract(); - pCallback2FromQI = NULL; + *ppCallback = pCallback2FromQI.Detach(); return S_OK; } @@ -681,8 +680,7 @@ HRESULT EEToProfInterfaceImpl::CreateProfiler( // Save profiler pointers into this. The reference ownership now // belongs to this class, so NULL out locals without allowing them to release - m_pCallback2 = pCallback2.Extract(); - pCallback2 = NULL; + m_pCallback2 = pCallback2.Detach(); m_hmodProfilerDLL = hmodProfilerDLL.Detach(); // ATTENTION: Please update EEToProfInterfaceImpl::~EEToProfInterfaceImpl() after adding the next ICorProfilerCallback interface here !!! @@ -695,8 +693,7 @@ HRESULT EEToProfInterfaceImpl::CreateProfiler( if (SUCCEEDED(hr) && (pCallback11 != NULL)) { _ASSERTE(m_pCallback11 == NULL); - m_pCallback11 = pCallback11.Extract(); - pCallback11 = NULL; + m_pCallback11 = pCallback11.Detach(); } if (m_pCallback11 == NULL) @@ -708,8 +705,7 @@ HRESULT EEToProfInterfaceImpl::CreateProfiler( if (SUCCEEDED(hr) && (pCallback10 != NULL)) { _ASSERTE(m_pCallback10 == NULL); - m_pCallback10 = pCallback10.Extract(); - pCallback10 = NULL; + m_pCallback10 = pCallback10.Detach(); } } else @@ -731,8 +727,7 @@ HRESULT EEToProfInterfaceImpl::CreateProfiler( if (SUCCEEDED(hr) && (pCallback9 != NULL)) { _ASSERTE(m_pCallback9 == NULL); - m_pCallback9 = pCallback9.Extract(); - pCallback9 = NULL; + m_pCallback9 = pCallback9.Detach(); } } else @@ -751,8 +746,7 @@ HRESULT EEToProfInterfaceImpl::CreateProfiler( if (SUCCEEDED(hr) && (pCallback8 != NULL)) { _ASSERTE(m_pCallback8 == NULL); - m_pCallback8 = pCallback8.Extract(); - pCallback8 = NULL; + m_pCallback8 = pCallback8.Detach(); } } else @@ -771,8 +765,7 @@ HRESULT EEToProfInterfaceImpl::CreateProfiler( if (SUCCEEDED(hr) && (pCallback7 != NULL)) { _ASSERTE(m_pCallback7 == NULL); - m_pCallback7 = pCallback7.Extract(); - pCallback7 = NULL; + m_pCallback7 = pCallback7.Detach(); } } else @@ -791,8 +784,7 @@ HRESULT EEToProfInterfaceImpl::CreateProfiler( if (SUCCEEDED(hr) && (pCallback6 != NULL)) { _ASSERTE(m_pCallback6 == NULL); - m_pCallback6 = pCallback6.Extract(); - pCallback6 = NULL; + m_pCallback6 = pCallback6.Detach(); } } else @@ -811,8 +803,7 @@ HRESULT EEToProfInterfaceImpl::CreateProfiler( if (SUCCEEDED(hr) && (pCallback5 != NULL)) { _ASSERTE(m_pCallback5 == NULL); - m_pCallback5 = pCallback5.Extract(); - pCallback5 = NULL; + m_pCallback5 = pCallback5.Detach(); } } else @@ -831,8 +822,7 @@ HRESULT EEToProfInterfaceImpl::CreateProfiler( if (SUCCEEDED(hr) && (pCallback4 != NULL)) { _ASSERTE(m_pCallback4 == NULL); - m_pCallback4 = pCallback4.Extract(); - pCallback4 = NULL; + m_pCallback4 = pCallback4.Detach(); } } else @@ -851,8 +841,7 @@ HRESULT EEToProfInterfaceImpl::CreateProfiler( if (SUCCEEDED(hr) && (pCallback3 != NULL)) { _ASSERTE(m_pCallback3 == NULL); - m_pCallback3 = pCallback3.Extract(); - pCallback3 = NULL; + m_pCallback3 = pCallback3.Detach(); } } else diff --git a/src/coreclr/vm/encee.cpp b/src/coreclr/vm/encee.cpp index 8e9c2761c40f70..c44d8f322c9cdb 100644 --- a/src/coreclr/vm/encee.cpp +++ b/src/coreclr/vm/encee.cpp @@ -152,9 +152,9 @@ HRESULT EditAndContinueModule::ApplyEditAndContinue( HRESULT hr = S_OK; - CONTRACT_VIOLATION(GCViolation); // ComHolderAnyMode goes to preemptive mode, which will trigger a GC - ComHolderAnyMode pIMDInternalImportENC; - ComHolderAnyMode pEmitter; + CONTRACT_VIOLATION(GCViolation); // ReleaseHolderAnyMode goes to preemptive mode, which will trigger a GC + ReleaseHolderAnyMode pIMDInternalImportENC; + ReleaseHolderAnyMode pEmitter; // Apply the changes. Note that ApplyEditAndContinue() requires read/write metadata. If the metadata is // not already RW, then ApplyEditAndContinue() will perform the conversion, invalidate the current diff --git a/src/coreclr/vm/interopconverter.cpp b/src/coreclr/vm/interopconverter.cpp index b652cb7b4ebfd3..7d5fa1fe8f4f4e 100644 --- a/src/coreclr/vm/interopconverter.cpp +++ b/src/coreclr/vm/interopconverter.cpp @@ -93,7 +93,7 @@ IUnknown *GetComIPFromObjectRef(OBJECTREF *poref, MethodTable *pMT, BOOL bEnable BOOL fReleaseWrapper = false; HRESULT hr = E_NOINTERFACE; - ComHolderAnyMode pUnk; + ReleaseHolderAnyMode pUnk; size_t ul = 0; if (*poref == NULL) @@ -123,7 +123,7 @@ IUnknown *GetComIPFromObjectRef(OBJECTREF *poref, MethodTable *pMT, BOOL bEnable // get the pUnk from the ComCallWrapper, otherwise from the RCW if ((NULL != pInteropInfo->GetCCW()) || (!pInteropInfo->RCWWasUsed())) { - CCWHolder pCCWHold = ComCallWrapper::InlineGetWrapper(poref); + CCWHolder pCCWHold{ ComCallWrapper::InlineGetWrapper(poref) }; GetComIPFromCCW::flags flags = GetComIPFromCCW::None; if (!bEnableCustomizedQueryInterface) { flags |= GetComIPFromCCW::SuppressCustomizedQueryInterface; } @@ -224,7 +224,7 @@ IUnknown *GetComIPFromObjectRef(OBJECTREF *poref, ComIpType ReqIpType, ComIpType if ( (NULL != pInteropInfo->GetCCW()) || (!pInteropInfo->RCWWasUsed()) ) { - CCWHolder pCCWHold = ComCallWrapper::InlineGetWrapper(poref); + CCWHolder pCCWHold{ ComCallWrapper::InlineGetWrapper(poref) }; // If the user requested IDispatch, then check for IDispatch first. if (ReqIpType & ComIpType_Dispatch) @@ -342,12 +342,12 @@ IUnknown *GetComIPFromObjectRef(OBJECTREF *poref, REFIID iid, bool throwIfNoComI if ((NULL != pInteropInfo->GetCCW()) || (!pInteropInfo->RCWWasUsed())) { - CCWHolder pCCWHold = ComCallWrapper::InlineGetWrapper(poref); + CCWHolder pCCWHold{ ComCallWrapper::InlineGetWrapper(poref) }; pUnk = ComCallWrapper::GetComIPFromCCW(pCCWHold, iid, NULL); } else { - ComHolderAnyMode pUnkHolder; + ReleaseHolderAnyMode pUnkHolder; RCWHolder pRCW(GetThread()); RCWPROTECT_BEGIN(pRCW, pBlock); @@ -407,7 +407,7 @@ void GetObjectRefFromComIP(OBJECTREF* pObjOut, IUnknown **ppUnk, MethodTable *pM Thread * pThread = GetThread(); IUnknown* pOuter = pUnk; - ComHolderAnyMode pAutoOuterUnk; + ReleaseHolderAnyMode pAutoOuterUnk; if (pUnk != NULL) { diff --git a/src/coreclr/vm/interoputil.cpp b/src/coreclr/vm/interoputil.cpp index 168d30bc606f12..b28071e0c3172a 100644 --- a/src/coreclr/vm/interoputil.cpp +++ b/src/coreclr/vm/interoputil.cpp @@ -3045,9 +3045,9 @@ void IUInvokeDispMethod( DISPID MemberID = 0; ByrefArgumentInfo* aByrefArgInfos = NULL; BOOL bSomeArgsAreByref = FALSE; - ComHolderAnyMode pUnk; - ComHolderAnyMode pDisp; - ComHolderAnyMode pDispEx; + ReleaseHolderAnyMode pUnk; + ReleaseHolderAnyMode pDisp; + ReleaseHolderAnyMode pDispEx; VariantPtrHolder pVarResult; NewArrayHolder params = NULL; @@ -3143,10 +3143,10 @@ void IUInvokeDispMethod( // we will not correctly detect that the user did something wrong and will crash. // This is a known issue with no solution. // Our check here is best effort to catch the simple case where a user may make a mistake. - ComHolderAnyMode pInvokedMTUnknown{ ComObject::GetComIPFromRCWThrowing(pTarget, pInvokedMT) }; + ReleaseHolderAnyMode pInvokedMTUnknown{ ComObject::GetComIPFromRCWThrowing(pTarget, pInvokedMT) }; // QI for IDispatch to catch the simple error case (COM object has no IDispatch but pInvokedMT is specified as a dispatch or dual interface) - ComHolderAnyMode pCanonicalDisp; + ReleaseHolderAnyMode pCanonicalDisp; hr = SafeQueryInterface(pInvokedMTUnknown, IID_IDispatch, &pCanonicalDisp); if (FAILED(hr)) COMPlusThrow(kTargetException, W("TargetInvocation_TargetDoesNotImplementIDispatch")); @@ -3924,7 +3924,7 @@ VOID LogInteropQI(IUnknown* pItf, REFIID iid, HRESULT hrArg, _In_z_ LPCSTR szMsg LPVOID pCurrCtx = NULL; HRESULT hr = S_OK; - ComHolderAnyMode pUnk; + ReleaseHolderAnyMode pUnk; CHAR szIID[MINIPAL_GUID_BUFFER_LEN]; hr = SafeQueryInterface(pItf, IID_IUnknown, &pUnk); @@ -3971,7 +3971,7 @@ VOID LogInteropAddRef(IUnknown* pItf, ULONG cbRef, _In_z_ LPCSTR szMsg) LPVOID pCurrCtx = NULL; HRESULT hr = S_OK; - ComHolderAnyMode pUnk; + ReleaseHolderAnyMode pUnk; hr = SafeQueryInterface(pItf, IID_IUnknown, &pUnk); diff --git a/src/coreclr/vm/marshalnative.cpp b/src/coreclr/vm/marshalnative.cpp index b63144baf8f916..de5285443fcd23 100644 --- a/src/coreclr/vm/marshalnative.cpp +++ b/src/coreclr/vm/marshalnative.cpp @@ -765,7 +765,7 @@ extern "C" IUnknown* QCALLTYPE MarshalNative_CreateAggregatedObject(IUnknown* pO COMPlusThrowArgumentException(W("o"), W("Argument_AlreadyACCW")); //get wrapper for the object, this could enable GC - CCWHolder pWrap = ComCallWrapper::InlineGetWrapper(&oref); + CCWHolder pWrap{ ComCallWrapper::InlineGetWrapper(&oref) }; // Aggregation support, pWrap->InitializeOuter(pOuter); @@ -1177,7 +1177,7 @@ extern "C" VOID QCALLTYPE MarshalNative_ChangeWrapperHandleStrength(QCall::Objec OBJECTREF oref = otp.Get(); GCPROTECT_BEGIN(oref); - CCWHolder pWrap = ComCallWrapper::InlineGetWrapper(&oref); + CCWHolder pWrap{ ComCallWrapper::InlineGetWrapper(&oref) }; if (fIsWeak) pWrap->MarkHandleWeak(); diff --git a/src/coreclr/vm/nativeimage.cpp b/src/coreclr/vm/nativeimage.cpp index 5f44ce17e0be38..35cbc188fd44be 100644 --- a/src/coreclr/vm/nativeimage.cpp +++ b/src/coreclr/vm/nativeimage.cpp @@ -146,7 +146,7 @@ namespace peLoadedImage = loaded; } - if (peLoadedImage.IsNull()) + if (peLoadedImage == NULL) { EX_TRY { @@ -193,7 +193,7 @@ namespace } EX_END_CATCH - if (peLoadedImage.IsNull()) + if (peLoadedImage == NULL) { // Failed to locate the native composite R2R image #ifdef LOGGING @@ -216,7 +216,7 @@ namespace return new ReadyToRunLoadedImage( (TADDR)peLoadedImage->GetBase(), peLoadedImage->GetVirtualSize(), - peLoadedImage.Extract(), + peLoadedImage.Detach(), [](void* img) { delete (PEImageLayout*)img; }); } } diff --git a/src/coreclr/vm/olecontexthelpers.cpp b/src/coreclr/vm/olecontexthelpers.cpp index aa6e84fada128b..cdeaa90bd26b35 100644 --- a/src/coreclr/vm/olecontexthelpers.cpp +++ b/src/coreclr/vm/olecontexthelpers.cpp @@ -115,7 +115,7 @@ HRESULT GetCurrentThreadTypeNT5(THDTYPE* pType) { GCX_PREEMP(); - ComHolderPreemp pThreadInfo; + ReleaseHolder pThreadInfo; hr = SafeQueryInterface(pObjCurrCtx, IID_IComThreadingInfo, (IUnknown **)&pThreadInfo); if(hr == S_OK) { @@ -146,7 +146,7 @@ HRESULT GetCurrentApartmentTypeNT5(IObjectContext *pObjCurrCtx, APTTYPE* pType) { GCX_PREEMP(); - ComHolderPreemp pThreadInfo; + ReleaseHolder pThreadInfo; hr = SafeQueryInterface(pObjCurrCtx, IID_IComThreadingInfo, (IUnknown **)&pThreadInfo); if(hr == S_OK) { diff --git a/src/coreclr/vm/olevariant.cpp b/src/coreclr/vm/olevariant.cpp index 90e847f3135c7b..4988fa93d3f52d 100644 --- a/src/coreclr/vm/olevariant.cpp +++ b/src/coreclr/vm/olevariant.cpp @@ -1671,8 +1671,8 @@ SAFEARRAY *OleVariant::CreateSafeArrayDescriptorForArrayRef(BASEARRAYREF *pArray { GCX_PREEMP(); - ComHolderPreemp pITI; - ComHolderPreemp pRecInfo; + ReleaseHolder pITI; + ReleaseHolder pRecInfo; IfFailThrow(GetITypeInfoForEEClass(pInterfaceMT, &pITI)); IfFailThrow(GetRecordInfoFromTypeInfo(pITI, &pRecInfo)); IfFailThrow(SafeArraySetRecordInfo(pSafeArray, pRecInfo)); @@ -2212,7 +2212,7 @@ void OleVariant::ConvertValueClassToVariant(OBJECTREF *pBoxedValueClass, VARIANT CONTRACTL_END; HRESULT hr = S_OK; - ComHolderAnyMode pTypeInfo; + ReleaseHolderAnyMode pTypeInfo; RecordVariantHolder pRecHolder(pOleVariant); // Initialize the OLE variant's VT_RECORD fields to NULL. diff --git a/src/coreclr/vm/peassembly.cpp b/src/coreclr/vm/peassembly.cpp index ec8ccbb3ee0de1..71596cec78fceb 100644 --- a/src/coreclr/vm/peassembly.cpp +++ b/src/coreclr/vm/peassembly.cpp @@ -856,7 +856,7 @@ PEAssembly *PEAssembly::Create(IMetaDataAssemblyEmit *pAssemblyEmit, AssemblyBin // Set up the metadata pointers in the PEAssembly. (This is the only identity // we have.) - ComHolderPreemp pEmit; + ReleaseHolder pEmit; pAssemblyEmit->QueryInterface(IID_IMetaDataEmit, (void **)&pEmit); RETURN new PEAssembly(NULL, pEmit, FALSE, pFallbackBinder); } diff --git a/src/coreclr/vm/peimage.cpp b/src/coreclr/vm/peimage.cpp index adbf7bfc7ed946..83463f11cd32dd 100644 --- a/src/coreclr/vm/peimage.cpp +++ b/src/coreclr/vm/peimage.cpp @@ -368,7 +368,7 @@ void PEImage::GetMVID(GUID *pMvid) if (pMeta == NULL) ThrowHR(COR_E_BADIMAGEFORMAT); - ComHolderAnyMode pMDImport; + ReleaseHolderAnyMode pMDImport; IfFailThrow(GetMDInternalInterface((void *) pMeta, cMeta, diff --git a/src/coreclr/vm/peimagelayout.cpp b/src/coreclr/vm/peimagelayout.cpp index 4147b00d74b273..2ce84104b1e404 100644 --- a/src/coreclr/vm/peimagelayout.cpp +++ b/src/coreclr/vm/peimagelayout.cpp @@ -119,7 +119,7 @@ PEImageLayout* PEImageLayout::LoadConverted(PEImage* pOwner, bool disableMapping } // we can use flat layout for this - return pFlat.Extract(); + return pFlat.Detach(); } PEImageLayout* PEImageLayout::Load(PEImage* pOwner, HRESULT* loadFailure) @@ -143,7 +143,7 @@ PEImageLayout* PEImageLayout::Load(PEImage* pOwner, HRESULT* loadFailure) PEImageLayoutHolder pAlloc(new LoadedImageLayout(pOwner, loadFailure)); if (pAlloc->GetBase() != NULL) - return pAlloc.Extract(); + return pAlloc.Detach(); #if TARGET_WINDOWS // For regular PE files always use OS loader on Windows. diff --git a/src/coreclr/vm/rejit.cpp b/src/coreclr/vm/rejit.cpp index dffc816938bbbc..295b7a3d8a6686 100644 --- a/src/coreclr/vm/rejit.cpp +++ b/src/coreclr/vm/rejit.cpp @@ -983,7 +983,7 @@ HRESULT ReJitManager::ConfigureILCodeVersion(ILCodeVersion ilCodeVersion) if (fNeedsParameters) { HRESULT hr = S_OK; - ReleaseHolder pFuncControl = NULL; + ReleaseHolder pFuncControl; if (ilCodeVersion.GetEnableReJITCallback()) { diff --git a/src/coreclr/vm/runtimecallablewrapper.cpp b/src/coreclr/vm/runtimecallablewrapper.cpp index dabbd8233e6514..61e9790dd1b29e 100644 --- a/src/coreclr/vm/runtimecallablewrapper.cpp +++ b/src/coreclr/vm/runtimecallablewrapper.cpp @@ -97,8 +97,8 @@ IUnknown *ComClassFactory::CreateInstanceFromClassFactory(IClassFactory *pClassF CONTRACT_END; HRESULT hr = S_OK; - ComHolderAnyMode pClassFact2; - ComHolderAnyMode pUnk; + ReleaseHolderAnyMode pClassFact2; + ReleaseHolderAnyMode pUnk; BSTRHolder bstrKey; // If the class doesn't support licensing or if it is missing a managed @@ -254,9 +254,9 @@ OBJECTREF ComClassFactory::CreateAggregatedInstance(MethodTable* pMTClass, BOOL _ASSERTE(pMT != NULL); #endif - ComHolderAnyMode pOuter; - ComHolderAnyMode pClassFact; - ComHolderAnyMode pUnk; + ReleaseHolderAnyMode pOuter; + ReleaseHolderAnyMode pClassFact; + ReleaseHolderAnyMode pUnk; HRESULT hr = S_OK; NewRCWHolder pNewRCW; @@ -268,7 +268,7 @@ OBJECTREF ComClassFactory::CreateAggregatedInstance(MethodTable* pMTClass, BOOL cref = (COMOBJECTREF)ComObject::CreateComObjectRef(pMTClass); //get wrapper for the object, this could enable GC - CCWHolder pComWrap = ComCallWrapper::InlineGetWrapper((OBJECTREF *)&cref); + CCWHolder pComWrap{ ComCallWrapper::InlineGetWrapper((OBJECTREF *)&cref) }; DebuggerExitFrame __def; @@ -281,9 +281,10 @@ OBJECTREF ComClassFactory::CreateAggregatedInstance(MethodTable* pMTClass, BOOL __def.Pop(); - // give up the extra addref that we did in our QI and suppress the auto-release. + // give up the extra addref that we did in our QI and relinquish the holder's + // auto-release; take a non-owning pointer for the remaining manual releases below. pComWrap->Release(); - pComWrap.SuppressRelease(); + ComCallWrapper* pComWrapRaw = pComWrap.Detach(); // Here's the scary part. If we are doing a managed 'new' of the aggregator, // then COM really isn't involved. We should not be counting for our caller @@ -293,7 +294,7 @@ OBJECTREF ComClassFactory::CreateAggregatedInstance(MethodTable* pMTClass, BOOL // Drive the instances count down to 0 -- and rely on the GCPROTECT to keep us // alive until we get back to our caller. if (ForManaged) - pComWrap->Release(); + pComWrapRaw->Release(); RCWCache* pCache = RCWCache::GetRCWCache(); @@ -378,7 +379,7 @@ IUnknown *ComClassFactory::CreateInstanceInternal(IUnknown *pOuter, BOOL *pfDidC } CONTRACT_END; - ComHolderAnyMode pClassFactory{ GetIClassFactory() }; + ReleaseHolderAnyMode pClassFactory{ GetIClassFactory() }; RETURN CreateInstanceFromClassFactory(pClassFactory, pOuter, pfDidContainment); } @@ -470,8 +471,8 @@ OBJECTREF ComClassFactory::CreateInstance(MethodTable* pMTClass, BOOL ForManaged GCPROTECT_BEGIN(coref) { { - ComHolderAnyMode pUnk; - ComHolderAnyMode pClassFact; + ReleaseHolderAnyMode pUnk; + ReleaseHolderAnyMode pClassFact; // Create the instance pUnk = CreateInstanceInternal(NULL, NULL); @@ -1247,7 +1248,7 @@ RCW* RCW::CreateRCWInternal(IUnknown *pUnk, DWORD dwSyncBlockIndex, DWORD flags, LogInteropAddRef(pUnk, cbRef, "RCWCache::CreateRCW: Addref pUnk because creating new RCW"); // Make sure we release AddRef-ed pUnk in case of exceptions - ComHolderPreemp pUnkHolder{ pUnk }; + ReleaseHolder pUnkHolder{ pUnk }; // Log the creation LogRCWCreate(pWrap, pUnk); @@ -1379,7 +1380,7 @@ RCW::MarshalingType RCW::GetMarshalingType(IUnknown* pUnk, MethodTable *pClassMT CONTRACTL_END; // Check whether the COM object can be marshaled. Hence we query for INoMarshal - ComHolderPreemp pNoMarshal; + ReleaseHolder pNoMarshal; HRESULT hr = SafeQueryInterfacePreemp(pUnk, IID_INoMarshal, (IUnknown**)&pNoMarshal); LogInteropQI(pUnk, IID_INoMarshal, hr, "RCW::GetMarshalingType: QI for INoMarshal"); @@ -1647,7 +1648,7 @@ void RCW::CreateDuplicateWrapper(MethodTable *pNewMT, RCWHolder* pNewRCW) COMOBJECTREF NewWrapperObj = (COMOBJECTREF)ComObject::CreateComObjectRef(pNewMT); GCPROTECT_BEGIN(NewWrapperObj) { - ComHolderAnyMode pAutoUnk; + ReleaseHolderAnyMode pAutoUnk; // Retrieve the RCWCache to use. RCWCache* pCache = RCWCache::GetRCWCache(); @@ -1706,7 +1707,7 @@ IUnknown* RCW::GetComIPFromRCW(REFIID iid) } CONTRACT_END; - ComHolderAnyMode pRet; + ReleaseHolderAnyMode pRet; HRESULT hr = S_OK; hr = SafeQueryInterfaceRemoteAware(iid, (IUnknown**)&pRet); @@ -1865,7 +1866,7 @@ HRESULT RCW::SafeQueryInterfaceRemoteAware(REFIID iid, IUnknown** ppResUnk) // GetIUnknown_NoAddRef() hands back a pointer we do not own; only the // GetIUnknown() fallback below returns a ref that must be released. IUnknown* pUnk = GetIUnknown_NoAddRef(); - ComHolderAnyMode pOwnedUnk; + ReleaseHolderAnyMode pOwnedUnk; if (pUnk == NULL) { // if we are not on the right thread we get a proxy which we need to keep AddRef'ed @@ -2017,7 +2018,7 @@ BOOL RCW::SupportsIProvideClassInfo() CONTRACTL_END; BOOL bSupportsIProvideClassInfo = FALSE; - ComHolderAnyMode pProvClassInfo; + ReleaseHolderAnyMode pProvClassInfo; // QI for IProvideClassInfo on the COM object. HRESULT hr = SafeQueryInterfaceRemoteAware(IID_IProvideClassInfo, &pProvClassInfo); @@ -2180,7 +2181,7 @@ BOOL ComObject::SupportsInterface(OBJECTREF oref, MethodTable* pIntfTable) } CONTRACTL_END - ComHolderAnyMode pUnk; + ReleaseHolderAnyMode pUnk; HRESULT hr; BOOL bSupportsItf = FALSE; @@ -2225,8 +2226,8 @@ BOOL ComObject::SupportsInterface(OBJECTREF oref, MethodTable* pIntfTable) MethodTable *pSrcItfClass = NULL; MethodTable *pEvProvClass = NULL; GUID SrcItfIID; - ComHolderAnyMode pCPC; - ComHolderAnyMode pCP; + ReleaseHolderAnyMode pCPC; + ReleaseHolderAnyMode pCP; // Retrieve the IID of the source interface associated with this // event interface. @@ -2326,7 +2327,7 @@ void ComObject::ThrowInvalidCastException(OBJECTREF *pObj, MethodTable *pCastToM } CONTRACT_END; - ComHolderAnyMode pItf; + ReleaseHolderAnyMode pItf; HRESULT hr = S_OK; IID *pNativeIID = NULL; GUID iid; @@ -2471,7 +2472,7 @@ IUnknown *ComObject::GetComIPFromRCW(OBJECTREF *pObj, MethodTable* pIntfTable) } CONTRACT_END; - ComHolderAnyMode pIUnk; + ReleaseHolderAnyMode pIUnk; RCWHolder pRCW(GetThread()); RCWPROTECT_BEGIN(pRCW, *pObj); diff --git a/src/coreclr/vm/stdinterfaces.cpp b/src/coreclr/vm/stdinterfaces.cpp index 419dde71b712f4..66e05882f5890e 100644 --- a/src/coreclr/vm/stdinterfaces.cpp +++ b/src/coreclr/vm/stdinterfaces.cpp @@ -102,7 +102,7 @@ Unknown_QueryInterface_Internal(ComCallWrapper* pWrap, IUnknown* pUnk, REFIID ri CONTRACTL_END; HRESULT hr = S_OK; - ComHolderPreemp pDestItf; + ReleaseHolder pDestItf; // Validate the arguments. if (!ppv) @@ -637,7 +637,7 @@ static bool TryDeferToMscorlib(MethodTable* pClass, ITypeInfo** ppTI) // code to .NET 8+. Try to load the .NET Framework's TLB to support this scenario. if (pClass == CoreLibBinder::GetClass(CLASS__GUID)) { - ComHolderPreemp pMscorlibTypeLib; + ReleaseHolder pMscorlibTypeLib; if (SUCCEEDED(::LoadRegTypeLib(s_MscorlibGuid, 2, 4, 0, &pMscorlibTypeLib))) { if (SUCCEEDED(pMscorlibTypeLib->GetTypeInfoOfGuid(s_GuidForSystemGuid, ppTI))) @@ -664,9 +664,9 @@ HRESULT GetITypeInfoForEEClass(MethodTable *pClass, ITypeInfo **ppTI, bool bClas ComMethodTable *pComMT = NULL; MethodTable* pOriginalClass = pClass; HRESULT hr = S_OK; - ComHolderAnyMode pITLB; - ComHolderAnyMode pTI; - ComHolderAnyMode pTIDef; // Default typeinfo of a coclass. + ReleaseHolderAnyMode pITLB; + ReleaseHolderAnyMode pTI; + ReleaseHolderAnyMode pTIDef; // Default typeinfo of a coclass. ComCallWrapperTemplate *pTemplate = NULL; GCX_PREEMP(); @@ -835,12 +835,12 @@ MethodTable* GetMethodTableForRecordInfo(IRecordInfo* recInfo) HRESULT hr; // Verify the associated TypeLib attribute - ComHolderPreemp typeInfo; + ReleaseHolder typeInfo; hr = recInfo->GetTypeInfo(&typeInfo); if (FAILED(hr)) return NULL; - ComHolderPreemp typeLib; + ReleaseHolder typeLib; UINT index; hr = typeInfo->GetContainingTypeLib(&typeLib, &index); if (FAILED(hr)) @@ -928,7 +928,7 @@ IErrorInfo *GetSupportedErrorInfo(IUnknown *iface, REFIID riid) { GCX_PREEMP(); HRESULT hr = S_OK; - ComHolderPreemp pErrorInfo; + ReleaseHolder pErrorInfo; // See if we have any error info. (Also this clears out the error info, // we want to do this whether it is a recent error or not.) @@ -941,7 +941,7 @@ IErrorInfo *GetSupportedErrorInfo(IUnknown *iface, REFIID riid) { // Make sure that the object we called follows the error info protocol, // otherwise the error may be stale, so we just throw it away. - ComHolderPreemp pSupport; + ReleaseHolder pSupport; hr = SafeQueryInterfacePreemp(iface, IID_ISupportErrorInfo, (IUnknown **) &pSupport); LogInteropQI(iface, IID_ISupportErrorInfo, hr, "ISupportErrorInfo"); if (SUCCEEDED(hr)) @@ -2085,7 +2085,7 @@ HRESULT GetSpecialMarshaler(IMarshal* pMarsh, SimpleComCallWrapper* pSimpleWrap, // In case of CoreCLR, we always use the standard marshaller. - ComHolderPreemp pMarshalerObj; + ReleaseHolder pMarshalerObj; IfFailRet(CoCreateFreeThreadedMarshaler(NULL, &pMarshalerObj)); return SafeQueryInterfacePreemp(pMarshalerObj, IID_IMarshal, (IUnknown**)ppMarshalRet); } @@ -2130,7 +2130,7 @@ HRESULT __stdcall Marshal_GetUnmarshalClass ( } } - ComHolderPreemp pMsh; + ReleaseHolder pMsh; hr = GetSpecialMarshaler(pMarsh, pSimpleWrap, dwDestContext, (IMarshal **)&pMsh); if (FAILED(hr)) return hr; @@ -2159,7 +2159,7 @@ HRESULT __stdcall Marshal_GetMarshalSizeMax ( SimpleComCallWrapper *pSimpleWrap = SimpleComCallWrapper::GetWrapperFromIP(pMarsh); - ComHolderPreemp pMsh; + ReleaseHolder pMsh; HRESULT hr = GetSpecialMarshaler(pMarsh, pSimpleWrap, dwDestContext, (IMarshal **)&pMsh); if (FAILED(hr)) return hr; @@ -2200,7 +2200,7 @@ HRESULT __stdcall Marshal_MarshalInterface ( } } - ComHolderPreemp pMsh; + ReleaseHolder pMsh; hr = GetSpecialMarshaler(pMarsh, pSimpleWrap, dwDestContext, (IMarshal **)&pMsh); if (FAILED(hr)) return hr; @@ -2360,7 +2360,7 @@ HRESULT __stdcall ObjectSafety_GetInterfaceSafetyOptions(IUnknown* pUnk, return E_POINTER; // Make sure the CLR object implements the requested interface. - ComHolderPreemp pItf; + ReleaseHolder pItf; HRESULT hr = SafeQueryInterfacePreemp(pUnk, riid, (IUnknown**)&pItf); LogInteropQI(pUnk, riid, hr, "QI to for riid in GetInterfaceSafetyOptions"); if (SUCCEEDED(hr)) @@ -2395,7 +2395,7 @@ HRESULT __stdcall ObjectSafety_SetInterfaceSafetyOptions(IUnknown* pUnk, CONTRACTL_END; // Make sure the CLR object implements the requested interface. - ComHolderPreemp pItf; + ReleaseHolder pItf; HRESULT hr = SafeQueryInterfacePreemp(pUnk, riid, (IUnknown**)&pItf); LogInteropQI(pUnk, riid, hr, "QI to for riid in SetInterfaceSafetyOptions"); if (FAILED(hr)) diff --git a/src/coreclr/vm/stubhelpers.cpp b/src/coreclr/vm/stubhelpers.cpp index cdfbe65fc8d648..4c7a022022793d 100644 --- a/src/coreclr/vm/stubhelpers.cpp +++ b/src/coreclr/vm/stubhelpers.cpp @@ -345,7 +345,7 @@ extern "C" IUnknown* QCALLTYPE StubHelpers_GetCOMIPFromRCWSlow(QCall::ObjectHand if (pIntf == NULL) { // Still not in the cache and we've ensured the OLE TLS data was created. - ComHolderAnyMode pRetUnk{ ComObject::GetComIPFromRCWThrowing(&objRef, pComInfo->m_pInterfaceMT) }; + ReleaseHolderAnyMode pRetUnk{ ComObject::GetComIPFromRCWThrowing(&objRef, pComInfo->m_pInterfaceMT) }; *ppTarget = GetCOMIPFromRCW_GetTarget(pRetUnk, pComInfo); _ASSERTE(*ppTarget != NULL); diff --git a/src/coreclr/vm/stubmgr.cpp b/src/coreclr/vm/stubmgr.cpp index 6533c427c4ef59..2716e8e518733f 100644 --- a/src/coreclr/vm/stubmgr.cpp +++ b/src/coreclr/vm/stubmgr.cpp @@ -1638,7 +1638,7 @@ static PCODE GetCOMTarget(Object *pThis, CLRToCOMCallInfo *pCLRToCOMCallInfo) CONTRACTL_END; // calculate the target interface pointer - ComHolderAnyMode pUnk; + ReleaseHolderAnyMode pUnk; OBJECTREF oref = ObjectToOBJECTREF(pThis); GCPROTECT_BEGIN(oref); @@ -1662,7 +1662,7 @@ static PCODE GetLateBoundCOMTarget(Object *pThis, CLRToCOMCallInfo *pCLRToCOMCal CONTRACTL_END; // calculate the target interface pointer - ComHolderAnyMode pUnk; + ReleaseHolderAnyMode pUnk; OBJECTREF oref = ObjectToOBJECTREF(pThis); GCPROTECT_BEGIN(oref); @@ -1675,7 +1675,7 @@ static PCODE GetLateBoundCOMTarget(Object *pThis, CLRToCOMCallInfo *pCLRToCOMCal // Make sure that our underlying RCW really has some IDispatch support. // We don't use this pointer as we don't want the "default" IDispatch interface. // We want the IDispatch pointer that corresponds to the actual interface we're calling on, which may not be the default IDispatch. - ComHolderAnyMode pDisp; + ReleaseHolderAnyMode pDisp; _ASSERTE(SUCCEEDED(((IUnknown *)pUnk)->QueryInterface(IID_IDispatch, (void**)&pDisp))); #endif diff --git a/src/coreclr/vm/weakreferencenative.cpp b/src/coreclr/vm/weakreferencenative.cpp index 3d6c5673972135..74dcf7c1ea71d5 100644 --- a/src/coreclr/vm/weakreferencenative.cpp +++ b/src/coreclr/vm/weakreferencenative.cpp @@ -56,11 +56,11 @@ extern "C" void QCALLTYPE ComWeakRefToObject(IWeakReference* pComWeakReference, // If the weak reference was in a state that it had an IWeakReference* for us to use, then we need to find the IUnknown // identity of the underlying COM object (assuming that object is still alive). - ComHolderPreemp pTargetIdentity; + ReleaseHolder pTargetIdentity; // Using the IWeakReference*, get ahold of the target native COM object's IInspectable*. If this resolve fails, then we // assume that the underlying native COM object is no longer alive, and thus we cannot create a new RCW for it. - ComHolderPreemp pTarget; + ReleaseHolder pTarget; if (SUCCEEDED(pComWeakReference->Resolve(IID_IInspectable, &pTarget))) { if (pTarget != nullptr) @@ -117,10 +117,10 @@ extern "C" IWeakReference * QCALLTYPE ObjectToComWeakRef(QCall::ObjectHandleOnSt GCPROTECT_END(); } - ComHolderPreemp pWeakReferenceSource{ pWeakReferenceSourceRaw }; + ReleaseHolder pWeakReferenceSource{ pWeakReferenceSourceRaw }; if (pWeakReferenceSource != nullptr) { - ComHolderPreemp weakReferenceHolder; + ReleaseHolder weakReferenceHolder; if (!FAILED(pWeakReferenceSource->GetWeakReference(&weakReferenceHolder))) { pWeakReference = weakReferenceHolder.Detach(); diff --git a/src/coreclr/vm/wrappers.h b/src/coreclr/vm/wrappers.h index 7d90ebd8067a1b..12d805c507b7c1 100644 --- a/src/coreclr/vm/wrappers.h +++ b/src/coreclr/vm/wrappers.h @@ -53,14 +53,14 @@ class MDEnumHolder }; template -struct ComHolderAnyModeTraits final +struct ReleaseHolderAnyModeTraits final { static_assert( - std::is_base_of::value, - "TYPE must derive from IUnknown"); + HolderDetail::HasReleaseMethod::value, + "TYPE must have a Release() member"); using Type = TYPE*; - static constexpr Type Default() { return nullptr; } + static constexpr Type Default() { return NULL; } static void Free(Type value) { CONTRACTL @@ -74,38 +74,11 @@ struct ComHolderAnyModeTraits final } }; -// Releases the held COM interface regardless of the current GC mode, switching -// to preemptive internally when required. Use ComHolderPreemp instead when +// Releases the held type with a Release() method regardless of the current GC mode, +// switching to preemptive internally when required. Use ReleaseHolder instead when // the release will always occur in preemptive mode. template -using ComHolderAnyMode = LifetimeHolder>; - -template -struct ComHolderPreempTraits final -{ - static_assert( - std::is_base_of::value, - "TYPE must derive from IUnknown"); - - using Type = TYPE*; - static constexpr Type Default() { return nullptr; } - static void Free(Type value) - { - CONTRACTL - { - NOTHROW; - GC_TRIGGERS; - MODE_PREEMPTIVE; - } CONTRACTL_END; - - SafeReleasePreemp(value); - } -}; - -// Use this holder if you're already in preemptive mode for other reasons, -// use ComHolderAnyMode otherwise. -template -using ComHolderPreemp = LifetimeHolder>; +using ReleaseHolderAnyMode = LifetimeHolder>; //-------------------------------------------------------------------------------- // safe variant helper From bc0d6fd43e0fa55ef5e6a96a4a786b664143deeb Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 8 Jul 2026 13:37:14 -0700 Subject: [PATCH 2/7] Break clrhost.h include cycle out of check.inl holder.h now includes contract.h (for the MODE_PREEMPTIVE CONTRACTL in ReleaseHolderTraits::Free), which pulled check.inl -> clrhost.h -> holder.h back into itself before Holder/LifetimeHolder were defined. Remove the clrhost.h include from check.inl to break the cycle, and make the two headers that were relying on that transitive path self-sufficient: - inc/sigbuilder.h: include corhdr.h for CorElementType/mdToken. - jit/jitstd/algorithm.h: include for std::swap used by jitstd::sort. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/inc/check.inl | 1 - src/coreclr/inc/holder.h | 7 ++++--- src/coreclr/inc/sigbuilder.h | 1 + src/coreclr/jit/jitstd/algorithm.h | 1 + src/coreclr/vm/appdomain.cpp | 2 +- 5 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/coreclr/inc/check.inl b/src/coreclr/inc/check.inl index ad2952d0cb8930..425cfcf2fda46a 100644 --- a/src/coreclr/inc/check.inl +++ b/src/coreclr/inc/check.inl @@ -5,7 +5,6 @@ #define CHECK_INL_ #include "check.h" -#include "clrhost.h" #include "debugmacros.h" #include "clrtypes.h" diff --git a/src/coreclr/inc/holder.h b/src/coreclr/inc/holder.h index 286d9ae6451372..f639af37246b0e 100644 --- a/src/coreclr/inc/holder.h +++ b/src/coreclr/inc/holder.h @@ -6,7 +6,7 @@ #define __HOLDER_H_ #include "cor.h" -#include "staticcontract.h" +#include "contract.h" #include "volatile.h" #include "palclr.h" #include @@ -1023,7 +1023,8 @@ struct ReleaseHolderTraits final #else STATIC_CONTRACT_NOTHROW; STATIC_CONTRACT_MODE_PREEMPTIVE; -#endif +#endif // ENABLE_CONTRACTS_IMPL + if (value != NULL) value->Release(); } @@ -1392,7 +1393,7 @@ namespace clr { STATIC_CONTRACT_LIMITED_METHOD; //@TODO: Would be good to add runtime validation that the return value is used. - return SafeAddRef((ItfT*)pItf); + return SafeAddRef(static_cast(pItf)); } namespace detail diff --git a/src/coreclr/inc/sigbuilder.h b/src/coreclr/inc/sigbuilder.h index 0097503d4c383f..47b66d5c9fcb7b 100644 --- a/src/coreclr/inc/sigbuilder.h +++ b/src/coreclr/inc/sigbuilder.h @@ -6,6 +6,7 @@ #define _SIGBUILDER_H_ #include "contract.h" +#include "corhdr.h" // // Simple signature builder diff --git a/src/coreclr/jit/jitstd/algorithm.h b/src/coreclr/jit/jitstd/algorithm.h index 3c1fc260068290..f75868812c85f3 100644 --- a/src/coreclr/jit/jitstd/algorithm.h +++ b/src/coreclr/jit/jitstd/algorithm.h @@ -4,6 +4,7 @@ #pragma once #include +#include namespace jitstd { diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index 12956082ebc49d..8c20ab446b2d16 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -3302,7 +3302,7 @@ PEAssembly * AppDomain::BindAssemblySpec( // Now, if it's a cacheable bind we need to re-fetch the result from the cache, as we may have been racing with another // thread to store our result. Note that we may throw from here, if there is a cached exception. // Note the non-cached result holder above may be released (if any). - // Returned cached files are not AddRef'd, so we call AddRef inorder to retain it. + // Returned cached files are not AddRef'd, so we call AddRef in order to retain it. PEAssemblyHolder result{ FindCachedFile(pSpec) }; if (result != NULL) result->AddRef(); From d0eff525f203d13de89d462283aec9d357427e63 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Fri, 10 Jul 2026 10:45:54 -0700 Subject: [PATCH 3/7] Enhance contract handling and IP validation across coreclr modules - Added DBI_COMPILE definition for conditional compilation in clrdefinitions.cmake. - Updated Debugger::GetAndSendTransitionStubInfo to use g_pEEInterface for IP validation. - Refined contract enabling conditions in contract.h to include DBI builds. - Modified ReleaseHolderTraits to use ENABLE_EE_CONTRACTS for contract checks. - Removed unused IsIPInModule function from utilcode.h and implemented it in ExecutionManager. - Updated various files to consistently use ExecutionManager::IsIPInModule for IP validation. --- src/coreclr/clrdefinitions.cmake | 1 + src/coreclr/debug/ee/debugger.cpp | 2 +- src/coreclr/inc/contract.h | 21 +++--- src/coreclr/inc/holder.h | 4 +- src/coreclr/inc/utilcode.h | 2 - src/coreclr/utilcode/util.cpp | 93 -------------------------- src/coreclr/vm/ceeload.cpp | 2 +- src/coreclr/vm/codeman.cpp | 96 ++++++++++++++++++++++++++- src/coreclr/vm/codeman.h | 2 + src/coreclr/vm/eecontract.h | 4 ++ src/coreclr/vm/eedbginterface.h | 2 + src/coreclr/vm/eedbginterfaceimpl.cpp | 6 ++ src/coreclr/vm/eedbginterfaceimpl.h | 2 + src/coreclr/vm/excep.cpp | 4 +- src/coreclr/vm/exceptionhandling.cpp | 2 +- 15 files changed, 127 insertions(+), 116 deletions(-) diff --git a/src/coreclr/clrdefinitions.cmake b/src/coreclr/clrdefinitions.cmake index ca058333d045aa..909fee419c820c 100644 --- a/src/coreclr/clrdefinitions.cmake +++ b/src/coreclr/clrdefinitions.cmake @@ -1,6 +1,7 @@ include(${CMAKE_CURRENT_LIST_DIR}/clrfeatures.cmake) add_compile_definitions($<$>:DACCESS_COMPILE>) +add_compile_definitions($<$>:DBI_COMPILE>) if (CLR_CMAKE_TARGET_UNIX) diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index de30ac0c73b973..7b8c613347adc3 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -11779,7 +11779,7 @@ void Debugger::GetAndSendTransitionStubInfo(CORDB_ADDRESS_TYPE *stubAddress) // If its not a stub, then maybe its an address in mscoree? if (result == false) { - result = (IsIPInModule(GetClrModuleBase(), (PCODE)stubAddress) == TRUE); + result = (g_pEEInterface->IsIPInModule(GetClrModuleBase(), (PCODE)stubAddress) == TRUE); } // This is a synchronous event (reply required) diff --git a/src/coreclr/inc/contract.h b/src/coreclr/inc/contract.h index 383c8323ba0d30..197225ba91eee9 100644 --- a/src/coreclr/inc/contract.h +++ b/src/coreclr/inc/contract.h @@ -4,9 +4,6 @@ // Contract.h // -// ! I am the owner for issues in the contract *infrastructure*, not for every -// ! CONTRACT_VIOLATION dialog that comes up. If you interrupt my work for a routine -// ! CONTRACT_VIOLATION, you will become the new owner of this file. //-------------------------------------------------------------------------------- // CONTRACTS - User Reference // @@ -215,17 +212,17 @@ #endif -// We only enable contracts in _DEBUG builds +// We only enable contract data in _DEBUG builds and non-JIT builds. #if defined(_DEBUG) && !defined(DISABLE_CONTRACTS) && !defined(JIT_BUILD) #define ENABLE_CONTRACTS_DATA #endif -// Also, we won't enable contracts if this is a DAC build. -#if defined(ENABLE_CONTRACTS_DATA) && !defined(DACCESS_COMPILE) && !defined(CROSS_COMPILE) +// Also, we won't enable contracts if a DAC or DBI build, or a cross build. +#if defined(ENABLE_CONTRACTS_DATA) && !defined(DACCESS_COMPILE) && !defined(DBI_COMPILE) && !defined(CROSS_COMPILE) #define ENABLE_CONTRACTS #endif -// Finally, only define the implementation parts of contracts if this isn't a DAC build. +// Finally, only define the implementation parts of contracts if this isn't for an external access (that is, _DEBUG_IMPL). #if defined(_DEBUG_IMPL) && defined(ENABLE_CONTRACTS) #define ENABLE_CONTRACTS_IMPL #endif @@ -234,6 +231,8 @@ #include "clrtypes.h" #include "check.h" #include "staticcontract.h" +#include "volatile.h" +#include "cor.h" #ifdef ENABLE_CONTRACTS_DATA @@ -655,7 +654,7 @@ struct ClrDebugState UINT GetCombinedLockCount(); }; -#endif // ENABLE_CONTRACTS +#endif // ENABLE_CONTRACTS_DATA #ifdef ENABLE_CONTRACTS_IMPL // Create ClrDebugState. @@ -681,8 +680,7 @@ void CONTRACT_ASSERT(const char *szElaboration, const char *szFile, int lineNum ); - -#endif +#endif // ENABLE_CONTRACTS_IMPL // This needs to be defined up here b/c it is used by ASSERT_CHECK which is used by the contract impl #ifdef _DEBUG @@ -775,7 +773,7 @@ class AutoCleanupDebugOnlyCodeHolder : public DebugOnlyCodeHolder #define END_DEBUG_ONLY_CODE #define ENTER_DEBUG_ONLY_CODE #define LEAVE_DEBUG_ONLY_CODE -#endif +#endif // ENABLE_CONTRACTS_IMPL #else // _DEBUG #define DEBUG_ONLY_REGION() @@ -2092,6 +2090,5 @@ extern Volatile g_DbgSuppressAllocationAsserts; STATIC_CONTRACT_MODE_PREEMPTIVE; #define AFTER_CONTRACTS -#include "volatile.h" #endif // CONTRACT_H_ diff --git a/src/coreclr/inc/holder.h b/src/coreclr/inc/holder.h index f639af37246b0e..3027ad6198912e 100644 --- a/src/coreclr/inc/holder.h +++ b/src/coreclr/inc/holder.h @@ -1013,7 +1013,7 @@ struct ReleaseHolderTraits final static constexpr Type Default() { return NULL; } static void Free(Type value) { -#ifdef ENABLE_CONTRACTS_IMPL +#ifdef ENABLE_EE_CONTRACTS CONTRACTL { NOTHROW; @@ -1023,7 +1023,7 @@ struct ReleaseHolderTraits final #else STATIC_CONTRACT_NOTHROW; STATIC_CONTRACT_MODE_PREEMPTIVE; -#endif // ENABLE_CONTRACTS_IMPL +#endif // ENABLE_EE_CONTRACTS if (value != NULL) value->Release(); diff --git a/src/coreclr/inc/utilcode.h b/src/coreclr/inc/utilcode.h index 0b802a91898cbe..20be20ad3981ef 100644 --- a/src/coreclr/inc/utilcode.h +++ b/src/coreclr/inc/utilcode.h @@ -3449,8 +3449,6 @@ namespace util INDEBUG(BOOL DbgIsExecutable(LPVOID lpMem, SIZE_T length);) -BOOL IsIPInModule(PTR_VOID pModuleBaseAddress, PCODE ip); - namespace UtilCode { // These are type-safe versions of Interlocked[Compare]Exchange diff --git a/src/coreclr/utilcode/util.cpp b/src/coreclr/utilcode/util.cpp index d419adf784c6db..e17f23c1deec94 100644 --- a/src/coreclr/utilcode/util.cpp +++ b/src/coreclr/utilcode/util.cpp @@ -2324,99 +2324,6 @@ void PutRiscV64AuipcCombo(UINT32 * pCode, INT64 offset, bool isStype) _ASSERTE(GetRiscV64AuipcCombo(pCode, isStype) == offset); } -//====================================================================== -// This function returns true, if it can determine that the instruction pointer -// refers to a code address that belongs in the range of the given image. -BOOL IsIPInModule(PTR_VOID pModuleBaseAddress, PCODE ip) -{ - STATIC_CONTRACT_LEAF; - SUPPORTS_DAC; - - struct Param - { - PTR_VOID pModuleBaseAddress; - PCODE ip; - BOOL fRet; - } param; - param.pModuleBaseAddress = pModuleBaseAddress; - param.ip = ip; - param.fRet = FALSE; - -// UNIXTODO: implement a proper version for PAL -#ifdef HOST_WINDOWS - PAL_TRY(Param *, pParam, ¶m) - { - PTR_BYTE pBase = dac_cast(pParam->pModuleBaseAddress); - - PTR_IMAGE_DOS_HEADER pDOS = NULL; - PTR_IMAGE_NT_HEADERS pNT = NULL; - USHORT cbOptHdr; - PCODE baseAddr; - - // - // First, must validate the format of the PE headers to make sure that - // the fields we're interested in using exist in the image. - // - - // Validate the DOS header. - pDOS = PTR_IMAGE_DOS_HEADER(pBase); - if (pDOS->e_magic != VAL16(IMAGE_DOS_SIGNATURE) || - pDOS->e_lfanew == 0) - { - goto lDone; - } - - // Validate the NT header - pNT = PTR_IMAGE_NT_HEADERS(pBase + VAL32(pDOS->e_lfanew)); - - if (pNT->Signature != VAL32(IMAGE_NT_SIGNATURE)) - { - goto lDone; - } - - // Validate that the optional header is large enough to contain the fields - // we're interested, namely IMAGE_OPTIONAL_HEADER::SizeOfImage. The reason - // we don't just check that SizeOfOptionalHeader == IMAGE_SIZEOF_NT_OPTIONAL_HEADER - // is due to VSW443590, which states that the extensibility of this structure - // is such that it is possible to include only a portion of the optional header. - cbOptHdr = pNT->FileHeader.SizeOfOptionalHeader; - - // Check that the magic field is contained by the optional header and set to the correct value. - if (cbOptHdr < (offsetof(IMAGE_OPTIONAL_HEADER, Magic) + sizeofmember(IMAGE_OPTIONAL_HEADER, Magic)) || - pNT->OptionalHeader.Magic != VAL16(IMAGE_NT_OPTIONAL_HDR_MAGIC)) - { - goto lDone; - } - - // Check that the SizeOfImage is contained by the optional header. - if (cbOptHdr < (offsetof(IMAGE_OPTIONAL_HEADER, SizeOfImage) + sizeofmember(IMAGE_OPTIONAL_HEADER, SizeOfImage))) - { - goto lDone; - } - - // - // The real check - // - - baseAddr = dac_cast(pBase); - if ((pParam->ip < baseAddr) || (pParam->ip >= (baseAddr + VAL32(pNT->OptionalHeader.SizeOfImage)))) - { - goto lDone; - } - - pParam->fRet = TRUE; - -lDone: ; - } - PAL_EXCEPT (EXCEPTION_EXECUTE_HANDLER) - { - } - PAL_ENDTRY -#endif // HOST_WINDOWS - - return param.fRet; -} - namespace Clr { namespace Util diff --git a/src/coreclr/vm/ceeload.cpp b/src/coreclr/vm/ceeload.cpp index c279eb4c7861d8..9e44b05917c2c3 100644 --- a/src/coreclr/vm/ceeload.cpp +++ b/src/coreclr/vm/ceeload.cpp @@ -1824,7 +1824,7 @@ ISymUnmanagedReader *Module::GetISymUnmanagedReader(void) // trying to get a symbol reader. This has to be done once per // Assembly. ReleaseHolder pUnk; - hr = GetReadablePublicMetaDataInterface(ofReadOnly, IID_IMetaDataImport, &pUnk); + hr = GetReadablePublicMetaDataInterface(ofReadOnly, IID_IMetaDataImport, (LPVOID*)&pUnk); if (SUCCEEDED(hr)) hr = pBinder->GetReaderForFile(pUnk, path, NULL, &pReader); } diff --git a/src/coreclr/vm/codeman.cpp b/src/coreclr/vm/codeman.cpp index d3a640139b1a2e..cbeeae459755c3 100644 --- a/src/coreclr/vm/codeman.cpp +++ b/src/coreclr/vm/codeman.cpp @@ -5506,6 +5506,98 @@ ExecutionManager::FindCodeRangeWithLock(PCODE currentPC) return result; } +//====================================================================== +// This function returns true, if it can determine that the instruction pointer +// refers to a code address that belongs in the range of the given image. +BOOL ExecutionManager::IsIPInModule(PTR_VOID pModuleBaseAddress, PCODE ip) +{ + STATIC_CONTRACT_LEAF; + SUPPORTS_DAC; + + struct Param + { + PTR_VOID pModuleBaseAddress; + PCODE ip; + BOOL fRet; + } param; + param.pModuleBaseAddress = pModuleBaseAddress; + param.ip = ip; + param.fRet = FALSE; + +// UNIXTODO: implement a proper version for PAL +#ifdef HOST_WINDOWS + PAL_TRY(Param *, pParam, ¶m) + { + PTR_BYTE pBase = dac_cast(pParam->pModuleBaseAddress); + + PTR_IMAGE_DOS_HEADER pDOS = NULL; + PTR_IMAGE_NT_HEADERS pNT = NULL; + USHORT cbOptHdr; + PCODE baseAddr; + + // + // First, must validate the format of the PE headers to make sure that + // the fields we're interested in using exist in the image. + // + + // Validate the DOS header. + pDOS = PTR_IMAGE_DOS_HEADER(pBase); + if (pDOS->e_magic != VAL16(IMAGE_DOS_SIGNATURE) || + pDOS->e_lfanew == 0) + { + goto lDone; + } + + // Validate the NT header + pNT = PTR_IMAGE_NT_HEADERS(pBase + VAL32(pDOS->e_lfanew)); + + if (pNT->Signature != VAL32(IMAGE_NT_SIGNATURE)) + { + goto lDone; + } + + // Validate that the optional header is large enough to contain the fields + // we're interested, namely IMAGE_OPTIONAL_HEADER::SizeOfImage. The reason + // we don't just check that SizeOfOptionalHeader == IMAGE_SIZEOF_NT_OPTIONAL_HEADER + // is due to VSW443590, which states that the extensibility of this structure + // is such that it is possible to include only a portion of the optional header. + cbOptHdr = pNT->FileHeader.SizeOfOptionalHeader; + + // Check that the magic field is contained by the optional header and set to the correct value. + if (cbOptHdr < (offsetof(IMAGE_OPTIONAL_HEADER, Magic) + sizeofmember(IMAGE_OPTIONAL_HEADER, Magic)) || + pNT->OptionalHeader.Magic != VAL16(IMAGE_NT_OPTIONAL_HDR_MAGIC)) + { + goto lDone; + } + + // Check that the SizeOfImage is contained by the optional header. + if (cbOptHdr < (offsetof(IMAGE_OPTIONAL_HEADER, SizeOfImage) + sizeofmember(IMAGE_OPTIONAL_HEADER, SizeOfImage))) + { + goto lDone; + } + + // + // The real check + // + + baseAddr = dac_cast(pBase); + if ((pParam->ip < baseAddr) || (pParam->ip >= (baseAddr + VAL32(pNT->OptionalHeader.SizeOfImage)))) + { + goto lDone; + } + + pParam->fRet = TRUE; + +lDone: ; + } + PAL_EXCEPT (EXCEPTION_EXECUTE_HANDLER) + { + } + PAL_ENDTRY +#endif // HOST_WINDOWS + + return param.fRet; +} //************************************************************************** PCODE ExecutionManager::GetCodeStartAddress(PCODE currentPC) @@ -5950,7 +6042,7 @@ TADDR ExecutionManager::AddVirtualIPRange(UINT32 numVirtualIPs, { endVIP = (TADDR)InterlockedAdd64((LONGLONG*)&s_nextVirtualIP, numVirtualIPs); } - + TADDR startVIP = endVIP - numVirtualIPs; // Check for overflow @@ -5965,7 +6057,7 @@ TADDR ExecutionManager::AddVirtualIPRange(UINT32 numVirtualIPs, pJit, RangeSection::RANGE_SECTION_VIRTUALIP, pModule); - + VirtualIPRangeSection* pOldRangeSection = nullptr; do { diff --git a/src/coreclr/vm/codeman.h b/src/coreclr/vm/codeman.h index 371174c434a9a8..5e9d9ca6698b8b 100644 --- a/src/coreclr/vm/codeman.h +++ b/src/coreclr/vm/codeman.h @@ -2418,6 +2418,8 @@ class ExecutionManager // Returns true if currentPC is ready to run codegen static BOOL IsReadyToRunCode(PCODE currentPC); + static BOOL IsIPInModule(PTR_VOID pModuleBaseAddress, PCODE ip); + // Returns method's start address for a given PC static PCODE GetCodeStartAddress(PCODE currentPC); diff --git a/src/coreclr/vm/eecontract.h b/src/coreclr/vm/eecontract.h index 290100775f7a8f..7fdcf88cc76174 100644 --- a/src/coreclr/vm/eecontract.h +++ b/src/coreclr/vm/eecontract.h @@ -26,6 +26,10 @@ #ifdef ENABLE_CONTRACTS_IMPL +// Indicates that the EE contract vocabulary (GC_TRIGGERS and MODE_COOPERATIVE/PREEMPTIVE/ANY) +// is available and enforced. +#define ENABLE_EE_CONTRACTS + class EEContract : public BaseContract { private: diff --git a/src/coreclr/vm/eedbginterface.h b/src/coreclr/vm/eedbginterface.h index f972e89e645488..478709e3facc5e 100644 --- a/src/coreclr/vm/eedbginterface.h +++ b/src/coreclr/vm/eedbginterface.h @@ -129,6 +129,8 @@ class EEDebugInterface #endif // #ifndef DACCESS_COMPILE + virtual BOOL IsIPInModule(PTR_VOID pModuleBaseAddress, PCODE ip) = 0; + virtual PCODE GetNativeCodeStartAddress(PCODE address) = 0; virtual MethodDesc *GetNativeCodeMethodDesc(const PCODE address) = 0; diff --git a/src/coreclr/vm/eedbginterfaceimpl.cpp b/src/coreclr/vm/eedbginterfaceimpl.cpp index 0a461dd28dd917..b611ac0debdef2 100644 --- a/src/coreclr/vm/eedbginterfaceimpl.cpp +++ b/src/coreclr/vm/eedbginterfaceimpl.cpp @@ -391,6 +391,12 @@ BOOL EEDbgInterfaceImpl::IsManagedNativeCode(const BYTE *address) return ExecutionManager::IsManagedCode((PCODE)address); } +BOOL EEDbgInterfaceImpl::IsIPInModule(PTR_VOID pModuleBaseAddress, PCODE ip) +{ + WRAPPER_NO_CONTRACT; + return ExecutionManager::IsIPInModule(pModuleBaseAddress, ip); +} + PCODE EEDbgInterfaceImpl::GetNativeCodeStartAddress(PCODE address) { WRAPPER_NO_CONTRACT; diff --git a/src/coreclr/vm/eedbginterfaceimpl.h b/src/coreclr/vm/eedbginterfaceimpl.h index acc1b2692b8d1f..70183ced67d53c 100644 --- a/src/coreclr/vm/eedbginterfaceimpl.h +++ b/src/coreclr/vm/eedbginterfaceimpl.h @@ -117,6 +117,8 @@ class EEDbgInterfaceImpl : public EEDebugInterface BOOL IsManagedNativeCode(const BYTE *address); + BOOL IsIPInModule(PTR_VOID pModuleBaseAddress, PCODE ip) DAC_UNEXPECTED(); + PCODE GetNativeCodeStartAddress(PCODE address) DAC_UNEXPECTED(); MethodDesc *GetNativeCodeMethodDesc(const PCODE address) DAC_UNEXPECTED(); diff --git a/src/coreclr/vm/excep.cpp b/src/coreclr/vm/excep.cpp index 1758528113b164..3c30ba18144c33 100644 --- a/src/coreclr/vm/excep.cpp +++ b/src/coreclr/vm/excep.cpp @@ -5948,7 +5948,7 @@ VEH_ACTION WINAPI CLRVectoredExceptionHandlerPhase2(PEXCEPTION_POINTERS pExcepti CONTRACT_VIOLATION(TakesLockViolation); fExternalException = (!ExecutionManager::IsManagedCode(GetIP(pExceptionInfo->ContextRecord)) && - !IsIPInModule(GetClrModuleBase(), GetIP(pExceptionInfo->ContextRecord))); + !ExecutionManager::IsIPInModule(GetClrModuleBase(), GetIP(pExceptionInfo->ContextRecord))); } if (fExternalException) @@ -6108,7 +6108,7 @@ VEH_ACTION WINAPI CLRVectoredExceptionHandlerPhase3(PEXCEPTION_POINTERS pExcepti if ((!fAVisOk) && !(pExceptionRecord->ExceptionFlags & EXCEPTION_UNWINDING)) { PCODE ip = (PCODE)GetIP(pContext); - if (IsIPInModule(GetClrModuleBase(), ip) || IsIPInModule(GCHeapUtilities::GetGCModuleBase(), ip)) + if (ExecutionManager::IsIPInModule(GetClrModuleBase(), ip) || ExecutionManager::IsIPInModule(GCHeapUtilities::GetGCModuleBase(), ip)) { CONTRACT_VIOLATION(ThrowsViolation|FaultViolation); diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index dd6cfe57b315a1..e9755973862d8d 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -2929,7 +2929,7 @@ StackFrame ExInfo::FindParentStackFrameHelper(CrawlFrame* pCF, #else // !DACCESS_COMPILE PTR_VOID eeBase = GetClrModuleBase(); #endif // !DACCESS_COMPILE - fIsCallerInVM = IsIPInModule(eeBase, callerIP); + fIsCallerInVM = ExecutionManager::IsIPInModule(eeBase, callerIP); #endif // TARGET_UNIX if (!fIsCallerInVM) From 9b81753d62d09f62e5a1fb014db44109909ad45b Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Fri, 10 Jul 2026 14:38:30 -0700 Subject: [PATCH 4/7] Fix null holder deref from Extract->Detach conversion in assembly binder ReleaseHolder's Detach() clears the holder's value (unlike the old Extract(), which retained it via SuppressRelease). BindAssemblyByProbingPaths detached pAssembly into *ppAssembly and then dereferenced pAssembly for the ref-def check, hitting the operator-> null assert (holder.h:974) on the TPA bind path at startup, crashing every coreclr test process. Use a local for the detached pointer and read from it. Also sequence the image reads before Detach() in nativeimage.cpp OpenR2RFromPE, where the member accesses and Detach() were unsequenced call arguments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6457cb5c-e732-41ad-8cc0-af22e64c1f49 --- src/coreclr/binder/assemblybindercommon.cpp | 5 +++-- src/coreclr/vm/nativeimage.cpp | 8 ++++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/coreclr/binder/assemblybindercommon.cpp b/src/coreclr/binder/assemblybindercommon.cpp index ca38b249d122e8..41818150b7690a 100644 --- a/src/coreclr/binder/assemblybindercommon.cpp +++ b/src/coreclr/binder/assemblybindercommon.cpp @@ -790,7 +790,8 @@ namespace BINDER_SPACE } // Set any found assembly. It is up to the caller to check the returned HRESULT for errors due to validation - *ppAssembly = pAssembly.Detach(); + Assembly* pFoundAssembly = pAssembly.Detach(); + *ppAssembly = pFoundAssembly; if (FAILED(hr)) return hr; @@ -800,7 +801,7 @@ namespace BINDER_SPACE // we fail the bind. // Compare requested AssemblyName with that from the candidate assembly - if (!TestCandidateRefMatchesDef(pRequestedAssemblyName, pAssembly->GetAssemblyName(), false /*tpaListAssembly*/)) + if (!TestCandidateRefMatchesDef(pRequestedAssemblyName, pFoundAssembly->GetAssemblyName(), false /*tpaListAssembly*/)) return FUSION_E_REF_DEF_MISMATCH; return S_OK; diff --git a/src/coreclr/vm/nativeimage.cpp b/src/coreclr/vm/nativeimage.cpp index 35cbc188fd44be..69fb043310fc89 100644 --- a/src/coreclr/vm/nativeimage.cpp +++ b/src/coreclr/vm/nativeimage.cpp @@ -213,9 +213,13 @@ namespace COMPlusThrowHR(COR_E_BADIMAGEFORMAT); } + // Read the image details before detaching the layout from the holder; Detach() + // clears the holder, so the member accesses must be sequenced before it. + TADDR imageBase = (TADDR)peLoadedImage->GetBase(); + uint32_t imageSize = peLoadedImage->GetVirtualSize(); return new ReadyToRunLoadedImage( - (TADDR)peLoadedImage->GetBase(), - peLoadedImage->GetVirtualSize(), + imageBase, + imageSize, peLoadedImage.Detach(), [](void* img) { delete (PEImageLayout*)img; }); } From 171c2e59c09f7adadb57c449a3281f71726902b6 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Mon, 13 Jul 2026 08:30:40 -0700 Subject: [PATCH 5/7] Apply suggestions from code review Co-authored-by: Aaron R Robinson --- src/coreclr/vm/ceeload.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/ceeload.cpp b/src/coreclr/vm/ceeload.cpp index 36df526b3e197f..614d073731e268 100644 --- a/src/coreclr/vm/ceeload.cpp +++ b/src/coreclr/vm/ceeload.cpp @@ -1816,7 +1816,7 @@ ISymUnmanagedReader *Module::GetISymUnmanagedReader(void) // (RW) metadata interface: the reader only needs it to satisfy the // binder, and producing the real importer would force this module's // metadata to its locked RW backing store. - ReleaseHolder pNoopImport = GetNoopMetaDataImport2(); + ReleaseHolder pNoopImport{ GetNoopMetaDataImport2() }; hr = pBinder->GetReaderFromStream(pNoopImport, pIStream, &pReader); } } @@ -1835,7 +1835,7 @@ ISymUnmanagedReader *Module::GetISymUnmanagedReader(void) if (SUCCEEDED(hr)) { - m_pISymUnmanagedReader = pReader.Extract(); + m_pISymUnmanagedReader = pReader.Detach(); LOG((LF_CORDB, LL_INFO10, "M::GISUR: Loaded symbols for module %s\n", GetDebugName())); } else From 08a3b7abfe045f0f9a4071a6001540e1c30e3a14 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Tue, 14 Jul 2026 15:59:10 -0700 Subject: [PATCH 6/7] Review feedback. --- src/coreclr/inc/contract.h | 14 +++++--------- src/coreclr/vm/nativeimage.cpp | 14 ++++++-------- 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/src/coreclr/inc/contract.h b/src/coreclr/inc/contract.h index 197225ba91eee9..9710e1da671646 100644 --- a/src/coreclr/inc/contract.h +++ b/src/coreclr/inc/contract.h @@ -201,9 +201,6 @@ // //-------------------------------------------------------------------------------- - - - #ifndef CONTRACT_H_ #define CONTRACT_H_ @@ -211,14 +208,15 @@ #pragma warning(disable:4189) //local variable is initialized but not referenced #endif - -// We only enable contract data in _DEBUG builds and non-JIT builds. +// Contract data types are limited to debug builds. The JIT doesn't link +// against any portions of the contract data system so it must be disabled. #if defined(_DEBUG) && !defined(DISABLE_CONTRACTS) && !defined(JIT_BUILD) #define ENABLE_CONTRACTS_DATA #endif -// Also, we won't enable contracts if a DAC or DBI build, or a cross build. -#if defined(ENABLE_CONTRACTS_DATA) && !defined(DACCESS_COMPILE) && !defined(DBI_COMPILE) && !defined(CROSS_COMPILE) +// The DAC and DBI builds reference contract data, but not implementation +// so we disable contracts for those builds. +#if defined(ENABLE_CONTRACTS_DATA) && !defined(DACCESS_COMPILE) && !defined(DBI_COMPILE) #define ENABLE_CONTRACTS #endif @@ -2089,6 +2087,4 @@ extern Volatile g_DbgSuppressAllocationAsserts; STATIC_CONTRACT_GC_TRIGGERS; \ STATIC_CONTRACT_MODE_PREEMPTIVE; -#define AFTER_CONTRACTS - #endif // CONTRACT_H_ diff --git a/src/coreclr/vm/nativeimage.cpp b/src/coreclr/vm/nativeimage.cpp index 69fb043310fc89..934588d8e11676 100644 --- a/src/coreclr/vm/nativeimage.cpp +++ b/src/coreclr/vm/nativeimage.cpp @@ -213,15 +213,13 @@ namespace COMPlusThrowHR(COR_E_BADIMAGEFORMAT); } - // Read the image details before detaching the layout from the holder; Detach() - // clears the holder, so the member accesses must be sequenced before it. - TADDR imageBase = (TADDR)peLoadedImage->GetBase(); - uint32_t imageSize = peLoadedImage->GetVirtualSize(); - return new ReadyToRunLoadedImage( - imageBase, - imageSize, - peLoadedImage.Detach(), + ReadyToRunLoadedImage* r2rImg = new ReadyToRunLoadedImage( + (TADDR)peLoadedImage->GetBase(), + peLoadedImage->GetVirtualSize(), + peLoadedImage, [](void* img) { delete (PEImageLayout*)img; }); + peLoadedImage.Detach(); + return r2rImg; } } From 5b5b1331b8e84dff9d94eabcc0801b68f792d6fa Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 15 Jul 2026 13:46:56 -0700 Subject: [PATCH 7/7] Refine contract data type definitions for debug builds --- src/coreclr/inc/contract.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/coreclr/inc/contract.h b/src/coreclr/inc/contract.h index 9710e1da671646..93a0446773909b 100644 --- a/src/coreclr/inc/contract.h +++ b/src/coreclr/inc/contract.h @@ -208,9 +208,7 @@ #pragma warning(disable:4189) //local variable is initialized but not referenced #endif -// Contract data types are limited to debug builds. The JIT doesn't link -// against any portions of the contract data system so it must be disabled. -#if defined(_DEBUG) && !defined(DISABLE_CONTRACTS) && !defined(JIT_BUILD) +#if defined(_DEBUG) && !defined(DISABLE_CONTRACTS) #define ENABLE_CONTRACTS_DATA #endif @@ -220,7 +218,6 @@ #define ENABLE_CONTRACTS #endif -// Finally, only define the implementation parts of contracts if this isn't for an external access (that is, _DEBUG_IMPL). #if defined(_DEBUG_IMPL) && defined(ENABLE_CONTRACTS) #define ENABLE_CONTRACTS_IMPL #endif