From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mellona.thatsmathematics.com (mellona.thatsmathematics.com [IPv6:2001:4801:7825:103:be76:4eff:fe10:2a2b]) by sourceware.org (Postfix) with ESMTPS id 6B2B63858D37; Mon, 11 Dec 2023 08:16:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6B2B63858D37 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=thatsmathematics.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=thatsmathematics.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 6B2B63858D37 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2001:4801:7825:103:be76:4eff:fe10:2a2b ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702282569; cv=none; b=iFoay9gvoeWDkYMNjy4cllZD56fEC2L3Yj0coAeR33ixjkMvlhSn5GULyeHv6p3GliBkw2lxPgCmgvd22G9gHXpmsjzRnY2/fhQ3a3nqqJaumobYKK4ds93g4x6nznyIVVVmHSPshvvmnhm0RCo1XP7uDBvpdbLl7Yp2KjCk/Ro= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702282569; c=relaxed/simple; bh=tw+DzJ1EKanUC98b53GtsDipI37b03ZBimtyyMHmqpY=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=baccL3F59tAeOr0OmXtChomtdDhmy3yLUd1fOJctyxdN29VXyCoRNRAiYiPPCw7OfsGNb4zoW0wZ80ZlVzHt97XeuO1wrRvmltksaa0tXzvkl1SXux5i0KKjsrHwOZbv1dDvlTdDibA2VWj/tzBi6WBCPkIPokeDvcjU7I1Y5CE= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=thatsmathematics.com; s=condor; t=1702282564; bh=tw+DzJ1EKanUC98b53GtsDipI37b03ZBimtyyMHmqpY=; h=Date:From:To:cc:Subject:From; b=sI8YACb0f3hr5Lzl982udrBmfZH6EaVX/SGhGjg4D9g6mAu+iyeqZ3WVR9NPUHhuL M9n2Q2sfTOJ4IE8wqH0EcRiTW/ntMc94lfG0Udf32VB92JCmmL/aisEsRzoSSATzAU DKl8a4XjATOqnc49J9I6zVT4IPkplont2u6gRQbvLpUPL8QMyhBRp09CwIkxQ1XaEk 4ijM9sEsWPuop1WlUvDZRY27MNsQJXcRRN6c06wG1Jm22Uf5O5AmTBfzQD4VkZs/eI mnCSN7GCvcPw3KWl/Qq8HS4KkN+3PQ4+rx+6c6fDIJp9UZZuir7ihMBCwzH+h3rZEE XOhqib+Hs6LoQ== Received: from moneta.lan (c-67-177-244-133.hsd1.co.comcast.net [67.177.244.133]) (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 mellona.thatsmathematics.com (Postfix) with ESMTPSA id 663E11E8A3D; Mon, 11 Dec 2023 08:16:04 +0000 (UTC) Date: Mon, 11 Dec 2023 01:16:02 -0700 (MST) From: Nate Eldredge To: jwakely@redhat.com cc: trodgers@redhat.com, gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org Subject: Re: [PATCH 1/2] libstdc++: Atomic wait/notify ABI stabilization Message-ID: <88357210-2f36-4711-1b2b-0b4185810e7c@thatsmathematics.com> MIME-Version: 1.0 Content-Type: text/plain; format=flowed; charset=US-ASCII X-Spam-Status: No, score=-0.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,KAM_SHORT,RCVD_IN_BARRACUDACENTRAL,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Ref: https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636805.html, https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636804.html I found a couple of bugs in this patch set. #1: In atomic_wait.h, we have __wait_flags defined to include: __do_spin = 4, __spin_only = 8 | __do_spin, // implies __do_spin So __spin_only is defined as two bits, which breaks when we test `__args & __wait_flags::__spin_only` in __wait_impl(). The test evaluates true even when we have only set __do_spin (bit 2) without bit 3, which is the default at least on Linux and which is supposed to request a limited number of spins before calling futex() and sleeping. You can observe this by seeing that a thread blocked on std::counting_semaphore::acquire() consumes 100% CPU indefinitely. There is another instance in atomic_timed_wait.h. Probably __spin_only should just be 8, and any configuration that wants it should be responsible for manually setting __do_spin as well. #2: __atomic_wait_address() does not populate __args._M_old before passing to __wait_impl(), so it remains at its default value 0 (set by the constructor). As a result, `__wait_impl` is effectively always waiting on the value 0 (i.e. until `*__addr != 0`) rather than whatever value is actually wanted. This is of course wrong, and can deadlock under the right race condition. To fix, we need something like `__args._M_old = __val;` inside the loop in __atomic_wait_address(), so that we always wait on the exact value that the predicate __pred() rejected. Again, there are similar instances in atomic_timed_wait.h. (In the current libstdc++ implementation, the predicate function loads the value itself and doesn't return it, so we wait instead on a possibly different value that was loaded somewhere else in _S_do_spin(). That is also wrong, because the latter value might have been acceptable to the predicate, and if so then waiting on it may deadlock. A classic TOCTOU. This is Bug #104928, reported in March 2022 and still unfixed: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104928. The patch proposed here would fix it, though.) -- Nate Eldredge nate@thatsmathematics.com