[libc] ensure tls dtors are called in main thread

[SchrodingerZhu]

This is a part of allocator patch since I want to make sure the TLS for allocators are correctly handled.

TLS dtors are not invoked on exit previously. This departures from major libc implementations.

This PR fixes the issue by aligning the behavior with bionic.

https://android.googlesource.com/platform/bionic/+/refs/heads/main/libc/bionic/exit.cpp

I believe the finalization order is also consistent with glibc now:

On main thread exiting:

• pthread_key dtors are not called (unless exiting with pthread_exit)
• __cxa based tls dtors are called
• ::__cxa_atexit and ::atexit dtors are called
• .fini dtors are called

[llvmbot]

@llvm/pr-subscribers-libc

Author: Schrodinger ZHU Yifan (SchrodingerZhu)

Changes

---

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

4 Files Affected:

• (modified) libc/src/stdlib/CMakeLists.txt (+1)
• (modified) libc/src/stdlib/atexit.cpp (+2)
• (modified) libc/test/integration/src/__support/threads/CMakeLists.txt (+10)
• (added) libc/test/integration/src/__support/threads/main_exit_test.cpp (+31)

diff --git a/libc/src/stdlib/CMakeLists.txt b/libc/src/stdlib/CMakeLists.txt
index 74ae864f72e23..2b0fb869935ef 100644
--- a/libc/src/stdlib/CMakeLists.txt
+++ b/libc/src/stdlib/CMakeLists.txt
@@ -598,6 +598,7 @@ add_entrypoint_object(
   CXX_STANDARD
     20 # For constinit
   DEPENDS
+    libc.src.__support.threads.thread
     .exit_handler
 )
 
diff --git a/libc/src/stdlib/atexit.cpp b/libc/src/stdlib/atexit.cpp
index 799aad136bda5..4289518b0d6cf 100644
--- a/libc/src/stdlib/atexit.cpp
+++ b/libc/src/stdlib/atexit.cpp
@@ -10,6 +10,7 @@
 #include "hdr/types/atexithandler_t.h"
 #include "src/__support/common.h"
 #include "src/__support/macros/config.h"
+#include "src/__support/threads/thread.h"
 #include "src/stdlib/exit_handler.h"
 
 namespace LIBC_NAMESPACE_DECL {
@@ -26,6 +27,7 @@ int __cxa_atexit(AtExitCallback *callback, void *payload, void *) {
 
 void __cxa_finalize(void *dso) {
   if (!dso) {
+    internal::call_atexit_callbacks(self.attrib);
     call_exit_callbacks(atexit_callbacks);
     if (teardown_main_tls)
       teardown_main_tls();
diff --git a/libc/test/integration/src/__support/threads/CMakeLists.txt b/libc/test/integration/src/__support/threads/CMakeLists.txt
index 5a12d28ada3fd..dae789e157667 100644
--- a/libc/test/integration/src/__support/threads/CMakeLists.txt
+++ b/libc/test/integration/src/__support/threads/CMakeLists.txt
@@ -25,3 +25,13 @@ add_integration_test(
   DEPENDS
     libc.src.__support.threads.thread
 )
+
+add_integration_test(
+  main_exit_test
+  SUITE
+    libc-support-threads-integration-tests
+  SRCS
+    main_exit_test.cpp
+  DEPENDS
+    libc.src.__support.threads.thread
+)
diff --git a/libc/test/integration/src/__support/threads/main_exit_test.cpp b/libc/test/integration/src/__support/threads/main_exit_test.cpp
new file mode 100644
index 0000000000000..1110be22a0336
--- /dev/null
+++ b/libc/test/integration/src/__support/threads/main_exit_test.cpp
@@ -0,0 +1,31 @@
+//===-- Test handling of thread local data --------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/__support/threads/thread.h"
+#include "test/IntegrationTest/test.h"
+
+bool called = false;
+
+extern "C" {
+[[gnu::weak]]
+void *__dso_handle = nullptr;
+
+int __cxa_thread_atexit_impl(void (*func)(void *), void *arg, void *dso);
+}
+
+[[gnu::destructor]]
+void destructor() {
+  if (!called)
+    __builtin_trap();
+}
+
+TEST_MAIN() {
+  __cxa_thread_atexit_impl([](void *) { called = true; }, nullptr,
+                           __dso_handle);
+  return 0;
+}

[jhuber6]

I'm not 100% sure what the proper handling of this, but seeing weak symbols thrown around like this makes me uneasy.

[jhuber6]

I was wondering earlier if it would be possible to just make these routines lock free, since I'm pretty sure it's an append-only data structure, pretty trivial to, I could probably add that functionality to the existing blockstore.

[SchrodingerZhu]

AFAIK, the weak symbol approach is also used in __cxa_finalize (for tearing down TLS).

I remember the reason is that some code is supposed to be used in overlay mode.

[SchrodingerZhu]

I was wondering earlier if it would be possible to just make these routines lock free, since I'm pretty sure it's an append-only data structure, pretty trivial to, I could probably add that functionality to the existing blockstore.

There are possibilities but I will need to check. The semantic of recursively adding exit hook or adding exit hook in parallel of running the hook is a grey area in the specification. We will need to work out the details.

[jhuber6]

Are these dtors thread local or something?

[SchrodingerZhu]

the whole attribute struct is in tls. But dtors are not.

[SchrodingerZhu]

The protection is for calling exit twice, where there is a inflight call while the atexit hook invokes exit again.

[SchrodingerZhu]

Sorry, I found the changes here are actually redundant because the way the mutex is handled already considers reentrance. I will keep the tests but remove the changes.

[SchrodingerZhu]

@jhuber6 a gentle ping on this

[Copilot]

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

• libc/test/integration/src/__support/threads/CMakeLists.txt: Language not supported

Comments suppressed due to low confidence (1)

libc/test/integration/src/__support/threads/double_exit_test.cpp:19

• [nitpick] Consider renaming 'call_num' to a more descriptive name (e.g., 'destructor_call_count') for better clarity in the test's intent.

int call_num = 0;

[Copilot]

The call to __cxa_thread_finalize is not protected by any locking mechanism; consider using a recursive mutex as noted in the TODO to prevent potential race conditions during exit.

[michaelrj-google]

LGTM