[libc++abi] Define __cxa_thread_atexit on platforms that need it

[whitequark]

Commit 3c100d5 removed the definition of __cxa_thread_atexit from all platforms except Fuchsia and Linux. This broke the use of thread_local variables with destructors on other downstream configurations that needed the definition.

This patch restores __cxa_thread_atexit on (presumably) all platforms that need it.

Old PR description (before PR was overhauled):

This was done in the commit 3c100d5 with the description "Enable -Wmissing-prototypes" which seems incongruent to me.

Since then it's made its way into a release and broke the use of thread_local variables with destructors on Wasm/WASI:

// repro.cc
struct c { ~c() {} };
thread_local c v;
int main() { (void)v; }

$ ./wasi-sdk-31.0-x86_64-linux/bin/clang++ repro.cc
wasm-ld: error: /tmp/repro-dd1ad7.o: undefined symbol: __cxa_thread_atexit
clang++: error: linker command failed with exit code 1 (use -v to see invocation)

[llvmbot]

@llvm/pr-subscribers-libcxxabi

Author: Catherine (whitequark)

Changes

This was done in the commit 3c100d5 with the description "Enable -Wmissing-prototypes" which seems incongruent to me.

Since then it's made its way into a release and broke the use of thread_local variables with destructors on Wasm/WASI:

// repro.cc
struct c { ~c() {} };
thread_local c v;
int main() { (void)v; }

$ ./wasi-sdk-31.0-x86_64-linux/bin/clang++ repro.cc
wasm-ld: error: /tmp/repro-dd1ad7.o: undefined symbol: __cxa_thread_atexit
clang++: error: linker command failed with exit code 1 (use -v to see invocation)

---

Full diff: https://github.com/llvm/llvm-project/pull/186054.diff

1 Files Affected:

• (modified) libcxxabi/src/cxa_thread_atexit.cpp (-2)

diff --git a/libcxxabi/src/cxa_thread_atexit.cpp b/libcxxabi/src/cxa_thread_atexit.cpp
index 402a52c741012..370e76344e89c 100644
--- a/libcxxabi/src/cxa_thread_atexit.cpp
+++ b/libcxxabi/src/cxa_thread_atexit.cpp
@@ -106,7 +106,6 @@ namespace {
 
 #endif // HAVE___CXA_THREAD_ATEXIT_IMPL
 
-#if defined(__linux__) || defined(__Fuchsia__)
 extern "C" {
 
   _LIBCXXABI_FUNC_VIS int __cxa_thread_atexit(Dtor dtor, void* obj, void* dso_symbol) throw() {
@@ -142,5 +141,4 @@ extern "C" {
 #endif // HAVE___CXA_THREAD_ATEXIT_IMPL
   }
 } // extern "C"
-#endif // defined(__linux__) || defined(__Fuchsia__)
 } // namespace __cxxabiv1

[whitequark]

cc @philnik777; could you explain a bit more about the rationale for that #ifdef? I removed it entirely because it seemed like a mistake to me, but in principle I don't see anything wrong with adding || defined(__wasi__) to the condition list.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff origin/main HEAD --extensions h,cpp -- libcxxabi/include/__cxxabi_config.h libcxxabi/include/cxxabi.h libcxxabi/src/cxa_thread_atexit.cpp --diff_from_common_commit

⚠️
The reproduction instructions above might return results for more than one PR
in a stack if you are using a stacked PR workflow. You can limit the results by
changing origin/main to the base branch/commit you want to compare against.
⚠️

View the diff from clang-format here.

diff --git a/libcxxabi/include/cxxabi.h b/libcxxabi/include/cxxabi.h
index 5d0513c4c..d9d56f8ec 100644
--- a/libcxxabi/include/cxxabi.h
+++ b/libcxxabi/include/cxxabi.h
@@ -179,7 +179,7 @@ __cxa_decrement_exception_refcount(void *primary_exception) _LIBCXXABI_NOEXCEPT;
 extern _LIBCXXABI_FUNC_VIS bool __cxa_uncaught_exception() _LIBCXXABI_NOEXCEPT;
 extern _LIBCXXABI_FUNC_VIS unsigned int __cxa_uncaught_exceptions() _LIBCXXABI_NOEXCEPT;
 
-#if _LIBCXXABI_DEFINE_THREAD_ATEXIT
+#  if _LIBCXXABI_DEFINE_THREAD_ATEXIT
 // Register a thread local destructor. This is not yet an official part of the Itanium ABI.
 // https://sourceware.org/glibc/wiki/Destructor%20support%20for%20thread_local%20variables
 extern _LIBCXXABI_FUNC_VIS int __cxa_thread_atexit(void (*)(void *), void *,
diff --git a/libcxxabi/src/cxa_thread_atexit.cpp b/libcxxabi/src/cxa_thread_atexit.cpp
index 366480f94..309858a5a 100644
--- a/libcxxabi/src/cxa_thread_atexit.cpp
+++ b/libcxxabi/src/cxa_thread_atexit.cpp
@@ -10,14 +10,14 @@
 
 #if !defined(_LIBCXXABI_HAS_NO_THREADS) && _LIBCXXABI_DEFINE_THREAD_ATEXIT
 
-#include <__thread/support.h>
-#include <stdlib.h>
+#  include <__thread/support.h>
+#  include <stdlib.h>
 
-#include "abort_message.h"
+#  include "abort_message.h"
 
-#if defined(__ELF__) && defined(_LIBCXXABI_LINK_PTHREAD_LIB)
-#pragma comment(lib, "pthread")
-#endif
+#  if defined(__ELF__) && defined(_LIBCXXABI_LINK_PTHREAD_LIB)
+#    pragma comment(lib, "pthread")
+#  endif
 
 namespace __cxxabiv1 {
 

[outtersg]

FWIW (just trying to build clang 22 on a FreeBSD 10.2, whose stock clang is a 3.4.1…), I'm not really fond of the addition of every OS to the #if, while the "no #if" policy worked well until now, except for the -Wmissing-prototypes.

Following up my comment on #116261, I successfully built a full 22.1.2 ecosystem (llvm, clang, libc++, libc++abi, openmp, clang-rt, lld, lldb, flang, flang-rt), both on FreeBSD 10.2 and 15.0 by removing the #if, for 2 passes (first "bootstrap" pass without lld, lldb, flang, flang-rt).
Of course I got the warning: no previous prototype for function '__cxa_thread_atexit' [-Wmissing-prototypes] that caused the CI failure you mentioned @ldionne; but this seems due to a 2014 commit which added a __cxa_thread_atexit implementation with diverging conditions:

• UNIX in the CMakeLists.txt
• __Linux__ only in cxxabi.h

Thus the warning (that the CI errors) on non-Linux Unix platforms.

So, with the example of my FreeBSD 10.2 and 15.0 sandboxes, wouldn't a cleaner solution be to
remove those limiting #ifs, and instead make cxxabi.h consistent with its CMakeLists by declaring and defining __cxa_thread_atexit on every non-Apple-or-Cygwin Unix platform?

[whitequark]

My position is that right now an architecture is broken. We should fix that first and then discuss a cleaner solution later.

[outtersg]

@whitequark wrote:

My position is that right now an architecture is broken. We should fix that first and then discuss a cleaner solution later.

I'm semi-agreeing with that:
on one hand, the initial culprit change dates back 12 years ago, and I'm militating to repair the build on another platform which is nearly as old;
on the other hand, won't we have other platforms that will emerge as broken, and require transforming the #if in a chaplet of opted-in platforms?

[philnik777]

This change was based on the fact that cxxabi.h guards this function behind #if defined(__linux__) || defined(__Fuchsia__). If this is wrong, that header needs to be updated as well. However, it's not clear to me at all which platforms actually expect this function if it's not declared on platforms that use it (probably through compiler magic?).

[outtersg]

@philnik777 wrote:

This change was based on the fact that cxxabi.h guards this function behind #if defined(__linux__) || defined(__Fuchsia__). If this is wrong, that header needs to be updated as well.

Thanks for your feedback. This assymetry introduced in 2014 was what puzzled me in my previous comment.
However, as cxxabi.h gets installed as a public header, I'm now wondering if there was a wish to:

• have it always defined (well, on all non-Apple non-Cygwin UNIX platforms, as said in the CMakeLists.txt), for internal use by libc++abi.so
• but not declared (to prevent accidental calls), except on __Linux__ that maybe had an obscure requirement?

That's not a very satisfying explanation… IMO removing it entirely (as you did @whitequark in your initial commit), or at least making the declaration in cxxabi.h consistent with the definition in CMakeLists.txt would be the KISS solution, if only we could get a hint from the CI that it doesn't break some other platform.

Note that CMakeLists.txt has now a more complex condition:

if (LIBCXXABI_ENABLE_THREADS AND (UNIX OR FUCHSIA) AND NOT (APPLE OR CYGWIN)
    AND NOT ("${CMAKE_SYSTEM_NAME}" MATCHES "AIX"))
  list(APPEND LIBCXXABI_SOURCES
    cxa_thread_atexit.cpp
  )
endif()

… So perhaps the complete solution would be to:

• remove the condition entirely in cxa_thread_at_exit.cpp (if it's included by CMakeLists.txt, then it should be compiled)
• in cxxabi.h, replace the condition by something like #ifdef _LIBCXXABI_CXA_THREAD_ATEXIT
• … which the CMakeLists.txt would add to __cxxabi_config.h at the same time it decides to include cxa_thread_atexit.cpp
  UPDATE: well, not __cxxabi_config.h, as I see it's static, not generated from a template filled with configure-time defines.

However, it's not clear to me at all which platforms actually expect this function if it's not declared on platforms that use it (probably through compiler magic?).

So for now we know that it is necessary on:

• Linux
• Fuchsia
• WASM
• SerenityOS (see comment by @ADKaster)
• FreeBSD 10.2
• and that having it defined on FreeBSD 15.0 is not necessary, but doesn't hurt

[whitequark]

I've updated the PR so that tests should pass now.

[ADKaster]

FWIW fixing this issue 'properly' as suggested by @outtersg would also remove a need for a patch on SerenityOS that looks remarkably similar to the current state of this PR:
https://github.com/SerenityOS/serenity/blob/5f2bd52d8dfa16d79e1b1cc66d01523e1447a690/Toolchain/Patches/llvm/0007-libcxxabi-Define-__cxa_thread_atexit-on-serenity.patch

[ldionne]

Upon further consideration, I think we should go with an opt-out instead of an opt-in.

[ldionne]

Alright folks, sorry for the traffic on this PR, but I finally got it to a place where I think it should address the original issue, and in addition improve the status quo:

1. We now unconditionally include the source file from CMake, that way there's no risk of drifting conditionals
2. We define the function all the time, except on the two (supported) platforms that we know don't use it.
3. As a side benefit, this means we don't have to hardcode a list of platforms to opt-in, which means we don't have to mention wasm or serenity OS, both of which are not officially supported (libc++'s policy is no CI = no support). This means that with this approach, we actually solve wasm and serenityos' problems without having to explicitly name them.

[whitequark]

Thanks for picking it up, @ldionne!

[ldionne]

@whitequark Could you confirm whether the PR in its current form fixes the original issue you were seeing? It would be sad if my refactoring ended up not solving the original problem :)

[whitequark]

Not easily, but I'm sure @alexcrichton would be able to!

stale

[LucasChollet]

I applied the patch locally, and it solves the issue for SerenityOS. Thanks, @ldionne!

[alexcrichton]

@whitequark Could you confirm whether the PR in its current form fixes the original issue you were seeing? It would be sad if my refactoring ended up not solving the original problem :)

Not easily, but I'm sure @alexcrichton would be able to!

Locally this PR in its current state looks like it works for wasi-sdk, and I've posted WebAssembly/wasi-sdk#649 for a full test across platforms too.

Thanks @ldionne!