Open SquallATF opened 2 years ago
See also freebsd/freebsd-src@8ae4c8aadfbd1ec3484dcd06336417c77ab1d2e0 (which uses a 32-bit type for __cxx_contention_t
) and freebsd/freebsd-src@0e426a4a346876e61adc0b3981a91fe1e039c1ee (which applies the abandoned review https://reviews.llvm.org/D75183.
Something similar could also be done for other achitectures, though the big #ifdef
might better be moved somewhere else.
@DimitryAndric Any appetite for upstreaming these FreeBSD fixes?
Hmm now that I'm looking at this again, I wonder why @ogiroux added the #ifdef
for __cxx_contention_t
at all... Oliver, any idea if this could possibly always be an int32_t
? What is the gain of using a 64-bit integer specifically?
In FreeBSD's version, Adrian Chadd added another #if
case specifically for 32-bit mips (freebsd/freebsd-src@d061adc48d54ebb91b1709b16c59e8ceca895be2), and I added a bunch of others to help with 32-bit arm pre-v5 which doesn't have 64-bit atomics.
If we do need 64 bits if at all possible, it might be better to check for __LP64__
or something like that instead. Or check if ATOMIC_LLONG_LOCK_FREE == 2
in combination with sizeof(long long) == sizeof(int64_t)
, and in that case use int64_t
, otherwise always use int32_t
? That should work generically for all platforms.
Is this still an issue?
Is this still an issue?
This dropped off my radar, but in FreeBSD we're still carrying this patch on top of libc++. So I should get to work again and submit a review for libc++. :)
So another form of https://reviews.llvm.org/D75183 was landed in c04bcadf30ee07d437a238f78c2fb2c90be5c40c, and the only diff regarding atomic we have left in FreeBSD is now (effectively):
--- a/libcxx/include/__atomic/contention_t.h
+++ b/libcxx/include/__atomic/contention_t.h
@@ -19,11 +19,11 @@
_LIBCPP_BEGIN_NAMESPACE_STD
-#if defined(__linux__) || (defined(_AIX) && !defined(__64BIT__))
+#if defined(__linux__) || (defined(_AIX) && !defined(__64BIT__)) || (defined(__FreeBSD__) && defined(__mips__))
using __cxx_contention_t = int32_t;
#else
using __cxx_contention_t = int64_t;
-#endif // __linux__ || (_AIX && !__64BIT__)
+#endif // __linux__ || (_AIX && !__64BIT__) || (__FreeBSD__ && __mips__)
using __cxx_atomic_contention_t = __cxx_atomic_impl<__cxx_contention_t>;
Since we have already dropped mips support in FreeBSD 13, and have no further plans for it, I think I will leave it at this. @emaste do you agree?
Another solution might be to use the existing __cxx_atomic_is_lock_free()
macro:
--- a/libcxx/include/__atomic/contention_t.h
+++ b/libcxx/include/__atomic/contention_t.h
@@ -19,11 +19,11 @@
_LIBCPP_BEGIN_NAMESPACE_STD
-#if defined(__linux__) || (defined(_AIX) && !defined(__64BIT__))
+#if defined(__linux__) || (defined(_AIX) && !defined(__64BIT__)) || !__cxx_atomic_is_lock_free(sizeof(int64_t))
using __cxx_contention_t = int32_t;
#else
using __cxx_contention_t = int64_t;
-#endif // __linux__ || (_AIX && !__64BIT__)
+#endif // __linux__ || (_AIX && !__64BIT__) || !is_lock_free(int64_t)
using __cxx_atomic_contention_t = __cxx_atomic_impl<__cxx_contention_t>;
But I don't really have enough time to write tests for this.
this commit 92832e4889ae6038cc8b3e6e449af2a9b9374ab4 enable \<atomic> when threads are disable. but armv6m target does not have any signed/unsigned lock-free types
https://github.com/llvm/llvm-project/blob/3042091168e99323217223a959df8675bdfa9b3c/libcxx/include/atomic#L1079-L1107 https://github.com/llvm/llvm-project/blob/3042091168e99323217223a959df8675bdfa9b3c/libcxx/include/atomic#L2680-L2698
libcxx build log