[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101761 --- Comment #16 from CVS Commits --- The releases/gcc-11 branch has been updated by Jonathan Wakely : https://gcc.gnu.org/g:90a4981e0951687056ec4735cfd3043b35a23502 commit r11-9109-g90a4981e0951687056ec4735cfd3043b35a23502 Author: Thomas Rodgers 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 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 target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101761 --- Comment #15 from Jonathan Wakely --- Backport to gcc-11 needed
[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101761 H.J. Lu changed: What|Removed |Added Status|ASSIGNED|RESOLVED Resolution|--- |FIXED --- Comment #14 from H.J. Lu --- Fixed for GCC 12.
[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101761 --- Comment #13 from CVS Commits --- The master branch has been updated by Thomas Rodgers : https://gcc.gnu.org/g:f9f1a6efaaeeec06d5c07378734cb8eb47b976a7 commit r12-3587-gf9f1a6efaaeeec06d5c07378734cb8eb47b976a7 Author: Thomas Rodgers 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 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101761 --- Comment #12 from Jonathan Wakely --- (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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101761 --- Comment #11 from Thomas Rodgers --- (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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101761 --- Comment #10 from Jonathan Wakely --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101761 --- Comment #9 from Jonathan Wakely --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101761 --- Comment #8 from Jonathan Wakely --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101761 --- Comment #7 from Florian Weimer --- (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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101761 --- Comment #6 from Jonathan Wakely --- (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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101761
--- Comment #5 from Thomas Rodgers ---
(In reply to Florian Weimer from comment #4)
> I think it's a test bug:
>
> std::atomic_ref 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101761
Florian Weimer changed:
What|Removed |Added
CC||fw at gcc dot gnu.org
--- Comment #4 from Florian Weimer ---
I think it's a test bug:
std::atomic_ref 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101761 --- Comment #3 from Thomas Rodgers --- This appears to be the test case itself.
[Bug target/101761] Random hang with 29_atomics/atomic_ref/wait_notify.cc
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101761 Jonathan Wakely 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101761 --- Comment #2 from Uroš Bizjak --- (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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101761 --- Comment #1 from Uroš Bizjak --- Probably related to PR97936.
