public inbox for libstdc++@gcc.gnu.org
 help / color / mirror / Atom feed
* [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
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).