Re: [PATCH] libstdc++: Allow std::condition_variable waits to be cancelled [PR103382]

[Jonathan Wakely <jwakely@redhat.com>]

[-- 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));
+      });
+}