Message ID | 20211207205803.1142706-1-jwakely@redhat.com |
---|---|
State | Dropped |
Delegated to: | Jonathan Wakely |
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 server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B01383858027 for <patchwork@sourceware.org>; 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 <gcc-patches@gcc.gnu.org>; 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 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII" 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 <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> From: Jonathan Wakely via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Jonathan Wakely <jwakely@redhat.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
libstdc++: Allow std::condition_variable waits to be cancelled [PR103382]
|
|
Commit Message
Jonathan Wakely
Dec. 7, 2021, 8:58 p.m. UTC
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<mutex>&) 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 <condition_variable> 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
Comments
* Jonathan Wakely via Libstdc: > 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. Note that if this fix escapes into the wild and then you have to make the symbol version change, you will break newer applications. In a few cases in glibc, we proactively added aliases at a different symbol version, but with the same implementation (at first). Thanks, Florian
On Tue, 7 Dec 2021, 21:20 Florian Weimer via Libstdc++, < libstdc++@gcc.gnu.org> wrote: > * Jonathan Wakely via Libstdc: > > > 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. > > Note that if this fix escapes into the wild and then you have to make > the symbol version change, you will break newer applications. In a few > cases in glibc, we proactively added aliases at a different symbol > version, but with the same implementation (at first). > To be safe, we probably should preserve the old behaviour for the old version of the symbol. If we decide that the new behaviour is always preferable, we could change that later by making the old symbol an alias for the new. If we don't decide that, we'll be glad we made it a separate symbol. I'll see if I can get it working with two versioned symbols. We don't actually do that in libstdc++ currently, we only have a single version of every symbol. Thanks for the input - you convinced me that the extra work is worthwhile.
* Jonathan Wakely: > On Tue, 7 Dec 2021, 21:20 Florian Weimer via Libstdc++, <libstdc++@gcc.gnu.org> > wrote: > > * Jonathan Wakely via Libstdc: > > > 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. > > Note that if this fix escapes into the wild and then you have to make > the symbol version change, you will break newer applications. In a few > cases in glibc, we proactively added aliases at a different symbol > version, but with the same implementation (at first). > > To be safe, we probably should preserve the old behaviour for the old > version of the symbol. If we decide that the new behaviour is always > preferable, we could change that later by making the old symbol an > alias for the new. If we don't decide that, we'll be glad we made it a > separate symbol. On the other hand, with separate versions, it's possible to reintroduce the old behavior at a later date, as a bugfix. It's not strictly necessary to do that work upfront. It's just nice to have this option. > I'll see if I can get it working with two versioned symbols. We don't > actually do that in libstdc++ currently, we only have a single version > of every symbol. Ping me if you want to discuss options. 8-> Out of curiosity—do you support building libstdc++ (the shared object) with a different compiler than the included GCC? Thanks, Florian
On Tue, 7 Dec 2021 at 21:52, Florian Weimer wrote: > > * Jonathan Wakely: > > > On Tue, 7 Dec 2021, 21:20 Florian Weimer via Libstdc++, <libstdc++@gcc.gnu.org> > > wrote: > > > > * Jonathan Wakely via Libstdc: > > > > > 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. > > > > Note that if this fix escapes into the wild and then you have to make > > the symbol version change, you will break newer applications. In a few > > cases in glibc, we proactively added aliases at a different symbol > > version, but with the same implementation (at first). > > > > To be safe, we probably should preserve the old behaviour for the old > > version of the symbol. If we decide that the new behaviour is always > > preferable, we could change that later by making the old symbol an > > alias for the new. If we don't decide that, we'll be glad we made it a > > separate symbol. > > On the other hand, with separate versions, it's possible to reintroduce > the old behavior at a later date, as a bugfix. It's not strictly > necessary to do that work upfront. It's just nice to have this option. Ah yes, a new symbol version gives us more flexibility in every direction. > > I'll see if I can get it working with two versioned symbols. We don't > > actually do that in libstdc++ currently, we only have a single version > > of every symbol. > > Ping me if you want to discuss options. 8-> Thanks. I'll try it and let you know how I get on. > Out of curiosity—do you support building libstdc++ (the shared object) > with a different compiler than the included GCC? No. Even using a different GCC is unsupported. I know of two people who've tried using clang, for reasons. But it's unsupported. You can use the installed libstdc++ headers and libs with other non-GCC compilers, but not other versions of GCC. But you can't build it with anything except the GCC built from the same revision of the source tree.
On Wed, 8 Dec 2021 at 00:36, Jonathan Wakely wrote: > > On Tue, 7 Dec 2021 at 21:52, Florian Weimer wrote: > > > > * Jonathan Wakely: > > > > > On Tue, 7 Dec 2021, 21:20 Florian Weimer via Libstdc++, <libstdc++@gcc.gnu.org> > > > wrote: > > > > > > * Jonathan Wakely via Libstdc: > > > > > > > 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. > > > > > > Note that if this fix escapes into the wild and then you have to make > > > the symbol version change, you will break newer applications. In a few > > > cases in glibc, we proactively added aliases at a different symbol > > > version, but with the same implementation (at first). > > > > > > To be safe, we probably should preserve the old behaviour for the old > > > version of the symbol. If we decide that the new behaviour is always > > > preferable, we could change that later by making the old symbol an > > > alias for the new. If we don't decide that, we'll be glad we made it a > > > separate symbol. > > > > On the other hand, with separate versions, it's possible to reintroduce > > the old behavior at a later date, as a bugfix. It's not strictly > > necessary to do that work upfront. It's just nice to have this option. > > Ah yes, a new symbol version gives us more flexibility in every direction. > > > > I'll see if I can get it working with two versioned symbols. We don't > > > actually do that in libstdc++ currently, we only have a single version > > > of every symbol. > > > > Ping me if you want to discuss options. 8-> > > Thanks. I'll try it and let you know how I get on. After resolving a PEBKAC issue, here's an incremental diff that preserves the old behaviour for the existing @GLIBCXX_3.4.11 symbol, but adds a new @@GLIBCXX_3.4.30 symbol that supports cancellation via __forced_unwind. Maybe we should also do this in the implementation of the old noexcept function: __attribute__((used)) void __nothrow_wait_cv::wait(std::unique_lock<std::mutex>& lock) noexcept { int old; int err = pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &old); this->condition_variable::wait(lock); if (!err && old != PTHREAD_CANCEL_DISABLE) pthread_setcancelstate(old, &old); } This would prevent cancellation from terminating a process if it uses the old symbol. So we'd have a new symbol that supports cancellation, and an old one that safely disables it. diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver index 8f3c7b3827e..b747351a1b9 100644 --- a/libstdc++-v3/config/abi/pre/gnu.ver +++ b/libstdc++-v3/config/abi/pre/gnu.ver @@ -1285,7 +1285,6 @@ GLIBCXX_3.4.11 { # condition_variable _ZNSt18condition_variable10notify_allEv; _ZNSt18condition_variable10notify_oneEv; - _ZNSt18condition_variable4waitERSt11unique_lockISt5mutexE; _ZNSt18condition_variableC1Ev; _ZNSt18condition_variableC2Ev; _ZNSt18condition_variableD1Ev; @@ -1295,6 +1294,12 @@ GLIBCXX_3.4.11 { _ZNSt22condition_variable_anyD1Ev; _ZNSt22condition_variable_anyD2Ev; +#ifndef HAVE_SYMVER_SYMBOL_RENAMING_RUNTIME_SUPPORT + # The original definition of this symbol gets versioned as @GLIBCXX_3.4.11 + # if ".symver" is supported, or as @@GLIBCXX_3.4.11 otherwise. + _ZNSt18condition_variable4waitERSt11unique_lockISt5mutexE; +#endif + # thread _ZNSt6thread4joinEv; _ZNSt6thread6detachEv; @@ -2401,6 +2406,11 @@ GLIBCXX_3.4.30 { _ZSt21__glibcxx_assert_fail*; +#ifdef HAVE_SYMVER_SYMBOL_RENAMING_RUNTIME_SUPPORT + # The new definition of this symbol gets versioned as @@GLIBCXX_3.4.30 + _ZNSt18condition_variable4waitERSt11unique_lockISt5mutexE; +#endif + } GLIBCXX_3.4.29; # Symbols in the support library (libsupc++) have their own tag. diff --git a/libstdc++-v3/doc/xml/manual/evolution.xml b/libstdc++-v3/doc/xml/manual/evolution.xml index 271d2225c3a..fd08cd84d20 100644 --- a/libstdc++-v3/doc/xml/manual/evolution.xml +++ b/libstdc++-v3/doc/xml/manual/evolution.xml @@ -1038,6 +1038,13 @@ The <literal>bitmap</literal>, <literal>mt</literal>, and <literal>pool</literal options for <option>--enable-libstdcxx-allocator</option> were removed. </para> +<para> +<function>std::condition_variable::wait</function> changed to be +<code>noexcept(false)</code> to allow thread cancellation exceptions to +be thrown from <function>pthread_cond_wait</function> without aborting +the process. +</para> + </section> </section> diff --git a/libstdc++-v3/src/c++11/compatibility-condvar.cc b/libstdc++-v3/src/c++11/compatibility-condvar.cc index 575d78055cb..439f1844e2c 100644 --- a/libstdc++-v3/src/c++11/compatibility-condvar.cc +++ b/libstdc++-v3/src/c++11/compatibility-condvar.cc @@ -54,4 +54,36 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _GLIBCXX_END_NAMESPACE_VERSION } // namespace std +#if ! _GLIBCXX_INLINE_VERSION +// XXX GLIBCXX_ABI Deprecated +// gcc-12.1 +// std::condition_variable::wait changed to noexcept(false) +#if defined(_GLIBCXX_SYMVER_GNU) && defined(_GLIBCXX_SHARED) \ + && defined(_GLIBCXX_HAVE_AS_SYMVER_DIRECTIVE) \ + && defined(_GLIBCXX_HAVE_SYMVER_SYMBOL_RENAMING_RUNTIME_SUPPORT) +namespace __gnu_cxx _GLIBCXX_VISIBILITY(default) +{ +struct __nothrow_wait_cv : std::condition_variable +{ + void wait(std::unique_lock<std::mutex>&) noexcept; +}; + +__attribute__((used)) +void +__nothrow_wait_cv::wait(std::unique_lock<std::mutex>& lock) noexcept +{ + this->condition_variable::wait(lock); +} +} // namespace __gnu_cxx + +// Export a noexcept wrapper around std::condition_variable::wait +// with the original @GLIBCXX_3.4.11 symbol version. +asm( + ".symver _ZN9__gnu_cxx17__nothrow_wait_cv4waitERSt11unique_lockISt5mutexE," + "_ZNSt18condition_variable4waitERSt11unique_lockISt5mutexE@GLIBCXX_3.4.11," + "local" +); +#endif +#endif + #endif // _GLIBCXX_HAS_GTHREADS && _GLIBCXX_USE_C99_STDINT_TR1
On Wed, 8 Dec 2021 at 19:27, Jonathan Wakely via Libstdc++ <libstdc++@gcc.gnu.org> wrote: > After resolving a PEBKAC issue, here's an incremental diff that > preserves the old behaviour for the existing @GLIBCXX_3.4.11 symbol, > but adds a new @@GLIBCXX_3.4.30 symbol that supports cancellation via > __forced_unwind. > > Maybe we should also do this in the implementation of the old noexcept function: > > __attribute__((used)) > void > __nothrow_wait_cv::wait(std::unique_lock<std::mutex>& lock) noexcept > { > int old; > int err = pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &old); > this->condition_variable::wait(lock); > if (!err && old != PTHREAD_CANCEL_DISABLE) > pthread_setcancelstate(old, &old); > } > > This would prevent cancellation from terminating a process if it uses > the old symbol. So we'd have a new symbol that supports cancellation, > and an old one that safely disables it. That sounds good to me. Also, I'm not sure it was pointed out, for the original: changing a noexcept function to start throwing can leak exceptions through other noexcept functions, hitting catch-blocks instead of terminates, or terminates that occur much later than intended. The compiler will assume that it doesn't need to set up the LSDA in a noexcept function if everything you call is noexcept, and then you don't have the LSDA that would terminate right then and there. That's probably a lesser problem for the thread cancellation exception than it would be for some others, but it's a blood-curdling/chilling possibility that we should just avoid. And you have done that, thanks for that.
On Wed, 8 Dec 2021 at 17:36, Ville Voutilainen wrote: > > On Wed, 8 Dec 2021 at 19:27, Jonathan Wakely via Libstdc++ > <libstdc++@gcc.gnu.org> wrote: > > After resolving a PEBKAC issue, here's an incremental diff that > > preserves the old behaviour for the existing @GLIBCXX_3.4.11 symbol, > > but adds a new @@GLIBCXX_3.4.30 symbol that supports cancellation via > > __forced_unwind. > > > > Maybe we should also do this in the implementation of the old noexcept function: > > > > __attribute__((used)) > > void > > __nothrow_wait_cv::wait(std::unique_lock<std::mutex>& lock) noexcept > > { > > int old; > > int err = pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &old); > > this->condition_variable::wait(lock); > > if (!err && old != PTHREAD_CANCEL_DISABLE) > > pthread_setcancelstate(old, &old); > > } > > > > This would prevent cancellation from terminating a process if it uses > > the old symbol. So we'd have a new symbol that supports cancellation, > > and an old one that safely disables it. > > That sounds good to me. I think I'll disable cancellation as a separate change, because it's not what the existing symbol does, and we should look at doing it anywhere else that will currently terminate. > Also, I'm not sure it was pointed out, for the original: changing a > noexcept function to start throwing can leak exceptions > through other noexcept functions, hitting catch-blocks instead of > terminates, or terminates that occur much later > than intended. The compiler will assume that it doesn't need to set up > the LSDA in a noexcept function if everything > you call is noexcept, and then you don't have the LSDA that would > terminate right then and there. That's probably > a lesser problem for the thread cancellation exception than it would > be for some others, but it's a blood-curdling/chilling possibility > that we should just avoid. And you have done that, thanks for that. Yes, and those kind of problems are probably more likely in practice, because the compiler *always* treated that function as noexcept. Users probably didn't care whether it was or not (and it isn't guaranteed to be by the standard) so wouldn't have gone out of their way to write code that depended on it being noexcept.
On Wed, 8 Dec 2021 at 17:26, Jonathan Wakely wrote: > > On Wed, 8 Dec 2021 at 00:36, Jonathan Wakely wrote: > > > > On Tue, 7 Dec 2021 at 21:52, Florian Weimer wrote: > > > > > > * Jonathan Wakely: > > > > > > > On Tue, 7 Dec 2021, 21:20 Florian Weimer via Libstdc++, <libstdc++@gcc.gnu.org> > > > > wrote: > > > > > > > > * Jonathan Wakely via Libstdc: > > > > > > > > > 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. > > > > > > > > Note that if this fix escapes into the wild and then you have to make > > > > the symbol version change, you will break newer applications. In a few > > > > cases in glibc, we proactively added aliases at a different symbol > > > > version, but with the same implementation (at first). > > > > > > > > To be safe, we probably should preserve the old behaviour for the old > > > > version of the symbol. If we decide that the new behaviour is always > > > > preferable, we could change that later by making the old symbol an > > > > alias for the new. If we don't decide that, we'll be glad we made it a > > > > separate symbol. > > > > > > On the other hand, with separate versions, it's possible to reintroduce > > > the old behavior at a later date, as a bugfix. It's not strictly > > > necessary to do that work upfront. It's just nice to have this option. > > > > Ah yes, a new symbol version gives us more flexibility in every direction. > > > > > > I'll see if I can get it working with two versioned symbols. We don't > > > > actually do that in libstdc++ currently, we only have a single version > > > > of every symbol. > > > > > > Ping me if you want to discuss options. 8-> > > > > Thanks. I'll try it and let you know how I get on. > > After resolving a PEBKAC issue, here's an incremental diff that > preserves the old behaviour for the existing @GLIBCXX_3.4.11 symbol, > but adds a new @@GLIBCXX_3.4.30 symbol that supports cancellation via > __forced_unwind. Pushed to trunk now, as attached. commit 9e18a25331fa25c3907249fede65a02c6817b06e Author: Jonathan Wakely <jwakely@redhat.com> Date: Tue Dec 7 15:11:15 2021 libstdc++: Allow std::condition_variable waits to be cancelled [PR103382] std::condition_variable::wait(unique_lock<mutex>&) 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. The new behaviour is exported as a new version of the symbol, to avoid an ABI break for existing code linked to the non-throwing definition of the function. Code linked against older releases will have a reference to the @GLIBCXX_3.4.11 version, andcode compiled against the new libstdc++ will get a reference to the @@GLIBCXX_3.4.30 version. libstdc++-v3/ChangeLog: PR libstdc++/103382 * config/abi/pre/gnu.ver (GLIBCXX_3.4.11): Do not export old symbol if .symver renaming is supported. (GLIBCXX_3.4.30): Export new symbol if .symver renaming is supported. * doc/xml/manual/evolution.xml: Document change. * doc/html/manual/api.html: Regenerate. * 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. * src/c++11/compatibility-condvar.cc (__nothrow_wait_cv::wait): Define nothrow wrapper around std::condition_variable::wait and export the old symbol as an alias to it. * testsuite/30_threads/condition_variable/members/103382.cc: New test. diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver index 8f3c7b3827e..b747351a1b9 100644 --- a/libstdc++-v3/config/abi/pre/gnu.ver +++ b/libstdc++-v3/config/abi/pre/gnu.ver @@ -1285,7 +1285,6 @@ GLIBCXX_3.4.11 { # condition_variable _ZNSt18condition_variable10notify_allEv; _ZNSt18condition_variable10notify_oneEv; - _ZNSt18condition_variable4waitERSt11unique_lockISt5mutexE; _ZNSt18condition_variableC1Ev; _ZNSt18condition_variableC2Ev; _ZNSt18condition_variableD1Ev; @@ -1295,6 +1294,12 @@ GLIBCXX_3.4.11 { _ZNSt22condition_variable_anyD1Ev; _ZNSt22condition_variable_anyD2Ev; +#ifndef HAVE_SYMVER_SYMBOL_RENAMING_RUNTIME_SUPPORT + # The original definition of this symbol gets versioned as @GLIBCXX_3.4.11 + # if ".symver" is supported, or as @@GLIBCXX_3.4.11 otherwise. + _ZNSt18condition_variable4waitERSt11unique_lockISt5mutexE; +#endif + # thread _ZNSt6thread4joinEv; _ZNSt6thread6detachEv; @@ -2401,6 +2406,11 @@ GLIBCXX_3.4.30 { _ZSt21__glibcxx_assert_fail*; +#ifdef HAVE_SYMVER_SYMBOL_RENAMING_RUNTIME_SUPPORT + # The new definition of this symbol gets versioned as @@GLIBCXX_3.4.30 + _ZNSt18condition_variable4waitERSt11unique_lockISt5mutexE; +#endif + } GLIBCXX_3.4.29; # Symbols in the support library (libsupc++) have their own tag. diff --git a/libstdc++-v3/doc/xml/manual/evolution.xml b/libstdc++-v3/doc/xml/manual/evolution.xml index a169102ef43..34e44ee93e4 100644 --- a/libstdc++-v3/doc/xml/manual/evolution.xml +++ b/libstdc++-v3/doc/xml/manual/evolution.xml @@ -1041,6 +1041,13 @@ no longer derives from <classname>__gnu_cxx::new_allocator</classname>; they both derive from <classname>std::__new_allocator</classname> instead. </para> +<para> +<function>std::condition_variable::wait</function> changed to be +<code>noexcept(false)</code> to allow thread cancellation exceptions to +be thrown from <function>pthread_cond_wait</function> without aborting +the process. +</para> + </section> </section> 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<mutex>& __lock) noexcept; + wait(unique_lock<mutex>& __lock); template<typename _Predicate> void diff --git a/libstdc++-v3/src/c++11/compatibility-condvar.cc b/libstdc++-v3/src/c++11/compatibility-condvar.cc index 575d78055cb..07c39d48f5a 100644 --- a/libstdc++-v3/src/c++11/compatibility-condvar.cc +++ b/libstdc++-v3/src/c++11/compatibility-condvar.cc @@ -54,4 +54,35 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _GLIBCXX_END_NAMESPACE_VERSION } // namespace std +#if ! _GLIBCXX_INLINE_VERSION +// XXX GLIBCXX_ABI Deprecated +// gcc-12.1 +// std::condition_variable::wait changed to noexcept(false) +#if defined(_GLIBCXX_SYMVER_GNU) && defined(_GLIBCXX_SHARED) \ + && defined(_GLIBCXX_HAVE_AS_SYMVER_DIRECTIVE) \ + && defined(_GLIBCXX_HAVE_SYMVER_SYMBOL_RENAMING_RUNTIME_SUPPORT) +namespace __gnu_cxx _GLIBCXX_VISIBILITY(default) +{ +struct __nothrow_wait_cv : std::condition_variable +{ + void wait(std::unique_lock<std::mutex>&) noexcept; +}; + +__attribute__((used)) +void +__nothrow_wait_cv::wait(std::unique_lock<std::mutex>& lock) noexcept +{ + this->condition_variable::wait(lock); +} +} // namespace __gnu_cxx + +// Export a noexcept wrapper around std::condition_variable::wait +// with the original @GLIBCXX_3.4.11 symbol version. +asm( + ".symver _ZN9__gnu_cxx17__nothrow_wait_cv4waitERSt11unique_lockISt5mutexE," + "_ZNSt18condition_variable4waitERSt11unique_lockISt5mutexE@GLIBCXX_3.4.11" +); +#endif +#endif + #endif // _GLIBCXX_HAS_GTHREADS && _GLIBCXX_USE_C99_STDINT_TR1 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<mutex>& __lock) noexcept + condition_variable::wait(unique_lock<mutex>& __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 <condition_variable> +#include <chrono> +#include <mutex> +#include <thread> + +// PR libstdc++/103382 + +template<typename F> +void +test_cancel(F wait) +{ + std::mutex m; + std::condition_variable cv; + bool waiting = false; + + std::thread t([&] { + std::unique_lock<std::mutex> 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<std::mutex> lock(m); + if (waiting) + break; + } + + pthread_cancel(t.native_handle()); + t.join(); +} + +int main() +{ + test_cancel( + [](std::condition_variable& cv, std::unique_lock<std::mutex>& l) { + cv.wait(l); + }); + + test_cancel( + [](std::condition_variable& cv, std::unique_lock<std::mutex>& l) { + cv.wait(l, []{ return false; }); + }); + + using mins = std::chrono::minutes; + + test_cancel( + [](std::condition_variable& cv, std::unique_lock<std::mutex>& l) { + cv.wait_for(l, mins(1)); + }); + + test_cancel( + [](std::condition_variable& cv, std::unique_lock<std::mutex>& l) { + cv.wait_until(l, std::chrono::system_clock::now() + mins(1)); + }); +}
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<mutex>& __lock) noexcept; + wait(unique_lock<mutex>& __lock); template<typename _Predicate> 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<mutex>& __lock) noexcept + condition_variable::wait(unique_lock<mutex>& __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 <condition_variable> +#include <chrono> +#include <mutex> +#include <thread> + +// PR libstdc++/103382 + +template<typename F> +void +test_cancel(F wait) +{ + std::mutex m; + std::condition_variable cv; + bool waiting = false; + + std::thread t([&] { + std::unique_lock<std::mutex> 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<std::mutex> lock(m); + if (waiting) + break; + } + + pthread_cancel(t.native_handle()); + t.join(); +} + +int main() +{ + test_cancel( + [](std::condition_variable& cv, std::unique_lock<std::mutex>& l) { + cv.wait(l); + }); + + test_cancel( + [](std::condition_variable& cv, std::unique_lock<std::mutex>& l) { + cv.wait(l, []{ return false; }); + }); + + using mins = std::chrono::minutes; + + test_cancel( + [](std::condition_variable& cv, std::unique_lock<std::mutex>& l) { + cv.wait_for(l, mins(1)); + }); + + test_cancel( + [](std::condition_variable& cv, std::unique_lock<std::mutex>& l) { + cv.wait_until(l, std::chrono::system_clock::now() + mins(1)); + }); +}