diff options
7 files changed, 130 insertions, 51 deletions
diff --git a/src/corelib/thread/qthread_unix.cpp b/src/corelib/thread/qthread_unix.cpp index 15851a6046b..f86e60ea9ad 100644 --- a/src/corelib/thread/qthread_unix.cpp +++ b/src/corelib/thread/qthread_unix.cpp @@ -116,14 +116,22 @@ int pthread_timedjoin_np(...) { return ENOSYS; } // pretend // 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. +// There's a good correlation between this C++11 feature and our ability to +// call QThreadPrivate::cleanup() from destroy_thread_data(). // // 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) +#endif +// +// However, we can't destroy the QThreadData for the thread that called +// ::exit() that early, because a lot of existing content (including in Qt) +// runs when the static destructors are run and they do depend on QThreadData +// being extant. Likewise, we can't destroy it at global static destructor time +// because it's too late: the event dispatcher is usually a class found in a +// plugin and the plugin's destructors (as well as QtGui's) will have run. So +// we strike a middle-ground and destroy at function-local static destructor +// time (see set_thread_data()), because those run after the thread_local ones, +// before the global ones, and in reverse order of creation. Q_CONSTINIT static thread_local QThreadData *currentThreadData = nullptr; @@ -165,34 +173,21 @@ static QThreadData *get_thread_data() return currentThreadData; } -#if QT_CONFIG(broken_threadlocal_dtors) -// The destructors registered with pthread_key_create() below are NOT run from -// exit(), so we must also use atexit(). -static void destroy_main_thread_data() -{ - if (QThreadData *d = get_thread_data()) - destroy_current_thread_data(d); -} -Q_DESTRUCTOR_FUNCTION(destroy_main_thread_data) -#endif - static void set_thread_data(QThreadData *data) noexcept { if (data) { - if constexpr (QT_CONFIG(broken_threadlocal_dtors)) { - static pthread_key_t tls_key; - struct TlsKey { - TlsKey() noexcept { 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; - } + static pthread_key_t tls_key; + struct TlsKey { + TlsKey() noexcept { pthread_key_create(&tls_key, destroy_current_thread_data); } + ~TlsKey() + { + if (QThreadData *data = currentThreadData) + destroy_current_thread_data(data); + pthread_key_delete(tls_key); + } + }; + static TlsKey currentThreadCleanup; + pthread_setspecific(tls_key, data); } currentThreadData = data; } diff --git a/src/corelib/thread/qthread_win.cpp b/src/corelib/thread/qthread_win.cpp index 137a609107a..3b9b8871e22 100644 --- a/src/corelib/thread/qthread_win.cpp +++ b/src/corelib/thread/qthread_win.cpp @@ -7,6 +7,7 @@ #include "qcoreapplication.h" #include <private/qcoreapplication_p.h> #include <private/qeventdispatcher_win_p.h> +#include "qloggingcategory.h" #include "qmutex.h" #include "qthreadstorage.h" diff --git a/tests/auto/corelib/kernel/qcoreapplication/CMakeLists.txt b/tests/auto/corelib/kernel/qcoreapplication/CMakeLists.txt index 8f9783088c0..afe6c22b40f 100644 --- a/tests/auto/corelib/kernel/qcoreapplication/CMakeLists.txt +++ b/tests/auto/corelib/kernel/qcoreapplication/CMakeLists.txt @@ -37,3 +37,10 @@ endif() if (ANDROID) set_property(TARGET tst_qcoreapplication PROPERTY QT_ANDROID_VERSION_NAME ${target_version}) endif() + +qt_internal_add_test(tst_static_qcoreapplication + SOURCES + tst_static_qcoreapplication.cpp + LIBRARIES + Qt::CorePrivate +) diff --git a/tests/auto/corelib/kernel/qcoreapplication/tst_qcoreapplication.cpp b/tests/auto/corelib/kernel/qcoreapplication/tst_qcoreapplication.cpp index 5c480f307ba..d7dfca824bb 100644 --- a/tests/auto/corelib/kernel/qcoreapplication/tst_qcoreapplication.cpp +++ b/tests/auto/corelib/kernel/qcoreapplication/tst_qcoreapplication.cpp @@ -1065,27 +1065,6 @@ static void createQObjectOnDestruction() // QThread) after the last QObject has been destroyed (especially after // QCoreApplication has). -#if defined(QT_QGUIAPPLICATIONTEST) - // If we've linked to QtGui, we make no representations about there being - // global static (not Q_GLOBAL_STATIC) variables that are QObject. -#elif QT_CONFIG(broken_threadlocal_dtors) - // With broken thread-local destructors, we cannot guarantee the ordering - // between thread_local destructors and static-lifetime destructors (hence - // why they're broken). - // - // On Unix systems, we use a Q_DESTRUCTOR_FUNCTION in qthread_unix.cpp to - // work around the issue, but that means it cannot have run yet. - // - // On Windows, given the nature of how destructors are run in DLLs, they're - // always considered broken. In fact, we know the destructor in - // qthread_win.cpp hasn't run yet. -#else - // The thread_local destructor in qthread_unix.cpp has run so the - // QAdoptedThread must have been cleaned up. - if (theMainThreadIsSet()) - qFatal("theMainThreadIsSet() returned true; some QObject must have leaked"); -#endif - // Before the fixes, this would cause a dangling pointer dereference. If // the problem comes back, it's possible that the following causes no // effect. diff --git a/tests/auto/corelib/kernel/qcoreapplication/tst_static_qcoreapplication.cpp b/tests/auto/corelib/kernel/qcoreapplication/tst_static_qcoreapplication.cpp new file mode 100644 index 00000000000..4f10aaed6e9 --- /dev/null +++ b/tests/auto/corelib/kernel/qcoreapplication/tst_static_qcoreapplication.cpp @@ -0,0 +1,81 @@ +// Copyright (C) 2024 Intel Corporation. +// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only + +#include <QtTest/QTest> +#include <private/qcoreapplication_p.h> +#include <private/qhooks_p.h> + +#ifdef QT_WIDGETS_LIB +# define tst_Static_QCoreApplication tst_Static_QApplication +using App = QApplication; +#elif defined(QT_GUI_LIB) +# define tst_Static_QCoreApplication tst_Static_QGuiApplication +using App = QGuiApplication; +#else +using App = QCoreApplication; +#endif + +class tst_Static_QCoreApplication : public QObject +{ + Q_OBJECT +public: + ~tst_Static_QCoreApplication() {} +private Q_SLOTS: + void staticApplication(); +}; + +void tst_Static_QCoreApplication::staticApplication() +{ + // This test only verifies that the destructor, when run inside of exit(), + // will not crash. + static int argc = 1; + const char *argv[] = { staticMetaObject.className(), nullptr }; + static App app(argc, const_cast<char **>(argv)); +} + +struct HookManager +{ + static inline QBasicAtomicInt objectCount = {}; + static void addObject(QObject *o) + { + // printf("+ %p = %s\n", o, o->metaObject()->className()); + objectCount.ref(); + Q_UNUSED(o); + } + static void removeObject(QObject *o) + { + objectCount.deref(); + // printf("- %p\n", o); + Q_UNUSED(o); + } + + HookManager() + { + qtHookData[QHooks::AddQObject] = reinterpret_cast<quintptr>(&addObject); + qtHookData[QHooks::RemoveQObject] = reinterpret_cast<quintptr>(&removeObject); + } + + ~HookManager() + { + qtHookData[QHooks::AddQObject] = 0; + qtHookData[QHooks::RemoveQObject] = 0; + + auto app = qApp; + if (app) + qFatal("qApp was not destroyed = %p", app); +#if !defined(QT_GUI_LIB) && !defined(Q_OS_WIN) + // Only tested for QtCore/QCoreApplication on Unix. QtGui has statics + // with QObject that haven't been cleaned up. + if (int c = objectCount.loadRelaxed()) + qFatal("%d objects still alive", c); + + if (void *id = QCoreApplicationPrivate::theMainThreadId.loadRelaxed()) + qFatal("QCoreApplicationPrivate::theMainThreadId is still set - %p", id); +#endif + } +}; +static HookManager hookManager; + +QTEST_APPLESS_MAIN(tst_Static_QCoreApplication) +#include "tst_static_qcoreapplication.moc" + diff --git a/tests/auto/gui/kernel/qguiapplication/CMakeLists.txt b/tests/auto/gui/kernel/qguiapplication/CMakeLists.txt index 6f1f845edd3..109c0813128 100644 --- a/tests/auto/gui/kernel/qguiapplication/CMakeLists.txt +++ b/tests/auto/gui/kernel/qguiapplication/CMakeLists.txt @@ -46,3 +46,11 @@ if (APPLE) set_property(TARGET tst_qguiapplication PROPERTY MACOSX_BUNDLE_INFO_PLIST "${CMAKE_CURRENT_SOURCE_DIR}/Info.plist") set_property(TARGET tst_qguiapplication PROPERTY PROPERTY MACOSX_BUNDLE TRUE) endif() + +qt_internal_add_test(tst_static_qguiapplication + SOURCES + ../../../corelib/kernel/qcoreapplication/tst_static_qcoreapplication.cpp + LIBRARIES + Qt::CorePrivate + Qt::Gui +) diff --git a/tests/auto/widgets/kernel/qapplication/CMakeLists.txt b/tests/auto/widgets/kernel/qapplication/CMakeLists.txt index 524db06560f..7825ed51c4e 100644 --- a/tests/auto/widgets/kernel/qapplication/CMakeLists.txt +++ b/tests/auto/widgets/kernel/qapplication/CMakeLists.txt @@ -15,3 +15,11 @@ add_dependencies(tst_qapplication desktopsettingsaware_helper modal_helper ) + +qt_internal_add_test(tst_static_qapplication + SOURCES + ../../../corelib/kernel/qcoreapplication/tst_static_qcoreapplication.cpp + LIBRARIES + Qt::CorePrivate + Qt::Widgets +) |