QThread/Unix: refactor to split QThreadPrivate::finish() in two phases

Commit 1ed0dd88a32cd2c5ae100b48e14ff55bcbb652e6 moved the finish()
functionality from immediately after run() returns to the time of
thread-local destruction, to make sure that user destructors didn't run
after our cleaning up. But as a side effect, it made other user code run
too late, after some thread-local statics had been destroyed.

This is a common practice, which causes the destructor for worker to run
too late:

    worker->moveToThread(thread);
...
    QObject::connect(thread, &QThread::finished, thread, &QObject::deleteLater);
    QObject::connect(thread, &QThread::finished, worker, &QObject::deleteLater);

This commit splits the cleanup in two phases: QThreadPrivate::finish(),
which runs immediately after run() and will call back out to user code
(finished() signal and delivery of deleteLater()), and cleanup() that
cleans up the QThread{Private,Data} state and destroys the event
dispatcher. That destruction is the only call out to user code.

I've removed the complex mix of pre-C++11 pthread_setspecific() content
and C++11 thread_local variables in favor of using one or the other, not
both. We prefer the thread-local for future-proofing and simplicity, on
platforms where we can verify this C++11 feature works, and because it
allows us to clean up QThreadData and the event dispatcher as late as
possible. (There's some code that runs even later, such as pthread TLS
destructors, used by Glib's GMainLoop)

Unfortunately, we can't use it everywhere. The commit above had already
noticed QNX has a problem and recent bug reports have shown other
platforms (Solaris, MUSL libc) that, 13 years after the ratification of
the standard, still have broken support, so we use pthread for them and
we call cleanup() from within finish() (that is, no late cleaning-up,
retaining the status quo from Qt 4 and 5). See QTBUG-129846 for an
analysis.

Drive-by moving the resetting of thread priority to after finished() is
emitted.

[ChangeLog][QtCore][QThread] Restored the Qt 6.7 timing of when the
finished() signal is emitted relative to the destruction of thread_local
variables. Qt 6.8.0 contained a change that moved this signal to a later
time on most Unix systems, which has caused problems with the order in
which those variables were accessed. The destruction of the event
dispatcher is kept at this late stage, wherever possible.

Fixes: QTBUG-129927
Fixes: QTBUG-129846
Fixes: QTBUG-130341
Task-number: QTBUG-117996
Pick-to: 6.8
Change-Id: Ie5e40dd18faa05d8f777fffdf7dc30fc4fe0c7e9
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

config.tests/cxa_thread_atexit/CMakeLists.txt

@@ -0,0 +1,6 @@
+# Copyright (C) 2024 Intel Corporation.
+# SPDX-License-Identifier: BSD-3-Clause
+cmake_minimum_required(VERSION 3.16)
+project(cxa_thread_atexit LANGUAGES C)
+add_executable(cxa_thread_atexit main.c)
+target_compile_definitions(cxa_thread_atexit PUBLIC TEST_FUNC=${TEST_FUNC})

config.tests/cxa_thread_atexit/main.c

@@ -0,0 +1,10 @@
+// Copyright (C) 2024 Intel Corporation
+// SPDX-License-Identifier: BSD-3-Clause
+#include <stddef.h>
+
+typedef void (*dtor_func) (void *);
+int TEST_FUNC(dtor_func func, void *obj, void *dso_symbol);
+int main()
+{
+    return TEST_FUNC(NULL, NULL, NULL);
+}

src/corelib/configure.cmake

@@ -143,6 +143,22 @@ int pipes[2];
 }
 ")
 
+# Check if __cxa_thread_atexit{,_impl} are present in the C library (hence why
+# PROJECT_PATH instead of CODE for C++). Either one suffices to disable
+# FEATURE_broken_threadlocal_dtors. See details in qthread_unix.cpp.
+qt_config_compile_test(cxa_thread_atexit
+    # Seen on Darwin and FreeBSD
+    LABEL "__cxa_thread_atexit in C library"
+    PROJECT_PATH "${CMAKE_CURRENT_SOURCE_DIR}/../config.tests/cxa_thread_atexit"
+    CMAKE_FLAGS -DTEST_FUNC=__cxa_thread_atexit
+)
+qt_config_compile_test(cxa_thread_atexit_impl
+    # Seen on Bionic, FreeBSD, glibc
+    LABEL "__cxa_thread_atexit_impl in C library"
+    PROJECT_PATH "${CMAKE_CURRENT_SOURCE_DIR}/../config.tests/cxa_thread_atexit"
+    CMAKE_FLAGS -DTEST_FUNC=__cxa_thread_atexit_impl
+)
+
 # cxx17_filesystem
 qt_config_compile_test(cxx17_filesystem
     LABEL "C++17 <filesystem>"
@@ -551,6 +567,11 @@ qt_feature("cxx17_filesystem" PUBLIC
     LABEL "C++17 <filesystem>"
     CONDITION TEST_cxx17_filesystem
 )
+qt_feature("broken-threadlocal-dtors" PRIVATE
+    LABEL "Broken execution of thread_local destructors at exit() time"
+    # Windows broken in different ways from Unix
+    CONDITION WIN32 OR NOT (TEST_cxa_thread_atexit OR TEST_cxa_thread_atexit_impl)
+)
 qt_feature("dladdr" PRIVATE
     LABEL "dladdr"
     CONDITION QT_FEATURE_dlopen AND TEST_dladdr

src/corelib/thread/qthread_p.h

@@ -189,7 +189,7 @@ public:
         NotStarted = 0,     // before start() or if failed to start
         Running = 1,        // in run()
         Finishing = 2,      // in QThreadPrivate::finish()
-        Finished = 3,       // QThreadPrivate::finish() is done
+        Finished = 3,       // QThreadPrivate::finish() or cleanup() is done
     };
 
     State threadState = NotStarted;
@@ -208,8 +208,8 @@ public:
     QWaitCondition thread_done;
 
     static void *start(void *arg);
-    static void finish(void *);
-
+    static void finish(void *);     // happens early (before thread-local dtors)
+    static void cleanup(void *);    // happens late (as a thread-local dtor, if possible)
 #endif // Q_OS_UNIX
 
 #ifdef Q_OS_WIN

src/corelib/thread/qthread_unix.cpp

@@ -78,26 +78,55 @@ static_assert(sizeof(pthread_t) <= sizeof(Qt::HANDLE));
 
 enum { ThreadPriorityResetFlag = 0x80000000 };
 
+#if QT_CONFIG(broken_threadlocal_dtors)
+// On most modern platforms, the C runtime has a helper function that helps the
+// C++ runtime run the thread_local non-trivial destructors when threads exit
+// and that code ensures that they are run in the correct order on program exit
+// too ([basic.start.term]/2: "The destruction of all constructed objects with
+// thread storage duration within that thread strongly happens before
+// destroying any object with static storage duration."). In the absence of
+// this function, the ordering can be wrong depending on when the first
+// non-trivial thread_local object was created relative to other statics.
+// Moreover, this can be racy and having our own thread_local early in
+// QThreadPrivate::start() made it even more so. See QTBUG-129846 for analysis.
+//
+// For the platforms where this C++11 feature is not properly implemented yet,
+// we fall back to a pthread_setspecific() call and do not perform late
+// clean-up, because then the order of registration of those pthread_specific_t
+// keys matters and Glib uses them too.
+//
+// https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libstdc%2B%2B-v3/libsupc%2B%2B/atexit_thread.cc;hb=releases/gcc-14.2.0#l133
+// https://github.com/llvm/llvm-project/blob/llvmorg-19.1.0/libcxxabi/src/cxa_thread_atexit.cpp#L118-L120
+#endif // QT_CONFIG(broken_threadlocal_dtors)
 
+// Always access this through the {get,set,clear}_thread_data() functions.
 Q_CONSTINIT static thread_local QThreadData *currentThreadData = nullptr;
 
-Q_CONSTINIT static pthread_once_t current_thread_data_once = PTHREAD_ONCE_INIT;
-Q_CONSTINIT static pthread_key_t current_thread_data_key;
-
 static void destroy_current_thread_data(void *p)
 {
     QThreadData *data = static_cast<QThreadData *>(p);
-    // thread_local variables are set to zero before calling this destructor function,
-    // if they are internally using pthread-specific data management,
-    // so we need to set it back to the right value...
-    currentThreadData = data;
+    QThread *thread = data->thread.loadAcquire();
+
     if (data->isAdopted) {
-        QThread *thread = data->thread.loadAcquire();
-        Q_ASSERT(thread);
+        // If this is an adopted thread, then QThreadData owns the QThread and
+        // this is very likely the last reference. These pointers cannot be
+        // null and there is no race.
         QThreadPrivate *thread_p = static_cast<QThreadPrivate *>(QObjectPrivate::get(thread));
-        Q_ASSERT(thread_p->threadState == QThreadPrivate::Running);
         thread_p->finish(thread);
+        if constexpr (!QT_CONFIG(broken_threadlocal_dtors))
+            thread_p->cleanup(thread);
+    } else if constexpr (!QT_CONFIG(broken_threadlocal_dtors)) {
+        // We may be racing the QThread destructor in another thread. With
+        // two-phase clean-up enabled, there's also no race because it will
+        // stop in a call to QThread::wait() until we call cleanup().
+        QThreadPrivate *thread_p = static_cast<QThreadPrivate *>(QObjectPrivate::get(thread));
+        thread_p->cleanup(thread);
+    } else {
+        // We may be racing the QThread destructor in another thread and it may
+        // have begun destruction; we must not dereference the QThread pointer.
     }
+
+    // the QThread object may still have a reference, so this may not delete
     data->deref();
 
     // ... but we must reset it to zero before returning so we aren't
@@ -105,25 +134,6 @@ static void destroy_current_thread_data(void *p)
     currentThreadData = nullptr;
 }
 
-static void create_current_thread_data_key()
-{
-    pthread_key_create(&current_thread_data_key, destroy_current_thread_data);
-}
-
-static void destroy_current_thread_data_key()
-{
-    pthread_once(&current_thread_data_once, create_current_thread_data_key);
-    pthread_key_delete(current_thread_data_key);
-
-    // Reset current_thread_data_once in case we end up recreating
-    // the thread-data in the rare case of QObject construction
-    // after destroying the QThreadData.
-    pthread_once_t pthread_once_init = PTHREAD_ONCE_INIT;
-    current_thread_data_once = pthread_once_init;
-}
-Q_DESTRUCTOR_FUNCTION(destroy_current_thread_data_key)
-
-
 // Utility functions for getting, setting and clearing thread specific data.
 static QThreadData *get_thread_data()
 {
@@ -132,9 +142,26 @@ static QThreadData *get_thread_data()
 
 static void set_thread_data(QThreadData *data)
 {
+    // Only activate the late cleanup for auxiliary threads. We can't use
+    // QThread::isMainThread() here because theMainThreadId will not have been
+    // set yet.
+    if (data && QCoreApplicationPrivate::theMainThreadId.loadAcquire()) {
+        if constexpr (QT_CONFIG(broken_threadlocal_dtors)) {
+            static pthread_key_t tls_key;
+            struct TlsKey {
+                TlsKey() { pthread_key_create(&tls_key, destroy_current_thread_data); }
+                ~TlsKey() { pthread_key_delete(tls_key); }
+            };
+            static TlsKey currentThreadCleanup;
+            pthread_setspecific(tls_key, data);
+        } else {
+            struct Cleanup {
+                ~Cleanup() { destroy_current_thread_data(currentThreadData); }
+            };
+            static thread_local Cleanup currentThreadCleanup;
+        }
+    }
     currentThreadData = data;
-    pthread_once(&current_thread_data_once, create_current_thread_data_key);
-    pthread_setspecific(current_thread_data_key, data);
 }
 
 static void clear_thread_data()
@@ -281,20 +308,14 @@ void *QThreadPrivate::start(void *arg)
 #ifdef PTHREAD_CANCEL_DISABLE
     pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, nullptr);
 #endif
-#if !defined(Q_OS_QNX) && !defined(Q_OS_VXWORKS)
-    // On QNX, calling finish() from a thread_local destructor causes the C
-    // library to hang.
-    // On VxWorks, its pthread implementation fails on call to `pthead_setspecific` which is made
-    // by first QObject constructor during `finish()`. This causes call to QThread::current, since
-    // QObject doesn't have parent, and since the pthread is already removed, it tries to set
-    // QThreadData for current pthread key, which crashes.
-    static thread_local
-#endif
-            auto cleanup = qScopeGuard([=] { finish(arg); });
-    terminate_on_exception([&] {
-        QThread *thr = reinterpret_cast<QThread *>(arg);
-        QThreadData *data = QThreadData::get2(thr);
+    QThread *thr = reinterpret_cast<QThread *>(arg);
+    QThreadData *data = QThreadData::get2(thr);
 
+    // this ensures the thread-local is created as early as possible
+    set_thread_data(data);
+
+    pthread_cleanup_push(QThreadPrivate::finish, arg);
+    terminate_on_exception([&] {
         {
             QMutexLocker locker(&thr->d_func()->mutex);
 
@@ -306,7 +327,6 @@ void *QThreadPrivate::start(void *arg)
             // threadId is set in QThread::start()
             Q_ASSERT(pthread_equal(from_HANDLE<pthread_t>(data->threadId.loadRelaxed()),
                                    pthread_self()));
-            set_thread_data(data);
 
             data->ref();
             data->quitNow = thr->d_func()->exited;
@@ -335,7 +355,9 @@ void *QThreadPrivate::start(void *arg)
         thr->run();
     });
 
-    // The qScopeGuard above call runs finish() below.
+    // This calls finish(); later, the currentThreadCleanup thread-local
+    // destructor will call cleanup().
+    pthread_cleanup_pop(1);
     return nullptr;
 }
 
@@ -355,14 +377,33 @@ void QThreadPrivate::finish(void *arg)
         QMutexLocker locker(&d->mutex);
 
         d->threadState = QThreadPrivate::Finishing;
-        d->priority = QThread::InheritPriority;
-        void *data = &d->data->tls;
         locker.unlock();
         emit thr->finished(QThread::QPrivateSignal());
         qCDebug(lcDeleteLater) << "Sending deferred delete events as part of finishing thread" << thr;
         QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete);
+
+        void *data = &d->data->tls;
         QThreadStorageData::finish((void **)data);
-        locker.relock();
+    });
+
+    if constexpr (QT_CONFIG(broken_threadlocal_dtors))
+        cleanup(arg);
+}
+
+void QThreadPrivate::cleanup(void *arg)
+{
+    terminate_on_exception([&] {
+        QThread *thr = reinterpret_cast<QThread *>(arg);
+        QThreadPrivate *d = thr->d_func();
+
+        // Disable cancellation again: we did it above, but some user code
+        // running between finish() and cleanup() may have turned them back on.
+#ifdef PTHREAD_CANCEL_DISABLE
+        pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, nullptr);
+#endif
+
+        QMutexLocker locker(&d->mutex);
+        d->priority = QThread::InheritPriority;
 
         QAbstractEventDispatcher *eventDispatcher = d->data->eventDispatcher.loadRelaxed();
         if (eventDispatcher) {
@@ -760,7 +801,7 @@ void QThread::terminate()
 
     d->terminated = true;
 
-    const bool selfCancelling = d->data == currentThreadData;
+    const bool selfCancelling = d->data == get_thread_data();
     if (selfCancelling) {
         // Posix doesn't seem to specify whether the stack of cancelled threads
         // is unwound, and there's nothing preventing a QThread from

tests/auto/corelib/thread/qthread/tst_qthread.cpp

@@ -1380,9 +1380,10 @@ void tst_QThread::customEventDispatcher()
 
     QSemaphore threadLocalSemaphore;
     QMetaObject::invokeMethod(&obj, [&]() {
-#ifndef Q_OS_WIN
+#if !QT_CONFIG(broken_threadlocal_dtors)
         // On Windows, the thread_locals are unsequenced between DLLs, so this
-        // could run after QThreadPrivate::finish()
+        // could run after QThreadPrivate::finish().
+        // On Unix, QThread doesn't use thread_local if support is broken.
         static thread_local
 #endif
                 ThreadLocalContent d(&obj, &threadLocalSemaphore);