From 1d81cece5d444229c600ed61030612a8e8a55265 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Wed, 20 Mar 2024 15:59:57 +0100 Subject: [PATCH 1/9] Move RhpLockCmpXchg64 implementation to C; move null checks from RhpLockCmpXchg64/RhpCheckedLockCmpXchg/RhpCheckedXchg to managed code --- src/coreclr/nativeaot/Runtime/CMakeLists.txt | 4 +-- src/coreclr/nativeaot/Runtime/EHHelpers.cpp | 14 +------- src/coreclr/nativeaot/Runtime/MiscHelpers.cpp | 8 +++++ .../nativeaot/Runtime/amd64/WriteBarriers.S | 8 ----- .../nativeaot/Runtime/amd64/WriteBarriers.asm | 8 ----- .../nativeaot/Runtime/arm/Interlocked.S | 27 -------------- .../nativeaot/Runtime/arm/WriteBarriers.S | 8 ----- .../nativeaot/Runtime/arm64/WriteBarriers.S | 4 --- .../nativeaot/Runtime/arm64/WriteBarriers.asm | 12 ------- .../nativeaot/Runtime/i386/Interlocked.S | 4 --- .../nativeaot/Runtime/i386/Interlocked.asm | 35 ------------------- .../nativeaot/Runtime/i386/WriteBarriers.asm | 8 ----- src/coreclr/nativeaot/Runtime/portable.cpp | 8 ----- .../src/System/Runtime/RuntimeImports.cs | 4 +++ .../src/System/Threading/Interlocked.cs | 30 +++++++++++----- 15 files changed, 36 insertions(+), 146 deletions(-) delete mode 100644 src/coreclr/nativeaot/Runtime/i386/Interlocked.S delete mode 100644 src/coreclr/nativeaot/Runtime/i386/Interlocked.asm diff --git a/src/coreclr/nativeaot/Runtime/CMakeLists.txt b/src/coreclr/nativeaot/Runtime/CMakeLists.txt index cb9a63c197377..bbed14b74861e 100644 --- a/src/coreclr/nativeaot/Runtime/CMakeLists.txt +++ b/src/coreclr/nativeaot/Runtime/CMakeLists.txt @@ -203,11 +203,11 @@ if (CLR_CMAKE_TARGET_ARCH_AMD64 AND CLR_CMAKE_TARGET_WIN32) ) endif (CLR_CMAKE_TARGET_ARCH_AMD64 AND CLR_CMAKE_TARGET_WIN32) -if (NOT (CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_ARM64)) +if (CLR_CMAKE_TARGET_ARCH_ARM) list(APPEND RUNTIME_SOURCES_ARCH_ASM ${ARCH_SOURCES_DIR}/Interlocked.${ASM_SUFFIX} ) -endif (NOT (CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_ARM64)) +endif (CLR_CMAKE_TARGET_ARCH_ARM) list(APPEND RUNTIME_SOURCES_ARCH_ASM ${ARCH_SOURCES_DIR}/AllocFast.${ASM_SUFFIX} diff --git a/src/coreclr/nativeaot/Runtime/EHHelpers.cpp b/src/coreclr/nativeaot/Runtime/EHHelpers.cpp index c440aced22dd9..5b9454f8adea4 100644 --- a/src/coreclr/nativeaot/Runtime/EHHelpers.cpp +++ b/src/coreclr/nativeaot/Runtime/EHHelpers.cpp @@ -326,13 +326,10 @@ EXTERN_C CODE_LOCATION RhpCheckedAssignRefESIAVLocation; EXTERN_C CODE_LOCATION RhpCheckedAssignRefEDIAVLocation; EXTERN_C CODE_LOCATION RhpCheckedAssignRefEBPAVLocation; #endif -EXTERN_C CODE_LOCATION RhpCheckedLockCmpXchgAVLocation; -EXTERN_C CODE_LOCATION RhpCheckedXchgAVLocation; #if !defined(HOST_AMD64) && !defined(HOST_ARM64) EXTERN_C CODE_LOCATION RhpLockCmpXchg8AVLocation; EXTERN_C CODE_LOCATION RhpLockCmpXchg16AVLocation; EXTERN_C CODE_LOCATION RhpLockCmpXchg32AVLocation; -EXTERN_C CODE_LOCATION RhpLockCmpXchg64AVLocation; #endif EXTERN_C CODE_LOCATION RhpByRefAssignRefAVLocation1; @@ -368,23 +365,14 @@ static bool InWriteBarrierHelper(uintptr_t faultingIP) (uintptr_t)&RhpCheckedAssignRefEDIAVLocation, (uintptr_t)&RhpCheckedAssignRefEBPAVLocation, #endif - (uintptr_t)&RhpCheckedLockCmpXchgAVLocation, - (uintptr_t)&RhpCheckedXchgAVLocation, -#if !defined(HOST_AMD64) && !defined(HOST_ARM64) -#if !defined(HOST_X86) +#if defined(HOST_ARM) (uintptr_t)&RhpLockCmpXchg8AVLocation, (uintptr_t)&RhpLockCmpXchg16AVLocation, (uintptr_t)&RhpLockCmpXchg32AVLocation, -#endif - (uintptr_t)&RhpLockCmpXchg64AVLocation, #endif (uintptr_t)&RhpByRefAssignRefAVLocation1, #if !defined(HOST_ARM64) (uintptr_t)&RhpByRefAssignRefAVLocation2, -#endif -#if defined(HOST_ARM64) && !defined(LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT) - (uintptr_t)&RhpCheckedLockCmpXchgAVLocation2, - (uintptr_t)&RhpCheckedXchgAVLocation2, #endif }; diff --git a/src/coreclr/nativeaot/Runtime/MiscHelpers.cpp b/src/coreclr/nativeaot/Runtime/MiscHelpers.cpp index 6189b60688010..715361b578228 100644 --- a/src/coreclr/nativeaot/Runtime/MiscHelpers.cpp +++ b/src/coreclr/nativeaot/Runtime/MiscHelpers.cpp @@ -416,3 +416,11 @@ EXTERN_C void QCALLTYPE RhCpuIdEx(int* cpuInfo, int functionId, int subFunctionI __cpuidex(cpuInfo, functionId, subFunctionId); } #endif + +#if defined(TARGET_X86) || defined(TARGET_ARM) +FCIMPL3(int64_t, RhpLockCmpXchg64, int64_t * location, int64_t value, int64_t comparand) +{ + return PalInterlockedCompareExchange64(location, value, comparand); +} +FCIMPLEND +#endif \ No newline at end of file diff --git a/src/coreclr/nativeaot/Runtime/amd64/WriteBarriers.S b/src/coreclr/nativeaot/Runtime/amd64/WriteBarriers.S index 8078be2a9a80d..e55e682653b51 100644 --- a/src/coreclr/nativeaot/Runtime/amd64/WriteBarriers.S +++ b/src/coreclr/nativeaot/Runtime/amd64/WriteBarriers.S @@ -221,12 +221,8 @@ LEAF_END RhpCheckedAssignRef\EXPORT_REG_NAME, _TEXT // just one write barrier that assumes the input register is RSI. DEFINE_CHECKED_WRITE_BARRIER RSI, ESI -// WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular: -// - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpCheckedLockCmpXchgAVLocation -// - Function "UnwindSimpleHelperToCaller" assumes the stack contains just the pushed return address LEAF_ENTRY RhpCheckedLockCmpXchg, _TEXT mov rax, rdx -ALTERNATE_ENTRY RhpCheckedLockCmpXchgAVLocation lock cmpxchg [rdi], rsi jne LOCAL_LABEL(RhpCheckedLockCmpXchg_NoBarrierRequired_RSI) @@ -234,15 +230,11 @@ ALTERNATE_ENTRY RhpCheckedLockCmpXchgAVLocation LEAF_END RhpCheckedLockCmpXchg, _TEXT -// WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular: -// - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpCheckedXchgAVLocation -// - Function "UnwindSimpleHelperToCaller" assumes the stack contains just the pushed return address LEAF_ENTRY RhpCheckedXchg, _TEXT // Setup rax with the new object for the exchange, that way it will automatically hold the correct result // afterwards and we can leave rdx unaltered ready for the GC write barrier below. mov rax, rsi -ALTERNATE_ENTRY RhpCheckedXchgAVLocation xchg [rdi], rax DEFINE_CHECKED_WRITE_BARRIER_CORE RhpCheckedXchg, RSI diff --git a/src/coreclr/nativeaot/Runtime/amd64/WriteBarriers.asm b/src/coreclr/nativeaot/Runtime/amd64/WriteBarriers.asm index 7fdf76f0fd832..302b9e0a8b1fe 100644 --- a/src/coreclr/nativeaot/Runtime/amd64/WriteBarriers.asm +++ b/src/coreclr/nativeaot/Runtime/amd64/WriteBarriers.asm @@ -237,12 +237,8 @@ endm ;; just one write barrier that assumes the input register is RDX. DEFINE_CHECKED_WRITE_BARRIER RDX, EDX -;; WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular: -;; - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpCheckedLockCmpXchgAVLocation -;; - Function "UnwindSimpleHelperToCaller" assumes the stack contains just the pushed return address LEAF_ENTRY RhpCheckedLockCmpXchg, _TEXT mov rax, r8 -ALTERNATE_ENTRY RhpCheckedLockCmpXchgAVLocation lock cmpxchg [rcx], rdx jne RhpCheckedLockCmpXchg_NoBarrierRequired_RDX @@ -250,15 +246,11 @@ ALTERNATE_ENTRY RhpCheckedLockCmpXchgAVLocation LEAF_END RhpCheckedLockCmpXchg, _TEXT -;; WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular: -;; - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpCheckedXchgAVLocation -;; - Function "UnwindSimpleHelperToCaller" assumes the stack contains just the pushed return address LEAF_ENTRY RhpCheckedXchg, _TEXT ;; Setup rax with the new object for the exchange, that way it will automatically hold the correct result ;; afterwards and we can leave rdx unaltered ready for the GC write barrier below. mov rax, rdx -ALTERNATE_ENTRY RhpCheckedXchgAVLocation xchg [rcx], rax DEFINE_CHECKED_WRITE_BARRIER_CORE RhpCheckedXchg, RDX diff --git a/src/coreclr/nativeaot/Runtime/arm/Interlocked.S b/src/coreclr/nativeaot/Runtime/arm/Interlocked.S index 631731c7e3a32..10c3d3f120254 100644 --- a/src/coreclr/nativeaot/Runtime/arm/Interlocked.S +++ b/src/coreclr/nativeaot/Runtime/arm/Interlocked.S @@ -73,30 +73,3 @@ LOCAL_LABEL(CmpXchg32Exit): dmb bx lr LEAF_END RhpLockCmpXchg32, _TEXT - -// WARNING: Code in EHHelpers.cpp makes assumptions about this helper, in particular: -// - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpLockCmpXchg64AVLocation -// - Function "UnwindSimpleHelperToCaller" assumes no registers were pushed and LR contains the return address -// r0 = destination address -// {r2,r3} = value -// sp[0+8] = comparand -LEAF_ENTRY RhpLockCmpXchg64, _TEXT -GLOBAL_LABEL RhpLockCmpXchg64AVLocation - ldr r12, [r0] // dummy read for null check - PROLOG_PUSH "{r4-r6,lr}" - dmb - ldrd r4, r5, [sp,#0x10] -LOCAL_LABEL(CmpXchg64Retry): - ldrexd r6, r1, [r0] - cmp r6, r4 - bne LOCAL_LABEL(CmpXchg64Exit) - cmp r1, r5 - bne LOCAL_LABEL(CmpXchg64Exit) - strexd r12, r2, r3, [r0] - cmp r12, #0 - bne LOCAL_LABEL(CmpXchg64Retry) -LOCAL_LABEL(CmpXchg64Exit): - mov r0, r6 - dmb - EPILOG_POP "{r4-r6,pc}" -LEAF_END RhpLockCmpXchg64, _TEXT diff --git a/src/coreclr/nativeaot/Runtime/arm/WriteBarriers.S b/src/coreclr/nativeaot/Runtime/arm/WriteBarriers.S index 3f7a10a859255..3bb862231a347 100644 --- a/src/coreclr/nativeaot/Runtime/arm/WriteBarriers.S +++ b/src/coreclr/nativeaot/Runtime/arm/WriteBarriers.S @@ -250,9 +250,6 @@ LEAF_END RhpCheckedAssignRef\EXPORT_REG_NAME, _TEXT // just one write barrier that assumes the input register is RSI. DEFINE_CHECKED_WRITE_BARRIER r1, r1 -// WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular: -// - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpCheckedLockCmpXchgAVLocation -// - Function "UnwindSimpleHelperToCaller" assumes no registers were pushed and LR contains the return address // r0 = destination address // r1 = value // r2 = comparand @@ -261,7 +258,6 @@ LEAF_ENTRY RhpCheckedLockCmpXchg, _TEXT // barrier must occur before the object reference update, so we have to do it unconditionally even // though the update may fail below. dmb -GLOBAL_LABEL RhpCheckedLockCmpXchgAVLocation LOCAL_LABEL(RhpCheckedLockCmpXchgRetry): ldrex r3, [r0] cmp r2, r3 @@ -277,16 +273,12 @@ LOCAL_LABEL(RhpCheckedLockCmpXchgRetry): bx lr LEAF_END RhpCheckedLockCmpXchg, _TEXT -// WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular: -// - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpCheckedXchgAVLocation -// - Function "UnwindSimpleHelperToCaller" assumes no registers were pushed and LR contains the return address // r0 = destination address // r1 = value LEAF_ENTRY RhpCheckedXchg, _TEXT // To implement our chosen memory model for ARM we insert a memory barrier at GC write barriers. This // barrier must occur before the object reference update. dmb -GLOBAL_LABEL RhpCheckedXchgAVLocation LOCAL_LABEL(RhpCheckedXchgRetry): ldrex r2, [r0] strex r3, r1, [r0] diff --git a/src/coreclr/nativeaot/Runtime/arm64/WriteBarriers.S b/src/coreclr/nativeaot/Runtime/arm64/WriteBarriers.S index 275ae9401dca7..474509ea587f6 100644 --- a/src/coreclr/nativeaot/Runtime/arm64/WriteBarriers.S +++ b/src/coreclr/nativeaot/Runtime/arm64/WriteBarriers.S @@ -302,7 +302,6 @@ LEAF_END RhpAssignRef, _TEXT #endif mov x10, x2 - ALTERNATE_ENTRY RhpCheckedLockCmpXchgAVLocation casal x10, x1, [x0] // exchange cmp x2, x10 bne LOCAL_LABEL(CmpXchgNoUpdate) @@ -311,7 +310,6 @@ LEAF_END RhpAssignRef, _TEXT b LOCAL_LABEL(DoCardsCmpXchg) LOCAL_LABEL(CmpXchgRetry): // Check location value is what we expect. - ALTERNATE_ENTRY RhpCheckedLockCmpXchgAVLocation2 ldaxr x10, [x0] cmp x10, x2 bne LOCAL_LABEL(CmpXchgNoUpdate) @@ -365,14 +363,12 @@ LOCAL_LABEL(NoBarrierCmpXchg): tbz w16, #ARM64_ATOMICS_FEATURE_FLAG_BIT, LOCAL_LABEL(ExchangeRetry) #endif - ALTERNATE_ENTRY RhpCheckedXchgAVLocation swpal x1, x10, [x0] // exchange #ifndef LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT b LOCAL_LABEL(DoCardsXchg) LOCAL_LABEL(ExchangeRetry): // Read the existing memory location. - ALTERNATE_ENTRY RhpCheckedXchgAVLocation2 ldaxr x10, [x0] // Attempt to update with the new value. diff --git a/src/coreclr/nativeaot/Runtime/arm64/WriteBarriers.asm b/src/coreclr/nativeaot/Runtime/arm64/WriteBarriers.asm index 1389921eff544..5ccccd2a301ec 100644 --- a/src/coreclr/nativeaot/Runtime/arm64/WriteBarriers.asm +++ b/src/coreclr/nativeaot/Runtime/arm64/WriteBarriers.asm @@ -286,10 +286,6 @@ NotInHeap ;; Interlocked operation helpers where the location is an objectref, thus requiring a GC write barrier upon ;; successful updates. -;; WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular: -;; - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpCheckedLockCmpXchgAVLocation -;; - Function "UnwindSimpleHelperToCaller" assumes no registers were pushed and LR contains the return address - ;; RhpCheckedLockCmpXchg(Object** dest, Object* value, Object* comparand) ;; ;; Interlocked compare exchange on objectref. @@ -311,7 +307,6 @@ NotInHeap #endif mov x10, x2 - ALTERNATE_ENTRY RhpCheckedLockCmpXchgAVLocation casal x10, x1, [x0] ;; exchange cmp x2, x10 bne CmpXchgNoUpdate @@ -320,7 +315,6 @@ NotInHeap b DoCardsCmpXchg CmpXchgRetry ;; Check location value is what we expect. - ALTERNATE_ENTRY RhpCheckedLockCmpXchgAVLocation2 ldaxr x10, [x0] cmp x10, x2 bne CmpXchgNoUpdate @@ -350,10 +344,6 @@ NoBarrierCmpXchg LEAF_END RhpCheckedLockCmpXchg -;; WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular: -;; - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen within at RhpCheckedXchgAVLocation -;; - Function "UnwindSimpleHelperToCaller" assumes no registers were pushed and LR contains the return address - ;; RhpCheckedXchg(Object** destination, Object* value) ;; ;; Interlocked exchange on objectref. @@ -374,14 +364,12 @@ NoBarrierCmpXchg tbz x16, #ARM64_ATOMICS_FEATURE_FLAG_BIT, ExchangeRetry #endif - ALTERNATE_ENTRY RhpCheckedXchgAVLocation swpal x1, x10, [x0] ;; exchange #ifndef LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT b DoCardsXchg ExchangeRetry ;; Read the existing memory location. - ALTERNATE_ENTRY RhpCheckedXchgAVLocation2 ldaxr x10, [x0] ;; Attempt to update with the new value. diff --git a/src/coreclr/nativeaot/Runtime/i386/Interlocked.S b/src/coreclr/nativeaot/Runtime/i386/Interlocked.S deleted file mode 100644 index 876f2dfbcb80d..0000000000000 --- a/src/coreclr/nativeaot/Runtime/i386/Interlocked.S +++ /dev/null @@ -1,4 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -// TODO: Implement diff --git a/src/coreclr/nativeaot/Runtime/i386/Interlocked.asm b/src/coreclr/nativeaot/Runtime/i386/Interlocked.asm deleted file mode 100644 index d862d4ddc0d88..0000000000000 --- a/src/coreclr/nativeaot/Runtime/i386/Interlocked.asm +++ /dev/null @@ -1,35 +0,0 @@ -;; Licensed to the .NET Foundation under one or more agreements. -;; The .NET Foundation licenses this file to you under the MIT license. - - .586 - .xmm - .model flat - option casemap:none - .code - -include AsmMacros.inc - -FASTCALL_FUNC RhpLockCmpXchg64, 20 - -_value$ = 16 -_comparand$ = 8 - -ALTERNATE_ENTRY _RhpLockCmpXchg64AVLocation - ;; Null check - cmp dword ptr [ecx], ecx - - mov eax, DWORD PTR _comparand$[esp-4] - mov edx, DWORD PTR _comparand$[esp] - push ebx - mov ebx, DWORD PTR _value$[esp] - push esi - mov esi, ecx - mov ecx, DWORD PTR _value$[esp+8] - lock cmpxchg8b QWORD PTR [esi] - pop esi - pop ebx - ret 16 - -FASTCALL_ENDFUNC - -end diff --git a/src/coreclr/nativeaot/Runtime/i386/WriteBarriers.asm b/src/coreclr/nativeaot/Runtime/i386/WriteBarriers.asm index 953ea11b74de6..133081bee8319 100644 --- a/src/coreclr/nativeaot/Runtime/i386/WriteBarriers.asm +++ b/src/coreclr/nativeaot/Runtime/i386/WriteBarriers.asm @@ -237,12 +237,8 @@ DEFINE_CHECKED_WRITE_BARRIER EDX, ESI DEFINE_CHECKED_WRITE_BARRIER EDX, EDI DEFINE_CHECKED_WRITE_BARRIER EDX, EBP -;; WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular: -;; - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at @RhpCheckedLockCmpXchgAVLocation@0 -;; - Function "UnwindSimpleHelperToCaller" assumes the stack contains just the pushed return address FASTCALL_FUNC RhpCheckedLockCmpXchg, 12 mov eax, [esp+4] -ALTERNATE_ENTRY _RhpCheckedLockCmpXchgAVLocation lock cmpxchg [ecx], edx jne RhpCheckedLockCmpXchg_NoBarrierRequired_ECX_EDX @@ -250,15 +246,11 @@ ALTERNATE_ENTRY _RhpCheckedLockCmpXchgAVLocation FASTCALL_ENDFUNC -;; WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular: -;; - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at @RhpCheckedXchgAVLocation@0 -;; - Function "UnwindSimpleHelperToCaller" assumes the stack contains just the pushed return address FASTCALL_FUNC RhpCheckedXchg, 8 ;; Setup eax with the new object for the exchange, that way it will automatically hold the correct result ;; afterwards and we can leave edx unaltered ready for the GC write barrier below. mov eax, edx -ALTERNATE_ENTRY _RhpCheckedXchgAVLocation xchg [ecx], eax DEFINE_CHECKED_WRITE_BARRIER_CORE RhpCheckedXchg, ECX, EDX, ret diff --git a/src/coreclr/nativeaot/Runtime/portable.cpp b/src/coreclr/nativeaot/Runtime/portable.cpp index d797ad14687ba..28fb100b2fd9c 100644 --- a/src/coreclr/nativeaot/Runtime/portable.cpp +++ b/src/coreclr/nativeaot/Runtime/portable.cpp @@ -374,7 +374,6 @@ FCIMPLEND FCIMPL3(Object *, RhpCheckedLockCmpXchg, Object ** location, Object * value, Object * comparand) { - // @TODO: USE_PORTABLE_HELPERS - Null check Object * ret = (Object *)PalInterlockedCompareExchangePointer((void * volatile *)location, value, comparand); InlineCheckedWriteBarrier(location, value); return ret; @@ -411,13 +410,6 @@ FCIMPL3(int32_t, RhpLockCmpXchg32, int32_t * location, int32_t value, int32_t co } FCIMPLEND -FCIMPL3(int64_t, RhpLockCmpXchg64, int64_t * location, int64_t value, int64_t comparand) -{ - // @TODO: USE_PORTABLE_HELPERS - Null check - return PalInterlockedCompareExchange64(location, value, comparand); -} -FCIMPLEND - FCIMPL0(void*, RhAllocateThunksMapping) { return NULL; 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 98604bc58561b..184e8c054b96b 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 @@ -645,6 +645,7 @@ internal static IntPtr RhGetModuleSection(TypeManagerHandle module, ReadyToRunSe // // Interlocked helpers // +#if TARGET_ARM [MethodImplAttribute(MethodImplOptions.InternalCall)] [RuntimeImport(RuntimeLibrary, "RhpLockCmpXchg8")] internal static extern byte InterlockedCompareExchange(ref byte location1, byte value, byte comparand); @@ -656,10 +657,13 @@ internal static IntPtr RhGetModuleSection(TypeManagerHandle module, ReadyToRunSe [MethodImplAttribute(MethodImplOptions.InternalCall)] [RuntimeImport(RuntimeLibrary, "RhpLockCmpXchg32")] internal static extern int InterlockedCompareExchange(ref int location1, int value, int comparand); +#endif +#if TARGET_ARM || TARGET_X86 [MethodImplAttribute(MethodImplOptions.InternalCall)] [RuntimeImport(RuntimeLibrary, "RhpLockCmpXchg64")] internal static extern long InterlockedCompareExchange(ref long location1, long value, long comparand); +#endif [MethodImplAttribute(MethodImplOptions.InternalCall)] [RuntimeImport(RuntimeLibrary, "RhpCheckedLockCmpXchg")] diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs index b4fd68cf7c703..8464a197dab53 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs @@ -15,7 +15,7 @@ public static partial class Interlocked public static byte CompareExchange(ref byte location1, byte value, byte comparand) { #if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 - return CompareExchange(ref location1, value, comparand); + return CompareExchange(ref location1, value, comparand); // Must expand intrinsic #else return RuntimeImports.InterlockedCompareExchange(ref location1, value, comparand); #endif @@ -25,7 +25,7 @@ public static byte CompareExchange(ref byte location1, byte value, byte comparan public static short CompareExchange(ref short location1, short value, short comparand) { #if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 - return CompareExchange(ref location1, value, comparand); + return CompareExchange(ref location1, value, comparand); // Must expand intrinsic #else return RuntimeImports.InterlockedCompareExchange(ref location1, value, comparand); #endif @@ -35,18 +35,21 @@ public static short CompareExchange(ref short location1, short value, short comp 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); + return CompareExchange(ref location1, value, comparand); // Must expand intrinsic #else return RuntimeImports.InterlockedCompareExchange(ref location1, value, comparand); #endif } [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); + return CompareExchange(ref location1, value, comparand); // Must expand intrinsic #else + if (Unsafe.IsNullRef(ref location1)) + ThrowHelper.ThrowNullReferenceException(); return RuntimeImports.InterlockedCompareExchange(ref location1, value, comparand); #endif } @@ -56,13 +59,17 @@ public static long CompareExchange(ref long location1, long value, long comparan [MethodImpl(MethodImplOptions.AggressiveInlining)] public static T CompareExchange(ref T location1, T value, T comparand) where T : class? { + Unsafe.As(CompareExchange(ref Unsafe.As(ref location1), value, comparand)); return Unsafe.As(RuntimeImports.InterlockedCompareExchange(ref Unsafe.As(ref location1), value, comparand)); } [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 RuntimeImports.InterlockedCompareExchange(ref location1, value, comparand); } @@ -74,7 +81,7 @@ public static T CompareExchange(ref T location1, T value, T comparand) where public static byte Exchange(ref byte location1, byte value) { #if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 - return Exchange(ref location1, value); + return Exchange(ref location1, value); // Must expand intrinsic #else byte oldValue; @@ -91,7 +98,7 @@ public static byte Exchange(ref byte location1, byte value) public static short Exchange(ref short location1, short value) { #if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 - return Exchange(ref location1, value); + return Exchange(ref location1, value); // Must expand intrinsic #else short oldValue; @@ -108,7 +115,7 @@ public static short Exchange(ref short location1, short value) public static int Exchange(ref int location1, int value) { #if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64 - return Exchange(ref location1, value); + return Exchange(ref location1, value); // Must expand intrinsic #else int oldValue; @@ -125,7 +132,7 @@ public static int Exchange(ref int location1, int value) public static long Exchange(ref long location1, long value) { #if TARGET_AMD64 || TARGET_ARM64 || TARGET_RISCV64 - return Exchange(ref location1, value); + return Exchange(ref location1, value); // Must expand intrinsic #else long oldValue; @@ -139,17 +146,22 @@ public static long Exchange(ref long location1, long value) } [Intrinsic] - [return: NotNullIfNotNull(nameof(location1))] [MethodImpl(MethodImplOptions.AggressiveInlining)] + [return: NotNullIfNotNull(nameof(location1))] public static T Exchange([NotNullIfNotNull(nameof(value))] ref T location1, T value) where T : class? { + if (Unsafe.IsNullRef(ref location1)) + ThrowHelper.ThrowNullReferenceException(); return Unsafe.As(RuntimeImports.InterlockedExchange(ref Unsafe.As(ref location1), value)); } [Intrinsic] + [MethodImpl(MethodImplOptions.AggressiveInlining)] [return: NotNullIfNotNull(nameof(location1))] public static object? Exchange([NotNullIfNotNull(nameof(value))] ref object? location1, object? value) { + if (Unsafe.IsNullRef(ref location1)) + ThrowHelper.ThrowNullReferenceException(); return RuntimeImports.InterlockedExchange(ref location1, value); } From 3b2e99e7a4196d454863a802c4945b99352ed350 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Wed, 20 Mar 2024 16:27:07 +0100 Subject: [PATCH 2/9] Removed #ifs per feedback --- src/coreclr/nativeaot/Runtime/MiscHelpers.cpp | 2 -- .../src/System/Runtime/RuntimeImports.cs | 4 ---- 2 files changed, 6 deletions(-) diff --git a/src/coreclr/nativeaot/Runtime/MiscHelpers.cpp b/src/coreclr/nativeaot/Runtime/MiscHelpers.cpp index 715361b578228..3c42953f4f772 100644 --- a/src/coreclr/nativeaot/Runtime/MiscHelpers.cpp +++ b/src/coreclr/nativeaot/Runtime/MiscHelpers.cpp @@ -417,10 +417,8 @@ EXTERN_C void QCALLTYPE RhCpuIdEx(int* cpuInfo, int functionId, int subFunctionI } #endif -#if defined(TARGET_X86) || defined(TARGET_ARM) FCIMPL3(int64_t, RhpLockCmpXchg64, int64_t * location, int64_t value, int64_t comparand) { return PalInterlockedCompareExchange64(location, value, comparand); } FCIMPLEND -#endif \ No newline at end of file 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 184e8c054b96b..98604bc58561b 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 @@ -645,7 +645,6 @@ internal static IntPtr RhGetModuleSection(TypeManagerHandle module, ReadyToRunSe // // Interlocked helpers // -#if TARGET_ARM [MethodImplAttribute(MethodImplOptions.InternalCall)] [RuntimeImport(RuntimeLibrary, "RhpLockCmpXchg8")] internal static extern byte InterlockedCompareExchange(ref byte location1, byte value, byte comparand); @@ -657,13 +656,10 @@ internal static IntPtr RhGetModuleSection(TypeManagerHandle module, ReadyToRunSe [MethodImplAttribute(MethodImplOptions.InternalCall)] [RuntimeImport(RuntimeLibrary, "RhpLockCmpXchg32")] internal static extern int InterlockedCompareExchange(ref int location1, int value, int comparand); -#endif -#if TARGET_ARM || TARGET_X86 [MethodImplAttribute(MethodImplOptions.InternalCall)] [RuntimeImport(RuntimeLibrary, "RhpLockCmpXchg64")] internal static extern long InterlockedCompareExchange(ref long location1, long value, long comparand); -#endif [MethodImplAttribute(MethodImplOptions.InternalCall)] [RuntimeImport(RuntimeLibrary, "RhpCheckedLockCmpXchg")] From fb6ec7638c26f61df2edfc97d43e71c7aaebe18a Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Wed, 20 Mar 2024 16:31:48 +0100 Subject: [PATCH 3/9] Fix cut & paste error --- .../System.Private.CoreLib/src/System/Threading/Interlocked.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs index 8464a197dab53..35b2ebbbc3223 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs @@ -59,8 +59,7 @@ public static long CompareExchange(ref long location1, long value, long comparan [MethodImpl(MethodImplOptions.AggressiveInlining)] public static T CompareExchange(ref T location1, T value, T comparand) where T : class? { - Unsafe.As(CompareExchange(ref Unsafe.As(ref location1), value, comparand)); - return Unsafe.As(RuntimeImports.InterlockedCompareExchange(ref Unsafe.As(ref location1), value, comparand)); + return Unsafe.As(CompareExchange(ref Unsafe.As(ref location1), value, comparand)); } [Intrinsic] From 9447a82cff237df73ab13421ec9aada7ef3fd5ba Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Wed, 20 Mar 2024 17:10:35 +0100 Subject: [PATCH 4/9] Update RhpLockCmpXchg[8/16/32] too --- src/coreclr/nativeaot/Runtime/EHHelpers.cpp | 5 ----- src/coreclr/nativeaot/Runtime/arm/Interlocked.S | 12 ------------ src/coreclr/nativeaot/Runtime/portable.cpp | 1 - .../src/System/Threading/Interlocked.cs | 6 ++++++ 4 files changed, 6 insertions(+), 18 deletions(-) diff --git a/src/coreclr/nativeaot/Runtime/EHHelpers.cpp b/src/coreclr/nativeaot/Runtime/EHHelpers.cpp index 5b9454f8adea4..98cc37d5ecac5 100644 --- a/src/coreclr/nativeaot/Runtime/EHHelpers.cpp +++ b/src/coreclr/nativeaot/Runtime/EHHelpers.cpp @@ -326,11 +326,6 @@ EXTERN_C CODE_LOCATION RhpCheckedAssignRefESIAVLocation; EXTERN_C CODE_LOCATION RhpCheckedAssignRefEDIAVLocation; EXTERN_C CODE_LOCATION RhpCheckedAssignRefEBPAVLocation; #endif -#if !defined(HOST_AMD64) && !defined(HOST_ARM64) -EXTERN_C CODE_LOCATION RhpLockCmpXchg8AVLocation; -EXTERN_C CODE_LOCATION RhpLockCmpXchg16AVLocation; -EXTERN_C CODE_LOCATION RhpLockCmpXchg32AVLocation; -#endif EXTERN_C CODE_LOCATION RhpByRefAssignRefAVLocation1; #if !defined(HOST_ARM64) diff --git a/src/coreclr/nativeaot/Runtime/arm/Interlocked.S b/src/coreclr/nativeaot/Runtime/arm/Interlocked.S index 10c3d3f120254..718845c897606 100644 --- a/src/coreclr/nativeaot/Runtime/arm/Interlocked.S +++ b/src/coreclr/nativeaot/Runtime/arm/Interlocked.S @@ -7,15 +7,11 @@ #include // generated by the build from AsmOffsets.cpp #include -// WARNING: Code in EHHelpers.cpp makes assumptions about this helper, in particular: -// - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpLockCmpXchg8AVLocation -// - Function "UnwindSimpleHelperToCaller" assumes no registers were pushed and LR contains the return address // r0 = destination address // r1 = value // r2 = comparand LEAF_ENTRY RhpLockCmpXchg8, _TEXT dmb -GLOBAL_LABEL RhpLockCmpXchg8AVLocation LOCAL_LABEL(CmpXchg8Retry): ldrexb r3, [r0] cmp r2, r3 @@ -29,16 +25,12 @@ LOCAL_LABEL(CmpXchg8Exit): bx lr LEAF_END RhpLockCmpXchg8, _TEXT -// WARNING: Code in EHHelpers.cpp makes assumptions about this helper, in particular: -// - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpLockCmpXchg16AVLocation -// - Function "UnwindSimpleHelperToCaller" assumes no registers were pushed and LR contains the return address // r0 = destination address // r1 = value // r2 = comparand LEAF_ENTRY RhpLockCmpXchg16, _TEXT uxth r2, r2 dmb -GLOBAL_LABEL RhpLockCmpXchg16AVLocation LOCAL_LABEL(CmpXchg16Retry): ldrexh r3, [r0] cmp r2, r3 @@ -52,15 +44,11 @@ LOCAL_LABEL(CmpXchg16Exit): bx lr LEAF_END RhpLockCmpXchg16, _TEXT -// WARNING: Code in EHHelpers.cpp makes assumptions about this helper, in particular: -// - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpLockCmpXchg32AVLocation -// - Function "UnwindSimpleHelperToCaller" assumes no registers were pushed and LR contains the return address // r0 = destination address // r1 = value // r2 = comparand LEAF_ENTRY RhpLockCmpXchg32, _TEXT dmb -GLOBAL_LABEL RhpLockCmpXchg32AVLocation LOCAL_LABEL(CmpXchg32Retry): ldrex r3, [r0] cmp r2, r3 diff --git a/src/coreclr/nativeaot/Runtime/portable.cpp b/src/coreclr/nativeaot/Runtime/portable.cpp index 28fb100b2fd9c..f66a2690d4d2d 100644 --- a/src/coreclr/nativeaot/Runtime/portable.cpp +++ b/src/coreclr/nativeaot/Runtime/portable.cpp @@ -405,7 +405,6 @@ FCIMPLEND FCIMPL3(int32_t, RhpLockCmpXchg32, int32_t * location, int32_t value, int32_t comparand) { - // @TODO: USE_PORTABLE_HELPERS - Null check return PalInterlockedCompareExchange(location, value, comparand); } FCIMPLEND diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs index 35b2ebbbc3223..6f3777345071b 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs @@ -17,6 +17,8 @@ public static byte CompareExchange(ref byte location1, byte value, byte comparan #if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 return CompareExchange(ref location1, value, comparand); // Must expand intrinsic #else + if (Unsafe.IsNullRef(ref location1)) + ThrowHelper.ThrowNullReferenceException(); return RuntimeImports.InterlockedCompareExchange(ref location1, value, comparand); #endif } @@ -27,6 +29,8 @@ public static short CompareExchange(ref short location1, short value, short comp #if TARGET_X86 || TARGET_AMD64 || TARGET_ARM64 return CompareExchange(ref location1, value, comparand); // Must expand intrinsic #else + if (Unsafe.IsNullRef(ref location1)) + ThrowHelper.ThrowNullReferenceException(); return RuntimeImports.InterlockedCompareExchange(ref location1, value, comparand); #endif } @@ -37,6 +41,8 @@ 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 RuntimeImports.InterlockedCompareExchange(ref location1, value, comparand); #endif } From 1cb0367f89b6c718e8bb117c2eb27f85419bb901 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Wed, 20 Mar 2024 17:33:46 +0100 Subject: [PATCH 5/9] Fix build --- src/coreclr/nativeaot/Runtime/EHHelpers.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/coreclr/nativeaot/Runtime/EHHelpers.cpp b/src/coreclr/nativeaot/Runtime/EHHelpers.cpp index 98cc37d5ecac5..325128c4e01fc 100644 --- a/src/coreclr/nativeaot/Runtime/EHHelpers.cpp +++ b/src/coreclr/nativeaot/Runtime/EHHelpers.cpp @@ -359,11 +359,6 @@ static bool InWriteBarrierHelper(uintptr_t faultingIP) (uintptr_t)&RhpCheckedAssignRefESIAVLocation, (uintptr_t)&RhpCheckedAssignRefEDIAVLocation, (uintptr_t)&RhpCheckedAssignRefEBPAVLocation, -#endif -#if defined(HOST_ARM) - (uintptr_t)&RhpLockCmpXchg8AVLocation, - (uintptr_t)&RhpLockCmpXchg16AVLocation, - (uintptr_t)&RhpLockCmpXchg32AVLocation, #endif (uintptr_t)&RhpByRefAssignRefAVLocation1, #if !defined(HOST_ARM64) From a4146518f0d17f6b45d66d1fd7b13fdc5cec77f3 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Thu, 21 Mar 2024 09:18:24 +0100 Subject: [PATCH 6/9] Add PalInterlockedOperationBarrier --- .../nativeaot/Runtime/unix/PalRedhawkInline.h | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/coreclr/nativeaot/Runtime/unix/PalRedhawkInline.h b/src/coreclr/nativeaot/Runtime/unix/PalRedhawkInline.h index 51c3a85727183..a7b18e0a21024 100644 --- a/src/coreclr/nativeaot/Runtime/unix/PalRedhawkInline.h +++ b/src/coreclr/nativeaot/Runtime/unix/PalRedhawkInline.h @@ -5,28 +5,51 @@ #include +FORCEINLINE void PalInterlockedOperationBarrier() +{ +#if (defined(HOST_ARM64) && !defined(LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT)) || defined(HOST_ARM) || defined(HOST_LOONGARCH64) || defined(HOST_RISCV64) + // On arm64, most of the __sync* functions generate a code sequence like: + // loop: + // ldaxr (load acquire exclusive) + // ... + // stlxr (store release exclusive) + // cbnz loop + // + // It is possible for a load following the code sequence above to be reordered to occur prior to the store above due to the + // release barrier, this is substantiated by https://github.com/dotnet/coreclr/pull/17508. Interlocked operations in the PAL + // require the load to occur after the store. This memory barrier should be used following a call to a __sync* function to + // prevent that reordering. Code generated for arm32 includes a 'dmb' after 'cbnz', so no issue there at the moment. + __sync_synchronize(); +#endif +} + FORCEINLINE int32_t PalInterlockedIncrement(_Inout_ int32_t volatile *pDst) { + PalInterlockedOperationBarrier(); return __sync_add_and_fetch(pDst, 1); } FORCEINLINE int32_t PalInterlockedDecrement(_Inout_ int32_t volatile *pDst) { + PalInterlockedOperationBarrier(); return __sync_sub_and_fetch(pDst, 1); } FORCEINLINE uint32_t PalInterlockedOr(_Inout_ uint32_t volatile *pDst, uint32_t iValue) { + PalInterlockedOperationBarrier(); return __sync_or_and_fetch(pDst, iValue); } FORCEINLINE uint32_t PalInterlockedAnd(_Inout_ uint32_t volatile *pDst, uint32_t iValue) { + PalInterlockedOperationBarrier(); return __sync_and_and_fetch(pDst, iValue); } FORCEINLINE int32_t PalInterlockedExchange(_Inout_ int32_t volatile *pDst, int32_t iValue) { + PalInterlockedOperationBarrier(); #ifdef __clang__ return __sync_swap(pDst, iValue); #else @@ -36,6 +59,7 @@ FORCEINLINE int32_t PalInterlockedExchange(_Inout_ int32_t volatile *pDst, int32 FORCEINLINE int64_t PalInterlockedExchange64(_Inout_ int64_t volatile *pDst, int64_t iValue) { + PalInterlockedOperationBarrier(); #ifdef __clang__ return __sync_swap(pDst, iValue); #else @@ -45,17 +69,20 @@ FORCEINLINE int64_t PalInterlockedExchange64(_Inout_ int64_t volatile *pDst, int FORCEINLINE int32_t PalInterlockedCompareExchange(_Inout_ int32_t volatile *pDst, int32_t iValue, int32_t iComparand) { + PalInterlockedOperationBarrier(); return __sync_val_compare_and_swap(pDst, iComparand, iValue); } FORCEINLINE int64_t PalInterlockedCompareExchange64(_Inout_ int64_t volatile *pDst, int64_t iValue, int64_t iComparand) { + PalInterlockedOperationBarrier(); return __sync_val_compare_and_swap(pDst, iComparand, iValue); } #if defined(HOST_AMD64) || defined(HOST_ARM64) FORCEINLINE uint8_t PalInterlockedCompareExchange128(_Inout_ int64_t volatile *pDst, int64_t iValueHigh, int64_t iValueLow, int64_t *pComparandAndResult) { + PalInterlockedOperationBarrier(); __int128_t iComparand = ((__int128_t)pComparandAndResult[1] << 64) + (uint64_t)pComparandAndResult[0]; __int128_t iResult = __sync_val_compare_and_swap((__int128_t volatile*)pDst, iComparand, ((__int128_t)iValueHigh << 64) + (uint64_t)iValueLow); pComparandAndResult[0] = (int64_t)iResult; pComparandAndResult[1] = (int64_t)(iResult >> 64); From 6a96d20862a5cf08fe13e3bb260ffac82c975bcb Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Thu, 21 Mar 2024 22:39:47 +0100 Subject: [PATCH 7/9] PR feedback --- .../nativeaot/Runtime/unix/PalRedhawkInline.h | 36 +++++++++++-------- src/coreclr/pal/inc/pal.h | 2 +- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/src/coreclr/nativeaot/Runtime/unix/PalRedhawkInline.h b/src/coreclr/nativeaot/Runtime/unix/PalRedhawkInline.h index a7b18e0a21024..983f17a36aba0 100644 --- a/src/coreclr/nativeaot/Runtime/unix/PalRedhawkInline.h +++ b/src/coreclr/nativeaot/Runtime/unix/PalRedhawkInline.h @@ -7,7 +7,7 @@ FORCEINLINE void PalInterlockedOperationBarrier() { -#if (defined(HOST_ARM64) && !defined(LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT)) || defined(HOST_ARM) || defined(HOST_LOONGARCH64) || defined(HOST_RISCV64) +#if (defined(HOST_ARM64) && !defined(LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT) && !defined(__clang__)) || defined(HOST_LOONGARCH64) || defined(HOST_RISCV64) // On arm64, most of the __sync* functions generate a code sequence like: // loop: // ldaxr (load acquire exclusive) @@ -25,66 +25,74 @@ FORCEINLINE void PalInterlockedOperationBarrier() FORCEINLINE int32_t PalInterlockedIncrement(_Inout_ int32_t volatile *pDst) { + int32_t result = __sync_add_and_fetch(pDst, 1); PalInterlockedOperationBarrier(); - return __sync_add_and_fetch(pDst, 1); + return result; } FORCEINLINE int32_t PalInterlockedDecrement(_Inout_ int32_t volatile *pDst) { + int32_t result = __sync_sub_and_fetch(pDst, 1); PalInterlockedOperationBarrier(); - return __sync_sub_and_fetch(pDst, 1); + return result; } FORCEINLINE uint32_t PalInterlockedOr(_Inout_ uint32_t volatile *pDst, uint32_t iValue) { + int32_t result = __sync_or_and_fetch(pDst, iValue); PalInterlockedOperationBarrier(); - return __sync_or_and_fetch(pDst, iValue); + return result; } FORCEINLINE uint32_t PalInterlockedAnd(_Inout_ uint32_t volatile *pDst, uint32_t iValue) { + int32_t result = __sync_and_and_fetch(pDst, iValue); PalInterlockedOperationBarrier(); - return __sync_and_and_fetch(pDst, iValue); + return result; } FORCEINLINE int32_t PalInterlockedExchange(_Inout_ int32_t volatile *pDst, int32_t iValue) { - PalInterlockedOperationBarrier(); #ifdef __clang__ - return __sync_swap(pDst, iValue); + int32_t result =__sync_swap(pDst, iValue); #else - return __atomic_exchange_n(pDst, iValue, __ATOMIC_ACQ_REL); + int32_t result =__atomic_exchange_n(pDst, iValue, __ATOMIC_ACQ_REL); #endif + PalInterlockedOperationBarrier(); + return result; } FORCEINLINE int64_t PalInterlockedExchange64(_Inout_ int64_t volatile *pDst, int64_t iValue) { - PalInterlockedOperationBarrier(); #ifdef __clang__ - return __sync_swap(pDst, iValue); + int32_t result =__sync_swap(pDst, iValue); #else - return __atomic_exchange_n(pDst, iValue, __ATOMIC_ACQ_REL); + int32_t result =__atomic_exchange_n(pDst, iValue, __ATOMIC_ACQ_REL); #endif + PalInterlockedOperationBarrier(); + return result; } FORCEINLINE int32_t PalInterlockedCompareExchange(_Inout_ int32_t volatile *pDst, int32_t iValue, int32_t iComparand) { + int32_t result = __sync_val_compare_and_swap(pDst, iComparand, iValue); PalInterlockedOperationBarrier(); - return __sync_val_compare_and_swap(pDst, iComparand, iValue); + return result; } FORCEINLINE int64_t PalInterlockedCompareExchange64(_Inout_ int64_t volatile *pDst, int64_t iValue, int64_t iComparand) { + int64_t result = __sync_val_compare_and_swap(pDst, iComparand, iValue); PalInterlockedOperationBarrier(); - return __sync_val_compare_and_swap(pDst, iComparand, iValue); + return result; } #if defined(HOST_AMD64) || defined(HOST_ARM64) FORCEINLINE uint8_t PalInterlockedCompareExchange128(_Inout_ int64_t volatile *pDst, int64_t iValueHigh, int64_t iValueLow, int64_t *pComparandAndResult) { - PalInterlockedOperationBarrier(); __int128_t iComparand = ((__int128_t)pComparandAndResult[1] << 64) + (uint64_t)pComparandAndResult[0]; __int128_t iResult = __sync_val_compare_and_swap((__int128_t volatile*)pDst, iComparand, ((__int128_t)iValueHigh << 64) + (uint64_t)iValueLow); + PalInterlockedOperationBarrier(); pComparandAndResult[0] = (int64_t)iResult; pComparandAndResult[1] = (int64_t)(iResult >> 64); return iComparand == iResult; } diff --git a/src/coreclr/pal/inc/pal.h b/src/coreclr/pal/inc/pal.h index 4a0a341b2272e..4ad1301f212cf 100644 --- a/src/coreclr/pal/inc/pal.h +++ b/src/coreclr/pal/inc/pal.h @@ -3448,7 +3448,7 @@ BitScanReverse64( FORCEINLINE void PAL_InterlockedOperationBarrier() { -#if defined(HOST_ARM64) || defined(HOST_LOONGARCH64) || defined(HOST_RISCV64) +#if (defined(HOST_ARM64) && !defined(__clang__)) || defined(HOST_LOONGARCH64) || defined(HOST_RISCV64) // On arm64, most of the __sync* functions generate a code sequence like: // loop: // ldaxr (load acquire exclusive) From cd2d77d67ac3b4fd5e7aed0c9f26902d87efeaa4 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Thu, 21 Mar 2024 22:40:59 +0100 Subject: [PATCH 8/9] PR feedback --- src/coreclr/pal/inc/pal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/pal/inc/pal.h b/src/coreclr/pal/inc/pal.h index 4ad1301f212cf..1a6c577da8cc2 100644 --- a/src/coreclr/pal/inc/pal.h +++ b/src/coreclr/pal/inc/pal.h @@ -3448,7 +3448,7 @@ BitScanReverse64( FORCEINLINE void PAL_InterlockedOperationBarrier() { -#if (defined(HOST_ARM64) && !defined(__clang__)) || defined(HOST_LOONGARCH64) || defined(HOST_RISCV64) +#if (defined(HOST_ARM64) && !defined(LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT) && !defined(__clang__)) || defined(HOST_LOONGARCH64) || defined(HOST_RISCV64) // On arm64, most of the __sync* functions generate a code sequence like: // loop: // ldaxr (load acquire exclusive) From 78b925817bf70e4f1f3ff118bbdbaa9daae59486 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Fri, 22 Mar 2024 04:49:56 +0100 Subject: [PATCH 9/9] Move RhpLockCmpXchg32 to C --- src/coreclr/nativeaot/Runtime/CMakeLists.txt | 6 ----- src/coreclr/nativeaot/Runtime/MiscHelpers.cpp | 6 +++++ .../nativeaot/Runtime/arm/Interlocked.S | 26 ------------------- src/coreclr/nativeaot/Runtime/portable.cpp | 6 ----- 4 files changed, 6 insertions(+), 38 deletions(-) delete mode 100644 src/coreclr/nativeaot/Runtime/arm/Interlocked.S diff --git a/src/coreclr/nativeaot/Runtime/CMakeLists.txt b/src/coreclr/nativeaot/Runtime/CMakeLists.txt index bbed14b74861e..2d163ea27d78b 100644 --- a/src/coreclr/nativeaot/Runtime/CMakeLists.txt +++ b/src/coreclr/nativeaot/Runtime/CMakeLists.txt @@ -203,12 +203,6 @@ if (CLR_CMAKE_TARGET_ARCH_AMD64 AND CLR_CMAKE_TARGET_WIN32) ) endif (CLR_CMAKE_TARGET_ARCH_AMD64 AND CLR_CMAKE_TARGET_WIN32) -if (CLR_CMAKE_TARGET_ARCH_ARM) - list(APPEND RUNTIME_SOURCES_ARCH_ASM - ${ARCH_SOURCES_DIR}/Interlocked.${ASM_SUFFIX} - ) -endif (CLR_CMAKE_TARGET_ARCH_ARM) - list(APPEND RUNTIME_SOURCES_ARCH_ASM ${ARCH_SOURCES_DIR}/AllocFast.${ASM_SUFFIX} ${ARCH_SOURCES_DIR}/ExceptionHandling.${ASM_SUFFIX} diff --git a/src/coreclr/nativeaot/Runtime/MiscHelpers.cpp b/src/coreclr/nativeaot/Runtime/MiscHelpers.cpp index 3c42953f4f772..d6c734bc0118a 100644 --- a/src/coreclr/nativeaot/Runtime/MiscHelpers.cpp +++ b/src/coreclr/nativeaot/Runtime/MiscHelpers.cpp @@ -417,6 +417,12 @@ EXTERN_C void QCALLTYPE RhCpuIdEx(int* cpuInfo, int functionId, int subFunctionI } #endif +FCIMPL3(int32_t, RhpLockCmpXchg32, int32_t * location, int32_t value, int32_t comparand) +{ + return PalInterlockedCompareExchange(location, value, comparand); +} +FCIMPLEND + FCIMPL3(int64_t, RhpLockCmpXchg64, int64_t * location, int64_t value, int64_t comparand) { return PalInterlockedCompareExchange64(location, value, comparand); diff --git a/src/coreclr/nativeaot/Runtime/arm/Interlocked.S b/src/coreclr/nativeaot/Runtime/arm/Interlocked.S deleted file mode 100644 index 366807b7348de..0000000000000 --- a/src/coreclr/nativeaot/Runtime/arm/Interlocked.S +++ /dev/null @@ -1,26 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -.syntax unified -.thumb - -#include // generated by the build from AsmOffsets.cpp -#include - -// r0 = destination address -// r1 = value -// r2 = comparand -LEAF_ENTRY RhpLockCmpXchg32, _TEXT - dmb -LOCAL_LABEL(CmpXchg32Retry): - ldrex r3, [r0] - cmp r2, r3 - bne LOCAL_LABEL(CmpXchg32Exit) - strex r12, r1, [r0] - cmp r12, #0 - bne LOCAL_LABEL(CmpXchg32Retry) -LOCAL_LABEL(CmpXchg32Exit): - mov r0, r3 - dmb - bx lr -LEAF_END RhpLockCmpXchg32, _TEXT diff --git a/src/coreclr/nativeaot/Runtime/portable.cpp b/src/coreclr/nativeaot/Runtime/portable.cpp index 2a3abb582123d..bcd99d051198d 100644 --- a/src/coreclr/nativeaot/Runtime/portable.cpp +++ b/src/coreclr/nativeaot/Runtime/portable.cpp @@ -389,12 +389,6 @@ FCIMPL2(Object *, RhpCheckedXchg, Object ** location, Object * value) } FCIMPLEND -FCIMPL3(int32_t, RhpLockCmpXchg32, int32_t * location, int32_t value, int32_t comparand) -{ - return PalInterlockedCompareExchange(location, value, comparand); -} -FCIMPLEND - FCIMPL0(void*, RhAllocateThunksMapping) { return NULL;