Remove the lock-based atomics fallback
On Windows, we can rely on Interlocked APIs. On non-Windows builds, we
currently require C11 but permit C11 atomics to be missing, via
__STDC_NO_ATOMICS__. This CL tightens this so C11 atomics are required
on non-MSVC builds.
My hope is that, now that we require C11 on non-Windows, this is a
fairly safe requirement. We already require pthreads on any platform
where this might apply, and it's hard to imagine someone has C11,
pthreads, but not C11 atomics.
This change means that, in later work, we can refactor the refcount
logic to instead be a <stdatomic.h> compatibility layer, and then an
atomics-targetting CRYPTO_refcount_t implementation. With a
<stdatomic.h> compatibility layer, we can use atomics in more places,
notably where our uses of read locks are causing cacheline contention.
The platform restriction isn't *strictly* necessary. We could, like with
refcounts, emulate <stdatomic.h> with a single, global lock. Indeed any
platforms in this situation have already been living with that lock for
refcounts without noticing. But then later work to add "atomics" to read
locks would regress contention for those platforms. So I'm starting by
rejecting this, so if any such platform exists, we can understand their
performance needs before doing that.
Update-Note: On non-Windows platforms, we now require C11 atomics
support. Note we already require C11 itself. If this affects your build,
get in touch with BoringSSL maintainers.
Bug: 570
Cq-Include-Trybots: luci.boringssl.try:linux_clang_rel_tsan
Change-Id: I868fa4ba87ed73dfc9d52e80d46853ef56715a5f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59847
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/CMakeLists.txt b/crypto/CMakeLists.txt
index 12d15a8..c427b20 100644
--- a/crypto/CMakeLists.txt
+++ b/crypto/CMakeLists.txt
@@ -203,7 +203,7 @@
rand_extra/windows.c
rc4/rc4.c
refcount_c11.c
- refcount_lock.c
+ refcount_no_threads.c
refcount_win.c
rsa_extra/rsa_asn1.c
rsa_extra/rsa_crypt.c
diff --git a/crypto/internal.h b/crypto/internal.h
index adcd444..5c04735 100644
--- a/crypto/internal.h
+++ b/crypto/internal.h
@@ -556,6 +556,16 @@
#define OPENSSL_WINDOWS_ATOMIC
#endif
+// Require some atomics implementation. Contact BoringSSL maintainers if you
+// have a platform with fails this check.
+//
+// Note this check can only be done in C. From C++, we don't know whether the
+// corresponding C mode would support C11 atomics.
+#if !defined(__cplusplus) && defined(OPENSSL_THREADS) && \
+ !defined(OPENSSL_C11_ATOMIC) && !defined(OPENSSL_WINDOWS_ATOMIC)
+#error "Thread-compatible configurations require atomics"
+#endif
+
// CRYPTO_REFCOUNT_MAX is the value at which the reference count saturates.
#define CRYPTO_REFCOUNT_MAX 0xffffffff
diff --git a/crypto/refcount_lock.c b/crypto/refcount_no_threads.c
similarity index 72%
rename from crypto/refcount_lock.c
rename to crypto/refcount_no_threads.c
index 7886bf8..096b4fa 100644
--- a/crypto/refcount_lock.c
+++ b/crypto/refcount_no_threads.c
@@ -18,35 +18,25 @@
#include <stdlib.h>
-#if !defined(OPENSSL_C11_ATOMIC) && !defined(OPENSSL_WINDOWS_ATOMIC)
+#if !defined(OPENSSL_THREADS)
static_assert((CRYPTO_refcount_t)-1 == CRYPTO_REFCOUNT_MAX,
"CRYPTO_REFCOUNT_MAX is incorrect");
-static struct CRYPTO_STATIC_MUTEX g_refcount_lock = CRYPTO_STATIC_MUTEX_INIT;
-
void CRYPTO_refcount_inc(CRYPTO_refcount_t *count) {
- CRYPTO_STATIC_MUTEX_lock_write(&g_refcount_lock);
if (*count < CRYPTO_REFCOUNT_MAX) {
(*count)++;
}
- CRYPTO_STATIC_MUTEX_unlock_write(&g_refcount_lock);
}
int CRYPTO_refcount_dec_and_test_zero(CRYPTO_refcount_t *count) {
- int ret;
-
- CRYPTO_STATIC_MUTEX_lock_write(&g_refcount_lock);
if (*count == 0) {
abort();
}
if (*count < CRYPTO_REFCOUNT_MAX) {
(*count)--;
}
- ret = (*count == 0);
- CRYPTO_STATIC_MUTEX_unlock_write(&g_refcount_lock);
-
- return ret;
+ return *count == 0;
}
-#endif // !OPENSSL_C11_ATOMIC && !OPENSSL_WINDOWS_ATOMICS
+#endif // !OPENSSL_THREADS
