| Message ID | 20260108214524.2292515-1-jwakely@redhat.com |
|---|---|
| State | New |
| Headers |
Return-Path: <gcc-patches-bounces~patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from vm01.sourceware.org (localhost [127.0.0.1]) by sourceware.org (Postfix) with ESMTP id A46104BA2E1F for <patchwork@sourceware.org>; Thu, 8 Jan 2026 21:46:32 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org A46104BA2E1F Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=gxjB0Bz1 X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTP id 784DA4BA2E05 for <gcc-patches@gcc.gnu.org>; Thu, 8 Jan 2026 21:45:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 784DA4BA2E05 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 784DA4BA2E05 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1767908731; cv=none; b=wf1Bdi7+vvSWS32WXQGkLTxIi50qUKtsNJ+1+jOXErfPH8SEkHdwJa9GE5v9qRs8u6Y9VLfZt7hFxyEc+qEMgwiph3c+QvVBFoAmIfnMR0psosCepH6Am33JQawkgK9U3Iec/5kW5+m0ICYFgM1bSbpwFVhLV2h8UUV/nr1o4AM= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1767908731; c=relaxed/simple; bh=lTrVEHoL77g0MirKiYrP3blYYYKFiZzUIF1O8t9HaOo=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=Y2sZ+RpI8Nxul/YfP1yL/P3Ryb9C6ViydXMt5fQ1QeCsS3vXjK2D3RicpGq3wdlAMpuRbtgU51gWIRSWQudhlfs4JQ+7KAWm9I+aQsK69arvQG/dXlee4Ea2XQyTXbaa7Vnftdzl6JolbH4EyARlp14nCA/C0aWWjEC3f2AUKsA= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 784DA4BA2E05 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1767908730; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=bENQcjDD41CD3KdnYUxWx4lx/CXOeH8qucvEG+SCzVA=; b=gxjB0Bz1H4wBUQnp0suaXxbqFr5mf+q5iLWeetltJfOvQdhSA4fJqaa3kdjpn8PzDhyC2r Z7PzZH/x0jtFVuaqGo7zQKdBW/qc8uiTZxsI5zLJvRRTPeB2+YbaaBNl5NYlpMbqty4420 pMyEVAZpiCeWSk1ULSkzBm8e36mP6+U= Received: from mx-prod-mc-03.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-597-oRZrAK91MDmVeiWhleKWRQ-1; Thu, 08 Jan 2026 16:45:28 -0500 X-MC-Unique: oRZrAK91MDmVeiWhleKWRQ-1 X-Mimecast-MFC-AGG-ID: oRZrAK91MDmVeiWhleKWRQ_1767908727 Received: from mx-prod-int-03.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-03.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.12]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-03.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 73D6B1956053; Thu, 8 Jan 2026 21:45:27 +0000 (UTC) Received: from zen.kayari.org (unknown [10.42.28.2]) by mx-prod-int-03.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id 8E07A19560A2; Thu, 8 Jan 2026 21:45:26 +0000 (UTC) From: Jonathan Wakely <jwakely@redhat.com> To: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org Subject: [PATCH 1/2] libstdc++: Fix proxy wait detection in atomic wait Date: Thu, 8 Jan 2026 21:44:17 +0000 Message-ID: <20260108214524.2292515-1-jwakely@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.0 on 10.30.177.12 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: td4deQFU80kJTvU_BwJ4XKU3NjFTLLsE9No7ylLBZ7E_1767908727 X-Mimecast-Originator: redhat.com Content-Type: text/plain Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_BLOCKED, RCVD_IN_MSPIKE_H2, RCVD_IN_VALIDITY_RPBL_BLOCKED, RCVD_IN_VALIDITY_SAFE_BLOCKED, SPF_HELO_PASS, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> Errors-To: gcc-patches-bounces~patchwork=sourceware.org@gcc.gnu.org |
| Series |
[1/2] libstdc++: Fix proxy wait detection in atomic wait
|
|
Commit Message
Jonathan Wakely
Jan. 8, 2026, 9:44 p.m. UTC
A failed assertion was observed with std::atomic<bool>::wait when the loop in __atomic_wait_address is entered and calls _M_setup_wait a second time. When the first call to _M_setup_wait makes a call to _M_setup_proxy_wait it decides that a proxy wait is needed for bool, and updates the _M_obj and _M_obj_size members to refer to the futex in the proxy state, instead of referring to the bool object. The next time _M_setup_wait is called it calls _M_setup_proxy_wait again but now it sees _M_obj_size == sizeof(futex) and so this time decides a proxy wait is *not* needed, and then fails the __glibcxx_assert(_M_obj == addr) check. The problem is that _M_setup_wait is calling _M_setup_proxy_wait twice, when it should avoid the second call because the decision to use a proxy wait has already been made. The caller should avoid making the second call by checking whether _M_obj != addr, because that implies that a decision to make a proxy wait has already happened. This change fixes the bug, by preventing a second call to _M_setup_proxy_wait after we've changed _M_obj and _M_obj_size. But it doesn't prevent a second call in the case where _M_setup_proxy_wait returned false because we made a runtime decision to do a non-proxy wait. That is suboptimal, because it means that code which can wait on the atomic variable directly makes redundant calls into the library just to be told the same answer every time. That's not a problem for now because firstly, it's only a performance degradation not a correctness bug, and secondly because we currently have no targets that ever make a runtime decision to do a non-proxy wait. Linux makes a compile-time decision to do non-proxy waits for futex-sized objects and does proxy waits for everything else, and all non-Linux targets always do proxy waits. So solving the performance problem for non-proxy waits can happen later. We might want to set a bit in __wait_args::_M_flags to say "I already told you this isn't a proxy wait, stop asking me". libstdc++-v3/ChangeLog: * include/bits/atomic_wait.h (__wait_args::_M_setup_wait): Do not call _M_setup_proxy_wait again if a proxy wait was already set up. --- Tested x86_64-linux and x86_64-freebsd14 (just the parts that use atomic waits). libstdc++-v3/include/bits/atomic_wait.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On Thu, Jan 8, 2026 at 10:50 PM Jonathan Wakely <jwakely@redhat.com> wrote: > A failed assertion was observed with std::atomic<bool>::wait when the > loop in __atomic_wait_address is entered and calls _M_setup_wait a > second time. When the first call to _M_setup_wait makes a call to > _M_setup_proxy_wait it decides that a proxy wait is needed for bool, and > updates the _M_obj and _M_obj_size members to refer to the futex in the > proxy state, instead of referring to the bool object. The next time > _M_setup_wait is called it calls _M_setup_proxy_wait again but now it > sees _M_obj_size == sizeof(futex) and so this time decides a proxy wait > is *not* needed, and then fails the __glibcxx_assert(_M_obj == addr) > check. > > The problem is that _M_setup_wait is calling _M_setup_proxy_wait twice, > when it should avoid the second call because the decision to use a proxy > wait has already been made. The caller should avoid making the second > call by checking whether _M_obj != addr, because that implies that a > decision to make a proxy wait has already happened. > I do not think this is correct behavior, the _M_setup_wait have two responsibilities now: * deciding it the proxy wait should be used * updating the _M_old value with the most recent _M_ver versions, I am referring to this line // Read the value of the _M_ver counter. _M_old = __atomic_load_n(&state->_M_ver, __ATOMIC_ACQUIRE); To hit this assertion, this means that there was contention on the proxy wait slot or had a spurious wake, and in this case _M_ver was changed, and we should still update the _M_old to the latest value. I think the proper fix matching the approach would be to have: bool __wait_args::_M_setup_proxy_wait(const void* addr) { if (_M_obj != addr) { // We already decided to use proxy wait, read the value of the _M_ver counter. _M_old = __atomic_load_n(&state->_M_ver, __ATOMIC_ACQUIRE); } if (!use_proxy_wait(*this)) // We can wait on this address directly. return false; // This will be a proxy wait, so get a waitable state. auto state = set_wait_state(addr, *this); // The address we will wait on is the version count of the waitable state: _M_obj = &state->_M_ver; // __wait_impl and __wait_until_impl need to know this size: _M_obj_size = sizeof(state->_M_ver); } } // Read the value of the _M_ver counter. _M_old = __atomic_load_n(&state->_M_ver, __ATOMIC_ACQUIRE); return true; } // See below for alternative > > This change fixes the bug, by preventing a second call to > _M_setup_proxy_wait after we've changed _M_obj and _M_obj_size. But it > doesn't prevent a second call in the case where _M_setup_proxy_wait > returned false because we made a runtime decision to do a non-proxy > wait. That is suboptimal, because it means that code which can wait on > the atomic variable directly makes redundant calls into the library just > to be told the same answer every time. That's not a problem for now > because firstly, it's only a performance degradation not a correctness > bug, and secondly because we currently have no targets that ever make a > runtime decision to do a non-proxy wait. Linux makes a compile-time > decision to do non-proxy waits for futex-sized objects and does proxy > waits for everything else, and all non-Linux targets always do proxy > waits. So solving the performance problem for non-proxy waits can happen > later. We might want to set a bit in __wait_args::_M_flags to say "I > already told you this isn't a proxy wait, stop asking me". > I think the best option would be to separate _M_setup_wait (initial setups) and later updates into separate functions. Like _M_setup_wait and _M_update_wait. I will post a patch implementing those suggestion. > > libstdc++-v3/ChangeLog: > > * include/bits/atomic_wait.h (__wait_args::_M_setup_wait): Do > not call _M_setup_proxy_wait again if a proxy wait was already > set up. > --- > > Tested x86_64-linux and x86_64-freebsd14 (just the parts that use atomic > waits). > > libstdc++-v3/include/bits/atomic_wait.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libstdc++-v3/include/bits/atomic_wait.h > b/libstdc++-v3/include/bits/atomic_wait.h > index b6240a95370f..a677dbf0a040 100644 > --- a/libstdc++-v3/include/bits/atomic_wait.h > +++ b/libstdc++-v3/include/bits/atomic_wait.h > @@ -230,7 +230,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > } > > if constexpr (!__platform_wait_uses_type<_Tp>) > - if (_M_setup_proxy_wait(__addr)) > + if (_M_obj != __addr || _M_setup_proxy_wait(__addr)) > Or to reload _M_wait here. > { > // We will use a proxy wait for this object. > // The library has set _M_obj and _M_obj_size and _M_old. > -- > 2.52.0 > >
On Fri, 9 Jan 2026 at 08:56, Tomasz Kaminski <tkaminsk@redhat.com> wrote: > > > > On Thu, Jan 8, 2026 at 10:50 PM Jonathan Wakely <jwakely@redhat.com> wrote: >> >> A failed assertion was observed with std::atomic<bool>::wait when the >> loop in __atomic_wait_address is entered and calls _M_setup_wait a >> second time. When the first call to _M_setup_wait makes a call to >> _M_setup_proxy_wait it decides that a proxy wait is needed for bool, and >> updates the _M_obj and _M_obj_size members to refer to the futex in the >> proxy state, instead of referring to the bool object. The next time >> _M_setup_wait is called it calls _M_setup_proxy_wait again but now it >> sees _M_obj_size == sizeof(futex) and so this time decides a proxy wait >> is *not* needed, and then fails the __glibcxx_assert(_M_obj == addr) >> check. >> >> The problem is that _M_setup_wait is calling _M_setup_proxy_wait twice, >> when it should avoid the second call because the decision to use a proxy >> wait has already been made. The caller should avoid making the second >> call by checking whether _M_obj != addr, because that implies that a >> decision to make a proxy wait has already happened. > > I do not think this is correct behavior, the _M_setup_wait have two responsibilities now: > * deciding it the proxy wait should be used > * updating the _M_old value with the most recent _M_ver versions, I am referring to this line > // Read the value of the _M_ver counter. > _M_old = __atomic_load_n(&state->_M_ver, __ATOMIC_ACQUIRE); > To hit this assertion, this means that there was contention on the proxy wait slot > or had a spurious wake, It happens on a non-spurious wake as well, the current code always calls _M_setup_wait after __wait_impl, even if it woke up because the value changed and a notify call was done. > still update the _M_old to the latest value. > > I think the proper fix matching the approach would be to have: > bool > __wait_args::_M_setup_proxy_wait(const void* addr) > { > if (_M_obj != addr) { > // We already decided to use proxy wait, read the value of the _M_ver counter. > _M_old = __atomic_load_n(&state->_M_ver, __ATOMIC_ACQUIRE); > } If the condition above is true, the we don't need to call use_proxy_wait, so this should be if-else: > if (!use_proxy_wait(*this)) // We can wait on this address directly. > return false; Or maybe you meant to do return true above, after setting _M_old > // This will be a proxy wait, so get a waitable state. > auto state = set_wait_state(addr, *this); > > // The address we will wait on is the version count of the waitable state: > _M_obj = &state->_M_ver; > // __wait_impl and __wait_until_impl need to know this size: > _M_obj_size = sizeof(state->_M_ver); > } > } > > // Read the value of the _M_ver counter. > _M_old = __atomic_load_n(&state->_M_ver, __ATOMIC_ACQUIRE); > > return true; > } > // See below for alternative >> >> >> This change fixes the bug, by preventing a second call to >> _M_setup_proxy_wait after we've changed _M_obj and _M_obj_size. But it >> doesn't prevent a second call in the case where _M_setup_proxy_wait >> returned false because we made a runtime decision to do a non-proxy >> wait. That is suboptimal, because it means that code which can wait on >> the atomic variable directly makes redundant calls into the library just >> to be told the same answer every time. That's not a problem for now >> because firstly, it's only a performance degradation not a correctness >> bug, and secondly because we currently have no targets that ever make a >> runtime decision to do a non-proxy wait. Linux makes a compile-time >> decision to do non-proxy waits for futex-sized objects and does proxy >> waits for everything else, and all non-Linux targets always do proxy >> waits. So solving the performance problem for non-proxy waits can happen >> later. We might want to set a bit in __wait_args::_M_flags to say "I >> already told you this isn't a proxy wait, stop asking me". > > I think the best option would be to separate _M_setup_wait (initial setups) > and later updates into separate functions. Like _M_setup_wait and _M_update_wait. > I will post a patch implementing those suggestion. > > >> >> >> libstdc++-v3/ChangeLog: >> >> * include/bits/atomic_wait.h (__wait_args::_M_setup_wait): Do >> not call _M_setup_proxy_wait again if a proxy wait was already >> set up. >> --- >> >> Tested x86_64-linux and x86_64-freebsd14 (just the parts that use atomic >> waits). >> >> libstdc++-v3/include/bits/atomic_wait.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libstdc++-v3/include/bits/atomic_wait.h b/libstdc++-v3/include/bits/atomic_wait.h >> index b6240a95370f..a677dbf0a040 100644 >> --- a/libstdc++-v3/include/bits/atomic_wait.h >> +++ b/libstdc++-v3/include/bits/atomic_wait.h >> @@ -230,7 +230,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> } >> >> if constexpr (!__platform_wait_uses_type<_Tp>) >> - if (_M_setup_proxy_wait(__addr)) >> + if (_M_obj != __addr || _M_setup_proxy_wait(__addr)) > > Or to reload _M_wait here. >> >> { >> // We will use a proxy wait for this object. >> // The library has set _M_obj and _M_obj_size and _M_old. >> -- >> 2.52.0 >>
On Fri, Jan 9, 2026 at 10:26 AM Jonathan Wakely <jwakely@redhat.com> wrote: > On Fri, 9 Jan 2026 at 08:56, Tomasz Kaminski <tkaminsk@redhat.com> wrote: > > > > > > > > On Thu, Jan 8, 2026 at 10:50 PM Jonathan Wakely <jwakely@redhat.com> > wrote: > >> > >> A failed assertion was observed with std::atomic<bool>::wait when the > >> loop in __atomic_wait_address is entered and calls _M_setup_wait a > >> second time. When the first call to _M_setup_wait makes a call to > >> _M_setup_proxy_wait it decides that a proxy wait is needed for bool, and > >> updates the _M_obj and _M_obj_size members to refer to the futex in the > >> proxy state, instead of referring to the bool object. The next time > >> _M_setup_wait is called it calls _M_setup_proxy_wait again but now it > >> sees _M_obj_size == sizeof(futex) and so this time decides a proxy wait > >> is *not* needed, and then fails the __glibcxx_assert(_M_obj == addr) > >> check. > >> > >> The problem is that _M_setup_wait is calling _M_setup_proxy_wait twice, > >> when it should avoid the second call because the decision to use a proxy > >> wait has already been made. The caller should avoid making the second > >> call by checking whether _M_obj != addr, because that implies that a > >> decision to make a proxy wait has already happened. > > > > I do not think this is correct behavior, the _M_setup_wait have two > responsibilities now: > > * deciding it the proxy wait should be used > > * updating the _M_old value with the most recent _M_ver versions, I am > referring to this line > > // Read the value of the _M_ver counter. > > _M_old = __atomic_load_n(&state->_M_ver, __ATOMIC_ACQUIRE); > > To hit this assertion, this means that there was contention on the proxy > wait slot > > or had a spurious wake, > > It happens on a non-spurious wake as well, the current code always > calls _M_setup_wait after __wait_impl, even if it woke up because the > value changed and a notify call was done. > That what I mean by contention on the wait slot, _M_ver in __watiable_state was notified, but the value of our variable hasn't changed. That most likely mean other variable in the same slot was updated. > > > > still update the _M_old to the latest value. > > > > I think the proper fix matching the approach would be to have: > > bool > > __wait_args::_M_setup_proxy_wait(const void* addr) > > { > > if (_M_obj != addr) { > > // We already decided to use proxy wait, read the value of the > _M_ver counter. > > _M_old = __atomic_load_n(&state->_M_ver, __ATOMIC_ACQUIRE); > > } > > If the condition above is true, the we don't need to call > use_proxy_wait, so this should be if-else: Yes, just return true. > > > if (!use_proxy_wait(*this)) // We can wait on this address directly. > > return false; > > Or maybe you meant to do return true above, after setting _M_old > > > > // This will be a proxy wait, so get a waitable state. > > auto state = set_wait_state(addr, *this); > > > > // The address we will wait on is the version count of the waitable > state: > > _M_obj = &state->_M_ver; > > // __wait_impl and __wait_until_impl need to know this size: > > _M_obj_size = sizeof(state->_M_ver); > > } > > } > > > > // Read the value of the _M_ver counter. > > _M_old = __atomic_load_n(&state->_M_ver, __ATOMIC_ACQUIRE); > > > > return true; > > } > > // See below for alternative > >> > >> > >> This change fixes the bug, by preventing a second call to > >> _M_setup_proxy_wait after we've changed _M_obj and _M_obj_size. But it > >> doesn't prevent a second call in the case where _M_setup_proxy_wait > >> returned false because we made a runtime decision to do a non-proxy > >> wait. That is suboptimal, because it means that code which can wait on > >> the atomic variable directly makes redundant calls into the library just > >> to be told the same answer every time. That's not a problem for now > >> because firstly, it's only a performance degradation not a correctness > >> bug, and secondly because we currently have no targets that ever make a > >> runtime decision to do a non-proxy wait. Linux makes a compile-time > >> decision to do non-proxy waits for futex-sized objects and does proxy > >> waits for everything else, and all non-Linux targets always do proxy > >> waits. So solving the performance problem for non-proxy waits can happen > >> later. We might want to set a bit in __wait_args::_M_flags to say "I > >> already told you this isn't a proxy wait, stop asking me". > > > > I think the best option would be to separate _M_setup_wait (initial > setups) > > and later updates into separate functions. Like _M_setup_wait and > _M_update_wait. > > I will post a patch implementing those suggestion. > > > > > >> > >> > >> libstdc++-v3/ChangeLog: > >> > >> * include/bits/atomic_wait.h (__wait_args::_M_setup_wait): Do > >> not call _M_setup_proxy_wait again if a proxy wait was already > >> set up. > >> --- > >> > >> Tested x86_64-linux and x86_64-freebsd14 (just the parts that use atomic > >> waits). > >> > >> libstdc++-v3/include/bits/atomic_wait.h | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/libstdc++-v3/include/bits/atomic_wait.h > b/libstdc++-v3/include/bits/atomic_wait.h > >> index b6240a95370f..a677dbf0a040 100644 > >> --- a/libstdc++-v3/include/bits/atomic_wait.h > >> +++ b/libstdc++-v3/include/bits/atomic_wait.h > >> @@ -230,7 +230,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >> } > >> > >> if constexpr (!__platform_wait_uses_type<_Tp>) > >> - if (_M_setup_proxy_wait(__addr)) > >> + if (_M_obj != __addr || _M_setup_proxy_wait(__addr)) > > > > Or to reload _M_wait here. > >> > >> { > >> // We will use a proxy wait for this object. > >> // The library has set _M_obj and _M_obj_size and > _M_old. > >> -- > >> 2.52.0 > >> > >
On Fri, 9 Jan 2026 at 09:40, Tomasz Kaminski <tkaminsk@redhat.com> wrote: > > > > On Fri, Jan 9, 2026 at 10:26 AM Jonathan Wakely <jwakely@redhat.com> wrote: >> >> On Fri, 9 Jan 2026 at 08:56, Tomasz Kaminski <tkaminsk@redhat.com> wrote: >> > >> > >> > >> > On Thu, Jan 8, 2026 at 10:50 PM Jonathan Wakely <jwakely@redhat.com> wrote: >> >> >> >> A failed assertion was observed with std::atomic<bool>::wait when the >> >> loop in __atomic_wait_address is entered and calls _M_setup_wait a >> >> second time. When the first call to _M_setup_wait makes a call to >> >> _M_setup_proxy_wait it decides that a proxy wait is needed for bool, and >> >> updates the _M_obj and _M_obj_size members to refer to the futex in the >> >> proxy state, instead of referring to the bool object. The next time >> >> _M_setup_wait is called it calls _M_setup_proxy_wait again but now it >> >> sees _M_obj_size == sizeof(futex) and so this time decides a proxy wait >> >> is *not* needed, and then fails the __glibcxx_assert(_M_obj == addr) >> >> check. >> >> >> >> The problem is that _M_setup_wait is calling _M_setup_proxy_wait twice, >> >> when it should avoid the second call because the decision to use a proxy >> >> wait has already been made. The caller should avoid making the second >> >> call by checking whether _M_obj != addr, because that implies that a >> >> decision to make a proxy wait has already happened. >> > >> > I do not think this is correct behavior, the _M_setup_wait have two responsibilities now: >> > * deciding it the proxy wait should be used >> > * updating the _M_old value with the most recent _M_ver versions, I am referring to this line >> > // Read the value of the _M_ver counter. >> > _M_old = __atomic_load_n(&state->_M_ver, __ATOMIC_ACQUIRE); >> > To hit this assertion, this means that there was contention on the proxy wait slot >> > or had a spurious wake, >> >> It happens on a non-spurious wake as well, the current code always >> calls _M_setup_wait after __wait_impl, even if it woke up because the >> value changed and a notify call was done. > > That what I mean by contention on the wait slot, _M_ver in __watiable_state was > notified, but the value of our variable hasn't changed. That most likely mean other > variable in the same slot was updated. No. The case where the assertion failure was observed is a proxy wait where there is a normal, non-contended wake-up. The value of the variable changed. >> >> >> >> > still update the _M_old to the latest value. >> > >> > I think the proper fix matching the approach would be to have: >> > bool >> > __wait_args::_M_setup_proxy_wait(const void* addr) >> > { >> > if (_M_obj != addr) { >> > // We already decided to use proxy wait, read the value of the _M_ver counter. >> > _M_old = __atomic_load_n(&state->_M_ver, __ATOMIC_ACQUIRE); >> > } >> >> If the condition above is true, the we don't need to call >> use_proxy_wait, so this should be if-else: > > Yes, just return true. >> >> >> >> >> > if (!use_proxy_wait(*this)) // We can wait on this address directly. >> > return false; >> >> Or maybe you meant to do return true above, after setting _M_old >> >> >> > // This will be a proxy wait, so get a waitable state. >> > auto state = set_wait_state(addr, *this); >> > >> > // The address we will wait on is the version count of the waitable state: >> > _M_obj = &state->_M_ver; >> > // __wait_impl and __wait_until_impl need to know this size: >> > _M_obj_size = sizeof(state->_M_ver); >> > } >> > } >> > >> > // Read the value of the _M_ver counter. >> > _M_old = __atomic_load_n(&state->_M_ver, __ATOMIC_ACQUIRE); >> > >> > return true; >> > } >> > // See below for alternative >> >> >> >> >> >> This change fixes the bug, by preventing a second call to >> >> _M_setup_proxy_wait after we've changed _M_obj and _M_obj_size. But it >> >> doesn't prevent a second call in the case where _M_setup_proxy_wait >> >> returned false because we made a runtime decision to do a non-proxy >> >> wait. That is suboptimal, because it means that code which can wait on >> >> the atomic variable directly makes redundant calls into the library just >> >> to be told the same answer every time. That's not a problem for now >> >> because firstly, it's only a performance degradation not a correctness >> >> bug, and secondly because we currently have no targets that ever make a >> >> runtime decision to do a non-proxy wait. Linux makes a compile-time >> >> decision to do non-proxy waits for futex-sized objects and does proxy >> >> waits for everything else, and all non-Linux targets always do proxy >> >> waits. So solving the performance problem for non-proxy waits can happen >> >> later. We might want to set a bit in __wait_args::_M_flags to say "I >> >> already told you this isn't a proxy wait, stop asking me". >> > >> > I think the best option would be to separate _M_setup_wait (initial setups) >> > and later updates into separate functions. Like _M_setup_wait and _M_update_wait. >> > I will post a patch implementing those suggestion. >> > >> > >> >> >> >> >> >> libstdc++-v3/ChangeLog: >> >> >> >> * include/bits/atomic_wait.h (__wait_args::_M_setup_wait): Do >> >> not call _M_setup_proxy_wait again if a proxy wait was already >> >> set up. >> >> --- >> >> >> >> Tested x86_64-linux and x86_64-freebsd14 (just the parts that use atomic >> >> waits). >> >> >> >> libstdc++-v3/include/bits/atomic_wait.h | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/libstdc++-v3/include/bits/atomic_wait.h b/libstdc++-v3/include/bits/atomic_wait.h >> >> index b6240a95370f..a677dbf0a040 100644 >> >> --- a/libstdc++-v3/include/bits/atomic_wait.h >> >> +++ b/libstdc++-v3/include/bits/atomic_wait.h >> >> @@ -230,7 +230,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> >> } >> >> >> >> if constexpr (!__platform_wait_uses_type<_Tp>) >> >> - if (_M_setup_proxy_wait(__addr)) >> >> + if (_M_obj != __addr || _M_setup_proxy_wait(__addr)) >> > >> > Or to reload _M_wait here. >> >> >> >> { >> >> // We will use a proxy wait for this object. >> >> // The library has set _M_obj and _M_obj_size and _M_old. >> >> -- >> >> 2.52.0 >> >> >>
On Fri, Jan 9, 2026 at 10:44 AM Jonathan Wakely <jwakely@redhat.com> wrote: > On Fri, 9 Jan 2026 at 09:40, Tomasz Kaminski <tkaminsk@redhat.com> wrote: > > > > > > > > On Fri, Jan 9, 2026 at 10:26 AM Jonathan Wakely <jwakely@redhat.com> > wrote: > >> > >> On Fri, 9 Jan 2026 at 08:56, Tomasz Kaminski <tkaminsk@redhat.com> > wrote: > >> > > >> > > >> > > >> > On Thu, Jan 8, 2026 at 10:50 PM Jonathan Wakely <jwakely@redhat.com> > wrote: > >> >> > >> >> A failed assertion was observed with std::atomic<bool>::wait when the > >> >> loop in __atomic_wait_address is entered and calls _M_setup_wait a > >> >> second time. When the first call to _M_setup_wait makes a call to > >> >> _M_setup_proxy_wait it decides that a proxy wait is needed for bool, > and > >> >> updates the _M_obj and _M_obj_size members to refer to the futex in > the > >> >> proxy state, instead of referring to the bool object. The next time > >> >> _M_setup_wait is called it calls _M_setup_proxy_wait again but now it > >> >> sees _M_obj_size == sizeof(futex) and so this time decides a proxy > wait > >> >> is *not* needed, and then fails the __glibcxx_assert(_M_obj == addr) > >> >> check. > >> >> > >> >> The problem is that _M_setup_wait is calling _M_setup_proxy_wait > twice, > >> >> when it should avoid the second call because the decision to use a > proxy > >> >> wait has already been made. The caller should avoid making the second > >> >> call by checking whether _M_obj != addr, because that implies that a > >> >> decision to make a proxy wait has already happened. > >> > > >> > I do not think this is correct behavior, the _M_setup_wait have two > responsibilities now: > >> > * deciding it the proxy wait should be used > >> > * updating the _M_old value with the most recent _M_ver versions, I > am referring to this line > >> > // Read the value of the _M_ver counter. > >> > _M_old = __atomic_load_n(&state->_M_ver, __ATOMIC_ACQUIRE); > >> > To hit this assertion, this means that there was contention on the > proxy wait slot > >> > or had a spurious wake, > >> > >> It happens on a non-spurious wake as well, the current code always > >> calls _M_setup_wait after __wait_impl, even if it woke up because the > >> value changed and a notify call was done. > > > > That what I mean by contention on the wait slot, _M_ver in > __watiable_state was > > notified, but the value of our variable hasn't changed. That most likely > mean other > > variable in the same slot was updated. > > No. The case where the assertion failure was observed is a proxy wait > where there is a normal, non-contended wake-up. The value of the > variable changed. > Ah yes, update wait is always called after successful wait, before the predicate is checked. > > > > >> > >> > >> > >> > still update the _M_old to the latest value. > >> > > >> > I think the proper fix matching the approach would be to have: > >> > bool > >> > __wait_args::_M_setup_proxy_wait(const void* addr) > >> > { > >> > if (_M_obj != addr) { > >> > // We already decided to use proxy wait, read the value of the > _M_ver counter. > >> > _M_old = __atomic_load_n(&state->_M_ver, __ATOMIC_ACQUIRE); > >> > } > >> > >> If the condition above is true, the we don't need to call > >> use_proxy_wait, so this should be if-else: > > > > Yes, just return true. > >> > >> > >> > >> > >> > if (!use_proxy_wait(*this)) // We can wait on this address > directly. > >> > return false; > >> > >> Or maybe you meant to do return true above, after setting _M_old > >> > >> > >> > // This will be a proxy wait, so get a waitable state. > >> > auto state = set_wait_state(addr, *this); > >> > > >> > // The address we will wait on is the version count of the > waitable state: > >> > _M_obj = &state->_M_ver; > >> > // __wait_impl and __wait_until_impl need to know this size: > >> > _M_obj_size = sizeof(state->_M_ver); > >> > } > >> > } > >> > > >> > // Read the value of the _M_ver counter. > >> > _M_old = __atomic_load_n(&state->_M_ver, __ATOMIC_ACQUIRE); > >> > > >> > return true; > >> > } > >> > // See below for alternative > >> >> > >> >> > >> >> This change fixes the bug, by preventing a second call to > >> >> _M_setup_proxy_wait after we've changed _M_obj and _M_obj_size. But > it > >> >> doesn't prevent a second call in the case where _M_setup_proxy_wait > >> >> returned false because we made a runtime decision to do a non-proxy > >> >> wait. That is suboptimal, because it means that code which can wait > on > >> >> the atomic variable directly makes redundant calls into the library > just > >> >> to be told the same answer every time. That's not a problem for now > >> >> because firstly, it's only a performance degradation not a > correctness > >> >> bug, and secondly because we currently have no targets that ever > make a > >> >> runtime decision to do a non-proxy wait. Linux makes a compile-time > >> >> decision to do non-proxy waits for futex-sized objects and does proxy > >> >> waits for everything else, and all non-Linux targets always do proxy > >> >> waits. So solving the performance problem for non-proxy waits can > happen > >> >> later. We might want to set a bit in __wait_args::_M_flags to say "I > >> >> already told you this isn't a proxy wait, stop asking me". > >> > > >> > I think the best option would be to separate _M_setup_wait (initial > setups) > >> > and later updates into separate functions. Like _M_setup_wait and > _M_update_wait. > >> > I will post a patch implementing those suggestion. > >> > > >> > > >> >> > >> >> > >> >> libstdc++-v3/ChangeLog: > >> >> > >> >> * include/bits/atomic_wait.h (__wait_args::_M_setup_wait): Do > >> >> not call _M_setup_proxy_wait again if a proxy wait was > already > >> >> set up. > >> >> --- > >> >> > >> >> Tested x86_64-linux and x86_64-freebsd14 (just the parts that use > atomic > >> >> waits). > >> >> > >> >> libstdc++-v3/include/bits/atomic_wait.h | 2 +- > >> >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> >> > >> >> diff --git a/libstdc++-v3/include/bits/atomic_wait.h > b/libstdc++-v3/include/bits/atomic_wait.h > >> >> index b6240a95370f..a677dbf0a040 100644 > >> >> --- a/libstdc++-v3/include/bits/atomic_wait.h > >> >> +++ b/libstdc++-v3/include/bits/atomic_wait.h > >> >> @@ -230,7 +230,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >> >> } > >> >> > >> >> if constexpr (!__platform_wait_uses_type<_Tp>) > >> >> - if (_M_setup_proxy_wait(__addr)) > >> >> + if (_M_obj != __addr || _M_setup_proxy_wait(__addr)) > >> > > >> > Or to reload _M_wait here. > >> >> > >> >> { > >> >> // We will use a proxy wait for this object. > >> >> // The library has set _M_obj and _M_obj_size and > _M_old. > >> >> -- > >> >> 2.52.0 > >> >> > >> > >
diff --git a/libstdc++-v3/include/bits/atomic_wait.h b/libstdc++-v3/include/bits/atomic_wait.h index b6240a95370f..a677dbf0a040 100644 --- a/libstdc++-v3/include/bits/atomic_wait.h +++ b/libstdc++-v3/include/bits/atomic_wait.h @@ -230,7 +230,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } if constexpr (!__platform_wait_uses_type<_Tp>) - if (_M_setup_proxy_wait(__addr)) + if (_M_obj != __addr || _M_setup_proxy_wait(__addr)) { // We will use a proxy wait for this object. // The library has set _M_obj and _M_obj_size and _M_old.