[libcxxabi] Always use thread_local for cxa_exception_storage

[smeenai]

Summary

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 https://github.com/android/ndk/issues/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).

[1] https://android-review.googlesource.com/c/toolchain/llvm-project/+/1285596

[MaskRay]

LGTM. Clang has some targets that do TLSSupported = false but they are not supported by libc++.

[smeenai]

In D138461#3942614, @MaskRay wrote:

LGTM. Clang has some targets that do TLSSupported = false but they are not supported by libc++.

Do those targets support emulated TLS? Android didn't support ELF TLS until fairly recently, for example, so the thread_local path would end up using emulated TLS (which is using pthread under the hood), but this saves libc++abi from having to worry about those details or manually do the pthread equivalent.

[thakis]

This breaks tests for us on macOS/arm (https://luci-milo.appspot.com/ui/inv/build-8796707812197344321/test-results?q=StackSamplingProfilerTest.UnloadedLibrary) and android/x86 (https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8796707998083656337/+/u/chrome_junit_tests__with_patch_/stdout?format=raw , look for [ FAILED ] org.chromium.chrome.features.start_surface.SecondaryTasksSurfaceViewBinderUnitTest.testSetTopMargin (603 ms)). Is that expected?

[smeenai]

It's not. I'm out for a few days; could you revert this for me please?

[thakis]

In D138461#3947029, @smeenai wrote:

It's not. I'm out for a few days; could you revert this for me please?

Will do, thanks.

Looks like the android/x86 failure also happens on other diffs, so that's very likely unrelated. The arm/mac thing looks like it might be real? Will confirm locally that it's indeed due to this patch and then revert if so.

[ldionne]

I actually did come across breakage on several downstream platforms that apparently do not support thread_local. I'll have to investigate. I would support reverting this to give us time to investigate a bit.

In D138461#3947029, @smeenai wrote:

It's not. I'm out for a few days; could you revert this for me please?

I'll do that now, but let's take the time to figure out what we can clean up here.

[ldionne]

libcxxabi/src/cxa_exception_storage.cpp

27

So IIUC, this was basically never defined previously, right?

[smeenai]

Thanks for the revert.

We could take an intermediate approach where we detect thread_local support via CMake and turn on HAS_THREAD_LOCAL in that case, instead of removing the non-thread_local code path entirely. That'd let us shake out any issues with the thread_local path while also preserving support for other platforms (though I'm surprised we're still supporting platforms which have threads but not thread_local).

libcxxabi/src/cxa_exception_storage.cpp

27

Yup. This seems to go back to the original implementation of this code back in 2011, but it's hard to determine if this was intended to be automatically turned on e.g. via CMake if thread_local was detected.

[ldionne]

In D138461#3954545, @smeenai wrote:

Thanks for the revert.

We could take an intermediate approach where we detect thread_local support via CMake and turn on HAS_THREAD_LOCAL in that case, instead of removing the non-thread_local code path entirely. That'd let us shake out any issues with the thread_local path while also preserving support for other platforms (though I'm surprised we're still supporting platforms which have threads but not thread_local).

Yes, I'd support that approach. Let's call the macro _LIBCXXABI_HAS_THREAD_LOCAL, though. I'll commit to figuring out what's wrong on the platforms I saw that didn't support thread_local.

[smeenai]

I'll be going on an extended vacation soon. If anyone needs this sooner, they should feel free to pick it up, otherwise I'll get to it when I'm back in February. (Similarly, there's no rush on our end for you to figure out the platform issues you observed.)

libcxxabi/src/cxa_exception_storage.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //
 //  This file implements the storage for the "Caught Exception Stack"
 //  https://itanium-cxx-abi.github.io/cxx-abi/abi-eh.html#cxx-exc-stack
 //
 //===----------------------------------------------------------------------===//
 
 #include "cxa_exception.h"
 
-#include <__threading_support>
+namespace __cxxabiv1 {
 
 #if defined(_LIBCXXABI_HAS_NO_THREADS)
 
-namespace __cxxabiv1 {
 extern "C" {
     static __cxa_eh_globals eh_globals;
     __cxa_eh_globals *__cxa_get_globals() { return &eh_globals; }
     __cxa_eh_globals *__cxa_get_globals_fast() { return &eh_globals; }
 } // extern "C"
-} // namespace __cxxabiv1
 
-#elif defined(HAS_THREAD_LOCAL)
+#else
 
-namespace __cxxabiv1 {
 namespace {
     __cxa_eh_globals *__globals() {
         static thread_local __cxa_eh_globals eh_globals;
         return &eh_globals;
     }
 } // namespace
 
 extern "C" {
     __cxa_eh_globals *__cxa_get_globals() { return __globals(); }
     __cxa_eh_globals *__cxa_get_globals_fast() { return __globals(); }
 } // 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

libcxxabi/src/fallback_malloc.h

 #include "__cxxabi_config.h"
 #include <stddef.h> // for size_t
 
 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);
 
 } // namespace __cxxabiv1
 
 #endif

libcxxabi/src/fallback_malloc.cpp

#else
   if (size == 0)
     size = 1;
   if (void* dest = std::__libcpp_aligned_alloc(__alignof(__aligned_type), size))
     return dest;
 #endif
   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);
   else {
 #if defined(_LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION)
     ::free(ptr);
 #else
     std::__libcpp_aligned_free(ptr);