From b4ead572501179244aa036e7a590fa7536929f2b Mon Sep 17 00:00:00 2001 From: Allan Sandfeld Jensen Date: Tue, 14 May 2019 15:04:18 +0200 Subject: Move away from using 0 as a pointer constant Cleans up most of corelib to use nullptr or default enums where appropriate. Change-Id: Ifcaac14ecdaaee730f87f10941db3ce407d71ef9 Reviewed-by: Thiago Macieira --- src/corelib/global/qlogging.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'src/corelib/global/qlogging.cpp') diff --git a/src/corelib/global/qlogging.cpp b/src/corelib/global/qlogging.cpp index 292a459e5d..49411306c2 100644 --- a/src/corelib/global/qlogging.cpp +++ b/src/corelib/global/qlogging.cpp @@ -1100,8 +1100,8 @@ Q_DECLARE_TYPEINFO(QMessagePattern::BacktraceParams, Q_MOVABLE_TYPE); QBasicMutex QMessagePattern::mutex; QMessagePattern::QMessagePattern() - : literals(0) - , tokens(0) + : literals(nullptr) + , tokens(nullptr) , fromEnvironment(false) { #ifndef QT_BOOTSTRAPPED @@ -1121,9 +1121,9 @@ QMessagePattern::~QMessagePattern() for (int i = 0; literals[i]; ++i) delete [] literals[i]; delete [] literals; - literals = 0; + literals = nullptr; delete [] tokens; - tokens = 0; + tokens = nullptr; } void QMessagePattern::setPattern(const QString &pattern) @@ -1173,7 +1173,7 @@ void QMessagePattern::setPattern(const QString &pattern) // tokenizer QVarLengthArray literalsVar; tokens = new const char*[lexemes.size() + 1]; - tokens[lexemes.size()] = 0; + tokens[lexemes.size()] = nullptr; bool nestedIfError = false; bool inIf = false; @@ -1280,7 +1280,7 @@ void QMessagePattern::setPattern(const QString &pattern) qt_message_print(error); literals = new const char*[literalsVar.size() + 1]; - literals[literalsVar.size()] = 0; + literals[literalsVar.size()] = nullptr; memcpy(literals, literalsVar.constData(), literalsVar.size() * sizeof(const char*)); } @@ -1406,7 +1406,7 @@ QString qFormatLogMessage(QtMsgType type, const QMessageLogContext &context, con #endif // we do not convert file, function, line literals to local encoding due to overhead - for (int i = 0; pattern->tokens[i] != 0; ++i) { + for (int i = 0; pattern->tokens[i]; ++i) { const char *token = pattern->tokens[i]; if (token == endifTokenC) { skip = false; -- cgit v1.2.3 From c4a94edbbc3a59d955bf2721e51b1448d00e0483 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Sat, 25 May 2019 08:58:00 +0200 Subject: QMessagePattern: replace manual memory handling with std::unique_ptr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dealing with 'tokens' is straight-forward. With 'literals', it is not quite so straight-forward, because the ownership chain here is two levels deep. But it's still worthwhile, because it replaces quite error-prone code with code which may be a bit more verbose, but is totally safe. As a drive-by, moved initialization of the fromEnvironment member to the body of the ctor in order to avoid code-churn (I needed to touch the ctor-init-list anyway). The QMessagePattern dtor is now empty and consequently defaulted. Change-Id: Iadb25e7aba1c5a94fd9068be7ae03f17e975328b Reviewed-by: Mårten Nordheim --- src/corelib/global/qlogging.cpp | 37 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 25 deletions(-) (limited to 'src/corelib/global/qlogging.cpp') diff --git a/src/corelib/global/qlogging.cpp b/src/corelib/global/qlogging.cpp index 49411306c2..8db6ab630a 100644 --- a/src/corelib/global/qlogging.cpp +++ b/src/corelib/global/qlogging.cpp @@ -158,6 +158,9 @@ static QT_PREPEND_NAMESPACE(qint64) qt_gettid() #endif // !QT_BOOTSTRAPPED #include +#include +#include +#include #include @@ -1076,8 +1079,8 @@ struct QMessagePattern { void setPattern(const QString &pattern); // 0 terminated arrays of literal tokens / literal or placeholder tokens - const char **literals; - const char **tokens; + std::unique_ptr[]> literals; + std::unique_ptr tokens; QList timeArgs; // timeFormats in sequence of %{time #ifndef QT_BOOTSTRAPPED QElapsedTimer timer; @@ -1100,9 +1103,6 @@ Q_DECLARE_TYPEINFO(QMessagePattern::BacktraceParams, Q_MOVABLE_TYPE); QBasicMutex QMessagePattern::mutex; QMessagePattern::QMessagePattern() - : literals(nullptr) - , tokens(nullptr) - , fromEnvironment(false) { #ifndef QT_BOOTSTRAPPED timer.start(); @@ -1110,6 +1110,7 @@ QMessagePattern::QMessagePattern() const QString envPattern = QString::fromLocal8Bit(qgetenv("QT_MESSAGE_PATTERN")); if (envPattern.isEmpty()) { setPattern(QLatin1String(defaultPattern)); + fromEnvironment = false; } else { setPattern(envPattern); fromEnvironment = true; @@ -1117,23 +1118,10 @@ QMessagePattern::QMessagePattern() } QMessagePattern::~QMessagePattern() -{ - for (int i = 0; literals[i]; ++i) - delete [] literals[i]; - delete [] literals; - literals = nullptr; - delete [] tokens; - tokens = nullptr; -} + = default; void QMessagePattern::setPattern(const QString &pattern) { - if (literals) { - for (int i = 0; literals[i]; ++i) - delete [] literals[i]; - delete [] literals; - } - delete [] tokens; timeArgs.clear(); #ifdef QLOGGING_HAVE_BACKTRACE backtraceArgs.clear(); @@ -1171,8 +1159,8 @@ void QMessagePattern::setPattern(const QString &pattern) lexemes.append(lexeme); // tokenizer - QVarLengthArray literalsVar; - tokens = new const char*[lexemes.size() + 1]; + std::vector> literalsVar; + tokens.reset(new const char*[lexemes.size() + 1]); tokens[lexemes.size()] = nullptr; bool nestedIfError = false; @@ -1267,7 +1255,7 @@ void QMessagePattern::setPattern(const QString &pattern) char *literal = new char[lexeme.size() + 1]; strncpy(literal, lexeme.toLatin1().constData(), lexeme.size()); literal[lexeme.size()] = '\0'; - literalsVar.append(literal); + literalsVar.emplace_back(literal); tokens[i] = literal; } } @@ -1279,9 +1267,8 @@ void QMessagePattern::setPattern(const QString &pattern) if (!error.isEmpty()) qt_message_print(error); - literals = new const char*[literalsVar.size() + 1]; - literals[literalsVar.size()] = nullptr; - memcpy(literals, literalsVar.constData(), literalsVar.size() * sizeof(const char*)); + literals.reset(new std::unique_ptr[literalsVar.size() + 1]); + std::move(literalsVar.begin(), literalsVar.end(), &literals[0]); } #if defined(QLOGGING_HAVE_BACKTRACE) && !defined(QT_BOOTSTRAPPED) -- cgit v1.2.3 From 34fe9232dbf6a9fe58ebc4c7680bb67d2f642c40 Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Mon, 10 Jun 2019 11:08:29 +0200 Subject: Port from QAtomic::load() to loadRelaxed() Semi-automated, just needed ~20 manual fixes: $ find \( -iname \*.cpp -or -iname \*.h \) -exec perl -pe 's/(\.|->)load\(\)/$1loadRelaxed\(\)/g' -i \{\} + $ find \( -iname \*.cpp -or -iname \*.h \) -exec perl -pe 's/(\.|->)store\(/$1storeRelaxed\(/g' -i \{\} + It can be easily improved (e.g. for store check that there are no commas after the opening parens). The most common offender is QLibrary::load, and some code using std::atomic directly. Change-Id: I07c38a3c8ed32c924ef4999e85c7e45cf48f0f6c Reviewed-by: Marc Mutz --- src/corelib/global/qlogging.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'src/corelib/global/qlogging.cpp') diff --git a/src/corelib/global/qlogging.cpp b/src/corelib/global/qlogging.cpp index 8db6ab630a..60ed619aae 100644 --- a/src/corelib/global/qlogging.cpp +++ b/src/corelib/global/qlogging.cpp @@ -197,7 +197,7 @@ static bool isFatal(QtMsgType msgType) // it's fatal if the current value is exactly 1, // otherwise decrement if it's non-zero - return fatalCriticals.load() && fatalCriticals.fetchAndAddRelaxed(-1) == 1; + return fatalCriticals.loadRelaxed() && fatalCriticals.fetchAndAddRelaxed(-1) == 1; } if (msgType == QtWarningMsg || msgType == QtCriticalMsg) { @@ -205,7 +205,7 @@ static bool isFatal(QtMsgType msgType) // it's fatal if the current value is exactly 1, // otherwise decrement if it's non-zero - return fatalWarnings.load() && fatalWarnings.fetchAndAddRelaxed(-1) == 1; + return fatalWarnings.loadRelaxed() && fatalWarnings.fetchAndAddRelaxed(-1) == 1; } return false; @@ -1814,11 +1814,11 @@ static void qt_message_print(QtMsgType msgType, const QMessageLogContext &contex // itself, e.g. by using Qt API if (grabMessageHandler()) { // prefer new message handler over the old one - if (msgHandler.load() == qDefaultMsgHandler - || messageHandler.load() != qDefaultMessageHandler) { - (*messageHandler.load())(msgType, context, message); + if (msgHandler.loadRelaxed() == qDefaultMsgHandler + || messageHandler.loadRelaxed() != qDefaultMessageHandler) { + (*messageHandler.loadRelaxed())(msgType, context, message); } else { - (*msgHandler.load())(msgType, message.toLocal8Bit().constData()); + (*msgHandler.loadRelaxed())(msgType, message.toLocal8Bit().constData()); } ungrabMessageHandler(); } else { -- cgit v1.2.3 From cd401b74a13cd9d9a47d977f195c7985cf725d55 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Fri, 21 Jun 2019 22:53:14 +0200 Subject: 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 --- src/corelib/global/qlogging.cpp | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) (limited to 'src/corelib/global/qlogging.cpp') 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 msgHandler = Q_BASIC_ATOMIC_INITIALIZER(qDefaultMsgHandler); +static QBasicAtomicPointer msgHandler = Q_BASIC_ATOMIC_INITIALIZER(nullptr); // pointer to QtMessageHandler debug handler (with context) -static QBasicAtomicPointer messageHandler = Q_BASIC_ATOMIC_INITIALIZER(qDefaultMessageHandler); +static QBasicAtomicPointer 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) -- cgit v1.2.3