Re: [PATCH] t/trodgers/c2a_synchronization

[Jonathan Wakely <jwakely@redhat.com>]

On 05/10/20 15:54 -0700, Thomas Rodgers wrote:
>From: Thomas Rodgers <trodgers@redhat.com>
>
>This *should* be the correct patch this time.

There are still some review comments not addressed. I'll keep all my
comments on this version in a single reply, so they aren't lost among
multiple replies.

I think the significant issues like the handling of spurious wakeups
and the missing "no effects" case in _M_try_acquire() are resolved,
thanks.

>diff --git a/libstdc++-v3/include/bits/atomic_timed_wait.h b/libstdc++-v3/include/bits/atomic_timed_wait.h
>new file mode 100644
>index 00000000000..cd5b6aabc44
>--- /dev/null
>+++ b/libstdc++-v3/include/bits/atomic_timed_wait.h

[...]

>+    struct __timed_waiters : __waiters
>+    {
>+      template<typename _Clock, typename _Duration>
>+	__atomic_wait_status
>+	_M_do_wait_until(__platform_wait_t __version,
>+			 const chrono::time_point<_Clock, _Duration>& __atime)
>+	{
>+#ifdef _GLIBCXX_HAVE_LINUX_FUTEX
>+	  return __platform_wait_until(&_M_ver, __version, __atime);

Qualify to prevent ADL in associated namespaces of the _Clock and
_Duration types.

>+  template<typename _Tp, typename _Pred,
>+	   typename _Clock, typename _Duration>
>+    bool
>+    __atomic_wait_until(const _Tp* __addr, _Tp __old, _Pred __pred,
>+			const chrono::time_point<_Clock, _Duration>&
>+			    __atime) noexcept
>+    {
>+      using namespace __detail;
>+
>+      if (std::__atomic_spin(__pred))
>+	return true;
>+
>+      auto& __w = __timed_waiters::_S_timed_for((void*)__addr);
>+      auto __version = __w._M_enter_wait();
>+      do
>+	{
>+	  __atomic_wait_status __res;
>+	  if constexpr (__platform_wait_uses_type<_Tp>)
>+	    {
>+	      __res = __platform_wait_until((__platform_wait_t*)(void*) __addr,

Qualify to prevent ADL.


>diff --git a/libstdc++-v3/include/bits/atomic_wait.h b/libstdc++-v3/include/bits/atomic_wait.h
>new file mode 100644
>index 00000000000..30f682e828a
>--- /dev/null
>+++ b/libstdc++-v3/include/bits/atomic_wait.h


>+  template<typename _Tp, typename _Pred>
>+    void
>+    __atomic_wait(const _Tp* __addr, _Tp __old, _Pred __pred) noexcept
>+    {
>+      using namespace __detail;
>+      if (__atomic_spin(__pred))

Qualify to prevent ADL.

>diff --git a/libstdc++-v3/include/bits/semaphore_base.h b/libstdc++-v3/include/bits/semaphore_base.h
>new file mode 100644
>index 00000000000..a32a6b12a1b
>--- /dev/null
>+++ b/libstdc++-v3/include/bits/semaphore_base.h
>@@ -0,0 +1,298 @@
>+// -*- C++ -*- header.
>+
>+// Copyright (C) 2020 Free Software Foundation, Inc.
>+//
>+// This file is part of the GNU ISO C++ Library.  This library is free
>+// software; you can redistribute it and/or modify it under the
>+// terms of the GNU General Public License as published by the
>+// Free Software Foundation; either version 3, or (at your option)
>+// any later version.
>+
>+// This library is distributed in the hope that it will be useful,
>+// but WITHOUT ANY WARRANTY; without even the implied warranty of
>+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>+// GNU General Public License for more details.
>+
>+// Under Section 7 of GPL version 3, you are granted additional
>+// permissions described in the GCC Runtime Library Exception, version
>+// 3.1, as published by the Free Software Foundation.
>+
>+// You should have received a copy of the GNU General Public License and
>+// a copy of the GCC Runtime Library Exception along with this program;
>+// see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>+// <http://www.gnu.org/licenses/>.
>+
>+/** @file bits/semaphore_base.h
>+ *  This is an internal header file, included by other library headers.
>+ *  Do not attempt to use it directly. @headername{semaphore}
>+ */
>+
>+#ifndef _GLIBCXX_SEMAPHORE_BASE_H
>+#define _GLIBCXX_SEMAPHORE_BASE_H 1
>+
>+#pragma GCC system_header
>+
>+#include <bits/c++config.h>
>+#include <bits/atomic_base.h>
>+#include <bits/atomic_timed_wait.h>
>+
>+#include <ext/numeric_traits.h>
>+
>+#if __has_include(<semaphore.h>)
>+#define _GLIBCXX_HAVE_POSIX_SEMAPHORE 1
>+#include <semaphore.h>
>+#endif
>+
>+#include <chrono>
>+#include <type_traits>
>+
>+#include <iostream>

This shouldn't be here.

>+
>+namespace std _GLIBCXX_VISIBILITY(default)
>+{
>+_GLIBCXX_BEGIN_NAMESPACE_VERSION
>+
>+#ifdef _GLIBCXX_HAVE_POSIX_SEMAPHORE
>+  struct __platform_semaphore
>+  {
>+    using __clock_t = chrono::system_clock;
>+    static constexpr ptrdiff_t _S_max = SEM_VALUE_MAX;
>+
>+    explicit __platform_semaphore(ptrdiff_t __count) noexcept
>+    {
>+      sem_init(&_M_semaphore, 0, __count);
>+    }
>+
>+    __platform_semaphore(const __platform_semaphore&) = delete;
>+    __platform_semaphore& operator=(const __platform_semaphore&) = delete;
>+
>+    ~__platform_semaphore()
>+    { sem_destroy(&_M_semaphore); }
>+
>+    _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;
>+	}
>+    }
>+
>+    _GLIBCXX_ALWAYS_INLINE void
>+    _M_release(std::ptrdiff_t __update) noexcept
>+    {
>+      for(; __update != 0; --__update)
>+	{
>+	   auto __err = sem_post(&_M_semaphore);
>+	   if (__err)
>+	     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);
>+
>+      struct timespec __ts =
>+      {
>+	static_cast<std::time_t>(__s.time_since_epoch().count()),
>+	static_cast<long>(__ns.count())
>+      };
>+
>+      for (;;)
>+	{
>+	  auto __err = sem_timedwait(&_M_semaphore, &__ts);
>+	  if (__err && (errno == EINTR))
>+	      continue;
>+	  else if (__err && (errno == ETIMEDOUT))
>+	      return false;
>+	  else if (__err && (errno == EINVAL))
>+	      return false; // caller supplied an invalid __atime
>+	  else if (__err)
>+	      std::terminate();
>+	  else
>+	    break;

It looks like this would be simpler with just one check for __err:

       auto __err = sem_timedwait(&_M_semaphore, &__ts);
       if (__err)
         {
           if (errno == EINTR)
             continue;
           if (errno == ETIMEDOUT || errno == EINVAL)
             return false;
           else
             std::terminate();
         }
       else
         break;

Hopefully the compiler wouldn't actually keep testing __err but I think
it reads better this way anyway.


>+	}
>+      return true;
>+    }
>+
>+    template<typename _Clock, typename _Duration>
>+      bool
>+      _M_try_acquire_until(const chrono::time_point<_Clock,
>+			   _Duration>& __atime) noexcept
>+      {
>+	if constexpr (std::is_same<__clock_t, _Clock>::value)

is_same_v (it's not just stylistic, the variable template is faster to
compile than the class template).

>+	  {
>+	    return _M_try_acquire_until_impl(__atime);
>+	  }
>+	else
>+	  {
>+	    const typename _Clock::time_point __c_entry = _Clock::now();
>+	    const __clock_t __s_entry = __clock_t::now();
>+	    const auto __delta = __atime - __c_entry;
>+	    const auto __s_atime = __s_entry + __delta;
>+	    if (_M_try_acquire_until_impl(__s_atime))
>+	      return true;
>+
>+	    // We got a timeout when measured against __clock_t but
>+	    // we need to check against the caller-supplied clock
>+	    // to tell whether we should return a timeout.
>+	    return (_Clock::now() < __atime);
>+	  }
>+      }
>+
>+    template<typename _Rep, typename _Period>
>+      _GLIBCXX_ALWAYS_INLINE bool
>+      _M_try_acquire_for(const chrono::duration<_Rep, _Period>& __rtime)
>+	noexcept
>+      { return _M_try_acquire_until(__clock_t::now() + __rtime); }
>+
>+    private:
>+      sem_t _M_semaphore;
>+    };
>+#endif // _GLIBCXX_HAVE_POSIX_SEMAPHORE
>+
>+    template<typename _Tp>
>+      struct __atomic_semaphore
>+      {
>+	static_assert(std::is_integral_v<_Tp>);
>+	static_assert(__gnu_cxx::__int_traits<_Tp>::__max
>+			<= __gnu_cxx::__int_traits<ptrdiff_t>::__max);
>+	static constexpr ptrdiff_t _S_max = __gnu_cxx::__int_traits<_Tp>::__max;
>+
>+	explicit __atomic_semaphore(_Tp __count) noexcept
>+	  : _M_counter(__count)
>+	{
>+	  __glibcxx_assert(__count >= 0 && __count <= _S_max);
>+	}
>+
>+	__atomic_semaphore(const __atomic_semaphore&) = delete;
>+	__atomic_semaphore& operator=(const __atomic_semaphore&) = delete;
>+
>+	_GLIBCXX_ALWAYS_INLINE void
>+	_M_acquire() noexcept
>+	{
>+	  auto const __pred = [this]
>+	    {
>+	      auto __old = __atomic_impl::load(&this->_M_counter,
>+			      memory_order::acquire);
>+	      if (__old == 0)
>+		return false;
>+	      return __atomic_impl::compare_exchange_strong(&this->_M_counter,
>+			__old, __old - 1,
>+			memory_order::acquire,
>+			memory_order::release);
>+	    };
>+	  auto __old = __atomic_impl::load(&_M_counter, memory_order_relaxed);
>+	  __atomic_wait(&_M_counter, __old, __pred);

Qualify to prevent ADL.


>--- /dev/null
>+++ b/libstdc++-v3/include/std/latch
>@@ -0,0 +1,91 @@
>+// <latch> -*- C++ -*-
>+
>+// Copyright (C) 2020 Free Software Foundation, Inc.
>+//
>+// This file is part of the GNU ISO C++ Library.	This library is free
>+// software; you can redistribute it and/or modify it under the
>+// terms of the GNU General Public License as published by the
>+// Free Software Foundation; either version 3, or (at your option)
>+// any later version.
>+
>+// This library is distributed in the hope that it will be useful,
>+// but WITHOUT ANY WARRANTY; without even the implied warranty of
>+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	 See the
>+// GNU General Public License for more details.
>+
>+// Under Section 7 of GPL version 3, you are granted additional
>+// permissions described in the GCC Runtime Library Exception, version
>+// 3.1, as published by the Free Software Foundation.
>+
>+// You should have received a copy of the GNU General Public License and
>+// a copy of the GCC Runtime Library Exception along with this program;
>+// see the files COPYING3 and COPYING.RUNTIME respectively.	 If not, see
>+// <http://www.gnu.org/licenses/>.
>+
>+/** @file include/latch
>+ *  This is a Standard C++ Library header.
>+ */
>+
>+#ifndef _GLIBCXX_LATCH
>+#define _GLIBCXX_LATCH
>+
>+#pragma GCC system_header
>+
>+#if __cplusplus > 201703L
>+#define __cpp_lib_latch 201907L
>+
>+#include <bits/atomic_base.h>
>+#include <ext/numeric_traits.h>
>+
>+namespace std _GLIBCXX_VISIBILITY(default)
>+{
>+_GLIBCXX_BEGIN_NAMESPACE_VERSION
>+
>+  class latch
>+  {
>+  public:
>+    static constexpr ptrdiff_t
>+    max() noexcept
>+    { return __gnu_cxx::__int_traits<ptrdiff_t>::__max; }
>+
>+    constexpr explicit latch(ptrdiff_t __expected) noexcept
>+      : _M_a(__expected) { }
>+
>+    ~latch() = default;
>+    latch(const latch&) = delete;
>+    latch& operator=(const latch&) = delete;
>+
>+    _GLIBCXX_ALWAYS_INLINE void
>+    count_down(ptrdiff_t __update = 1)
>+    {
>+      auto const __old = __atomic_impl::fetch_sub(&_M_a,
>+				    __update, memory_order::release);
>+      if (__old == __update)
>+	__atomic_impl::notify_all(&_M_a);
>+    }
>+
>+    _GLIBCXX_ALWAYS_INLINE bool
>+    try_wait() const noexcept
>+    { return __atomic_impl::load(&_M_a, memory_order::acquire) == 0; }
>+
>+    _GLIBCXX_ALWAYS_INLINE void
>+    wait() const noexcept
>+    {
>+      auto const __old = __atomic_impl::load(&_M_a, memory_order::acquire);
>+      __atomic_wait(&_M_a, __old, [this] { return this->try_wait(); });

Qualify to prevent ADL.

>+    }
>+
>+    _GLIBCXX_ALWAYS_INLINE void
>+    arrive_and_wait(ptrdiff_t __update = 1) noexcept
>+    {
>+      count_down(__update);
>+      wait();
>+    }
>+
>+  private:
>+    alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a;
>+  };
>+_GLIBCXX_END_NAMESPACE_VERSION
>+} // namespace
>+#endif // __cplusplus > 201703L
>+#endif // _GLIBCXX_LATCH