From b2233f19c9be9b87f4a49a4169aab6f5e7617563 Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Sun, 10 Apr 2022 12:29:47 +0200 Subject: [PATCH] Annotate QMutex with TSAN annotations The Q(Basic)Mutex fast paths are entirely inline in the caller, which means we need to annotate its operations directly or TSAN doesn't know what's going on. Also annotate QRecursiveMutex. The tryLock code could be in principle simplified via a QScopeGuard but I didn't want to make a central class like QMutex depend on it. [ChangeLog][QtCore][QMutex] QMutex now has annotations for ThreadSanitizer. Change-Id: Ibb130404e63a5ec9bcef9675f9addd16a2c38b7f Reviewed-by: David Faure Reviewed-by: Thiago Macieira --- src/corelib/thread/qfutex_p.h | 31 ++------- src/corelib/thread/qmutex.cpp | 12 ++++ src/corelib/thread/qmutex.h | 41 ++++++++++-- src/corelib/thread/qtsan_impl.h | 115 ++++++++++++++++++++++++++++++++ 4 files changed, 169 insertions(+), 30 deletions(-) create mode 100644 src/corelib/thread/qtsan_impl.h diff --git a/src/corelib/thread/qfutex_p.h b/src/corelib/thread/qfutex_p.h index bda8f03ee16..7cb37003b44 100644 --- a/src/corelib/thread/qfutex_p.h +++ b/src/corelib/thread/qfutex_p.h @@ -52,28 +52,7 @@ // #include - -#if (__has_feature(thread_sanitizer) || defined(__SANITIZE_THREAD__)) && __has_include() -# include -inline void _q_tsan_acquire(void *addr, void *addr2 = nullptr) -{ - // A futex call ensures total ordering on the futex words - // (in either success or failure of the call). Instruct TSAN accordingly, - // as TSAN does not understand the futex(2) syscall (or equivalent). - __tsan_acquire(addr); - if (addr2) - __tsan_acquire(addr2); -} -inline void _q_tsan_release(void *addr, void *addr2 = nullptr) -{ - if (addr2) - __tsan_release(addr2); - __tsan_release(addr); -} -#else -inline void _q_tsan_acquire(void *, void * = nullptr) {} -inline void _q_tsan_release(void *, void * = nullptr) {} -#endif // building for TSAN and __has_include() +#include QT_BEGIN_NAMESPACE @@ -114,13 +93,13 @@ namespace QtLinuxFutex { inline int _q_futex(int *addr, int op, int val, quintptr val2 = 0, int *addr2 = nullptr, int val3 = 0) noexcept { - _q_tsan_release(addr, addr2); + QtTsan::futexRelease(addr, addr2); // we use __NR_futex because some libcs (like Android's bionic) don't // provide SYS_futex etc. int result = syscall(__NR_futex, addr, op | FUTEX_PRIVATE_FLAG, val, val2, addr2, val3); - _q_tsan_acquire(addr, addr2); + QtTsan::futexAcquire(addr, addr2); return result; } @@ -176,9 +155,9 @@ constexpr inline bool futexAvailable() { return true; } template inline void futexWait(Atomic &futex, typename Atomic::Type expectedValue) { - _q_tsan_release(&futex); + QtTsan::futexRelease(&futex); WaitOnAddress(&futex, &expectedValue, sizeof(expectedValue), INFINITE); - _q_tsan_acquire(&futex); + QtTsan::futexAcquire(&futex); } template inline bool futexWait(Atomic &futex, typename Atomic::Type expectedValue, qint64 nstimeout) diff --git a/src/corelib/thread/qmutex.cpp b/src/corelib/thread/qmutex.cpp index 1e3ba796247..d88658ab499 100644 --- a/src/corelib/thread/qmutex.cpp +++ b/src/corelib/thread/qmutex.cpp @@ -334,10 +334,14 @@ QRecursiveMutex::~QRecursiveMutex() */ bool QRecursiveMutex::tryLock(int timeout) QT_MUTEX_LOCK_NOEXCEPT { + unsigned tsanFlags = QtTsan::MutexWriteReentrant | QtTsan::TryLock; + QtTsan::mutexPreLock(this, tsanFlags); + Qt::HANDLE self = QThread::currentThreadId(); if (owner.loadRelaxed() == self) { ++count; Q_ASSERT_X(count != 0, "QMutex::lock", "Overflow in recursion counter"); + QtTsan::mutexPostLock(this, tsanFlags, 0); return true; } bool success = true; @@ -349,6 +353,11 @@ bool QRecursiveMutex::tryLock(int timeout) QT_MUTEX_LOCK_NOEXCEPT if (success) owner.storeRelaxed(self); + else + tsanFlags |= QtTsan::TryLockFailed; + + QtTsan::mutexPostLock(this, tsanFlags, 0); + return success; } @@ -412,6 +421,7 @@ bool QRecursiveMutex::tryLock(int timeout) QT_MUTEX_LOCK_NOEXCEPT void QRecursiveMutex::unlock() noexcept { Q_ASSERT(owner.loadRelaxed() == QThread::currentThreadId()); + QtTsan::mutexPreUnlock(this, 0u); if (count > 0) { count--; @@ -419,6 +429,8 @@ void QRecursiveMutex::unlock() noexcept owner.storeRelaxed(nullptr); mutex.unlock(); } + + QtTsan::mutexPostUnlock(this, 0u); } diff --git a/src/corelib/thread/qmutex.h b/src/corelib/thread/qmutex.h index 9423585f641..85279413bfb 100644 --- a/src/corelib/thread/qmutex.h +++ b/src/corelib/thread/qmutex.h @@ -42,6 +42,7 @@ #include #include +#include #include #if __has_include() @@ -104,19 +105,37 @@ public: // BasicLockable concept inline void lock() QT_MUTEX_LOCK_NOEXCEPT { + QtTsan::mutexPreLock(this, 0u); + if (!fastTryLock()) lockInternal(); + + QtTsan::mutexPostLock(this, 0u, 0); } // BasicLockable concept inline void unlock() noexcept { Q_ASSERT(d_ptr.loadRelaxed()); //mutex must be locked + + QtTsan::mutexPreUnlock(this, 0u); + if (!fastTryUnlock()) unlockInternal(); + + QtTsan::mutexPostUnlock(this, 0u); } bool tryLock() noexcept { - return fastTryLock(); + unsigned tsanFlags = QtTsan::TryLock; + QtTsan::mutexPreLock(this, tsanFlags); + + const bool success = fastTryLock(); + + if (!success) + tsanFlags |= QtTsan::TryLockFailed; + QtTsan::mutexPostLock(this, tsanFlags, 0); + + return success; } // Lockable concept @@ -168,9 +187,23 @@ public: using QBasicMutex::tryLock; bool tryLock(int timeout) QT_MUTEX_LOCK_NOEXCEPT { - if (fastTryLock()) - return true; - return lockInternal(timeout); + unsigned tsanFlags = QtTsan::TryLock; + QtTsan::mutexPreLock(this, tsanFlags); + + bool success = fastTryLock(); + + if (success) { + QtTsan::mutexPostLock(this, tsanFlags, 0); + return success; + } + + success = lockInternal(timeout); + + if (!success) + tsanFlags |= QtTsan::TryLockFailed; + QtTsan::mutexPostLock(this, tsanFlags, 0); + + return success; } // TimedLockable concept diff --git a/src/corelib/thread/qtsan_impl.h b/src/corelib/thread/qtsan_impl.h new file mode 100644 index 00000000000..580a738b914 --- /dev/null +++ b/src/corelib/thread/qtsan_impl.h @@ -0,0 +1,115 @@ +/**************************************************************************** +** +** Copyright (C) 2017 Intel Corporation. +** Copyright (C) 2022 Klarälvdalens Datakonsult AB, a KDAB Group company, info@kdab.com, author Giuseppe D'Angelo +** Contact: https://www.qt.io/licensing/ +** +** This file is part of the QtCore module of the Qt Toolkit. +** +** $QT_BEGIN_LICENSE:LGPL$ +** Commercial License Usage +** Licensees holding valid commercial Qt licenses may use this file in +** accordance with the commercial license agreement provided with the +** Software or, alternatively, in accordance with the terms contained in +** a written agreement between you and The Qt Company. For licensing terms +** and conditions see https://www.qt.io/terms-conditions. For further +** information use the contact form at https://www.qt.io/contact-us. +** +** GNU Lesser General Public License Usage +** Alternatively, this file may be used under the terms of the GNU Lesser +** General Public License version 3 as published by the Free Software +** Foundation and appearing in the file LICENSE.LGPL3 included in the +** packaging of this file. Please review the following information to +** ensure the GNU Lesser General Public License version 3 requirements +** will be met: https://www.gnu.org/licenses/lgpl-3.0.html. +** +** GNU General Public License Usage +** Alternatively, this file may be used under the terms of the GNU +** General Public License version 2.0 or (at your option) the GNU General +** Public license version 3 or any later version approved by the KDE Free +** Qt Foundation. The licenses are as published by the Free Software +** Foundation and appearing in the file LICENSE.GPL2 and LICENSE.GPL3 +** included in the packaging of this file. Please review the following +** information to ensure the GNU General Public License requirements will +** be met: https://www.gnu.org/licenses/gpl-2.0.html and +** https://www.gnu.org/licenses/gpl-3.0.html. +** +** $QT_END_LICENSE$ +** +****************************************************************************/ + +#ifndef QTSAN_IMPL_H +#define QTSAN_IMPL_H + +#include + +#if (__has_feature(thread_sanitizer) || defined(__SANITIZE_THREAD__)) && __has_include() +# define QT_BUILDING_UNDER_TSAN +# include +#endif + +QT_BEGIN_NAMESPACE + +namespace QtTsan { +#ifdef QT_BUILDING_UNDER_TSAN +inline void futexAcquire(void *addr, void *addr2 = nullptr) +{ + // A futex call ensures total ordering on the futex words + // (in either success or failure of the call). Instruct TSAN accordingly, + // as TSAN does not understand the futex(2) syscall (or equivalent). + ::__tsan_acquire(addr); + if (addr2) + ::__tsan_acquire(addr2); +} + +inline void futexRelease(void *addr, void *addr2 = nullptr) +{ + if (addr2) + ::__tsan_release(addr2); + ::__tsan_release(addr); +} + +inline void mutexPreLock(void *addr, unsigned flags) +{ + ::__tsan_mutex_pre_lock(addr, flags); +} + +inline void mutexPostLock(void *addr, unsigned flags, int recursion) +{ + ::__tsan_mutex_post_lock(addr, flags, recursion); +} + +inline void mutexPreUnlock(void *addr, unsigned flags) +{ + ::__tsan_mutex_pre_unlock(addr, flags); +} + +inline void mutexPostUnlock(void *addr, unsigned flags) +{ + ::__tsan_mutex_post_unlock(addr, flags); +} + +enum : unsigned { + MutexWriteReentrant = ::__tsan_mutex_write_reentrant, + TryLock = ::__tsan_mutex_try_lock, + TryLockFailed = ::__tsan_mutex_try_lock_failed, +}; +#else +inline void futexAcquire(void *, void * = nullptr) {} +inline void futexRelease(void *, void * = nullptr) {} + +enum : unsigned { + MutexWriteReentrant, + TryLock, + TryLockFailed, +}; +inline void mutexPreLock(void *, unsigned) {} +inline void mutexPostLock(void *, unsigned, int) {} +inline void mutexPreUnlock(void *, unsigned) {} +inline void mutexPostUnlock(void *, unsigned) {} +#endif // QT_BUILDING_UNDER_TSAN +} // namespace QtTsan + +QT_END_NAMESPACE + +#endif // QTSAN_IMPL_H