Optimize and fix handling of QtMessageHandlers

A function may almost always have static storage duration, but that
does not necessarily mean that we can store and load pointers to them
without memory ordering. Play it safe and use store-release and
load-acquire for them (which combines to ordered for the fetchAndSet
call in qInstall*Handler(), as we don't know what the caller will do
with the returned function pointer).

Also change the initial value of the atomic pointer to nullptr.
Nullptr already signified the default handler in qInstall*Handler(),
so the API doesn't change. But by using nullptr to mean default, we
place these variables in the BSS segment instead of TEXT, save dynamic
init, or at least a relocation, and we dodge the smelly comparison of
function pointers, using comparison against nullptr instead.

Also, as a drive-by, put the call to ungrapMessageHandler() in a
scope-guard. Both the message handler, as well as the Qt code calling
it (toLocal8Bit()!), may throw, and that would stop all further
logging.

The code still has one problem: When a logging action is underway, and
another thread exchanges the message handler, we might still execute
code in the old handler. This is probably not a problem in practice,
since no-one will use a dynamically-compiled function for logging
(right? :), but should probably be documented or fixed. This patch
does not address this issue, though.

Change-Id: I21aa907288b9c8c6646787b4001002d145b114a5
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
 
 
 

1 files changed, 19 insertions, 15 deletions

diff --git a/src/corelib/global/qlogging.cpp b/src/corelib/global/qlogging.cpp
index 60ed619aae..621b6d7d13 100644
--- a/src/corelib/global/qlogging.cpp
+++ b/src/corelib/global/qlogging.cpp
@@ -44,6 +44,7 @@
 #include "qlogging_p.h"
 #include "qlist.h"
 #include "qbytearray.h"
+#include "qscopeguard.h"
 #include "qstring.h"
 #include "qvarlengtharray.h"
 #include "qdebug.h"
@@ -1499,9 +1500,9 @@ static void qDefaultMsgHandler(QtMsgType type, const char *buf);
 static void qDefaultMessageHandler(QtMsgType type, const QMessageLogContext &context, const QString &buf);
 
 // pointer to QtMsgHandler debug handler (without context)
-static QBasicAtomicPointer<void (QtMsgType, const char*)> msgHandler = Q_BASIC_ATOMIC_INITIALIZER(qDefaultMsgHandler);
+static QBasicAtomicPointer<void (QtMsgType, const char*)> msgHandler = Q_BASIC_ATOMIC_INITIALIZER(nullptr);
 // pointer to QtMessageHandler debug handler (with context)
-static QBasicAtomicPointer<void (QtMsgType, const QMessageLogContext &, const QString &)> messageHandler = Q_BASIC_ATOMIC_INITIALIZER(qDefaultMessageHandler);
+static QBasicAtomicPointer<void (QtMsgType, const QMessageLogContext &, const QString &)> messageHandler = Q_BASIC_ATOMIC_INITIALIZER(nullptr);
 
 // ------------------------ Alternate logging sinks -------------------------
 
@@ -1813,14 +1814,15 @@ static void qt_message_print(QtMsgType msgType, const QMessageLogContext &contex
     // prevent recursion in case the message handler generates messages
     // itself, e.g. by using Qt API
     if (grabMessageHandler()) {
+        const auto ungrab = qScopeGuard([]{ ungrabMessageHandler(); });
+        auto oldStyle = msgHandler.loadAcquire();
+        auto newStye = messageHandler.loadAcquire();
         // prefer new message handler over the old one
-        if (msgHandler.loadRelaxed() == qDefaultMsgHandler
-                || messageHandler.loadRelaxed() != qDefaultMessageHandler) {
-            (*messageHandler.loadRelaxed())(msgType, context, message);
+        if (newStye || !oldStyle) {
+            (newStye ? newStye : qDefaultMessageHandler)(msgType, context, message);
         } else {
-            (*msgHandler.loadRelaxed())(msgType, message.toLocal8Bit().constData());
+            (oldStyle ? oldStyle : qDefaultMsgHandler)(msgType, message.toLocal8Bit().constData());
         }
-        ungrabMessageHandler();
     } else {
         fprintf(stderr, "%s\n", message.toLocal8Bit().constData());
     }
@@ -2071,18 +2073,20 @@ void qErrnoWarning(int code, const char *msg, ...)
 
 QtMessageHandler qInstallMessageHandler(QtMessageHandler h)
 {
-    if (!h)
-        h = qDefaultMessageHandler;
-    //set 'h' and return old message handler
-    return messageHandler.fetchAndStoreRelaxed(h);
+    const auto old = messageHandler.fetchAndStoreOrdered(h);
+    if (old)
+        return old;
+    else
+        return qDefaultMessageHandler;
 }
 
 QtMsgHandler qInstallMsgHandler(QtMsgHandler h)
 {
-    if (!h)
-        h = qDefaultMsgHandler;
-    //set 'h' and return old message handler
-    return msgHandler.fetchAndStoreRelaxed(h);
+    const auto old = msgHandler.fetchAndStoreOrdered(h);
+    if (old)
+        return old;
+    else
+        return qDefaultMsgHandler;
 }
 
 void qSetMessagePattern(const QString &pattern)