From patchwork Tue Dec 7 20:58:03 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 48610 X-Patchwork-Delegate: jwakely.gcc@gmail.com Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B01383858027 for ; Tue, 7 Dec 2021 20:59:34 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B01383858027 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1638910774; bh=CDBJJEqoD2M63mIe7UHTQXzJun7+uffGawjHXdzoCFA=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=fncvSAHcWKmpao7ijVHbOBFVFN8VH8ZY74X+oDXxkZ/G3PdGnbnu+iFR6sb5hCh7Y B2qoXMSW8COcSARSySbKc1KdCNAABiWIg1YqdbAu1kQF4DWghtyuTqBJ9TTpaS1sS9 cx0b7VS0bf9uAwZ5kCTrfh+e00363RsCwZU8v+hs= 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.133.124]) by sourceware.org (Postfix) with ESMTPS id AC1B33858013 for ; Tue, 7 Dec 2021 20:58:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AC1B33858013 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-194-85EeLHKLPiO8bPXNipfnlg-1; Tue, 07 Dec 2021 15:58:06 -0500 X-MC-Unique: 85EeLHKLPiO8bPXNipfnlg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 51BFE61265; Tue, 7 Dec 2021 20:58:05 +0000 (UTC) Received: from localhost (unknown [10.33.36.71]) by smtp.corp.redhat.com (Postfix) with ESMTP id E80351346F; Tue, 7 Dec 2021 20:58:04 +0000 (UTC) To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: [PATCH] libstdc++: Allow std::condition_variable waits to be cancelled [PR103382] Date: Tue, 7 Dec 2021 20:58:03 +0000 Message-Id: <20211207205803.1142706-1-jwakely@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-13.9 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_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP, URI_HEX autolearn=unavailable autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Jonathan Wakely via Gcc-patches From: Jonathan Wakely Reply-To: Jonathan Wakely Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" Tested x86_64-linux. As noted below, I don't think using symbol versioning to preserve the old behaviour is necessary here. Does anybody want to convince me otherwise? std::condition_variable::wait(unique_lock&) is incorrectly marked noexcept, which means that the __forced_unwind exception used by NPTL cancellation will terminate the process. It should allow exceptions to pass through, so that a thread can be cleanly cancelled when waiting on a condition variable. This is arguably an ABI break, because existing code compiled against the old header thinks the function is noexcept, but the definition in the new libstdc++.so can throw. In practice the only exception it can throw is __cxxabiv1::__forced_unwind, and programs using pthread_cancel would probably prefer that behaviour to the old behaviour that terminates the whole process. If necessary we could keep the terminate-on-cancellation behaviour as _ZNSt18condition_variable4waitERSt11unique_lockISt5mutexE@GLIBCXX_3.4.11 and export the new behaviour as @@GLIBCXX_3.4.30, although this patch doesn't do that. libstdc++-v3/ChangeLog: PR libstdc++/103382 * include/bits/std_mutex.h (__condvar::wait, __condvar::wait_until): Remove noexcept. * include/std/condition_variable (condition_variable::wait): Likewise. * src/c++11/condition_variable.cc (condition_variable::wait): Likewise. * testsuite/30_threads/condition_variable/members/103382.cc: New test. --- libstdc++-v3/include/bits/std_mutex.h | 6 +- libstdc++-v3/include/std/condition_variable | 2 +- libstdc++-v3/src/c++11/condition_variable.cc | 2 +- .../condition_variable/members/103382.cc | 66 +++++++++++++++++++ 4 files changed, 71 insertions(+), 5 deletions(-) create mode 100644 libstdc++-v3/testsuite/30_threads/condition_variable/members/103382.cc diff --git a/libstdc++-v3/include/bits/std_mutex.h b/libstdc++-v3/include/bits/std_mutex.h index 357c6891438..6618a54396f 100644 --- a/libstdc++-v3/include/bits/std_mutex.h +++ b/libstdc++-v3/include/bits/std_mutex.h @@ -149,7 +149,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION // Expects: Calling thread has locked __m. void - wait(mutex& __m) noexcept + wait(mutex& __m) { int __e __attribute__((__unused__)) = __gthread_cond_wait(&_M_cond, __m.native_handle()); @@ -157,14 +157,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } void - wait_until(mutex& __m, timespec& __abs_time) noexcept + wait_until(mutex& __m, timespec& __abs_time) { __gthread_cond_timedwait(&_M_cond, __m.native_handle(), &__abs_time); } #ifdef _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT void - wait_until(mutex& __m, clockid_t __clock, timespec& __abs_time) noexcept + wait_until(mutex& __m, clockid_t __clock, timespec& __abs_time) { pthread_cond_clockwait(&_M_cond, __m.native_handle(), __clock, &__abs_time); diff --git a/libstdc++-v3/include/std/condition_variable b/libstdc++-v3/include/std/condition_variable index 4fcec6aa73d..3930cf487e9 100644 --- a/libstdc++-v3/include/std/condition_variable +++ b/libstdc++-v3/include/std/condition_variable @@ -92,7 +92,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION notify_all() noexcept; void - wait(unique_lock& __lock) noexcept; + wait(unique_lock& __lock); template void diff --git a/libstdc++-v3/src/c++11/condition_variable.cc b/libstdc++-v3/src/c++11/condition_variable.cc index ca7902e0373..2d7b8a19b9d 100644 --- a/libstdc++-v3/src/c++11/condition_variable.cc +++ b/libstdc++-v3/src/c++11/condition_variable.cc @@ -36,7 +36,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION condition_variable::~condition_variable() noexcept = default; void - condition_variable::wait(unique_lock& __lock) noexcept + condition_variable::wait(unique_lock& __lock) { _M_cond.wait(*__lock.mutex()); } diff --git a/libstdc++-v3/testsuite/30_threads/condition_variable/members/103382.cc b/libstdc++-v3/testsuite/30_threads/condition_variable/members/103382.cc new file mode 100644 index 00000000000..67396ebf323 --- /dev/null +++ b/libstdc++-v3/testsuite/30_threads/condition_variable/members/103382.cc @@ -0,0 +1,66 @@ +// { dg-options "-pthread" } +// { dg-do run { target { *-*-linux* *-*-gnu* } } } +// { dg-require-effective-target c++11 } +// { dg-require-effective-target pthread } +// { dg-require-gthreads "" } + +#include +#include +#include +#include + +// PR libstdc++/103382 + +template +void +test_cancel(F wait) +{ + std::mutex m; + std::condition_variable cv; + bool waiting = false; + + std::thread t([&] { + std::unique_lock lock(m); + waiting = true; + wait(cv, lock); // __forced_unwind exception should not terminate process. + }); + + // Ensure the condition variable is waiting before we cancel. + // This shouldn't be necessary because pthread_mutex_lock is not + // a cancellation point, but no harm in making sure we test what + // we intend to test: that cancel during a wait doesn't abort. + while (true) + { + std::unique_lock lock(m); + if (waiting) + break; + } + + pthread_cancel(t.native_handle()); + t.join(); +} + +int main() +{ + test_cancel( + [](std::condition_variable& cv, std::unique_lock& l) { + cv.wait(l); + }); + + test_cancel( + [](std::condition_variable& cv, std::unique_lock& l) { + cv.wait(l, []{ return false; }); + }); + + using mins = std::chrono::minutes; + + test_cancel( + [](std::condition_variable& cv, std::unique_lock& l) { + cv.wait_for(l, mins(1)); + }); + + test_cancel( + [](std::condition_variable& cv, std::unique_lock& l) { + cv.wait_until(l, std::chrono::system_clock::now() + mins(1)); + }); +}