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++,
> > > > <libstd...@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 <jwak...@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));
+ });
+}
