https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104475
--- Comment #19 from Thiago Macieira <thiago at kde dot org> ---
(In reply to Richard Biener from comment #15)
> Thanks, it's still the same reason - we isolate a nullptr case and end up
> with
>
> __atomic_or_fetch_4 (184B, 64, 0); [tail call]
>
> The path we isolate is d->m_mutex == nullptr && !enable in
>
> void QFutureInterfaceBase::setThrottled(bool enable)
> {
> QMutexLocker lock(&d->m_mutex);
Thank you for the analysis, Richard. But do note that it's &d->m_mutex, not
d->m_mutex that is passed to the locker. C++ says that if you do d-> then d !=
nullptr, so &d->m_mutex can't be nullptr either.
However, I guess GCC thinks it can be because the offset of m_mutex in QFIBP is
zero. pahole says:
public:
void QFutureInterfaceBasePrivate(class QFutureInterfaceBasePrivate *,
enum State);
void ~QFutureInterfaceBasePrivate(class QFutureInterfaceBasePrivate *,
int);
class QMutex m_mutex; /* 0 8 */
class QBasicMutex continuationMutex; /* 8 8 */
So there's a missed optimisation here. But it doesn't look like GCC is the only
one to miss it, see https://gcc.godbolt.org/z/WW5hbW6sW. Maybe it's an
intentional choice?
> we predict the path to be unlikely but the adjustment to the threader
> covered probably never executed paths (with probability zero). The
> threading opportunity arises because the DTOR calls
>
> inline void unlock() noexcept
> {
> if (!isLocked)
> return;
> m->unlock();
> isLocked = false;
> }
>
> and we know isLocked on the nullptr path.
We know it can't be true.
> I thought we could maybe enhance prediction to look for nullptr based
> accesses but at the time we estimate probabilities the QMutexLocker
> CTOR isn't yet inlined (the DTOR is partially inlined, exposing the
> isLocked check).
>
> Note the "impossible" path is actually in the sources - so there might
> be a missing conditional somewhere.
I don't see it, but that's probably because I'm looking at it from the C++
side. If the mutex pointer that was passed is null, then isLocked is never set
to true. What you're saying is that the unlock() function above was inlined and
that GCC knew m to be nullptr, but didn't know isLocked's value... which makes
no sense to me. If the constructor wasn't inlined, it couldn't know the value
of m either. If the constructor was inlined, then it should know the value of
both.
Anyway, this discussion made me realise there's a series of changes to
QMutexLocker ending in "QMutexLocker: strenghten the locking operations"
(https://code.qt.io/cgit/qt/qtbase.git/commit/?id=1b1456975347b044c11169458b53c9f6083dbc59).
This probably did change how the optimiser works, explaining why the warnings
went away.
But it shouldn't have. We went from
inline ~QMutexLocker() {
unlock();
}
inline void unlock() noexcept
{
if (!isLocked)
return;
m->unlock();
isLocked = false;
}
to
inline ~QMutexLocker()
{
if (m_isLocked)
unlock();
}
inline void unlock() noexcept
{
Q_ASSERT(m_isLocked);
m_mutex->unlock();
m_isLocked = false;
}
with the Q_ASSERT expanding to nothing in release builds, it should be
effectively identical code.
