From d7e85d93c12aaa43937f8ef5f920c36b5afc756d Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Wed, 18 Dec 2024 07:21:59 -0800 Subject: [PATCH] Move ComWrappers AddRef to C/C++ (#110762) * Move ComWrappers AddRef to C/C++ Xaml invokes AddRef while holding a lock that it *also* holds while a GC is in progress. Managed AddRef had to synchronize with the GC that caused intermittent deadlocks with the other thread holding Xaml's lock. This change reverts the managed AddRef implementation to match .NET Native and CoreCLR. Fixes #110747 Co-authored-by: Aaron Robinson --- .../nativeaot/Runtime/HandleTableHelpers.cpp | 54 +++++++++++++++++++ .../nativeaot/Runtime/unix/PalRedhawkInline.h | 7 +++ .../Runtime/windows/PalRedhawkInline.h | 18 +++++++ .../InteropServices/ComWrappers.NativeAot.cs | 12 +---- .../src/System/Runtime/RuntimeImports.cs | 4 ++ 5 files changed, 85 insertions(+), 10 deletions(-) diff --git a/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp b/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp index 23e985357d343a..8eddc1d3440d26 100644 --- a/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp +++ b/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp @@ -70,3 +70,57 @@ FCIMPL2(void, RhUnregisterRefCountedHandleCallback, void * pCallout, MethodTable RestrictedCallouts::UnregisterRefCountedHandleCallback(pCallout, pTypeFilter); } FCIMPLEND + +// This structure mirrors the managed type System.Runtime.InteropServices.ComWrappers.ManagedObjectWrapper. +struct ManagedObjectWrapper +{ + intptr_t HolderHandle; + uint64_t RefCount; + + int32_t UserDefinedCount; + void* /* ComInterfaceEntry */ UserDefined; + void* /* InternalComInterfaceDispatch* */ Dispatches; + + int32_t /* CreateComInterfaceFlagsEx */ Flags; + + uint32_t AddRef() + { + return GetComCount((uint64_t)PalInterlockedIncrement64((int64_t*)&RefCount)); + } + + static const uint64_t ComRefCountMask = 0x000000007fffffffUL; + + static uint32_t GetComCount(uint64_t c) + { + return (uint32_t)(c & ComRefCountMask); + } +}; + +// This structure mirrors the managed type System.Runtime.InteropServices.ComWrappers.InternalComInterfaceDispatch. +struct InternalComInterfaceDispatch +{ + void* Vtable; + ManagedObjectWrapper* _thisPtr; +}; + +static ManagedObjectWrapper* ToManagedObjectWrapper(void* dispatchPtr) +{ + return ((InternalComInterfaceDispatch*)dispatchPtr)->_thisPtr; +} + +// +// AddRef is implemented in native code so that it does not need to synchronize with the GC. This is important because Xaml +// invokes AddRef while holding a lock that it *also* holds while a GC is in progress. If AddRef was managed, we would have +// to synchronize with the GC before entering AddRef, which would deadlock with the other thread holding Xaml's lock. +// +static uint32_t __stdcall IUnknown_AddRef(void* pComThis) +{ + ManagedObjectWrapper* wrapper = ToManagedObjectWrapper(pComThis); + return wrapper->AddRef(); +} + +FCIMPL0(void*, RhGetIUnknownAddRef) +{ + return (void*)&IUnknown_AddRef; +} +FCIMPLEND diff --git a/src/coreclr/nativeaot/Runtime/unix/PalRedhawkInline.h b/src/coreclr/nativeaot/Runtime/unix/PalRedhawkInline.h index 983f17a36aba0a..2380bacdf02a1b 100644 --- a/src/coreclr/nativeaot/Runtime/unix/PalRedhawkInline.h +++ b/src/coreclr/nativeaot/Runtime/unix/PalRedhawkInline.h @@ -30,6 +30,13 @@ FORCEINLINE int32_t PalInterlockedIncrement(_Inout_ int32_t volatile *pDst) return result; } +FORCEINLINE int64_t PalInterlockedIncrement64(_Inout_ int64_t volatile *pDst) +{ + int64_t result = __sync_add_and_fetch(pDst, 1); + PalInterlockedOperationBarrier(); + return result; +} + FORCEINLINE int32_t PalInterlockedDecrement(_Inout_ int32_t volatile *pDst) { int32_t result = __sync_sub_and_fetch(pDst, 1); diff --git a/src/coreclr/nativeaot/Runtime/windows/PalRedhawkInline.h b/src/coreclr/nativeaot/Runtime/windows/PalRedhawkInline.h index 1f2a74dcd15100..595c7f663b9d13 100644 --- a/src/coreclr/nativeaot/Runtime/windows/PalRedhawkInline.h +++ b/src/coreclr/nativeaot/Runtime/windows/PalRedhawkInline.h @@ -67,6 +67,17 @@ FORCEINLINE int64_t PalInterlockedExchange64(_Inout_ int64_t volatile *pDst, int iOldValue) != iOldValue); return iOldValue; } + +FORCEINLINE int64_t PalInterlockedIncrement64(_Inout_ int64_t volatile *Addend) +{ + int64_t Old; + do { + Old = *Addend; + } while (PalInterlockedCompareExchange64(Addend, + Old + 1, + Old) != Old); + return Old + 1; +} #else // HOST_X86 EXTERN_C int64_t _InterlockedExchange64(int64_t volatile *, int64_t); #pragma intrinsic(_InterlockedExchange64) @@ -74,6 +85,13 @@ FORCEINLINE int64_t PalInterlockedExchange64(_Inout_ int64_t volatile *pDst, int { return _InterlockedExchange64(pDst, iValue); } + +EXTERN_C int64_t _InterlockedIncrement64(int64_t volatile *); +#pragma intrinsic(_InterlockedIncrement64) +FORCEINLINE int64_t PalInterlockedIncrement64(_Inout_ int64_t volatile *pDst) +{ + return _InterlockedIncrement64(pDst); +} #endif // HOST_X86 #if defined(HOST_AMD64) || defined(HOST_ARM64) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs index b4d70b66c2a937..75a49c362ff2a0 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs @@ -1157,7 +1157,7 @@ public static void RegisterForMarshalling(ComWrappers instance) public static unsafe void GetIUnknownImpl(out IntPtr fpQueryInterface, out IntPtr fpAddRef, out IntPtr fpRelease) { fpQueryInterface = (IntPtr)(delegate* unmanaged)&ComWrappers.IUnknown_QueryInterface; - fpAddRef = (IntPtr)(delegate* unmanaged)&ComWrappers.IUnknown_AddRef; + fpAddRef = RuntimeImports.RhGetIUnknownAddRef(); // Implemented in C/C++ to avoid GC transitions fpRelease = (IntPtr)(delegate* unmanaged)&ComWrappers.IUnknown_Release; } @@ -1302,13 +1302,6 @@ internal static unsafe int IUnknown_QueryInterface(IntPtr pThis, Guid* guid, Int return wrapper->QueryInterface(in *guid, out *ppObject); } - [UnmanagedCallersOnly] - internal static unsafe uint IUnknown_AddRef(IntPtr pThis) - { - ManagedObjectWrapper* wrapper = ComInterfaceDispatch.ToManagedObjectWrapper((ComInterfaceDispatch*)pThis); - return wrapper->AddRef(); - } - [UnmanagedCallersOnly] internal static unsafe uint IUnknown_Release(IntPtr pThis) { @@ -1381,8 +1374,7 @@ private static unsafe IntPtr CreateDefaultIReferenceTrackerTargetVftbl() { IntPtr* vftbl = (IntPtr*)RuntimeHelpers.AllocateTypeAssociatedMemory(typeof(ComWrappers), 7 * sizeof(IntPtr)); vftbl[0] = (IntPtr)(delegate* unmanaged)&ComWrappers.IReferenceTrackerTarget_QueryInterface; - vftbl[1] = (IntPtr)(delegate* unmanaged)&ComWrappers.IUnknown_AddRef; - vftbl[2] = (IntPtr)(delegate* unmanaged)&ComWrappers.IUnknown_Release; + GetIUnknownImpl(out _, out vftbl[1], out vftbl[2]); vftbl[3] = (IntPtr)(delegate* unmanaged)&ComWrappers.IReferenceTrackerTarget_AddRefFromReferenceTracker; vftbl[4] = (IntPtr)(delegate* unmanaged)&ComWrappers.IReferenceTrackerTarget_ReleaseFromReferenceTracker; vftbl[5] = (IntPtr)(delegate* unmanaged)&ComWrappers.IReferenceTrackerTarget_Peg; diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs index 454c5a1e0c4be0..29de35fe4c3083 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs @@ -503,6 +503,10 @@ internal enum GcRestrictedCalloutKind [RuntimeImport(RuntimeLibrary, "RhUnregisterRefCountedHandleCallback")] internal static extern unsafe void RhUnregisterRefCountedHandleCallback(IntPtr pCalloutMethod, MethodTable* pTypeFilter); + [MethodImplAttribute(MethodImplOptions.InternalCall)] + [RuntimeImport(RuntimeLibrary, "RhGetIUnknownAddRef")] + internal static extern IntPtr RhGetIUnknownAddRef(); + #if FEATURE_OBJCMARSHAL [MethodImplAttribute(MethodImplOptions.InternalCall)] [RuntimeImport(RuntimeLibrary, "RhRegisterObjectiveCMarshalBeginEndCallback")]