Fix data race on QLoggingCategory when using qDebug from multiple threads

setEnabled() would race with isEnabled()/isDebugEnabled()/etc.

Change-Id: I2004cba81d5417a634b97f5c2f98d3a4ab71770d
Reviewed-by: David Faure <david.faure@kdab.com>

5 files changed, 97 insertions, 20 deletions

diff --git a/src/corelib/arch/qatomic_bootstrap.h b/src/corelib/arch/qatomic_bootstrap.h
index 7f17387c9c..0d6843a3e5 100644
--- a/src/corelib/arch/qatomic_bootstrap.h
+++ b/src/corelib/arch/qatomic_bootstrap.h
@@ -67,8 +67,10 @@ template <typename T> struct QAtomicOps: QGenericAtomicOps<QAtomicOps<T> >
         return --_q_value != 0;
     }
 
-    static bool testAndSetRelaxed(T &_q_value, T expectedValue, T newValue) Q_DECL_NOTHROW
+    static bool testAndSetRelaxed(T &_q_value, T expectedValue, T newValue, T *currentValue = 0) Q_DECL_NOTHROW
     {
+        if (currentValue)
+            *currentValue = _q_value;
         if (_q_value == expectedValue) {
             _q_value = newValue;
             return true;
diff --git a/src/corelib/io/qloggingcategory.cpp b/src/corelib/io/qloggingcategory.cpp
index 518052e537..45fab11939 100644
--- a/src/corelib/io/qloggingcategory.cpp
+++ b/src/corelib/io/qloggingcategory.cpp
@@ -50,6 +50,18 @@ const char qtDefaultCategoryName[] = "default";
 Q_GLOBAL_STATIC_WITH_ARGS(QLoggingCategory, qtDefaultCategory,
                           (qtDefaultCategoryName))
 
+#ifndef Q_ATOMIC_INT8_IS_SUPPORTED
+static void setBoolLane(QBasicAtomicInt *atomic, bool enable, int shift)
+{
+    const int bit = 1 << shift;
+
+    if (enable)
+        atomic->fetchAndOrRelaxed(bit);
+    else
+        atomic->fetchAndAndRelaxed(~bit);
+}
+#endif
+
 /*!
     \class QLoggingCategory
     \inmodule QtCore
@@ -171,13 +183,11 @@ Q_GLOBAL_STATIC_WITH_ARGS(QLoggingCategory, qtDefaultCategory,
 */
 QLoggingCategory::QLoggingCategory(const char *category)
     : d(0),
-      name(0),
-      enabledDebug(true),
-      enabledWarning(true),
-      enabledCritical(true)
+      name(0)
 {
     Q_UNUSED(d);
     Q_UNUSED(placeholder);
+    enabled.store(0x01010101);   // enabledDebug = enabledWarning = enabledCritical = true;
 
     const bool isDefaultCategory
             = (category == 0) || (strcmp(category, qtDefaultCategoryName) == 0);
@@ -249,9 +259,9 @@ QLoggingCategory::~QLoggingCategory()
 bool QLoggingCategory::isEnabled(QtMsgType msgtype) const
 {
     switch (msgtype) {
-    case QtDebugMsg: return enabledDebug;
-    case QtWarningMsg: return enabledWarning;
-    case QtCriticalMsg: return enabledCritical;
+    case QtDebugMsg: return isDebugEnabled();
+    case QtWarningMsg: return isWarningEnabled();
+    case QtCriticalMsg: return isCriticalEnabled();
     case QtFatalMsg: return true;
     }
     return false;
@@ -270,9 +280,15 @@ bool QLoggingCategory::isEnabled(QtMsgType msgtype) const
 void QLoggingCategory::setEnabled(QtMsgType type, bool enable)
 {
     switch (type) {
-    case QtDebugMsg: enabledDebug = enable; break;
-    case QtWarningMsg: enabledWarning = enable; break;
-    case QtCriticalMsg: enabledCritical = enable; break;
+#ifdef Q_ATOMIC_INT8_IS_SUPPORTED
+    case QtDebugMsg: bools.enabledDebug.store(enable); break;
+    case QtWarningMsg: bools.enabledWarning.store(enable); break;
+    case QtCriticalMsg: bools.enabledCritical.store(enable); break;
+#else
+    case QtDebugMsg: setBoolLane(&enabled, enable, DebugShift); break;
+    case QtWarningMsg: setBoolLane(&enabled, enable, WarningShift); break;
+    case QtCriticalMsg: setBoolLane(&enabled, enable, CriticalShift); break;
+#endif
     case QtFatalMsg: break;
     }
 }
diff --git a/src/corelib/io/qloggingcategory.h b/src/corelib/io/qloggingcategory.h
index 4aec8e63bf..573af2105c 100644
--- a/src/corelib/io/qloggingcategory.h
+++ b/src/corelib/io/qloggingcategory.h
@@ -57,10 +57,15 @@ public:
     bool isEnabled(QtMsgType type) const;
     void setEnabled(QtMsgType type, bool enable);
 
-    bool isDebugEnabled() const { return enabledDebug; }
-    bool isWarningEnabled() const { return enabledWarning; }
-    bool isCriticalEnabled() const { return enabledCritical; }
-
+#ifdef Q_ATOMIC_INT8_IS_SUPPORTED
+    bool isDebugEnabled() const { return bools.enabledDebug.load(); }
+    bool isWarningEnabled() const { return bools.enabledWarning.load(); }
+    bool isCriticalEnabled() const { return bools.enabledCritical.load(); }
+#else
+    bool isDebugEnabled() const { return enabled.load() >> DebugShift & 1; }
+    bool isWarningEnabled() const { return enabled.load() >> WarningShift & 1; }
+    bool isCriticalEnabled() const { return enabled.load() >> CriticalShift & 1; }
+#endif
     const char *categoryName() const { return name; }
 
     // allows usage of both factory method and variable in qCX macros
@@ -78,10 +83,24 @@ private:
     void *d; // reserved for future use
     const char *name;
 
-    bool enabledDebug;
-    bool enabledWarning;
-    bool enabledCritical;
-    bool placeholder[5]; // reserve for future use
+#ifdef Q_BIG_ENDIAN
+    enum { DebugShift = 0, WarningShift = 8, CriticalShift = 16 };
+#else
+    enum { DebugShift = 24, WarningShift = 16, CriticalShift = 8 };
+#endif
+
+    struct AtomicBools {
+#ifdef Q_ATOMIC_INT8_IS_SUPPORTED
+        QBasicAtomicInteger<bool> enabledDebug;
+        QBasicAtomicInteger<bool> enabledWarning;
+        QBasicAtomicInteger<bool> enabledCritical;
+#endif
+    };
+    union {
+        AtomicBools bools;
+        QBasicAtomicInt enabled;
+    };
+    bool placeholder[4]; // reserve for future use
 };
 
 #define Q_DECLARE_LOGGING_CATEGORY(name) \
diff --git a/tests/auto/corelib/io/qdebug/qdebug.pro b/tests/auto/corelib/io/qdebug/qdebug.pro
index 820c17fc69..5e902bb105 100644
--- a/tests/auto/corelib/io/qdebug/qdebug.pro
+++ b/tests/auto/corelib/io/qdebug/qdebug.pro
@@ -1,5 +1,5 @@
 CONFIG += testcase parallel_test
 TARGET = tst_qdebug
-QT = core testlib
+QT = core testlib concurrent
 SOURCES = tst_qdebug.cpp
 DEFINES += QT_DISABLE_DEPRECATED_BEFORE=0
diff --git a/tests/auto/corelib/io/qdebug/tst_qdebug.cpp b/tests/auto/corelib/io/qdebug/tst_qdebug.cpp
index 80144dba20..8fd830a839 100644
--- a/tests/auto/corelib/io/qdebug/tst_qdebug.cpp
+++ b/tests/auto/corelib/io/qdebug/tst_qdebug.cpp
@@ -44,6 +44,9 @@
 #include <QtCore/QtDebug>
 #include <QtTest/QtTest>
 
+#include <QtConcurrentRun>
+#include <QFutureSynchronizer>
+
 class tst_QDebug: public QObject
 {
     Q_OBJECT
@@ -59,6 +62,7 @@ private slots:
     void qDebugQLatin1String() const;
     void textStreamModifiers() const;
     void defaultMessagehandler() const;
+    void threadSafety() const;
 };
 
 void tst_QDebug::assignment() const
@@ -305,5 +309,41 @@ void tst_QDebug::defaultMessagehandler() const
     QVERIFY(same);
 }
 
+QMutex s_mutex;
+QStringList s_messages;
+QSemaphore s_sema;
+
+static void threadSafeMessageHandler(QtMsgType type, const QMessageLogContext &context, const QString &msg)
+{
+    QMutexLocker lock(&s_mutex);
+    s_messages.append(msg);
+    Q_UNUSED(type);
+    Q_UNUSED(context);
+}
+
+static void doDebug() // called in each thread
+{
+    s_sema.acquire();
+    qDebug() << "doDebug";
+}
+
+void tst_QDebug::threadSafety() const
+{
+    MessageHandlerSetter mhs(threadSafeMessageHandler);
+    const int numThreads = 10;
+    QThreadPool::globalInstance()->setMaxThreadCount(numThreads);
+    QFutureSynchronizer<void> sync;
+    for (int i = 0; i < numThreads; ++i) {
+        sync.addFuture(QtConcurrent::run(&doDebug));
+    }
+    s_sema.release(numThreads);
+    sync.waitForFinished();
+    QMutexLocker lock(&s_mutex);
+    QCOMPARE(s_messages.count(), numThreads);
+    for (int i = 0; i < numThreads; ++i) {
+        QCOMPARE(s_messages.at(i), QStringLiteral("doDebug"));
+    }
+}
+
 QTEST_MAIN(tst_QDebug);
 #include "tst_qdebug.moc"