From b3ac039fcc470832e35ac7eaa7ce24d86352ad3f Mon Sep 17 00:00:00 2001 From: David Faure Date: Sun, 9 Mar 2025 14:02:05 +0100 Subject: [PATCH] QRhi: fix data race on rubLogEnabled * Handle QSG_INFO with the proper macro rather than with an invalid const_cast * Remove the racy bool rubLogEnabled, read the atomic from the logging category directly WARNING: ThreadSanitizer: data race (pid=427588) Write of size 1 at 0x7f89d5be2530 by thread T12: #0 QRhiImplementation::prepareForCreate(QRhi*, ...) qrhi.cpp:8863 Previous read of size 1 at 0x7f89d5be2530 by thread T11: #0 QRhiResourceUpdateBatchPrivate::free() qrhi.cpp:9907 Change-Id: Ied5171ea5bb97372daced8afe3ad894d49961069 Reviewed-by: Laszlo Agocs (cherry picked from commit 3cc947e98f6237c86160589143e69c13c965e226) (cherry picked from commit b3f39fdd556be62a277224ba1045caed6affc423) --- src/gui/rhi/qrhi.cpp | 25 +++++++++---------------- src/gui/rhi/qrhi_p.h | 6 +++--- 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/src/gui/rhi/qrhi.cpp b/src/gui/rhi/qrhi.cpp index 774334f8292..e20b526ab5a 100644 --- a/src/gui/rhi/qrhi.cpp +++ b/src/gui/rhi/qrhi.cpp @@ -4,6 +4,7 @@ #include "qrhi_p.h" #include #include +#include "private/qloggingregistry_p.h" #include "qrhinull_p.h" #ifndef QT_NO_OPENGL @@ -24,7 +25,11 @@ QT_BEGIN_NAMESPACE -Q_LOGGING_CATEGORY(QRHI_LOG_INFO, "qt.rhi.general") +// Play nice with QSG_INFO since that is still the most commonly used +// way to get graphics info printed from Qt Quick apps, and the Quick +// scenegraph is our primary user. +Q_LOGGING_CATEGORY_WITH_ENV_OVERRIDE(QRHI_LOG_INFO, "QSG_INFO", "qt.rhi.general") + Q_LOGGING_CATEGORY(QRHI_LOG_RUB, "qt.rhi.rub") /*! @@ -8498,22 +8503,12 @@ QRhi::~QRhi() delete d; } -bool QRhiImplementation::rubLogEnabled = false; - void QRhiImplementation::prepareForCreate(QRhi *rhi, QRhi::Implementation impl, QRhi::Flags flags) { q = rhi; - // Play nice with QSG_INFO since that is still the most commonly used - // way to get graphics info printed from Qt Quick apps, and the Quick - // scenegraph is our primary user. - if (qEnvironmentVariableIsSet("QSG_INFO")) - const_cast(QRHI_LOG_INFO()).setEnabled(QtDebugMsg, true); - debugMarkers = flags.testFlag(QRhi::EnableDebugMarkers); - rubLogEnabled = QRHI_LOG_RUB().isDebugEnabled(); - implType = impl; implThread = QThread::currentThread(); } @@ -9386,7 +9381,7 @@ void QRhiResourceUpdateBatchPrivate::free() bufferLargeAllocTotal += op.data.largeAlloc(); // alloc when > 1 KB } - if (rhi->rubLogEnabled) { + if (QRHI_LOG_RUB().isDebugEnabled()) { qDebug() << "[rub] release to pool upd.batch #" << poolIndex << "/ bufferOps active" << activeBufferOpCount << "of" << bufferOps.count() @@ -10845,8 +10840,7 @@ QRhi::FrameOpResult QRhi::beginFrame(QRhiSwapChain *swapChain, BeginFrameFlags f if (d->inFrame) qWarning("Attempted to call beginFrame() within a still active frame; ignored"); - if (d->rubLogEnabled) - qDebug("[rub] new frame"); + qCDebug(QRHI_LOG_RUB) << "[rub] new frame"; QRhi::FrameOpResult r = !d->inFrame ? d->beginFrame(swapChain, flags) : FrameOpSuccess; if (r == FrameOpSuccess) @@ -10996,8 +10990,7 @@ QRhi::FrameOpResult QRhi::beginOffscreenFrame(QRhiCommandBuffer **cb, BeginFrame if (d->inFrame) qWarning("Attempted to call beginOffscreenFrame() within a still active frame; ignored"); - if (d->rubLogEnabled) - qDebug("[rub] new offscreen frame"); + qCDebug(QRHI_LOG_RUB) << "[rub] new offscreen frame"; QRhi::FrameOpResult r = !d->inFrame ? d->beginOffscreenFrame(cb, flags) : FrameOpSuccess; if (r == FrameOpSuccess) diff --git a/src/gui/rhi/qrhi_p.h b/src/gui/rhi/qrhi_p.h index 68e3533053b..cc4478e4d4a 100644 --- a/src/gui/rhi/qrhi_p.h +++ b/src/gui/rhi/qrhi_p.h @@ -29,6 +29,7 @@ QT_BEGIN_NAMESPACE #define QRHI_RES_RHI(t) t *rhiD = static_cast(m_rhi) Q_DECLARE_LOGGING_CATEGORY(QRHI_LOG_INFO) +Q_DECLARE_LOGGING_CATEGORY(QRHI_LOG_RUB) class QRhiImplementation { @@ -254,7 +255,6 @@ private: QHash keyedCleanupCallbacks; QElapsedTimer pipelineCreationTimer; qint64 accumulatedPipelineCreationTime = 0; - static bool rubLogEnabled; friend class QRhi; friend class QRhiResourceUpdateBatchPrivate; @@ -369,7 +369,7 @@ public: if (!d) { d = new QRhiBufferDataPrivate; } else if (d->ref != 1) { - if (QRhiImplementation::rubLogEnabled) + if (QRHI_LOG_RUB().isDebugEnabled()) qDebug("[rub] QRhiBufferData %p/%p new backing due to no-copy detach, ref was %d", this, d, d->ref); d->ref -= 1; d = new QRhiBufferDataPrivate; @@ -379,7 +379,7 @@ public: memcpy(d->data, s, size); } else { if (d->largeAlloc < size) { - if (QRhiImplementation::rubLogEnabled) + if (QRHI_LOG_RUB().isDebugEnabled()) qDebug("[rub] QRhiBufferData %p/%p new large data allocation %u -> %u", this, d, d->largeAlloc, size); delete[] d->largeData; d->largeAlloc = size;