diff --git a/config.tests/cxa_thread_atexit/CMakeLists.txt b/config.tests/cxa_thread_atexit/CMakeLists.txt new file mode 100644 index 00000000000..a1df9cd9d81 --- /dev/null +++ b/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}) diff --git a/config.tests/cxa_thread_atexit/main.c b/config.tests/cxa_thread_atexit/main.c new file mode 100644 index 00000000000..19eef494392 --- /dev/null +++ b/config.tests/cxa_thread_atexit/main.c @@ -0,0 +1,10 @@ +// Copyright (C) 2024 Intel Corporation +// SPDX-License-Identifier: BSD-3-Clause +#include + +typedef void (*dtor_func) (void *); +int TEST_FUNC(dtor_func func, void *obj, void *dso_symbol); +int main() +{ + return TEST_FUNC(NULL, NULL, NULL); +} diff --git a/src/corelib/configure.cmake b/src/corelib/configure.cmake index 20524d49973..c17dccf0235 100644 --- a/src/corelib/configure.cmake +++ b/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 " @@ -551,6 +567,11 @@ qt_feature("cxx17_filesystem" PUBLIC LABEL "C++17 " 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 diff --git a/src/corelib/thread/qthread_p.h b/src/corelib/thread/qthread_p.h index 3c876cdcf15..04405e1f08e 100644 --- a/src/corelib/thread/qthread_p.h +++ b/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 diff --git a/src/corelib/thread/qthread_unix.cpp b/src/corelib/thread/qthread_unix.cpp index 22209b15a0e..1074b041209 100644 --- a/src/corelib/thread/qthread_unix.cpp +++ b/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(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(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(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(¤t_thread_data_key, destroy_current_thread_data); -} - -static void destroy_current_thread_data_key() -{ - pthread_once(¤t_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(¤t_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(arg); - QThreadData *data = QThreadData::get2(thr); + QThread *thr = reinterpret_cast(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(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(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 diff --git a/tests/auto/corelib/thread/qthread/tst_qthread.cpp b/tests/auto/corelib/thread/qthread/tst_qthread.cpp index 65cf97cdf34..45742206729 100644 --- a/tests/auto/corelib/thread/qthread/tst_qthread.cpp +++ b/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);