[Bug c++/104475] New: Wstringop-overflow + atomics incorrect warning on dynamic object

[Bug c++/104475] New: Wstringop-overflow + atomics incorrect warning on dynamic object

[thiago at kde dot org]

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

            Bug ID: 104475
           Summary: Wstringop-overflow + atomics incorrect warning on
                    dynamic object
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: thiago at kde dot org
  Target Milestone: ---

Created attachment 52399
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52399&action=edit
qfutureinterface.cpp preprocessed

In:
static inline int switch_on(QAtomicInt &a, int which)
{
    return a.fetchAndOrRelaxed(which) | which;
}

static inline int switch_off(QAtomicInt &a, int which)
{
    return a.fetchAndAndRelaxed(~which) & ~which;
}

void QFutureInterfaceBase::setThrottled(bool enable)
{
    QMutexLocker lock(&d->m_mutex);
    if (enable) {
        switch_on(d->state, Throttled);
    } else {
        switch_off(d->state, Throttled);
        if (!(d->state.loadRelaxed() & suspendingOrSuspended))
            d->pausedWaitCondition.wakeAll();
    }
}

Compiling the attached preprocessed sources with:

g++ -Wall -Wextra -march=haswell -O2 -c -o /dev/null qfutureinterface.cpp.ii

Produces:

In member function ‘std::__atomic_base<_IntTp>::__int_type
std::__atomic_base<_IntTp>::fetch_or(__int_type, std::memory_order) [with _ITp
= int]’,
    inlined from ‘static T QAtomicOps<X>::fetchAndOrRelaxed(std::atomic<T>&,
typename QAtomicAdditiveType<T>::AdditiveT) [with T = int; X = int]’ at
/home/tjmaciei/obj/qt/qt6/qtbase/include/QtCore/../../../../../../src/qt/qt6/qtbase/src/corelib/thread/qatomic_cxx11.h:449:33,
    inlined from ‘T QBasicAtomicInteger<T>::fetchAndOrRelaxed(T) [with T =
int]’ at
/home/tjmaciei/obj/qt/qt6/qtbase/include/QtCore/../../../../../../src/qt/qt6/qtbase/src/corelib/thread/qbasicatomic.h:168:36,
    inlined from ‘int switch_on(QAtomicInt&, int)’ at
/home/tjmaciei/src/qt/qt6/qtbase/src/corelib/thread/qfutureinterface.cpp:59:31,
    inlined from ‘void QFutureInterfaceBase::setThrottled(bool)’ at
/home/tjmaciei/src/qt/qt6/qtbase/src/corelib/thread/qfutureinterface.cpp:71:18:
/home/tjmaciei/dev/gcc/include/c++/12.0.1/bits/atomic_base.h:648:33: warning:
‘unsigned int __atomic_or_fetch_4(volatile void*, unsigned int, int)’ writing 4
bytes into a region of size 0 overflows the destination [-Wstringop-overflow=]
  648 |       { return __atomic_fetch_or(&_M_i, __i, int(__m)); }
      |                ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
In member function ‘std::__atomic_base<_IntTp>::__int_type
std::__atomic_base<_IntTp>::fetch_and(__int_type, std::memory_order) [with _ITp
= int]’,
    inlined from ‘static T QAtomicOps<X>::fetchAndAndRelaxed(std::atomic<T>&,
typename QAtomicAdditiveType<T>::AdditiveT) [with T = int; X = int]’ at
/home/tjmaciei/obj/qt/qt6/qtbase/include/QtCore/../../../../../../src/qt/qt6/qtbase/src/corelib/thread/qatomic_cxx11.h:425:34,
    inlined from ‘T QBasicAtomicInteger<T>::fetchAndAndRelaxed(T) [with T =
int]’ at
/home/tjmaciei/obj/qt/qt6/qtbase/include/QtCore/../../../../../../src/qt/qt6/qtbase/src/corelib/thread/qbasicatomic.h:159:37,
    inlined from ‘int switch_off(QAtomicInt&, int)’ at
/home/tjmaciei/src/qt/qt6/qtbase/src/corelib/thread/qfutureinterface.cpp:64:32,
    inlined from ‘void QFutureInterfaceBase::setThrottled(bool)’ at
/home/tjmaciei/src/qt/qt6/qtbase/src/corelib/thread/qfutureinterface.cpp:73:19:
/home/tjmaciei/dev/gcc/include/c++/12.0.1/bits/atomic_base.h:638:34: warning:
‘unsigned int __atomic_fetch_and_4(volatile void*, unsigned int, int)’ writing
4 bytes into a region of size 0 overflows the destination
[-Wstringop-overflow=]
  638 |       { return __atomic_fetch_and(&_M_i, __i, int(__m)); }
      |                ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~

GCC Git commit 1ce5395977f37e8d0c03394f7b932a584ce85cc7, built today.

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

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Wstringop-overflow +        |[12 Regression]
                   |atomics incorrect warning   |Wstringop-overflow +
                   |on dynamic object           |atomics incorrect warning
                   |                            |on dynamic object
          Component|c++                         |tree-optimization
   Target Milestone|---                         |12.0
           Keywords|                            |diagnostic, needs-reduction

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

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |msebor at gcc dot gnu.org
   Last reconfirmed|                            |2022-02-09
             Blocks|                            |88443
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

--- Comment #1 from Martin Sebor <msebor at gcc dot gnu.org> ---
Both warnings trigger for the same reason.  The second one is for the
__atomic_or_fetch_4() call in basic block 4 below with the invalid pointer as
the first argument (184B).

  <bb 2> [local count: 1073741824]:
  _1 = this_10(D)->d;
  _2 = &_1->m_mutex;                              <<< address of a member can
never be null
  MEM[(struct __as_base  &)&lock] ={v} {CLOBBER};
  if (_2 != 0B)                                   <<< must be true
    goto <bb 5>; [90.00%]
  else
    goto <bb 3>; [10.00%]                          <<< never taken

  <bb 3> [local count: 107374184]:
  if (enable_12(D) != 0)
    goto <bb 4>; [50.00%]
  else
    goto <bb 18>; [50.00%]

  <bb 4> [local count: 53687092]:
  __atomic_or_fetch_4 (184B, 64, 0); [tail call]   <<< warning here
  goto <bb 11>; [100.00%]

The call is emitted as a result of the test in bb 2.  That in turn is the
result of the test for null in the QMutexLocker ctor:

    inline explicit QMutexLocker(Mutex *mutex) noexcept
    {
        m = mutex;
        if (__builtin_expect(!!(mutex), true)) {
            mutex->lock();
            isLocked = true;
        }
    }

The test should be pointless in setThrottled() where the ctor is invoked
because the address of no member can be null:

void QFutureInterfaceBase::setThrottled(bool enable)
{
    QMutexLocker lock(&d->m_mutex);
    if (enable) {
        switch_on(d->state, Throttled);

The invalid call first appears in the dom2 dump so I'm guessing it's the result
of the prior thread1 pass.

The test (and the warning) can be avoided by asserting that in setThrottled()
like so:

    if (!d) __builtin_unreachable ();
    QMutexLocker lock(&d->m_mutex);

GCC folds equality tests involving the addresses of members, except for the
first one when it involves a pointer access (below), presumably out of an
overabundance of caution, in case the pointer is null (GCC 4.1 eliminated all
tests but that's changed since then).  The warning GCC issues indicates it
considers the address to be nonnull regardless (that's new in GCC 12).

$ cat t.c && gcc -O2 -S -Wall -march=haswell -fdump-tree-optimized=/dev/stdout
t.c
struct A { int x, y; };

void f (struct A *p)
{
  if (!&p->y)              // -Waddress
    __builtin_abort ();    // eliminated
}

void g (struct A *p)
{
  if (!&p->x)              // -Waddress
    __builtin_abort ();    // not eliminated
}
t.c: In function ‘f’:
t.c:5:7: warning: the comparison will always evaluate as ‘true’ for the address
of ‘y’ will never be NULL [-Waddress]
    5 |   if (!&p->y)              // -Waddress
      |       ^
t.c:1:19: note: ‘y’ declared here
    1 | struct A { int x, y; };
      |                   ^
t.c: In function ‘g’:
t.c:11:7: warning: the comparison will always evaluate as ‘true’ for the
address of ‘x’ will never be NULL [-Waddress]
   11 |   if (!&p->x)              // -Waddress
      |       ^
t.c:1:16: note: ‘x’ declared here
    1 | struct A { int x, y; };
      |                ^

;; Function f (f, funcdef_no=0, decl_uid=2479, cgraph_uid=1, symbol_order=0)

void f (struct A * p)
{
  <bb 2> [local count: 1073741824]:
  return;

}



;; Function g (g, funcdef_no=1, decl_uid=2482, cgraph_uid=2, symbol_order=1)

void g (struct A * p)
{
  int * _1;

  <bb 2> [local count: 1073741824]:
  _1 = &p_2(D)->x;
  if (_1 == 0B)
    goto <bb 3>; [0.00%]
  else
    goto <bb 4>; [100.00%]

  <bb 3> [count: 0]:
  __builtin_abort ();

  <bb 4> [local count: 1073741824]:
  return;

}


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88443
[Bug 88443] [meta-bug] bogus/missing -Wstringop-overflow warnings

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

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |aldyh at gcc dot gnu.org,
                   |                            |jakub at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I'm afraid the amount of code in the wild that assumes that &ptr->first_member
== NULL for ptr == NULL is huge in real-world code, so I'm not sure if we can
afford to fold such comparisons to false.
But perhaps the threader shouldn't thread such paths because we should consider
them extremely unlikely.

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

[aldyh at gcc dot gnu.org]

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

Aldy Hernandez <aldyh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amacleod at redhat dot com

--- Comment #3 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
This isn't the threader but VRP/ranger.

What happens is that the threader isolates the path, making it easier for VRP
to see the equivalence, and then CCP4 folds the constant into the problematic
call.  This is from the .ccp4 pass:

Folding statement: __atomic_or_fetch_4 (pretmp_29, 64, 0);
Folded into: __atomic_or_fetch_4 (184B, 64, 0);

In VRP2 the ranger is folding:

Folding statement: pretmp_29 = &MEM[(struct __atomic_base *)_1 + 184B]._M_i;
Folded into: pretmp_29 = 184B;

The ranger is determining that _1 is 0 because it has determined that since _2
is 0 on the 2->3 edge, so is _1, as m_mutex is the first field of _1:

=========== BB 2 ============
Imports: _1  
Exports: _1  _2  
         _2 : _1(I)  
    <bb 2> [local count: 1073741824]:
    _1 = this_10(D)->d;
    _2 = &_1->m_mutex;
    MEM[(struct __as_base  &)&lock] ={v} {CLOBBER};
    if (_2 != 0B)
      goto <bb 5>; [90.00%]
    else
      goto <bb 3>; [10.00%]

2->5  (T) _1 :  struct QFutureInterfaceBasePrivate * [1B, +INF]
2->5  (T) _2 :  struct QMutex * [1B, +INF]
2->3  (F) _1 :  struct QFutureInterfaceBasePrivate * [0B, 0B]
2->3  (F) _2 :  struct QMutex * [0B, 0B]

Andrew, how/where is that we relate _1 and _2 here?  I can't seem to find it.

My gut feeling is that special casing anything in the ranger for this is wrong.

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

[amacleod at redhat dot com]

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

--- Comment #4 from Andrew Macleod <amacleod at redhat dot com> ---
(In reply to Aldy Hernandez from comment #3)
> This isn't the threader but VRP/ranger.
> 
> What happens is that the threader isolates the path, making it easier for
> VRP to see the equivalence, and then CCP4 folds the constant into the
> problematic call.  This is from the .ccp4 pass:
> 
> Folding statement: __atomic_or_fetch_4 (pretmp_29, 64, 0);
> Folded into: __atomic_or_fetch_4 (184B, 64, 0);
> 
> In VRP2 the ranger is folding:
> 
> Folding statement: pretmp_29 = &MEM[(struct __atomic_base *)_1 + 184B]._M_i;
> Folded into: pretmp_29 = 184B;
> 
> The ranger is determining that _1 is 0 because it has determined that since
> _2 is 0 on the 2->3 edge, so is _1, as m_mutex is the first field of _1:
> 
> =========== BB 2 ============
> Imports: _1  
> Exports: _1  _2  
>          _2 : _1(I)  
>     <bb 2> [local count: 1073741824]:
>     _1 = this_10(D)->d;
>     _2 = &_1->m_mutex;
>     MEM[(struct __as_base  &)&lock] ={v} {CLOBBER};
>     if (_2 != 0B)
>       goto <bb 5>; [90.00%]
>     else
>       goto <bb 3>; [10.00%]
> 
> 2->5  (T) _1 : 	struct QFutureInterfaceBasePrivate * [1B, +INF]
> 2->5  (T) _2 : 	struct QMutex * [1B, +INF]
> 2->3  (F) _1 : 	struct QFutureInterfaceBasePrivate * [0B, 0B]
> 2->3  (F) _2 : 	struct QMutex * [0B, 0B]
> 
> Andrew, how/where is that we relate _1 and _2 here?  I can't seem to find it.
> 
> My gut feeling is that special casing anything in the ranger for this is
> wrong.


Its via op1_range for OP_ADDR:  
--param=ranger-debug=tracegori shows:

2120    GORI  outgoing_edge for _1 on edge 2->3
2121    GORI    compute op 1 (_2) at if (_2 != 0B)
        GORI      LHS =bool [0, 0]
        GORI      Computes _2 = struct QMutex * [0B, 0B] intersect Known range
: struct QMutex * VARYING
        GORI    TRUE : (2121) produces  (_2) struct QMutex * [0B, 0B]
2122    GORI    compute op 1 (_1) at _2 = &_1->m_mutex;
        GORI      LHS =struct QMutex * [0B, 0B]
        GORI      Computes _1 = struct QFutureInterfaceBasePrivate * [0B, 0B]
intersect Known range : struct QFutureInterfaceBasePrivate * VARYING
        GORI    TRUE : (2122) produces  (_1) struct QFutureInterfaceBasePrivate
* [0B, 0B]
        GORI  TRUE : (2120) outgoing_edge (_1) struct
QFutureInterfaceBasePrivate * [0B, 0B]

so with _2 == 0, the 2122 trace element is solving for _1 in
    _2 = &_1->m_mutex
[0,0] = &_1->m_mutex

is  it possible for _1 to be anything other than 0 in this case?  If so we need
to adjust range-ops

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

[amacleod at redhat dot com]

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

--- Comment #5 from Andrew Macleod <amacleod at redhat dot com> ---
We do have the option of not trying to determine anything about _1 in
situations like this..
I tried removing the op1_range() routine for addr_expr, and we pass all tests
just fine.

we would pick up non-null, if relevant,  during normal processing. It may not
be necessary to try to determine things like this during the outgoing edge
calculation.. especially if its causing issues...

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

[amacleod at redhat dot com]

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

--- Comment #6 from Andrew Macleod <amacleod at redhat dot com> ---
Created attachment 52564
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52564&action=edit
possible patch

Perhaps better would be to not try to propagate NULL-ness.

    _1 = this_10(D)->d;
    _2 = &_1->m_mutex;
     if (_2 != 0B)
      goto <bb 5>; [90.00%]
    else
      goto <bb 3>; [10.00%]

2->5  (T) _1 :  struct QFutureInterfaceBasePrivate * [1B, +INF]
2->5  (T) _2 :  struct QMutex * [1B, +INF]
2->3  (F) _2 :  struct QMutex * [0B, 0B]

ON the edge 2->3 where _2 is 0, while it is also true that _1 must be 0,
perhaps that doesn't buy us much? and may lead is down the UB path of differing
expectations.     We certainly would like to preserve knowing non-null, so
instead of calling operator_addr_expr::fold_range, we could do the same
actions, just not deal with the NULL side, thus sidestepping issues that arise
with this whacky behaviour?

So if the _2 is non-null,  _1 is nonnull.  OTherwie we claim to know nothing of
_1..


The attached patch does just that, for what its owrth.

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

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think we shouldn't do anything on the optimization side - sure, the path
isolation in the end turns out not profitable, but it's likely hard to decide
that.

Maybe we can somehow avoid threading for a ptr == 0 condition?  Why do we make
the threading profitable?

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

[aldyh at gcc dot gnu.org]

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

Aldy Hernandez <aldyh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |law at gcc dot gnu.org

--- Comment #8 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #7)
> I think we shouldn't do anything on the optimization side - sure, the path
> isolation in the end turns out not profitable, but it's likely hard to decide
> that.
> 
> Maybe we can somehow avoid threading for a ptr == 0 condition?  Why do we
> make
> the threading profitable?

The threaders are mostly blind wrt threading a path.  If they see it, they will
thread it.  There are some minor things we disallow (see calls to cancel_thread
throughout) and the big hammer we introduced in this release with
cancel_invalid_paths() where we disallow problematic loop transformations.  But
mostly we thread everything that's possible.

Disallowing ptr == 0 as well as highly unlikely paths should be trivial.  We
could add a simple check in cancel_invalid_paths(). Though, I assume that would
affect warning code all over, since we're reducing the amount of threadable
paths on which they depend?

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

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think threading unlikely paths is never worth it and usually NULL pointer
checks are statically predicted.

I guess one idea would be to scale BB cost based on entry_BB->count vs.
BB->count so we more quickly run into the threading size limit for unlikely
executed blocks?  Likewise since we first evaluate all threading opportunities
in a function (do we?) we should sort them so we first thread the "best" one
and use a global threading limit instead one applied to each individual path
(so we'd just stop threading once we hit the limit).

Can we, for GCC 12, at least stop threading !maybe_hot_edge_p()?  Eventually
we can produce sth like a maybe_hot_threading_path_p () taking into account
the whole path.

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

[aldyh at gcc dot gnu.org]

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

--- Comment #10 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #9)
> I think threading unlikely paths is never worth it and usually NULL pointer
> checks are statically predicted.
> 
> I guess one idea would be to scale BB cost based on entry_BB->count vs.
> BB->count so we more quickly run into the threading size limit for unlikely
> executed blocks?  Likewise since we first evaluate all threading
> opportunities in a function (do we?) we should sort them so we first thread
> the "best" one and use a global threading limit instead one applied to each
> individual path (so we'd just stop threading once we hit the limit).

Yes, we first queue all threading opportunities and then reorganize the blocks
at the end.

> 
> Can we, for GCC 12, at least stop threading !maybe_hot_edge_p()?  Eventually
> we can produce sth like a maybe_hot_threading_path_p () taking into account
> the whole path.

Sure, sounds reasonable, especially since as we get the last holdout converted
to the new model (DOM threading), we'll have even more threading opportunities
which could use some pruning.

The whole cost model, as well as profiling, etc, was beyond the scope of my
work this year.  I'm curious if Jeff had any long term plans for this?

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

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|12.0                        |12.2

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 12.1 is being released, retargeting bugs to GCC 12.2.

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

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

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

[rguenth at gcc dot gnu.org]

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

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
There's been some changes on trunk but the preprocessed source doesn't work
there.

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

[thiago at kde dot org]

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

--- Comment #14 from Thiago Macieira <thiago at kde dot org> ---
Created attachment 54015
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54015&action=edit
qfutureinterface.cpp preprocessed [gcc trunk-20221205]

(In reply to Richard Biener from comment #13)
> There's been some changes on trunk but the preprocessed source doesn't work
> there.

Uploaded the updated preprocessed source with current trunk, from roughly the
same Qt commit (I chose a date just before this bug report was opened).

I can still reproduce this issue with the minimal original sources and these
preprocessed, but somehow not with the actual qfutureinterface.cpp, either then
or now.

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

[rguenth at gcc dot gnu.org]

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

--- Comment #15 from Richard Biener <rguenth at gcc dot gnu.org> ---
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);
    if (enable) {
        switch_on(d->state, Throttled);
    } else {
        switch_off(d->state, Throttled);
        if (!(d->state.loadRelaxed() & suspendingOrSuspended))
            d->pausedWaitCondition.wakeAll();
    }
}

where for unknown reason QMutexLocker happily allows a nullptr mutex
(it's documented that way)

    inline explicit QMutexLocker(Mutex *mutex) noexcept
    {
        m = mutex;
        if (__builtin_expect(!!(mutex), true)) {
            mutex->lock();
            isLocked = true;
        }
    }

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.

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.

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

[rguenth at gcc dot gnu.org]

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

--- Comment #16 from Richard Biener <rguenth at gcc dot gnu.org> ---
The odd thing is that we do

      /* Pointer constants other than null smaller than param_min_pagesize
         might be the result of erroneous null pointer addition/subtraction.
         Unless zero is a valid address set size to zero.  For null pointers,
         set size to the maximum for now since those may be the result of
         jump threading.  Similarly, for values >= param_min_pagesize in
         order to support (type *) 0x7cdeab00.  */
      if (integer_zerop (ptr)
          || wi::to_widest (ptr) >= param_min_pagesize)
        pref->set_max_size_range ();

so if we plain dereference nullptr we will not diagnose the access but if
we dereference at an address between zero and param_min_pagesize we will.

The machinery unfortunately doesn't propagate this decision so the
diagnostic itself is quite unhelpful (or would have to replicate the
above).  The code also doesn't catch upcasting of nullptr which would
result in small "negative" pointers.

I have a patch improving the diagnostic by means of printing a note like

/home/tjmaciei/dev/gcc/include/c++/13.0.0/bits/atomic_base.h:655:34: warning:
'unsigned int __atomic_fetch_and_4(volatile void*, unsigned int, int)' writing
4 bytes into a region of size 0 overflows the destination
[-Wstringop-overflow=]
In member function 'void QFutureInterfaceBase::setThrottled(bool)':
cc1plus: note: destination object is likely at address zero

amending each and every "into a region of size 0" case would be tedious since
the API used there doesn't pass down the object I amended.

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

[cvs-commit at gcc dot gnu.org]

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

--- Comment #17 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:926f5059bb8d295e2b68cea7c9af53606946eb1a

commit r13-4503-g926f5059bb8d295e2b68cea7c9af53606946eb1a
Author: Richard Biener <rguenther@suse.de>
Date:   Tue Dec 6 10:12:01 2022 +0100

    tree-optimization/104475 - improve access diagnostics

    When we end up isolating a nullptr path it happens we diagnose
    accesses to offsetted nullptr objects.  The current diagnostics
    have no good indication that this happens so the following records
    the fact that our heuristic detected a nullptr based access in
    the access_ref structure and sets up diagnostics to inform
    of that detail.  The diagnostic itself could probably be
    improved here but its API is twisted and the necessary object
    isn't passed around.

    Instead of just

    ...bits/atomic_base.h:655:34: warning: 'unsigned int
__atomic_fetch_and_4(volatile void*, unsigned int, int)' writing 4 bytes into a
region of size 0 overflows the destination [-Wstringop-overflow=]

    we now add

    In member function 'void QFutureInterfaceBase::setThrottled(bool)':
    cc1plus: note: destination object is likely at address zero

            PR tree-optimization/104475
            * pointer-query.h (access_ref::ref_nullptr_p): New flag.
            * pointer-query.cc (access_ref::access_ref): Initialize
            ref_nullptr_p.
            (compute_objsize_r): Set ref_nullptr_p if we treat it that way.
            (access_ref::inform_access): If ref was treated as nullptr
            based, indicate that.

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

[rguenth at gcc dot gnu.org]

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

--- Comment #18 from Richard Biener <rguenth at gcc dot gnu.org> ---
The change improved the wording of the diagnostic by appending the note
indicating an object at zero address.  It didn't mitigate the diagnostic which
as far as I analyzed is technically correct (but not very helpful).

An improvement for these diagnostics would be analyzer-style reporting of
the guarding conditions.  Another possible improvement would be to somehow
keep a pointer to the symbolic base we equality-propagated from the
conditional so that we, for

 if (!d)
   if (enabled)
     *d = 0;

can say the object pointed-to by 'd' when 'd' is nullptr is accessed here.
The IL currently just has a pointer constant and doesn't know that was
originally derived from 'd'.

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

[thiago at kde dot 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.

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

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at gcc dot gnu.org

--- Comment #20 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Thiago Macieira from comment #19)
> (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.

Hmm, I see

<bb 2> [local count: 1073741824]:
_1 = this_10(D)->d;
_2 = &_1->m_mutex;
MEM[(struct __as_base  &)&lock] ={v} {CLOBBER};
if (_2 != 0B)

so we load the pointer 'this_10(D)->d' and then indeed check &d->m_mutex
for being NULL.

The middle-end long allowed &nullptr->x for the sake of legacy code
implementing offsetof by pointer arithmetic on an object at NULL.

Given

struct X { int a; int b ; };
int foo (struct X *x)
{
  return &x->a != (int *)0;
}

we do indeed not simplify this.  Doing &x->b != (int *)0 shows even that
is only simplified by recent code in Ranger (via DOM at -O1 or VRP at -O2).
That's also true for the

struct X { int a; int b ; };
int foo (struct X *x)
{
  int *p = (int *)x + 1;
  return p != (int *)0;
}

variant.  Special-casing this in the language frontends probably won't
fix the testcase since the use in a nullptr comparison is hidden
via a function call of the CTOR:

    QMutexLocker lock(&d->m_mutex);

and as you say m_mutex is at offset zero.

Since consider pointer overflow undefined we should be able to optimize
&x->b != nullptr in the middle-end earlier and more consistently.  The
offset zero case is harder since we consider &x->b as just pointer
arithmetic and x + 0 != nullptr cannot be optimized I think.

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

[rguenth at gcc dot gnu.org]

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

--- Comment #21 from Richard Biener <rguenth at gcc dot gnu.org> ---
When the C++ frontend sees

    QMutexLocker lock(&d->m_mutex);

it could hint the middle-end via emitting

    [[assume (&d->m_mutex != nullptr)]]
    QMutexLocker lock(&d->m_mutex);

instead.

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

[rguenth at gcc dot gnu.org]

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

--- Comment #22 from Richard Biener <rguenth at gcc dot gnu.org> ---
Re-ordering m_mutex and continuationMutex avoids the bogus diagnostic.

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

[rguenth at gcc dot gnu.org]

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

--- Comment #23 from Richard Biener <rguenth at gcc dot gnu.org> ---
Interestingly doing

void QFutureInterfaceBase::setThrottled(bool enable)
{   
    if (&d->m_mutex == nullptr)
      __builtin_unreachable ();
    QMutexLocker lock(&d->m_mutex);

will avoid the diagnostic but instead complains

qfutureinterface.cpp:67:21:warning: the address of
'QFutureInterfaceBasePrivate::m_mutex' will never be NULL [-Waddress]

(that's without the re-ordering).  So the C++ frontend already knows this
but doesn't transfer it to the middle-end.  Maybe we want something
like ADDR_EXPR_NONZERO on the built ADDR_EXPR?

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

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |https://gcc.gnu.org/piperma
                   |                            |il/gcc-patches/2022-Decembe
                   |                            |r/608077.html

--- Comment #24 from Richard Biener <rguenth at gcc dot gnu.org> ---
Here's a patch doing that (ADDR_NONZERO) which fixes the issue.

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

[jason at gcc dot gnu.org]

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

--- Comment #25 from Jason Merrill <jason at gcc dot gnu.org> ---
This was clarified for C++ last year by http://wg21.link/cwg2535

I notice that C warn_for_null_address also warns about &p->mem; I don't know
where that is justified in the C standard.

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

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|12.3                        |12.4

--- Comment #26 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 12.3 is being released, retargeting bugs to GCC 12.4.

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

[aph at gcc dot gnu.org]

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

Andrew Haley <aph at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |aph at gcc dot gnu.org

--- Comment #27 from Andrew Haley <aph at gcc dot gnu.org> ---
Is this one still alive? It's still causing problems in the wild.

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

[rguenth at gcc dot gnu.org]

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

--- Comment #28 from Richard Biener <rguenth at gcc dot gnu.org> ---
I don't think anybody is working on this currently.