From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <gcc-bugzilla@gcc.gnu.org>
Received: by sourceware.org (Postfix, from userid 48)
	id 5BFC2383FD4A; Tue,  6 Dec 2022 18:03:34 +0000 (GMT)
DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 5BFC2383FD4A
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org;
	s=default; t=1670349814;
	bh=2XMLGXbWaWpdJijr2YrTgW9KsP1+0TAkD146R8S5zbw=;
	h=From:To:Subject:Date:In-Reply-To:References:From;
	b=f7JTRr22+OuVQtDcJWBqYeQL1WzDsgmVrGP3KqWl4ekfD3mmIwdmxukV3hUzNVB5k
	 /wqf/kRkkfqT/TkvG7lQFHllj43TC+kntalLubh204NPXVkGFS/Vt97bUaxLpnNWYU
	 g6WVXXGM44WqzPlaAf/YvBwL7eqkeRbo5X7u57g0=
From: "thiago at kde dot org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug tree-optimization/104475] [12/13 Regression] Wstringop-overflow
 + atomics incorrect warning on dynamic object
Date: Tue, 06 Dec 2022 18:03:33 +0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: tree-optimization
X-Bugzilla-Version: 12.0
X-Bugzilla-Keywords: diagnostic, missed-optimization
X-Bugzilla-Severity: normal
X-Bugzilla-Who: thiago at kde dot org
X-Bugzilla-Status: NEW
X-Bugzilla-Resolution: 
X-Bugzilla-Priority: P2
X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org
X-Bugzilla-Target-Milestone: 12.3
X-Bugzilla-Flags: 
X-Bugzilla-Changed-Fields: 
Message-ID: <bug-104475-4-fnWzrcxdYh@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-104475-4@http.gcc.gnu.org/bugzilla/>
References: <bug-104475-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
List-Id: <gcc-bugs.sourceware.org>

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D104475

--- 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
>=20
> __atomic_or_fetch_4 (184B, 64, 0); [tail call]
>=20
> The path we isolate is d->m_mutex =3D=3D nullptr && !enable in
>=20
> 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=
 !=3D
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 QFIB=
P 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
>=20
>     inline void unlock() noexcept
>     {=20=20=20
>         if (!isLocked)
>             return;
>         m->unlock();
>         isLocked =3D false;
>     }
>=20
> 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).
>=20
> 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 ma=
kes
no sense to me. If the constructor wasn't inlined, it couldn't know the val=
ue
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=3D1b1456975347b044c111694=
58b53c9f6083dbc59).
This probably did change how the optimiser works, explaining why the warnin=
gs
went away.

But it shouldn't have. We went from

    inline ~QMutexLocker() {
        unlock();
    }
    inline void unlock() noexcept
    {
        if (!isLocked)
            return;
        m->unlock();
        isLocked =3D false;
    }

to

    inline ~QMutexLocker()
    {
        if (m_isLocked)
            unlock();
    }
    inline void unlock() noexcept
    {
        Q_ASSERT(m_isLocked);
        m_mutex->unlock();
        m_isLocked =3D false;
    }

with the Q_ASSERT expanding to nothing in release builds, it should be
effectively identical code.=