[Bug tree-optimization/104475] [12/13 Regression] Wstringop-overflow + atomics incorrect warning on dynamic object

["thiago at kde dot org" <gcc-bugzilla@gcc.gnu.org>]

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.