From 3e2bc17b0dc12ef9bf12c5e33114817a5a20cc09 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Fri, 12 Jan 2024 17:12:32 -0800 Subject: [PATCH] Remove uses of FCThrow in interlocked helpers (#96916) FCThrow is implemented usign HELPER_METHOD_FRAME. All uses of FCThrow need to be removed to fix #95695. --- .../System/Threading/Interlocked.CoreCLR.cs | 112 ++++++++++++++++-- src/coreclr/vm/comutilnative.cpp | 36 +----- src/coreclr/vm/comutilnative.h | 8 +- src/coreclr/vm/ecalllist.h | 16 +-- 4 files changed, 115 insertions(+), 57 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs index 1bce1b45748c9..93df3bdfed9fb 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs @@ -48,8 +48,20 @@ public static long Decrement(ref long location) => /// The original value of . /// The address of location1 is a null pointer. [Intrinsic] + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static int Exchange(ref int location1, int value) + { +#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64 + return Exchange(ref location1, value); // Must expand intrinsic +#else + if (Unsafe.IsNullRef(ref location1)) + ThrowHelper.ThrowNullReferenceException(); + return Exchange32(ref location1, value); +#endif + } + [MethodImpl(MethodImplOptions.InternalCall)] - public static extern int Exchange(ref int location1, int value); + private static extern int Exchange32(ref int location1, int value); /// Sets a 64-bit signed integer to a specified value and returns the original value, as an atomic operation. /// The variable to set to the specified value. @@ -57,8 +69,20 @@ public static long Decrement(ref long location) => /// The original value of . /// The address of location1 is a null pointer. [Intrinsic] + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static long Exchange(ref long location1, long value) + { +#if TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64 + return Exchange(ref location1, value); // Must expand intrinsic +#else + if (Unsafe.IsNullRef(ref location1)) + ThrowHelper.ThrowNullReferenceException(); + return Exchange64(ref location1, value); +#endif + } + [MethodImpl(MethodImplOptions.InternalCall)] - public static extern long Exchange(ref long location1, long value); + private static extern long Exchange64(ref long location1, long value); /// Sets an object to the specified value and returns a reference to the original object, as an atomic operation. /// The variable to set to the specified value. @@ -66,9 +90,18 @@ public static long Decrement(ref long location) => /// The original value of . /// The address of location1 is a null pointer. [Intrinsic] - [MethodImpl(MethodImplOptions.InternalCall)] + [MethodImpl(MethodImplOptions.AggressiveInlining)] [return: NotNullIfNotNull(nameof(location1))] - public static extern object? Exchange([NotNullIfNotNull(nameof(value))] ref object? location1, object? value); + public static object? Exchange([NotNullIfNotNull(nameof(value))] ref object? location1, object? value) + { + if (Unsafe.IsNullRef(ref location1)) + ThrowHelper.ThrowNullReferenceException(); + return ExchangeObject(ref location1, value); + } + + [return: NotNullIfNotNull(nameof(location1))] + [MethodImpl(MethodImplOptions.InternalCall)] + private static extern object? ExchangeObject([NotNullIfNotNull(nameof(value))] ref object? location1, object? value); // The below whole method reduces to a single call to Exchange(ref object, object) but // the JIT thinks that it will generate more native code than it actually does. @@ -84,7 +117,7 @@ public static long Decrement(ref long location) => [MethodImpl(MethodImplOptions.AggressiveInlining)] public static T Exchange([NotNullIfNotNull(nameof(value))] ref T location1, T value) where T : class? => Unsafe.As(Exchange(ref Unsafe.As(ref location1), value)); - #endregion +#endregion #region CompareExchange /// Compares two 32-bit signed integers for equality and, if they are equal, replaces the first value. @@ -94,8 +127,20 @@ public static T Exchange([NotNullIfNotNull(nameof(value))] ref T location1, T /// The original value in . /// The address of is a null pointer. [Intrinsic] + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static int CompareExchange(ref int location1, int value, int comparand) + { +#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64 + return CompareExchange(ref location1, value, comparand); // Must expand intrinsic +#else + if (Unsafe.IsNullRef(ref location1)) + ThrowHelper.ThrowNullReferenceException(); + return CompareExchange32(ref location1, value, comparand); +#endif + } + [MethodImpl(MethodImplOptions.InternalCall)] - public static extern int CompareExchange(ref int location1, int value, int comparand); + private static extern int CompareExchange32(ref int location1, int value, int comparand); /// Compares two 64-bit signed integers for equality and, if they are equal, replaces the first value. /// The destination, whose value is compared with and possibly replaced. @@ -104,8 +149,20 @@ public static T Exchange([NotNullIfNotNull(nameof(value))] ref T location1, T /// The original value in . /// The address of is a null pointer. [Intrinsic] + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static long CompareExchange(ref long location1, long value, long comparand) + { +#if TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64 + return CompareExchange(ref location1, value, comparand); // Must expand intrinsic +#else + if (Unsafe.IsNullRef(ref location1)) + ThrowHelper.ThrowNullReferenceException(); + return CompareExchange64(ref location1, value, comparand); +#endif + } + [MethodImpl(MethodImplOptions.InternalCall)] - public static extern long CompareExchange(ref long location1, long value, long comparand); + private static extern long CompareExchange64(ref long location1, long value, long comparand); /// Compares two objects for reference equality and, if they are equal, replaces the first object. /// The destination object that is compared by reference with and possibly replaced. @@ -114,9 +171,18 @@ public static T Exchange([NotNullIfNotNull(nameof(value))] ref T location1, T /// The original value in . /// The address of is a null pointer. [Intrinsic] + [MethodImpl(MethodImplOptions.AggressiveInlining)] + [return: NotNullIfNotNull(nameof(location1))] + public static object? CompareExchange(ref object? location1, object? value, object? comparand) + { + if (Unsafe.IsNullRef(ref location1)) + ThrowHelper.ThrowNullReferenceException(); + return CompareExchangeObject(ref location1, value, comparand); + } + [MethodImpl(MethodImplOptions.InternalCall)] [return: NotNullIfNotNull(nameof(location1))] - public static extern object? CompareExchange(ref object? location1, object? value, object? comparand); + private static extern object? CompareExchangeObject(ref object? location1, object? value, object? comparand); // Note that getILIntrinsicImplementationForInterlocked() in vm\jitinterface.cpp replaces // the body of the following method with the following IL: @@ -136,8 +202,8 @@ public static T Exchange([NotNullIfNotNull(nameof(value))] ref T location1, T /// The address of is a null pointer. /// The type to be used for , , and . This type must be a reference type. [Intrinsic] - [return: NotNullIfNotNull(nameof(location1))] [MethodImpl(MethodImplOptions.AggressiveInlining)] + [return: NotNullIfNotNull(nameof(location1))] public static T CompareExchange(ref T location1, T value, T comparand) where T : class? => Unsafe.As(CompareExchange(ref Unsafe.As(ref location1), value, comparand)); #endregion @@ -160,12 +226,36 @@ public static long Add(ref long location1, long value) => ExchangeAdd(ref location1, value) + value; [Intrinsic] + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static int ExchangeAdd(ref int location1, int value) + { +#if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64 + return ExchangeAdd(ref location1, value); // Must expand intrinsic +#else + if (Unsafe.IsNullRef(ref location1)) + ThrowHelper.ThrowNullReferenceException(); + return ExchangeAdd32(ref location1, value); +#endif + } + [MethodImpl(MethodImplOptions.InternalCall)] - private static extern int ExchangeAdd(ref int location1, int value); + private static extern int ExchangeAdd32(ref int location1, int value); [Intrinsic] + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static long ExchangeAdd(ref long location1, long value) + { +#if TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64 + return ExchangeAdd(ref location1, value); // Must expand intrinsic +#else + if (Unsafe.IsNullRef(ref location1)) + ThrowHelper.ThrowNullReferenceException(); + return ExchangeAdd64(ref location1, value); +#endif + } + [MethodImpl(MethodImplOptions.InternalCall)] - private static extern long ExchangeAdd(ref long location1, long value); + private static extern long ExchangeAdd64(ref long location1, long value); #endregion #region Read diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index 01fa331306554..c016311cd6231 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/src/coreclr/vm/comutilnative.cpp @@ -1490,14 +1490,10 @@ NOINLINE void GCInterface::GarbageCollectModeAny(int generation) #include -FCIMPL2(INT32,COMInterlocked::Exchange, INT32 *location, INT32 value) +FCIMPL2(INT32,COMInterlocked::Exchange32, INT32 *location, INT32 value) { FCALL_CONTRACT; - if( NULL == location) { - FCThrow(kNullReferenceException); - } - return InterlockedExchange((LONG *) location, value); } FCIMPLEND @@ -1506,22 +1502,14 @@ FCIMPL2_IV(INT64,COMInterlocked::Exchange64, INT64 *location, INT64 value) { FCALL_CONTRACT; - if( NULL == location) { - FCThrow(kNullReferenceException); - } - return InterlockedExchange64((INT64 *) location, value); } FCIMPLEND -FCIMPL3(INT32, COMInterlocked::CompareExchange, INT32* location, INT32 value, INT32 comparand) +FCIMPL3(INT32, COMInterlocked::CompareExchange32, INT32* location, INT32 value, INT32 comparand) { FCALL_CONTRACT; - if( NULL == location) { - FCThrow(kNullReferenceException); - } - return InterlockedCompareExchange((LONG*)location, value, comparand); } FCIMPLEND @@ -1530,10 +1518,6 @@ FCIMPL3_IVV(INT64, COMInterlocked::CompareExchange64, INT64* location, INT64 val { FCALL_CONTRACT; - if( NULL == location) { - FCThrow(kNullReferenceException); - } - return InterlockedCompareExchange64((INT64*)location, value, comparand); } FCIMPLEND @@ -1542,10 +1526,6 @@ FCIMPL2(LPVOID,COMInterlocked::ExchangeObject, LPVOID*location, LPVOID value) { FCALL_CONTRACT; - if( NULL == location) { - FCThrow(kNullReferenceException); - } - LPVOID ret = InterlockedExchangeT(location, value); #ifdef _DEBUG Thread::ObjectRefAssign((OBJECTREF *)location); @@ -1559,10 +1539,6 @@ FCIMPL3(LPVOID,COMInterlocked::CompareExchangeObject, LPVOID *location, LPVOID v { FCALL_CONTRACT; - if( NULL == location) { - FCThrow(kNullReferenceException); - } - // @todo: only set ref if is updated LPVOID ret = InterlockedCompareExchangeT(location, value, comparand); if (ret == comparand) { @@ -1579,10 +1555,6 @@ FCIMPL2(INT32,COMInterlocked::ExchangeAdd32, INT32 *location, INT32 value) { FCALL_CONTRACT; - if( NULL == location) { - FCThrow(kNullReferenceException); - } - return InterlockedExchangeAdd((LONG *) location, value); } FCIMPLEND @@ -1591,10 +1563,6 @@ FCIMPL2_IV(INT64,COMInterlocked::ExchangeAdd64, INT64 *location, INT64 value) { FCALL_CONTRACT; - if( NULL == location) { - FCThrow(kNullReferenceException); - } - return InterlockedExchangeAdd64((INT64 *) location, value); } FCIMPLEND diff --git a/src/coreclr/vm/comutilnative.h b/src/coreclr/vm/comutilnative.h index 18c5238f17b7d..2152d1f89b85b 100644 --- a/src/coreclr/vm/comutilnative.h +++ b/src/coreclr/vm/comutilnative.h @@ -230,10 +230,10 @@ extern "C" uint64_t QCALLTYPE GCInterface_GetGenerationBudget(int generation); class COMInterlocked { public: - static FCDECL2(INT32, Exchange, INT32 *location, INT32 value); - static FCDECL2_IV(INT64, Exchange64, INT64 *location, INT64 value); - static FCDECL3(INT32, CompareExchange, INT32* location, INT32 value, INT32 comparand); - static FCDECL3_IVV(INT64, CompareExchange64, INT64* location, INT64 value, INT64 comparand); + static FCDECL2(INT32, Exchange32, INT32 *location, INT32 value); + static FCDECL2_IV(INT64, Exchange64, INT64 *location, INT64 value); + static FCDECL3(INT32, CompareExchange32, INT32* location, INT32 value, INT32 comparand); + static FCDECL3_IVV(INT64, CompareExchange64, INT64* location, INT64 value, INT64 comparand); static FCDECL2(LPVOID, ExchangeObject, LPVOID* location, LPVOID value); static FCDECL3(LPVOID, CompareExchangeObject, LPVOID* location, LPVOID value, LPVOID comparand); static FCDECL2(INT32, ExchangeAdd32, INT32 *location, INT32 value); diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index e106d59fef60b..24e15e01ba21f 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -457,14 +457,14 @@ FCFuncStart(gInteropMarshalFuncs) FCFuncEnd() FCFuncStart(gInterlockedFuncs) - FCFuncElementSig("Exchange", &gsig_SM_RefInt_Int_RetInt, COMInterlocked::Exchange) - FCFuncElementSig("Exchange", &gsig_SM_RefLong_Long_RetLong, COMInterlocked::Exchange64) - FCFuncElementSig("Exchange", &gsig_SM_RefObj_Obj_RetObj, COMInterlocked::ExchangeObject) - FCFuncElementSig("CompareExchange", &gsig_SM_RefInt_Int_Int_RetInt, COMInterlocked::CompareExchange) - FCFuncElementSig("CompareExchange", &gsig_SM_RefLong_Long_Long_RetLong, COMInterlocked::CompareExchange64) - FCFuncElementSig("CompareExchange", &gsig_SM_RefObj_Obj_Obj_RetObj, COMInterlocked::CompareExchangeObject) - FCFuncElementSig("ExchangeAdd", &gsig_SM_RefInt_Int_RetInt, COMInterlocked::ExchangeAdd32) - FCFuncElementSig("ExchangeAdd", &gsig_SM_RefLong_Long_RetLong, COMInterlocked::ExchangeAdd64) + FCFuncElement("Exchange32", COMInterlocked::Exchange32) + FCFuncElement("Exchange64", COMInterlocked::Exchange64) + FCFuncElement("ExchangeObject", COMInterlocked::ExchangeObject) + FCFuncElement("CompareExchange32", COMInterlocked::CompareExchange32) + FCFuncElement("CompareExchange64", COMInterlocked::CompareExchange64) + FCFuncElement("CompareExchangeObject", COMInterlocked::CompareExchangeObject) + FCFuncElement("ExchangeAdd32", COMInterlocked::ExchangeAdd32) + FCFuncElement("ExchangeAdd64", COMInterlocked::ExchangeAdd64) FCFuncEnd() FCFuncStart(gJitInfoFuncs)