diff mbox series

libstdc++: Refactor loops in std::__platform_semaphore

Message ID 20240912204812.494089-1-jwakely@redhat.com
State New
Headers show
Series libstdc++: Refactor loops in std::__platform_semaphore | expand

Commit Message

Jonathan Wakely Sept. 12, 2024, 8:45 p.m. UTC 
You might notice that this removes handling of EINVAL from the call to
sem_timedwait. That error can only happen with a negative ts_nsec value,
which can only happen for a timestamp before the epoch. We should handle
that properly, not just for the case where ts_nsec happens to be
negative. I opened PR 116586 for that, as it's bigger than just
<semaphore>.

Tested x86_64-linux.

-- >8 --

Refactor the loops to all use the same form, and to not need explicit
'break' or 'continue' jumps. This also avoids a -Wunused-variable
warning with -Wsystem-headers.

Also fix a bug for absolute timeouts specified with a time that isn't
implicitly convertible to __clock_t::time_point, e.g. one with a higher
resolution such as picoseconds. Use chrono::ceil to round up to the next
time point representable by the clock.

libstdc++-v3/ChangeLog:

	* include/bits/semaphore_base.h (__platform_semaphore): Refactor
	loops to all use similar forms.
	(__platform_semaphore::_M_try_acquire_until): Use chrono::ceil
	to explicitly convert to __clock_t::time_point.
	* testsuite/30_threads/semaphore/try_acquire_for.cc: Check that
	using a very high resolution timeout compiles.
	* testsuite/30_threads/semaphore/platform_try_acquire_for.cc:
	New test.
---
 libstdc++-v3/include/bits/semaphore_base.h    | 58 +++++++------------
 .../semaphore/platform_try_acquire_for.cc     |  7 +++
 .../30_threads/semaphore/try_acquire_for.cc   | 13 +++++
 3 files changed, 40 insertions(+), 38 deletions(-)
 create mode 100644 libstdc++-v3/testsuite/30_threads/semaphore/platform_try_acquire_for.cc
diff mbox series

Patch

diff --git a/libstdc++-v3/include/bits/semaphore_base.h b/libstdc++-v3/include/bits/semaphore_base.h
index 44a68645e47..2b19c9c6c6a 100644
--- a/libstdc++-v3/include/bits/semaphore_base.h
+++ b/libstdc++-v3/include/bits/semaphore_base.h
@@ -73,52 +73,37 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     _GLIBCXX_ALWAYS_INLINE void
     _M_acquire() noexcept
     {
-      for (;;)
-	{
-	  auto __err = sem_wait(&_M_semaphore);
-	  if (__err && (errno == EINTR))
-	    continue;
-	  else if (__err)
-	    std::__terminate();
-	  else
-	    break;
-	}
+      while (sem_wait(&_M_semaphore))
+	if (errno != EINTR)
+	  std::__terminate();
     }
 
     _GLIBCXX_ALWAYS_INLINE bool
     _M_try_acquire() noexcept
     {
-      for (;;)
+      while (sem_trywait(&_M_semaphore))
 	{
-	  auto __err = sem_trywait(&_M_semaphore);
-	  if (__err && (errno == EINTR))
-	    continue;
-	  else if (__err && (errno == EAGAIN))
+	  if (errno == EAGAIN) // already locked
 	    return false;
-	  else if (__err)
+	  else if (errno != EINTR)
 	    std::__terminate();
-	  else
-	    break;
+	  // else got EINTR so retry
 	}
       return true;
     }
 
     _GLIBCXX_ALWAYS_INLINE void
-    _M_release(std::ptrdiff_t __update) noexcept
+    _M_release(ptrdiff_t __update) noexcept
     {
       for(; __update != 0; --__update)
-	{
-	   auto __err = sem_post(&_M_semaphore);
-	   if (__err)
-	     std::__terminate();
-	}
+	if (sem_post(&_M_semaphore))
+	  std::__terminate();
     }
 
     bool
     _M_try_acquire_until_impl(const chrono::time_point<__clock_t>& __atime)
       noexcept
     {
-
       auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
       auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
 
@@ -128,19 +113,12 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	static_cast<long>(__ns.count())
       };
 
-      for (;;)
+      while (sem_timedwait(&_M_semaphore, &__ts))
 	{
-	  if (auto __err = sem_timedwait(&_M_semaphore, &__ts))
-	    {
-	      if (errno == EINTR)
-		continue;
-	      else if (errno == ETIMEDOUT || errno == EINVAL)
-		return false;
-	      else
-		std::__terminate();
-	    }
-	  else
-	    break;
+	  if (errno == ETIMEDOUT)
+	    return false;
+	  else if (errno != EINTR)
+	    std::__terminate();
 	}
       return true;
     }
@@ -152,10 +130,14 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
 	if constexpr (std::is_same_v<__clock_t, _Clock>)
 	  {
-	    return _M_try_acquire_until_impl(__atime);
+	    using _Dur = __clock_t::duration;
+	    return _M_try_acquire_until_impl(chrono::ceil<_Dur>(__atime));
 	  }
 	else
 	  {
+	    // TODO: if _Clock is monotonic_clock we could use
+	    // sem_clockwait with CLOCK_MONOTONIC.
+
 	    const typename _Clock::time_point __c_entry = _Clock::now();
 	    const auto __s_entry = __clock_t::now();
 	    const auto __delta = __atime - __c_entry;
diff --git a/libstdc++-v3/testsuite/30_threads/semaphore/platform_try_acquire_for.cc b/libstdc++-v3/testsuite/30_threads/semaphore/platform_try_acquire_for.cc
new file mode 100644
index 00000000000..bf6cd142bf0
--- /dev/null
+++ b/libstdc++-v3/testsuite/30_threads/semaphore/platform_try_acquire_for.cc
@@ -0,0 +1,7 @@ 
+// { dg-options "-D_GLIBCXX_USE_POSIX_SEMAPHORE" }
+// { dg-do run { target c++20 } }
+// { dg-additional-options "-pthread" { target pthread } }
+// { dg-require-gthreads "" }
+// { dg-add-options libatomic }
+
+#include "try_acquire_for.cc"
diff --git a/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_for.cc b/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_for.cc
index 53fe34d0753..330f8887e58 100644
--- a/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_for.cc
+++ b/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_for.cc
@@ -78,8 +78,21 @@  void test02()
   b.wait(1);
 }
 
+void
+test03()
+{
+  using tick = std::chrono::system_clock::duration::period;
+  using halftick = std::ratio<tick::num, 2 * tick::den>;
+  std::chrono::duration<long long, halftick> timeout(1);
+  std::binary_semaphore s(1);
+
+  // Using higher resolution than chrono::system_clock should compile:
+  s.try_acquire_for(timeout);
+}
+
 int main()
 {
   test01();
   test02();
+  test03();
 }