-
Notifications
You must be signed in to change notification settings - Fork 1.1k
atomic.h: ARM guards are broken for mi_atomic_yield #1046
Description
PR #339 added an ARM version check (__ARM_ARCH__ >= 7) to guard __asm__ volatile("yield" ::: "memory");
However, GCC prefers to use __ARM_ARCH per the ACLE https://arm-software.github.io/acle/main/acle.html#mapping-of-object-build-attributes-to-predefines
armeb-buildroot-linux-gnueabi-gcc -dM -E - < /dev/null | grep -i arm
#define __ARM_SIZEOF_WCHAR_T 4
#define __ARM_FEATURE_SAT 1
#define __ARM_ARCH_ISA_ARM 1
#define __ARM_FEATURE_IDIV 1
#define __ARM_SIZEOF_MINIMAL_ENUM 4
#define __ARMEB__ 1
#define __ARM_FEATURE_LDREX 15
#define __ARM_PCS 1
#define __ARM_FEATURE_QBIT 1
#define __ARM_ARCH_PROFILE 65
#define __ARM_32BIT_STATE 1
#define __ARM_FEATURE_CLZ 1
#define __ARM_ARCH_ISA_THUMB 2
#define __ARM_ARCH 7
#define __ARM_BIG_ENDIAN 1
#define __ARM_FEATURE_UNALIGNED 1
#define __arm__ 1
#define __ARM_ARCH_7A__ 1
#define __ARM_FEATURE_SIMD32 1
#define __ARM_FEATURE_COPROC 15
#define __ARM_FEATURE_DSP 1
#define __ARM_ARCH_EXT_IDIV__ 1
#define __ARM_EABI__ 1
This wasn't a huge issue because it would fall back to the following implementation (I think):
static inline void mi_atomic_yield(void) {
sleep(0);
}However, since PR #350 the mi_atomic_yield function may be undefined or use the incorrect instruction during some builds:
On ARM builds:
__arm__is likely always defined so we enter into the #ifdef- if
__ARM_ARCH__is not defined, which is probably standard on most GCCs, and it's a little-endian build, it's caught bydefined(__armel__) || defined(__ARMEL__)and has a body emitted, though if the architecture is v7+ it will not useyieldas probably intended. - If it's a big-endian architecture, no function body is emitted and the build fails.
I'm not sure there needs to be a distinction between EL and EB builds, but i'll leave that to the experts. The define can maybe be something like:
#elif defined(__arm__)
#if __ARM_ARCH >= 7
static inline void mi_atomic_yield(void) {
__asm__ volatile("yield" ::: "memory");
#else
asm volatile ("nop" ::: "memory");
#endifI don't know enough about the arm32 to say that nop ::: memory should be used over sched_yield or sleep 0.
Regardless, as-is, big endian ARM builds fail and LE builds are likely using nop even when yield is supported. This is currently affecting CPython 3.13 builds for Buildroot (https://autobuild.buildroot.org/results/26b752738022e8b46e810a08e28d687120e5c4e3/build-end.log)
I can patch our build process, but felt like I should report it here. I will likely create an issue in CPython some time tomorrow and link to this issue for the fix to be backported.