Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NativeAOT] Move Interlocked null checks to managed, RhpLockCmpXchg64 to C #100021

Merged
merged 10 commits into from
Mar 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions src/coreclr/nativeaot/Runtime/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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 (NOT (CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_ARM64))
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))

list(APPEND RUNTIME_SOURCES_ARCH_ASM
${ARCH_SOURCES_DIR}/AllocFast.${ASM_SUFFIX}
${ARCH_SOURCES_DIR}/ExceptionHandling.${ASM_SUFFIX}
Expand Down
18 changes: 0 additions & 18 deletions src/coreclr/nativeaot/Runtime/EHHelpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -326,12 +326,6 @@ 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 RhpLockCmpXchg32AVLocation;
EXTERN_C CODE_LOCATION RhpLockCmpXchg64AVLocation;
#endif
EXTERN_C CODE_LOCATION RhpByRefAssignRefAVLocation1;

#if !defined(HOST_ARM64)
Expand Down Expand Up @@ -365,22 +359,10 @@ static bool InWriteBarrierHelper(uintptr_t faultingIP)
(uintptr_t)&RhpCheckedAssignRefESIAVLocation,
(uintptr_t)&RhpCheckedAssignRefEDIAVLocation,
(uintptr_t)&RhpCheckedAssignRefEBPAVLocation,
#endif
(uintptr_t)&RhpCheckedLockCmpXchgAVLocation,
(uintptr_t)&RhpCheckedXchgAVLocation,
#if !defined(HOST_AMD64) && !defined(HOST_ARM64)
#if !defined(HOST_X86)
(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
};

Expand Down
12 changes: 12 additions & 0 deletions src/coreclr/nativeaot/Runtime/MiscHelpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -416,3 +416,15 @@ EXTERN_C void QCALLTYPE RhCpuIdEx(int* cpuInfo, int functionId, int subFunctionI
__cpuidex(cpuInfo, functionId, subFunctionId);
}
#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);
}
FCIMPLEND
8 changes: 0 additions & 8 deletions src/coreclr/nativeaot/Runtime/amd64/WriteBarriers.S
Original file line number Diff line number Diff line change
Expand Up @@ -221,28 +221,20 @@ 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)

DEFINE_CHECKED_WRITE_BARRIER_CORE RhpCheckedLockCmpXchg, RSI

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
Expand Down
8 changes: 0 additions & 8 deletions src/coreclr/nativeaot/Runtime/amd64/WriteBarriers.asm
Original file line number Diff line number Diff line change
Expand Up @@ -237,28 +237,20 @@ 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

DEFINE_CHECKED_WRITE_BARRIER_CORE RhpCheckedLockCmpXchg, RDX

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
Expand Down
57 changes: 0 additions & 57 deletions src/coreclr/nativeaot/Runtime/arm/Interlocked.S

This file was deleted.

8 changes: 0 additions & 8 deletions src/coreclr/nativeaot/Runtime/arm/WriteBarriers.S
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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]
Expand Down
4 changes: 0 additions & 4 deletions src/coreclr/nativeaot/Runtime/arm64/WriteBarriers.S
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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.
Expand Down
12 changes: 0 additions & 12 deletions src/coreclr/nativeaot/Runtime/arm64/WriteBarriers.asm
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -311,7 +307,6 @@ NotInHeap
#endif

mov x10, x2
ALTERNATE_ENTRY RhpCheckedLockCmpXchgAVLocation
casal x10, x1, [x0] ;; exchange
cmp x2, x10
bne CmpXchgNoUpdate
Expand All @@ -320,7 +315,6 @@ NotInHeap
b DoCardsCmpXchg
CmpXchgRetry
;; Check location value is what we expect.
ALTERNATE_ENTRY RhpCheckedLockCmpXchgAVLocation2
ldaxr x10, [x0]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RhpCheckedLockCmpXchgAVLocation2 and RhpCheckedXchgAVLocation2 checks are weird. They seem to be present for the same location that was already null checked and unnecessary.

cmp x10, x2
bne CmpXchgNoUpdate
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand Down
4 changes: 0 additions & 4 deletions src/coreclr/nativeaot/Runtime/i386/Interlocked.S

This file was deleted.

35 changes: 0 additions & 35 deletions src/coreclr/nativeaot/Runtime/i386/Interlocked.asm

This file was deleted.

8 changes: 0 additions & 8 deletions src/coreclr/nativeaot/Runtime/i386/WriteBarriers.asm
Original file line number Diff line number Diff line change
Expand Up @@ -237,28 +237,20 @@ 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

DEFINE_CHECKED_WRITE_BARRIER_CORE RhpCheckedLockCmpXchg, ECX, EDX, ret 4

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
Expand Down
15 changes: 0 additions & 15 deletions src/coreclr/nativeaot/Runtime/portable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -390,20 +389,6 @@ FCIMPL2(Object *, RhpCheckedXchg, Object ** location, Object * value)
}
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

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;
Expand Down
Loading
Loading