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

[Jonathan Wakely <jwakely@redhat.com>]

[-- 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