From ac5311b825681024314ed96eaafeab13fe85c508 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Mon, 16 Dec 2024 16:33:51 -0800 Subject: [PATCH 1/8] 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 --- .../nativeaot/Runtime/HandleTableHelpers.cpp | 54 +++++++++++++++++++ .../nativeaot/Runtime/unix/PalRedhawkInline.h | 7 +++ .../Runtime/windows/PalRedhawkInline.h | 7 +++ .../InteropServices/ComWrappers.NativeAot.cs | 7 +-- .../src/System/Runtime/RuntimeImports.cs | 4 ++ 5 files changed, 76 insertions(+), 3 deletions(-) diff --git a/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp b/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp index 23e985357d343a..935da58996d70b 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.ComInterfaceDispatch! +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 can be invoked during a 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 deadlocks 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 &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..70679e02ced49d 100644 --- a/src/coreclr/nativeaot/Runtime/windows/PalRedhawkInline.h +++ b/src/coreclr/nativeaot/Runtime/windows/PalRedhawkInline.h @@ -14,6 +14,13 @@ FORCEINLINE int32_t PalInterlockedIncrement(_Inout_ int32_t volatile *pDst) return _InterlockedIncrement((long volatile *)pDst); } +EXTERN_C int64_t __cdecl _InterlockedIncrement64(int64_t volatile *); +#pragma intrinsic(_InterlockedIncrement64) +FORCEINLINE int64_t PalInterlockedIncrement64(_Inout_ int64_t volatile *pDst) +{ + return _InterlockedIncrement64(pDst); +} + EXTERN_C long __cdecl _InterlockedDecrement(long volatile *); #pragma intrinsic(_InterlockedDecrement) FORCEINLINE int32_t PalInterlockedDecrement(_Inout_ int32_t volatile *pDst) 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..1c883148a95a51 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(); fpRelease = (IntPtr)(delegate* unmanaged)&ComWrappers.IUnknown_Release; } @@ -1302,12 +1302,14 @@ internal static unsafe int IUnknown_QueryInterface(IntPtr pThis, Guid* guid, Int return wrapper->QueryInterface(in *guid, out *ppObject); } +#if false // Implemented in C/C++ to avoid GC transitions [UnmanagedCallersOnly] internal static unsafe uint IUnknown_AddRef(IntPtr pThis) { ManagedObjectWrapper* wrapper = ComInterfaceDispatch.ToManagedObjectWrapper((ComInterfaceDispatch*)pThis); return wrapper->AddRef(); } +#endif [UnmanagedCallersOnly] internal static unsafe uint IUnknown_Release(IntPtr pThis) @@ -1381,8 +1383,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")] From 73e93b3c1d9008ca1781c3336faf1b603c7e6d34 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Mon, 16 Dec 2024 16:58:22 -0800 Subject: [PATCH 2/8] Apply suggestions from code review Co-authored-by: Aaron Robinson --- src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp b/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp index 935da58996d70b..6f0f6d28581c84 100644 --- a/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp +++ b/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp @@ -71,7 +71,7 @@ FCIMPL2(void, RhUnregisterRefCountedHandleCallback, void * pCallout, MethodTable } FCIMPLEND -// This structure mirrors the managed type System.Runtime.InteropServices.ComWrappers.ComInterfaceDispatch! +// This structure mirrors the managed type System.Runtime.InteropServices.ComWrappers.ManagedObjectWrapper. struct ManagedObjectWrapper { intptr_t HolderHandle; @@ -96,7 +96,7 @@ struct ManagedObjectWrapper } }; -// This structure mirrors the managed type System.Runtime.InteropServices.ComWrappers.InternalComInterfaceDispatch! +// This structure mirrors the managed type System.Runtime.InteropServices.ComWrappers.InternalComInterfaceDispatch. struct InternalComInterfaceDispatch { void* Vtable; From 0bb54a44f3c253bfd602386a15da93e26d3aac5f Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Mon, 16 Dec 2024 16:59:01 -0800 Subject: [PATCH 3/8] Update src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp --- src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp b/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp index 6f0f6d28581c84..2a18006c9638d7 100644 --- a/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp +++ b/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp @@ -111,7 +111,7 @@ static ManagedObjectWrapper* ToManagedObjectWrapper(void* dispatchPtr) // // AddRef is implemented in native code so that it can be invoked during a 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 deadlocks with the other thread holding Xaml's lock. +// 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) { From d21241bdeb5ac14dd9b58389a8152f1a35977dc7 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Mon, 16 Dec 2024 18:05:09 -0800 Subject: [PATCH 4/8] Update src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp --- src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp b/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp index 2a18006c9638d7..f2289a1a0145a8 100644 --- a/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp +++ b/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp @@ -121,6 +121,6 @@ static uint32_t __stdcall IUnknown_AddRef(void* pComThis) FCIMPL0(void*, RhGetIUnknownAddRef) { - return &IUnknown_AddRef; + return (void*)&IUnknown_AddRef; } FCIMPLEND From c4500e8b61c9675ca2435dedae95425d5a526f17 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Mon, 16 Dec 2024 18:24:00 -0800 Subject: [PATCH 5/8] Build break --- .../Runtime/windows/PalRedhawkInline.h | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/coreclr/nativeaot/Runtime/windows/PalRedhawkInline.h b/src/coreclr/nativeaot/Runtime/windows/PalRedhawkInline.h index 70679e02ced49d..595c7f663b9d13 100644 --- a/src/coreclr/nativeaot/Runtime/windows/PalRedhawkInline.h +++ b/src/coreclr/nativeaot/Runtime/windows/PalRedhawkInline.h @@ -14,13 +14,6 @@ FORCEINLINE int32_t PalInterlockedIncrement(_Inout_ int32_t volatile *pDst) return _InterlockedIncrement((long volatile *)pDst); } -EXTERN_C int64_t __cdecl _InterlockedIncrement64(int64_t volatile *); -#pragma intrinsic(_InterlockedIncrement64) -FORCEINLINE int64_t PalInterlockedIncrement64(_Inout_ int64_t volatile *pDst) -{ - return _InterlockedIncrement64(pDst); -} - EXTERN_C long __cdecl _InterlockedDecrement(long volatile *); #pragma intrinsic(_InterlockedDecrement) FORCEINLINE int32_t PalInterlockedDecrement(_Inout_ int32_t volatile *pDst) @@ -74,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) @@ -81,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) From fd229c03f3eb774714233dc23ad591f8d49b3b39 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 17 Dec 2024 06:16:34 -0800 Subject: [PATCH 6/8] Update src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp --- src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp b/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp index f2289a1a0145a8..8eddc1d3440d26 100644 --- a/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp +++ b/src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp @@ -109,9 +109,9 @@ static ManagedObjectWrapper* ToManagedObjectWrapper(void* dispatchPtr) } // -// AddRef is implemented in native code so that it can be invoked during a 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. +// 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) { From 5aee0a9663a3dbf4d79d89508d07ef28c2a50c7c Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 17 Dec 2024 10:21:16 -0800 Subject: [PATCH 7/8] Apply suggestions from code review --- .../Runtime/InteropServices/ComWrappers.NativeAot.cs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) 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 1c883148a95a51..08810e20ee79d8 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 = RuntimeImports.RhGetIUnknownAddRef(); + fpAddRef = RuntimeImports.RhGetIUnknownAddRef(); // Implemented in C/C++ to avoid GC transitions fpRelease = (IntPtr)(delegate* unmanaged)&ComWrappers.IUnknown_Release; } @@ -1302,14 +1302,6 @@ internal static unsafe int IUnknown_QueryInterface(IntPtr pThis, Guid* guid, Int return wrapper->QueryInterface(in *guid, out *ppObject); } -#if false // Implemented in C/C++ to avoid GC transitions - [UnmanagedCallersOnly] - internal static unsafe uint IUnknown_AddRef(IntPtr pThis) - { - ManagedObjectWrapper* wrapper = ComInterfaceDispatch.ToManagedObjectWrapper((ComInterfaceDispatch*)pThis); - return wrapper->AddRef(); - } -#endif [UnmanagedCallersOnly] internal static unsafe uint IUnknown_Release(IntPtr pThis) From 511dc8d9c5a04b4d4c4e3ac81bc1491f98ce4c0e Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 17 Dec 2024 10:21:36 -0800 Subject: [PATCH 8/8] Update src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs --- .../src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs | 1 - 1 file changed, 1 deletion(-) 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 08810e20ee79d8..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 @@ -1302,7 +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_Release(IntPtr pThis) {