[libc++abi] Always use thread_local for cxa_exception_storage #79868
Conversation
|
@llvm/pr-subscribers-libcxxabi Author: Louis Dionne (ldionne) ChangesThis was previously guarded by HAS_THREAD_LOCAL, which was never set by CMake and had to be specified manually. Android has been setting this to solve android/ndk#1200 1, but every compiler and platform libc++abi supports should have thread_local by now, so we can just get rid of the fallback implementation and simplify things significantly (including removing the now unused fallback calloc). This is a re-application of https://reviews.llvm.org/D138461. Fixes #78207. Full diff: https://github.com/llvm/llvm-project/pull/79868.diff 3 Files Affected:
diff --git a/libcxxabi/src/cxa_exception_storage.cpp b/libcxxabi/src/cxa_exception_storage.cpp
index 3a3233a1b92722..90370c760f9843 100644
--- a/libcxxabi/src/cxa_exception_storage.cpp
+++ b/libcxxabi/src/cxa_exception_storage.cpp
@@ -12,8 +12,6 @@
#include "cxa_exception.h"
-#include <__threading_support>
-
#if defined(_LIBCXXABI_HAS_NO_THREADS)
namespace __cxxabiv1 {
@@ -24,7 +22,7 @@ extern "C" {
} // extern "C"
} // namespace __cxxabiv1
-#elif defined(HAS_THREAD_LOCAL)
+#else
namespace __cxxabiv1 {
namespace {
@@ -40,64 +38,4 @@ extern "C" {
} // extern "C"
} // namespace __cxxabiv1
-#else
-
-#include "abort_message.h"
-#include "fallback_malloc.h"
-
-#if defined(__ELF__) && defined(_LIBCXXABI_LINK_PTHREAD_LIB)
-#pragma comment(lib, "pthread")
-#endif
-
-// In general, we treat all threading errors as fatal.
-// We cannot call std::terminate() because that will in turn
-// call __cxa_get_globals() and cause infinite recursion.
-
-namespace __cxxabiv1 {
-namespace {
- std::__libcpp_tls_key key_;
- std::__libcpp_exec_once_flag flag_ = _LIBCPP_EXEC_ONCE_INITIALIZER;
-
- void _LIBCPP_TLS_DESTRUCTOR_CC destruct_(void *p) {
- __free_with_fallback(p);
- if (0 != std::__libcpp_tls_set(key_, NULL))
- abort_message("cannot zero out thread value for __cxa_get_globals()");
- }
-
- void construct_() {
- if (0 != std::__libcpp_tls_create(&key_, destruct_))
- abort_message("cannot create thread specific key for __cxa_get_globals()");
- }
-} // namespace
-
-extern "C" {
- __cxa_eh_globals *__cxa_get_globals() {
- // Try to get the globals for this thread
- __cxa_eh_globals *retVal = __cxa_get_globals_fast();
-
- // If this is the first time we've been asked for these globals, create them
- if (NULL == retVal) {
- retVal = static_cast<__cxa_eh_globals*>(
- __calloc_with_fallback(1, sizeof(__cxa_eh_globals)));
- if (NULL == retVal)
- abort_message("cannot allocate __cxa_eh_globals");
- if (0 != std::__libcpp_tls_set(key_, retVal))
- abort_message("std::__libcpp_tls_set failure in __cxa_get_globals()");
- }
- return retVal;
- }
-
- // Note that this implementation will reliably return NULL if not
- // preceded by a call to __cxa_get_globals(). This is an extension
- // to the Itanium ABI and is taken advantage of in several places in
- // libc++abi.
- __cxa_eh_globals *__cxa_get_globals_fast() {
- // First time through, create the key.
- if (0 != std::__libcpp_execute_once(&flag_, construct_))
- abort_message("execute once failure in __cxa_get_globals_fast()");
- return static_cast<__cxa_eh_globals*>(std::__libcpp_tls_get(key_));
- }
-} // extern "C"
-} // namespace __cxxabiv1
-
#endif
diff --git a/libcxxabi/src/fallback_malloc.cpp b/libcxxabi/src/fallback_malloc.cpp
index fa802b2d81a745..88594a4d720f82 100644
--- a/libcxxabi/src/fallback_malloc.cpp
+++ b/libcxxabi/src/fallback_malloc.cpp
@@ -271,17 +271,6 @@ void* __aligned_malloc_with_fallback(size_t size) {
return fallback_malloc(size);
}
-void* __calloc_with_fallback(size_t count, size_t size) {
- void* ptr = ::calloc(count, size);
- if (NULL != ptr)
- return ptr;
- // if calloc fails, fall back to emergency stash
- ptr = fallback_malloc(size * count);
- if (NULL != ptr)
- ::memset(ptr, 0, size * count);
- return ptr;
-}
-
void __aligned_free_with_fallback(void* ptr) {
if (is_fallback_ptr(ptr))
fallback_free(ptr);
diff --git a/libcxxabi/src/fallback_malloc.h b/libcxxabi/src/fallback_malloc.h
index 816e691ed23139..528014b3456d8b 100644
--- a/libcxxabi/src/fallback_malloc.h
+++ b/libcxxabi/src/fallback_malloc.h
@@ -17,9 +17,6 @@ namespace __cxxabiv1 {
// Allocate some memory from _somewhere_
_LIBCXXABI_HIDDEN void * __aligned_malloc_with_fallback(size_t size);
-// Allocate and zero-initialize memory from _somewhere_
-_LIBCXXABI_HIDDEN void * __calloc_with_fallback(size_t count, size_t size);
-
_LIBCXXABI_HIDDEN void __aligned_free_with_fallback(void *ptr);
_LIBCXXABI_HIDDEN void __free_with_fallback(void *ptr);
|
|
CC @nico since that broke Chrome last time this was landed (see the Phab review). |
ldionne
force-pushed
the
review/libcxxabi-thread-local
branch
2 times, most recently
from
b203586 to
a764f11
Compare
This was previously guarded by HAS_THREAD_LOCAL, which was never set by CMake and had to be specified manually. Android has been setting this to solve android/ndk#1200 [1], but every compiler and platform libc++abi supports should have thread_local by now, so we can just get rid of the fallback implementation and simplify things significantly (including removing the now unused fallback calloc). This is a re-application of https://reviews.llvm.org/D138461. Fixes llvm#78207. [1]: https://android-review.googlesource.com/c/toolchain/llvm-project/+/1285596 Co-Authored-By: Shoaib Meenai <[email protected]>
ldionne
force-pushed
the
review/libcxxabi-thread-local
branch
from
a764f11 to
d1b971a
Compare
|
I am going to land this if CI is green. CC @llvm/libcxx-vendors for potential impact. |
|
If I understand correctly, it's a no-op for Android, so it's good from Android's PoV. For reference: Android's LLVM uses |
@ldionne Please don't. The z/OS is does not support thread local yet and it will brake us if you land this. |
|
With this change I'm getting: |
|
z/OS doesn't fully support thread local storage yet. The current state is:
|
|
@perry-ca @zibi2 Is there a timeline for adding support for TLS? TLS is technically a required part of the spec since it's mentioned in the spec, and it would be nice to drive towards that on all platforms whenever possible. Also, in your build where do you define |
The currently supported versions of z/OS are LE 2.4, LE 2.5 and LE 3.1. TLS is supported in LE 3.1 (no C++ ctors/dtors) but not LE 2.4 or 2.5. The libc++ library needs to run on all of these versions of z/OS. We will need to continue building with the pthread method for a number of years until LE 2.5 is out of service and we no longer support that version of LE.
We don't define this macro. We use the code in the |
Ah yes, of course. I'm going to drop this patch for now, since I just realized that basically all platforms have been taking the "no thread local support" path. Perhaps this patch should instead remove the path where we use |
|
Actually, I can just switch to |
|
See #97591 |
This looks good though I thought you wanted to remove the entire block |
This was previously guarded by HAS_THREAD_LOCAL, which was never set by CMake and had to be specified manually. Android has been setting this to solve android/ndk#1200 1, but every compiler and platform libc++abi supports should have thread_local by now, so we can just get rid of the fallback implementation and simplify things significantly (including removing the now unused fallback calloc).
This is a re-application of https://reviews.llvm.org/D138461.
Fixes #78207.
Co-Authored-By: Shoaib Meenai [email protected]