Note regarding thenew-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Tagging subscribers to this area:@mangod9
See info inarea-owners.md if you want to be subscribed.

@jkotas I've realised there's a potential issue with atomics as FCalls: some compilers might implement them with locking on a mutex which AFAIR is not okay inside of an FCall. Checking withCLang on godbolt makes it seem to be the case on arm32.

These are results forstdatomic.h header. We do not usestdatomic.h. We use the compiler intrinsics directly. You may want to step through the disassembly on arm32 to see what the compiler intrinsics end up doing.

I've checked that earlier, it calls into a__atomic_exchange_1/2/4/8 helper (which means it probably implicitly linkslibatomic there). It does a similar thing for arm 8.0, just with__aarch64_swp1_acq_rel (which is fine according to that define).

Additionally, on arm32 it gives such warning:

<source>:5:12: warning: large atomic operation may incur significant performance penalty; the access size (1 bytes) exceeds the max lock-free size (0  bytes) [-Watomic-alignment]    5 |     return __atomic_exchange_n(ptr, val, __ATOMIC_ACQ_REL);<source>:9:12: warning: large atomic operation may incur significant performance penalty; the access size (2 bytes) exceeds the max lock-free size (0  bytes) [-Watomic-alignment]    9 |     return __atomic_exchange_n(ptr, val, __ATOMIC_ACQ_REL);<source>:13:12: warning: large atomic operation may incur significant performance penalty; the access size (4 bytes) exceeds the max lock-free size (0  bytes) [-Watomic-alignment]   13 |     return __atomic_exchange_n(ptr, val, __ATOMIC_ACQ_REL);<source>:17:12: warning: large atomic operation may incur significant performance penalty; the access size (8 bytes) exceeds the max lock-free size (0  bytes) [-Watomic-alignment]   17 |     return __atomic_exchange_n(ptr, val, __ATOMIC_ACQ_REL);

It is a problem with__atomic_exchange_n that is used for InterlockedExchange.__atomic_exchange_n is also the one that caused troubles on Linux x86 before.

Other operations are implemented using__sync_val_... intrinsics. Do__sync_val_... intrinsics have the same problem?

If__sync_val_... intrinsics are fine:

• Use__sync_swap to implement InterlockedExchange when compiling using LLVM. We do that here already:

  runtime/src/coreclr/nativeaot/Runtime/unix/PalRedhawkInline.h

  Lines 30 to 34 ind3583b8

  	#ifdef__clang__
  	return__sync_swap(pDst,iValue);
  	#else
  	return__atomic_exchange_n(pDst,iValue,__ATOMIC_ACQ_REL);
  	#endif

• Implement InterlockedExchange using__sync_val_compare_and_swap when not compiling using LLVM. Similar to what I have done to fix the Linux x86 break:

  runtime/src/coreclr/pal/inc/pal.h

  Lines 3645 to 3661 ind3583b8

  	#if defined(HOST_X86)
  	// 64-bit __atomic_exchange_n is not expanded as a compiler intrinsic on Linux x86.
  	// Use inline implementation instead.
  	inline LONGLONGInterlockedExchange64(LONGLONGvolatile * Target, LONGLONG Value)
  	{
  	LONGLONG Old;
  	do {
  	Old = *Target;
  	}while (__sync_val_compare_and_swap(Target, Old, Value) != Old);
  	return Old;
  	}
  	#else

I've opened a new issue to track this as this PR doesn't introduce the issue:#97452.

ARM32 failure seems like an unrelated JIT bug withlongs.
Turns out a small bug in the helpers somehow trashed the stack.

I've opened a new issue to track this as this PR doesn't introduce the issue:#97452.

It does not introduce it, but it makes the problem worse. Could you please replace the new uses of__atomic_exchange_n with what I have suggested? It should address the issue. (Bonus points for fixing the existing uses too.)

I've opened a new issue to track this as this PR doesn't introduce the issue:#97452.

It does not introduce it, but it makes the problem worse. Could you please replace the new uses of__atomic_exchange_n with what I have suggested? It should address the issue. (Bonus points for fixing the existing uses too.)

As I've noted in that issue, those other APIs also use helper calls so I'd assume they have the same issue.

@MihuBot

@jkotas I think the arm32 atomic helper issue should be fixed in a separate PR (I can take a look at using the safe LLVM helpers later today) so this should be ready to merge.

Turns out a small bug in the helpers somehow trashed the stack.

What was this bug?

/azp run runtime-coreclr outerloop, runtime-nativeaot-outerloop

Turns out a small bug in the helpers somehow trashed the stack.

What was this bug?

A missing cast toUINT8 fromCHAR which was returned byInterlockedExchange8 which somehow overrode half of a long on stack with 0.