Details
-
Bug
-
Resolution: Done
-
P3: Somewhat important
-
5.12.2
-
None
-
567837a7422fe8d96011fbb3b23e7f34bc344e73 (qt/qtbase/5.12)
Description
QObjectPrivate::addConnection and QObjectPrivete::cleanConnectionLists may modify connectedSignals, and are correctly protected by a mutex. On the other hand, QObjectPrivate::isSignalConnected reads connectedSignals, but is NOT protected by a mutex.
As these accesses are not atomic and there exists no happens-before relationship, the behavior is undefined and makes TSAN unhappy.
One fix is to add a mutex, but they could also be made QAtomicIntegers (as in the patch below) Our benchmarking showed that there was a larger performance hit for the mutex option over the atomic.
--- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -232,7 +232,7 @@ QObjectPrivate::QObjectPrivate(int version) receiveChildEvents = true; postedEvents = 0; extraData = 0; - connectedSignals[0] = connectedSignals[1] = 0; + // connectedSignals[0] = connectedSignals[1] = 0; // already 0. metaObject = 0; isWindow = false; deleteLaterCalled = false; @@ -411,9 +411,10 @@ void QObjectPrivate::addConnection(int signal, Connection *c) c->next->prev = &c->next; if (signal < 0) { - connectedSignals[0] = connectedSignals[1] = ~0; + connectedSignals[0].store(~0); + connectedSignals[1].store(~0); } else if (signal < (int)sizeof(connectedSignals) * 8) { - connectedSignals[signal >> 5] |= (1 << (signal & 0x1f)); + connectedSignals[signal >> 5].store(connectedSignals[signal >> 5].load() | (1 << (signal & 0x1f))); } } @@ -455,7 +456,7 @@ void QObjectPrivate::cleanConnectionLists() if (!allConnected && !connected && signal >= 0 && size_t(signal) < sizeof(connectedSignals) * 8) { // This signal is no longer connected - connectedSignals[signal >> 5] &= ~(1 << (signal & 0x1f)); + connectedSignals[signal >> 5].store(connectedSignals[signal >> 5].load() & ~(1 << (signal & 0x1f))); } else if (signal == -1) { allConnected = connected; } --- a/src/corelib/kernel/qobject_p.h +++ b/src/corelib/kernel/qobject_p.h @@ -232,7 +232,7 @@ public: Connection *senders; // linked list of connections connected to this object Sender *currentSender; // object currently activating the object - mutable quint32 connectedSignals[2]; + mutable QAtomicInteger<quint32> connectedSignals[2]; union { QObject *currentChildBeingDeleted; // should only be used when QObjectData::isDeletingChildren is set @@ -257,7 +257,7 @@ Q_DECLARE_TYPEINFO(QObjectPrivate::ConnectionList, Q_MOVABLE_TYPE); inline bool QObjectPrivate::isSignalConnected(uint signal_index, bool checkDeclarative) const { return signal_index >= sizeof(connectedSignals) * 8 - || (connectedSignals[signal_index >> 5] & (1 << (signal_index & 0x1f)) + || (connectedSignals[signal_index >> 5].load() & (1 << (signal_index & 0x1f)) || (checkDeclarative && isDeclarativeSignalConnected(signal_index))); }