[Bug target/101761] New: Random hang with 29_atomics/atomic_ref/wait_notify.cc

[Bug target/101761] New: Random hang with 29_atomics/atomic_ref/wait_notify.cc

[hjl.tools at gmail dot com]

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

            Bug ID: 101761
           Summary: Random hang with 29_atomics/atomic_ref/wait_notify.cc
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: hjl.tools at gmail dot com
                CC: crazylht at gmail dot com, wwwhhhyyy333 at gmail dot com
  Target Milestone: ---
            Target: x86-64

29_atomics/atomic_ref/wait_notify.cc in 64-bit on Skylake server:

(gdb) bt
#0  0x00007f897288cc1d in syscall () from /lib64/libc.so.6
#1  0x00000000004018be in std::__detail::__platform_wait<int> (
    __addr=__addr@entry=0x405400
<std::__detail::__waiter_pool_base::_S_for(void const*)::__w+832>,
__val=__val@entry=3)
    at
/export/users/hjl/build/gnu/tools-build/gcc-gitlab-native/build-x86_64-linux/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/atomic_wait.h:104
#2  0x00000000004022a3 in std::__detail::__waiter_pool::_M_do_wait (__old=3, 
    __addr=0x405400 <std::__detail::__waiter_pool_base::_S_for(void
const*)::__w+832>, this=<optimized out>)
    at
/export/users/hjl/build/gnu/tools-build/gcc-gitlab-native/build-x86_64-linux/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/atomic_wait.h:261
#3  std::__detail::__waiter<std::integral_constant<bool, true>
>::_M_do_wait_v<void*, std::__atomic_impl::wait<void*>(void* const*,
std::remove_volatile<void*>::type, std::memory_order)::{lambda()#1}>(void*,
std::__atomic_impl::wait<void*>(void* const*,
std::remove_volatile<void*>::type, std::memory_order)::{lambda()#1})
(__vfn=..., __old=0x2a, this=<synthetic pointer>)
    at
/export/users/hjl/build/gnu/tools-build/gcc-gitlab-native/build-x86_64-linux/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/atomic_wait.h:400
#4  std::__atomic_wait_address_v<void*, std::__atomic_impl::wait<void*>(void*
const*, std::remove_volatile<void*>::type,
std::memory_order)::{lambda()#1}>(void* const*, void*,
std::__atomic_impl::wait<void*>(void* const*,
std::remove_volatile<void*>::type, std::memory_order)::{lambda()#1})
(__addr=<optimized out>, 
    __old=0x2a, __vfn=...)
    at
/export/users/hjl/build/gnu/tools-build/gcc-gitlab-native/build-x86_64-linux/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/atomic_wait.h:430
#5  0x0000000000402366 in std::__atomic_impl::wait<void*> (
    __m=std::memory_order::seq_cst, __old=<optimized out>, 
    __ptr=<optimized out>)
    at
/export/users/hjl/build/gnu/tools-build/gcc-gitlab-native/build-x86_64-linux/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/atomic_base.h:1018
#6  std::__atomic_ref<void*, false, false>::wait (
    __m=std::memory_order::seq_cst, __old=<optimized out>, this=0x7ffed19864e8)
    at
/export/users/hjl/build/gnu/tools-build/gcc-gitlab-native/build-x86_64-linux/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/atomic_base.h:1874
#7  test<void*> (va=va@entry=0x0, vb=vb@entry=0x2a)
    at
/export/gnu/import/git/gitlab/x86-gcc/libstdc++-v3/testsuite/29_atomics/atomic_ref/wait_notify.cc:44
#8  0x000000000040125b in main ()
    at
/export/gnu/import/git/gitlab/x86-gcc/libstdc++-v3/testsuite/29_atomics/atomic_ref/wait_notify.cc:57
(gdb)

In 32-bit,

(gdb) bt
#0  0xf7fc655d in __kernel_vsyscall ()
#1  0xf7bac46b in syscall () from /lib/libc.so.6
#2  0x0804995d in std::__detail::__platform_wait<int> (
    __addr=0x804d680 <std::__detail::__waiter_pool_base::_S_for(void
const*)::__w+1472>, __val=3)
    at
/export/gnu/import/git/gcc-test-master-intel64-native/bld/x86_64-pc-linux-gnu/32/libstdc++-v3/include/bits/atomic_wait.h:104
#3  0x08049e3a in std::__detail::__waiter_pool::_M_do_wait (__old=3, 
    __addr=0x804d680 <std::__detail::__waiter_pool_base::_S_for(void
const*)::__w+1472>, this=<optimized out>)
    at
/export/gnu/import/git/gcc-test-master-intel64-native/bld/x86_64-pc-linux-gnu/32/libstdc++-v3/include/bits/atomic_wait.h:261
#4  std::__detail::__waiter<std::integral_constant<bool, true>
>::_M_do_wait_v<unsigned int, std::__atomic_impl::wait<unsigned int>(unsigned
int const*, std::remove_volatile<unsigned int>::type,
std::memory_order)::{lambda()#1}>(unsigned int,
std::__atomic_impl::wait<unsigned int>(unsigned int const*,
std::remove_volatile<unsigned int>::type, std::memory_order)::{lambda()#1})
(__vfn=..., 
    __old=42, this=<synthetic pointer>)
    at
/export/gnu/import/git/gcc-test-master-intel64-native/bld/x86_64-pc-linux-gnu/32/libstdc++-v3/include/bits/atomic_wait.h:400
#5  std::__atomic_wait_address_v<unsigned int,
std::__atomic_impl::wait<unsigned int>(unsigned int const*,
std::remove_volatile<unsigned int>::type,
std::memory_order)::{lambda()#1}>(unsigned int const*, unsigned int,
std::__atomic_impl::wa--Type <RET> for more, q to quit, c to continue without
paging--
it<unsigned int>(unsigned int const*, std::remove_volatile<unsigned int>::type,
std::memory_order)::{lambda()#1}) (__addr=0xffeb402c, __old=42, __vfn=...)
    at
/export/gnu/import/git/gcc-test-master-intel64-native/bld/x86_64-pc-linux-gnu/32/libstdc++-v3/include/bits/atomic_wait.h:430
#6  0x08049ef2 in std::__atomic_impl::wait<unsigned int> (
    __m=std::memory_order::seq_cst, __old=<optimized out>, 
    __ptr=<optimized out>)
    at
/export/gnu/import/git/gcc-test-master-intel64-native/bld/x86_64-pc-linux-gnu/32/libstdc++-v3/include/bits/atomic_base.h:1018
#7  std::__atomic_ref<unsigned int, true, false>::wait (
    __m=std::memory_order::seq_cst, __old=<optimized out>, this=0xffeb4034)
    at
/export/gnu/import/git/gcc-test-master-intel64-native/bld/x86_64-pc-linux-gnu/32/libstdc++-v3/include/bits/atomic_base.h:1570
#8  test<unsigned int> (va=0, vb=42)
    at
/export/gnu/import/git/gcc-test-master-intel64-native/src-master/libstdc++-v3/testsuite/29_atomics/atomic_ref/wait_notify.cc:44
#9  0x0804926b in main ()
    at
/export/gnu/import/git/gcc-test-master-intel64-native/src-master/libstdc++-v3/testsuite/29_atomics/atomic_ref/wait_notify.cc:54
(gdb) 

GCC was configured with

--with-arch=native --with-cpu=native --prefix=/usr/12.0.0 --enable-clocale=gnu
--with-system-zlib --enable-shared --enable-cet --with-demangler-in-ld
--enable-libmpx --with-multilib-list=m32,m64,mx32 --with-fpmath=sse

It happens about once a few weeks.

[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc

[ubizjak at gmail dot com]

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

--- Comment #1 from Uroš Bizjak <ubizjak at gmail dot com> ---
Probably related to PR97936.

[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc

[ubizjak at gmail dot com]

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

--- Comment #2 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to H.J. Lu from comment #0)
> 29_atomics/atomic_ref/wait_notify.cc in 64-bit on Skylake server:
> 
> It happens about once a few weeks.

while true ; do ./a.out ; done

will hang immediately.

[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2021-08-04
           Assignee|unassigned at gcc dot gnu.org      |rodgertq at gcc dot gnu.org
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1

[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc

[rodgertq at gcc dot gnu.org]

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

--- Comment #3 from Thomas Rodgers <rodgertq at gcc dot gnu.org> ---
This appears to be the test case itself.

[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc

[fw at gcc dot gnu.org]

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

Florian Weimer <fw at gcc dot gnu.org> changed:

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

--- Comment #4 from Florian Weimer <fw at gcc dot gnu.org> ---
I think it's a test bug:

      std::atomic_ref<S> a{ aa };

      std::thread t([&]
        {
          a.store(bb);
          a.notify_one();
        });
      a.wait(aa);

Due to the use of std::atomic_ref, store() overwrites aa with the value of bb.
If the notify_one() call completes before the wait() call, wait() blocks
because aa == bb (due to the previous store()), and the wakeup never happens
because wakes are not queued.

[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc

[rodgertq at gcc dot gnu.org]

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

--- Comment #5 from Thomas Rodgers <rodgertq at gcc dot gnu.org> ---
(In reply to Florian Weimer from comment #4)
> I think it's a test bug:
> 
>       std::atomic_ref<S> a{ aa };
> 
>       std::thread t([&]
>         {
> 	  a.store(bb);
> 	  a.notify_one();
>         });
>       a.wait(aa);
> 
> Due to the use of std::atomic_ref, store() overwrites aa with the value of
> bb. If the notify_one() call completes before the wait() call, wait() blocks
> because aa == bb (due to the previous store()), and the wakeup never happens
> because wakes are not queued.

This was my initial suspicion. It applies to all of the
29_atomics/**/wait_notify.cc tests. The libc++ tests for wait/notify have the
same problem (see
https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.wait/atomic_wait.pass.cpp)

I have, as part of this investigation, extensively re-reviewed the
implementation detail in bits/atomic_wait.h and I think there is a race
condition lurking in the implementation which also needs to be address.
Specifically tracking waiters to try to void making the syscall for
FUTEX_WAKE_PRIVATE if there are no current waiters. In order for this to work
the increment of the waiter count would have to be atomic with syscall. I
believe that libc++ also has this same issue.

[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc

[redi at gcc dot gnu.org]

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

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Florian Weimer from comment #4)
>       a.wait(aa);

This line is undefined and needs to be a.wait(va) anyway.

[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc

[fw at gcc dot gnu.org]

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

--- Comment #7 from Florian Weimer <fw at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #6)
> (In reply to Florian Weimer from comment #4)
> >       a.wait(aa);
> 
> This line is undefined and needs to be a.wait(va) anyway.

Yes, that's what I meant. Or  the test needs to use std::atomic, not
std::atomic_ref.

[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc

[redi at gcc dot gnu.org]

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

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
It's a test for std::atomic_ref, it has to use atomic_ref :-)

[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc

[redi at gcc dot gnu.org]

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

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
My point is that *any* read of aa is undefined while there is an atomic_ref
using it, even in the absence of other threads.

It happens to cause the race you describe, because the value of aa is changing
concurrently. But it's undefined according to the standard even without that.

[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc

[redi at gcc dot gnu.org]

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

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
http://eel.is/c++draft/atomics#ref.generic.general-3.sentence-2

"While any atomic_­ref instances exist that reference the *ptr object, all
accesses to that object shall exclusively occur through those atomic_­ref
instances."

[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc

[rodgertq at gcc dot gnu.org]

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

--- Comment #11 from Thomas Rodgers <rodgertq at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #10)
> http://eel.is/c++draft/atomics#ref.generic.general-3.sentence-2
> 
> "While any atomic_­ref instances exist that reference the *ptr object, all
> accesses to that object shall exclusively occur through those atomic_­ref
> instances."

Yes. I will submit a patch for this test shortly.

Having said that, the atomic integral tests also spuriously deadlock, they
don't have this UB issue.

[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc

[redi at gcc dot gnu.org]

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

--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Thomas Rodgers from comment #11)
> Yes. I will submit a patch for this test shortly.

The a.wait(aa) to a.wait(va) change is pre-approved, please just push when it's
ready.

[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc

[cvs-commit at gcc dot gnu.org]

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

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Thomas Rodgers <rodgertq@gcc.gnu.org>:

https://gcc.gnu.org/g:f9f1a6efaaeeec06d5c07378734cb8eb47b976a7

commit r12-3587-gf9f1a6efaaeeec06d5c07378734cb8eb47b976a7
Author: Thomas Rodgers <rodgert@twrodgers.com>
Date:   Thu Sep 16 14:42:58 2021 -0700

    libstdc++: Fix UB in atomic_ref/wait_notify.cc [PR101761]

    Remove UB in atomic_ref/wait_notify test.

    Signed-off-by: Thomas Rodgers <trodgers@redhat.com>

    libstdc++-v3/ChangeLog:

            PR libstdc++/101761
            * testsuite/29_atomics/atomic_ref/wait_notify.cc (test): Use
            va and vb as arguments to wait/notify, remove unused bb local.

[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc

[hjl.tools at gmail dot com]

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

H.J. Lu <hjl.tools at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #14 from H.J. Lu <hjl.tools at gmail dot com> ---
Fixed for GCC 12.

[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc

[redi at gcc dot gnu.org]

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

--- Comment #15 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Backport to gcc-11 needed

[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc

[cvs-commit at gcc dot gnu.org]

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

--- Comment #16 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

https://gcc.gnu.org/g:90a4981e0951687056ec4735cfd3043b35a23502

commit r11-9109-g90a4981e0951687056ec4735cfd3043b35a23502
Author: Thomas Rodgers <rodgert@twrodgers.com>
Date:   Thu Sep 16 14:42:58 2021 -0700

    libstdc++: Fix UB in atomic_ref/wait_notify.cc [PR101761]

    Remove UB in atomic_ref/wait_notify test.

    Signed-off-by: Thomas Rodgers <trodgers@redhat.com>

    libstdc++-v3/ChangeLog:

            PR libstdc++/101761
            * testsuite/29_atomics/atomic_ref/wait_notify.cc (test): Use
            va and vb as arguments to wait/notify, remove unused bb local.

    (cherry picked from commit f9f1a6efaaeeec06d5c07378734cb8eb47b976a7)

[Bug testsuite/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|target                      |testsuite
      Known to work|                            |11.3.0, 12.0
   Target Milestone|---                         |11.3