* [PATCH] libstdc++: Allow std::condition_variable waits to be cancelled [PR103382]
@ 2021-12-07 20:58 Jonathan Wakely
2021-12-07 21:18 ` Florian Weimer
0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Wakely @ 2021-12-07 20:58 UTC (permalink / raw)
To: libstdc++, 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<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
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));
+ });
+}
--
2.31.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++: Allow std::condition_variable waits to be cancelled [PR103382]
2021-12-07 20:58 [PATCH] libstdc++: Allow std::condition_variable waits to be cancelled [PR103382] Jonathan Wakely
@ 2021-12-07 21:18 ` Florian Weimer
2021-12-07 21:38 ` Jonathan Wakely
0 siblings, 1 reply; 9+ messages in thread
From: Florian Weimer @ 2021-12-07 21:18 UTC (permalink / raw)
To: Jonathan Wakely via Libstdc++; +Cc: gcc-patches, Jonathan Wakely
* 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++: Allow std::condition_variable waits to be cancelled [PR103382]
2021-12-07 21:18 ` Florian Weimer
@ 2021-12-07 21:38 ` Jonathan Wakely
2021-12-07 21:52 ` Florian Weimer
0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Wakely @ 2021-12-07 21:38 UTC (permalink / raw)
To: Florian Weimer
Cc: Jonathan Wakely via Libstdc++, Jonathan Wakely, gcc-patches
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++: Allow std::condition_variable waits to be cancelled [PR103382]
2021-12-07 21:38 ` Jonathan Wakely
@ 2021-12-07 21:52 ` Florian Weimer
2021-12-08 0:36 ` Jonathan Wakely
0 siblings, 1 reply; 9+ messages in thread
From: Florian Weimer @ 2021-12-07 21:52 UTC (permalink / raw)
To: Jonathan Wakely
Cc: Jonathan Wakely via Libstdc++, Jonathan Wakely, gcc-patches
* 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++: Allow std::condition_variable waits to be cancelled [PR103382]
2021-12-07 21:52 ` Florian Weimer
@ 2021-12-08 0:36 ` Jonathan Wakely
2021-12-08 17:26 ` Jonathan Wakely
0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Wakely @ 2021-12-08 0:36 UTC (permalink / raw)
To: Florian Weimer
Cc: Jonathan Wakely via Libstdc++, Jonathan Wakely, gcc-patches
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++: Allow std::condition_variable waits to be cancelled [PR103382]
2021-12-08 0:36 ` Jonathan Wakely
@ 2021-12-08 17:26 ` Jonathan Wakely
2021-12-08 17:36 ` Ville Voutilainen
2021-12-09 23:30 ` Jonathan Wakely
0 siblings, 2 replies; 9+ messages in thread
From: Jonathan Wakely @ 2021-12-08 17:26 UTC (permalink / raw)
To: Jonathan Wakely
Cc: Florian Weimer, Jonathan Wakely via Libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2556 bytes --]
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.
[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 3472 bytes --]
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++: Allow std::condition_variable waits to be cancelled [PR103382]
2021-12-08 17:26 ` Jonathan Wakely
@ 2021-12-08 17:36 ` Ville Voutilainen
2021-12-08 18:14 ` Jonathan Wakely
2021-12-09 23:30 ` Jonathan Wakely
1 sibling, 1 reply; 9+ messages in thread
From: Ville Voutilainen @ 2021-12-08 17:36 UTC (permalink / raw)
To: Jonathan Wakely
Cc: Jonathan Wakely, Florian Weimer, Jonathan Wakely via Libstdc++,
gcc-patches
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++: Allow std::condition_variable waits to be cancelled [PR103382]
2021-12-08 17:36 ` Ville Voutilainen
@ 2021-12-08 18:14 ` Jonathan Wakely
0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Wakely @ 2021-12-08 18:14 UTC (permalink / raw)
To: Ville Voutilainen
Cc: Jonathan Wakely, Florian Weimer, Jonathan Wakely via Libstdc++,
gcc-patches
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++: Allow std::condition_variable waits to be cancelled [PR103382]
2021-12-08 17:26 ` Jonathan Wakely
2021-12-08 17:36 ` Ville Voutilainen
@ 2021-12-09 23:30 ` Jonathan Wakely
1 sibling, 0 replies; 9+ messages in thread
From: Jonathan Wakely @ 2021-12-09 23:30 UTC (permalink / raw)
To: Jonathan Wakely
Cc: Florian Weimer, Jonathan Wakely via Libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2169 bytes --]
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.
[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 9395 bytes --]
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));
+ });
+}
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-12-09 23:30 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-07 20:58 [PATCH] libstdc++: Allow std::condition_variable waits to be cancelled [PR103382] Jonathan Wakely
2021-12-07 21:18 ` Florian Weimer
2021-12-07 21:38 ` Jonathan Wakely
2021-12-07 21:52 ` Florian Weimer
2021-12-08 0:36 ` Jonathan Wakely
2021-12-08 17:26 ` Jonathan Wakely
2021-12-08 17:36 ` Ville Voutilainen
2021-12-08 18:14 ` Jonathan Wakely
2021-12-09 23:30 ` Jonathan Wakely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).