Use LifetimeHolder for COM interop holders

src/coreclr/debug/ee/debugger.cpp

@@ -1050,7 +1050,7 @@ void Debugger::InitDebugEventCounting()
     memset(&g_iDbgDebuggerCounter, 0, DBG_DEBUGGER_MAX*sizeof(int));
 
     // retrieve the possible counter for break point
-    CLRConfigStringHolder wstrValue = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_DebuggerBreakPoint);
+    CLRConfigStringHolder wstrValue(CLRConfig::GetConfigValue(CLRConfig::INTERNAL_DebuggerBreakPoint));
     // The string value is of the following format
     // <Event Name>=Count;<Event Name>=Count;....;
     // The string must end with ;

src/coreclr/inc/clrconfig.h

@@ -150,6 +150,13 @@ inline CLRConfig::LookupOptions operator&(CLRConfig::LookupOptions lhs, CLRConfi
     return static_cast<CLRConfig::LookupOptions>(static_cast<DWORD>(lhs) & static_cast<DWORD>(rhs));
 }
 
-typedef Wrapper<LPWSTR, DoNothing, CLRConfig::FreeConfigString, 0> CLRConfigStringHolder;
+struct CLRConfigStringTraits final
+{
+    using Type = LPWSTR;
+    static constexpr Type Default() { return NULL; }
+    static void Free(Type value) { CLRConfig::FreeConfigString(value); }
+};
+
+using CLRConfigStringHolder = LifetimeHolder<CLRConfigStringTraits>;
 
 #endif //__CLRConfig_h__

src/coreclr/inc/clrhost.h

@@ -89,8 +89,19 @@ DWORD ClrSleepEx(DWORD dwMilliseconds, BOOL bAlertable);
 typedef Holder<CRITSEC_COOKIE, ClrEnterCriticalSection, ClrLeaveCriticalSection, 0> CRITSEC_Holder;
 
 // Use this holder to manage CRITSEC_COOKIE allocation to ensure it will be released if anything goes wrong
-FORCEINLINE void VoidClrDeleteCriticalSection(CRITSEC_COOKIE cs) { if (cs != NULL) ClrDeleteCriticalSection(cs); }
-typedef Wrapper<CRITSEC_COOKIE, DoNothing<CRITSEC_COOKIE>, VoidClrDeleteCriticalSection, 0> CRITSEC_AllocationHolder;
+struct CRITSECCookieAllocationTraits final
+{
+    using Type = CRITSEC_COOKIE;
+    static constexpr Type Default() { return NULL; }
+    static void Free(Type cs)
+    {
+        STATIC_CONTRACT_WRAPPER;
+        if (cs != NULL)
+            ClrDeleteCriticalSection(cs);
+    }
+};
+
+using CRITSEC_AllocationHolder = LifetimeHolder<CRITSECCookieAllocationTraits>;
 
 #ifndef DACCESS_COMPILE
 // Suspend/resume APIs that fail-fast on errors

src/coreclr/inc/holder.h

@@ -618,54 +618,8 @@ FORCEINLINE void DoNothing()
 {
 }
 
-// Prefast stuff.We should have DoNothing<type*> in the holder declaration, but currently
-// prefast doesnt support, it, so im stuffing all these here so if we need to change the template you can change
-// everything here. When prefast works, remove the following functions
-struct ConnectionCookie;
-FORCEINLINE void ConnectionCookieDoNothing(ConnectionCookie* p)
-{
-}
-
-class ComCallWrapper;
-FORCEINLINE void CCWHolderDoNothing(ComCallWrapper* p)
-{
-}
-
-
-FORCEINLINE void DispParamHolderDoNothing(VARIANT* p)
-{
-}
-
-FORCEINLINE void VariantPtrDoNothing(VARIANT* p)
-{
-}
-
-FORCEINLINE void VariantDoNothing(VARIANT)
-{
-}
-
-FORCEINLINE void ZeroDoNothing(VOID* p)
-{
-}
-
-class CtxEntry;
-FORCEINLINE void CtxEntryDoNothing(CtxEntry* p)
-{
-}
-
-struct RCW;
-FORCEINLINE void NewRCWHolderDoNothing(RCW*)
-{
-}
-
-// Prefast stuff.We should have DoNothing<SafeArray*> in the holder declaration
-FORCEINLINE void SafeArrayDoNothing(SAFEARRAY* p)
-{
-}
-
-
 //-----------------------------------------------------------------------------
-// Holder/Wrapper are the simplest way to define holders - they synthesizes a base class out of
+// Holder/Wrapper are the simplest way to define holders - they synthesize a base class from
 // function pointers
 //-----------------------------------------------------------------------------
 
@@ -1145,7 +1099,24 @@ class LifetimeHolder final
     }
 
     operator Type() const { STATIC_CONTRACT_LEAF; return m_value; }
-    Type* operator&() { STATIC_CONTRACT_LEAF; _ASSERTE(m_value == T::Default()); return &m_value; }
+
+    Type* operator&()
+    {
+        STATIC_CONTRACT_LEAF;
+#ifdef _DEBUG
+        Type tmp = T::Default();
+        _ASSERTE(memcmp(&m_value, &tmp, sizeof(Type)) == 0 && "Taking address of non-empty holder");
+#endif // _DEBUG
+        return &m_value;
+    }
+
+    template <typename U = Type, typename = std::enable_if_t<std::is_pointer<U>::value>>
+    U operator->() const
+    {
+        STATIC_CONTRACT_LEAF;
+        _ASSERTE(m_value != NULL);
+        return m_value;
+    }
 
     void Free()
     {

src/coreclr/nativeaot/Runtime/forward_declarations.h

@@ -21,12 +21,10 @@ FWD_DECL(EEThreadId)
 FWD_DECL(MethodInfo)
 FWD_DECL(Module)
 FWD_DECL(Object)
-FWD_DECL(OBJECTHANDLEHolder)
 FWD_DECL(PageEntry)
 FWD_DECL(PAL_EnterHolder)
 FWD_DECL(PAL_LeaveHolder)
 FWD_DECL(SpinLock)
-FWD_DECL(RCOBJECTHANDLEHolder)
 FWD_DECL(RuntimeInstance)
 FWD_DECL(StackFrameIterator)
 FWD_DECL(SyncClean)

src/coreclr/vm/appdomain.cpp

@@ -2137,12 +2137,6 @@ FileLoadLock::FileLoadLock(PEFileListLock* pLock, PEAssembly* pPEAssembly)
     pPEAssembly->AddRef();
 }
 
-void FileLoadLock::HolderLeave(FileLoadLock *pThis)
-{
-    LIMITED_METHOD_CONTRACT;
-    pThis->Leave();
-}
-
 
 //
 // Assembly loading:
@@ -2649,7 +2643,7 @@ void AppDomain::TryIncrementalLoad(FileLoadLevel workLevel, FileLoadLockHolder&
     // This is factored out so we don't call EX_TRY in a loop (EX_TRY can _alloca)
 
     BOOL released = FALSE;
-    FileLoadLock* pLoadLock = lockHolder.GetValue();
+    FileLoadLock* pLoadLock = lockHolder;
     Assembly* pAssembly = pLoadLock->GetAssembly();
 
     EX_TRY
@@ -2690,7 +2684,7 @@ void AppDomain::TryIncrementalLoad(FileLoadLevel workLevel, FileLoadLockHolder&
         if (pLoadLock->CompleteLoadLevel(workLevel, success) &&
             pLoadLock->GetLoadLevel()==FILE_LOAD_DELIVER_EVENTS)
         {
-            lockHolder.Release();
+            lockHolder.Free();
             released = TRUE;
             pAssembly->DeliverAsyncEvents();
         };

src/coreclr/vm/appdomain.hpp

@@ -333,10 +333,20 @@ class FileLoadLock : public ListLockEntry
 
     FileLoadLock(PEFileListLock* pLock, PEAssembly* pPEAssembly);
 
-    static void HolderLeave(FileLoadLock *pThis);
-
 public:
-    typedef Wrapper<FileLoadLock *, DoNothing, FileLoadLock::HolderLeave> Holder;
+    struct HolderTraits final
+    {
+        using Type = FileLoadLock*;
+        static constexpr Type Default() { return NULL; }
+        static void Free(Type pThis)
+        {
+            LIMITED_METHOD_CONTRACT;
+            if (pThis != NULL)
+                pThis->Leave();
+        }
+    };
+
+    using Holder = LifetimeHolder<HolderTraits>;
 
 };
 

src/coreclr/vm/assemblynative.cpp

@@ -196,13 +196,13 @@ extern "C" void QCALLTYPE AssemblyNative_LoadFromPath(INT_PTR ptrNativeAssemblyB
 
         // Need to verify that this is a valid CLR assembly.
         if (!pILImage->CheckILFormat())
-            THROW_BAD_FORMAT(BFA_BAD_IL, pILImage.GetValue());
+            THROW_BAD_FORMAT(BFA_BAD_IL, static_cast<PEImage*>(pILImage));
 
         LoaderAllocator* pLoaderAllocator = pBinder->GetLoaderAllocator();
         if (pLoaderAllocator && pLoaderAllocator->IsCollectible() && !pILImage->IsILOnly())
         {
             // Loading IJW assemblies into a collectible AssemblyLoadContext is not allowed
-            THROW_BAD_FORMAT(BFA_IJW_IN_COLLECTIBLE_ALC, pILImage.GetValue());
+            THROW_BAD_FORMAT(BFA_IJW_IN_COLLECTIBLE_ALC, static_cast<PEImage*>(pILImage));
         }
     }
 

src/coreclr/vm/assemblyspec.cpp

@@ -431,7 +431,7 @@ Assembly *AssemblySpec::LoadAssembly(LPCWSTR pFilePath)
 
     // Need to verify that this is a valid CLR assembly.
     if (!pILImage->CheckILFormat())
-        THROW_BAD_FORMAT(BFA_BAD_IL, pILImage.GetValue());
+        THROW_BAD_FORMAT(BFA_BAD_IL, static_cast<PEImage*>(pILImage));
 
     RETURN AssemblyNative::LoadFromPEImage(AppDomain::GetCurrentDomain()->GetDefaultBinder(), pILImage, true /* excludeAppPaths */);
 }

src/coreclr/vm/ceeload.cpp

@@ -623,7 +623,7 @@ Module *Module::Create(Assembly *pAssembly, PEAssembly *pPEAssembly, AllocMemTra
     ModuleHolder pModuleSafe(pModule);
     pModuleSafe->DoInit(pamTracker, NULL);
 
-    RETURN pModuleSafe.Extract();
+    RETURN pModuleSafe.Detach();
 }
 
 void Module::ApplyMetaData()
@@ -3824,7 +3824,7 @@ ReflectionModule *ReflectionModule::Create(Assembly *pAssembly, PEAssembly *pPEA
     pModule->DoInit(pamTracker, szName);
     pModule->SetIsRuntimeWrapExceptionsCached_ForReflectionEmitModules();
 
-    RETURN pModule.Extract();
+    RETURN pModule.Detach();
 }
 
 

src/coreclr/vm/ceeload.h

@@ -1790,27 +1790,37 @@ class ReflectionModule : public Module
     void CaptureModuleMetaDataToMemory();
 };
 
-// Module holders
-FORCEINLINE void VoidModuleDestruct(Module *pModule)
+struct ModuleHolderTraits final
 {
+    using Type = Module*;
+    static constexpr Type Default() { return NULL; }
+    static void Free(Type pModule)
+    {
+        STATIC_CONTRACT_WRAPPER;
 #ifndef DACCESS_COMPILE
-    if (g_fEEStarted)
-        pModule->Destruct();
+        if (g_fEEStarted && pModule != NULL)
+            pModule->Destruct();
 #endif
-}
-
-typedef Wrapper<Module*, DoNothing, VoidModuleDestruct, 0> ModuleHolder;
-
+    }
+};
 
+using ModuleHolder = LifetimeHolder<ModuleHolderTraits>;
 
-FORCEINLINE void VoidReflectionModuleDestruct(ReflectionModule *pModule)
+struct ReflectionModuleHolderTraits final
 {
+    using Type = ReflectionModule*;
+    static constexpr Type Default() { return NULL; }
+    static void Free(Type pModule)
+    {
+        STATIC_CONTRACT_WRAPPER;
 #ifndef DACCESS_COMPILE
-    pModule->Destruct();
+        if (pModule != NULL)
+            pModule->Destruct();
 #endif
-}
+    }
+};
 
-typedef Wrapper<ReflectionModule*, DoNothing, VoidReflectionModuleDestruct, 0> ReflectionModuleHolder;
+using ReflectionModuleHolder = LifetimeHolder<ReflectionModuleHolderTraits>;
 
 
 

src/coreclr/vm/ceemain.cpp

@@ -709,7 +709,7 @@ void EEStartupHelper()
             unsigned level = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_LogLevel, LL_INFO1000);
             unsigned bytesPerThread = CLRConfig::GetConfigValue(CLRConfig::UNSUPPORTED_StressLogSize, STRESSLOG_CHUNK_SIZE * 4);
             unsigned totalBytes = CLRConfig::GetConfigValue(CLRConfig::UNSUPPORTED_TotalStressLogSize, STRESSLOG_CHUNK_SIZE * 1024);
-            CLRConfigStringHolder logFilename = CLRConfig::GetConfigValue(CLRConfig::UNSUPPORTED_StressLogFilename);
+            CLRConfigStringHolder logFilename(CLRConfig::GetConfigValue(CLRConfig::UNSUPPORTED_StressLogFilename));
             StressLog::Initialize(facilities, level, bytesPerThread, totalBytes, GetClrModuleBase(), logFilename);
             g_pStressLog = &StressLog::theLog;
         }

src/coreclr/vm/comcallablewrapper.cpp

@@ -338,44 +338,30 @@ extern "C" PCODE ComPreStubWorker(UMEntryThunkData* pEntryThunk)
     return pStub;
 }
 
-FORCEINLINE void CPListRelease(CQuickArray<ConnectionPoint*>* value)
-{
-    WRAPPER_NO_CONTRACT;
-
-    if (value)
-    {
-        // Delete all the connection points.
-        for (UINT i = 0; i < value->Size(); i++)
-            delete (*value)[i];
-
-        // Delete the list itself.
-        delete value;
-    }
-}
-
 typedef CQuickArray<ConnectionPoint*> CPArray;
 
-FORCEINLINE void CPListDoNothing(CPArray*)
+struct CPListHolderTraits final
 {
-    LIMITED_METHOD_CONTRACT;
-}
-
-class CPListHolder : public Wrapper<CPArray*, CPListDoNothing, CPListRelease, 0>
-{
-public:
-    CPListHolder(CPArray* p = NULL)
-        : Wrapper<CPArray*, CPListDoNothing, CPListRelease, 0>(p)
+    using Type = CPArray*;
+    static constexpr Type Default() { return NULL; }
+    static void Free(Type value)
     {
         WRAPPER_NO_CONTRACT;
-    }
 
-    FORCEINLINE void operator=(CPArray* p)
-    {
-        WRAPPER_NO_CONTRACT;
-        Wrapper<CPArray*, CPListDoNothing, CPListRelease, 0>::operator=(p);
+        if (value != NULL)
+        {
+            // Delete all the connection points.
+            for (UINT i = 0; i < value->Size(); i++)
+                delete (*value)[i];
+
+            // Delete the list itself.
+            delete value;
+        }
     }
 };
 
+using CPListHolder = LifetimeHolder<CPListHolderTraits>;
+
 NOINLINE void LogCCWRefCountChange_BREAKPOINT(ComCallWrapper *pCCW)
 {
     LIMITED_METHOD_CONTRACT;
@@ -816,7 +802,7 @@ void SimpleComCallWrapper::SetUpCPListHelper(MethodTable **apSrcItfMTs, int cSrc
     }
     CONTRACTL_END;
 
-    CPListHolder pCPList = NULL;
+    CPListHolder pCPList;
     ComCallWrapper *pWrap = GetMainWrapper();
     int NumCPs = 0;
 
@@ -844,8 +830,8 @@ void SimpleComCallWrapper::SetUpCPListHelper(MethodTable **apSrcItfMTs, int cSrc
     // Finally, we set the connection point list in the simple wrapper. If
     // no other thread already set it, we set pCPList to NULL to indicate
     // that ownership has been transferred to the simple wrapper.
-    if (InterlockedCompareExchangeT(&m_pCPList, pCPList.GetValue(), NULL) == NULL)
-        pCPList.SuppressRelease();
+    if (InterlockedCompareExchangeT(&m_pCPList, static_cast<CPArray*>(pCPList), NULL) == NULL)
+        pCPList.Detach();
 }
 
 ConnectionPoint *SimpleComCallWrapper::TryCreateConnectionPoint(ComCallWrapper *pWrap, MethodTable *pEventMT)