Re: GCC 11.1 Release Candidate available from gcc.gnu.org

Re: GCC 11.1 Release Candidate available from gcc.gnu.org

[Jakub Jelinek]

On Tue, Apr 20, 2021 at 10:12:33PM -0700, Thomas Rodgers wrote:
> I think the attached patch (also in BZ) addresses the issue in
> bits/semaphore_base.h, but I'm going to defer to Jonathan on why the macro
> name is being transformed incorrectly in the first place.

Jonathan's call, but for me it looks much better to fix up the
macro name on the configure side.
The include/Makefile.am sed is:
        sed -e 's/HAVE_/_GLIBCXX_HAVE_/g' \
            -e 's/PACKAGE/_GLIBCXX_PACKAGE/g' \
            -e 's/VERSION/_GLIBCXX_VERSION/g' \
            -e 's/WORDS_/_GLIBCXX_WORDS_/g' \
            -e 's/_DARWIN_USE_64_BIT_INODE/_GLIBCXX_DARWIN_USE_64_BIT_INODE/g' \
            -e 's/_FILE_OFFSET_BITS/_GLIBCXX_FILE_OFFSET_BITS/g' \
            -e 's/_LARGE_FILES/_GLIBCXX_LARGE_FILES/g' \
            -e 's/ICONV_CONST/_GLIBCXX_ICONV_CONST/g' \
            -e '/[       ]_GLIBCXX_LONG_DOUBLE_COMPAT[   ]/d' \
            -e '/[       ]_GLIBCXX_LONG_DOUBLE_ALT128_COMPAT[    ]/d' \
            < ${CONFIG_HEADER} >> $@ ;\
so for many macros one needs _GLIBCXX_ prefixes already in configure,
as can be seen in grep AC_DEFINE.*_GLIBCXX configure.ac acinclude.m4
But _GLIBCXX_HAVE_POSIX_SEMAPHORE is the only one that shouldn't have
that prefix because the sed is adding that.
E.g. on i686-linux, I see
grep _GLIBCXX__GLIBCXX c++config.h 
#define _GLIBCXX__GLIBCXX_HAVE_POSIX_SEMAPHORE 1
that proves it is the only broken one.

So, I think the right fix is:

2021-04-21  Jakub Jelinek  <jakub@redhat.com>

	PR libstdc++/100164
	* acinclude.m4: For POSIX semaphores AC_DEFINE HAVE_POSIX_SEMAPHORE
	rather than _GLIBCXX_HAVE_POSIX_SEMAPHORE.
	* configure: Regenerated.
	* config.h.in: Regenerated.

--- libstdc++-v3/acinclude.m4.jj	2020-12-16 14:42:46.501084530 +0100
+++ libstdc++-v3/acinclude.m4	2021-04-21 10:51:52.450419650 +0200
@@ -4097,7 +4097,7 @@ AC_DEFUN([GLIBCXX_CHECK_GTHREADS], [
       [ac_have_posix_semaphore=no])
 
   if test $ac_have_posix_semaphore = yes ; then
-    AC_DEFINE(_GLIBCXX_HAVE_POSIX_SEMAPHORE,
+    AC_DEFINE(HAVE_POSIX_SEMAPHORE,
 	      1,
 	      [Define to 1 if POSIX Semaphores with sem_timedwait are available in <semaphore.h>.])
   fi
--- libstdc++-v3/configure.jj	2021-04-09 10:18:15.701265030 +0200
+++ libstdc++-v3/configure	2021-04-21 10:52:06.730259672 +0200
@@ -75836,7 +75836,7 @@ fi
 
   if test $ac_have_posix_semaphore = yes ; then
 
-$as_echo "#define _GLIBCXX_HAVE_POSIX_SEMAPHORE 1" >>confdefs.h
+$as_echo "#define HAVE_POSIX_SEMAPHORE 1" >>confdefs.h
 
   fi
   { $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_have_posix_semaphore" >&5
--- libstdc++-v3/config.h.in.jj	2020-12-17 16:06:39.808526661 +0100
+++ libstdc++-v3/config.h.in	2021-04-21 10:52:09.709226370 +0200
@@ -291,6 +291,10 @@
 /* Define to 1 if you have the `posix_memalign' function. */
 #undef HAVE_POSIX_MEMALIGN
 
+/* Define to 1 if POSIX Semaphores with sem_timedwait are available in
+   <semaphore.h>. */
+#undef HAVE_POSIX_SEMAPHORE
+
 /* Define to 1 if you have the `powf' function. */
 #undef HAVE_POWF
 
@@ -818,10 +822,6 @@
 /* Define if gthreads library is available. */
 #undef _GLIBCXX_HAS_GTHREADS
 
-/* Define to 1 if POSIX Semaphores with sem_timedwait are available in
-   <semaphore.h>. */
-#undef _GLIBCXX_HAVE_POSIX_SEMAPHORE
-
 /* Define to 1 if a full hosted library is built, or 0 if freestanding. */
 #undef _GLIBCXX_HOSTED
 


	Jakub

Re: GCC 11.1 Release Candidate available from gcc.gnu.org

[Jonathan Wakely]

On 21/04/21 10:56 +0200, Jakub Jelinek via Libstdc++ wrote:
>On Tue, Apr 20, 2021 at 10:12:33PM -0700, Thomas Rodgers wrote:
>> I think the attached patch (also in BZ) addresses the issue in
>> bits/semaphore_base.h, but I'm going to defer to Jonathan on why the macro
>> name is being transformed incorrectly in the first place.
>
>Jonathan's call, but for me it looks much better to fix up the
>macro name on the configure side.
>The include/Makefile.am sed is:
>        sed -e 's/HAVE_/_GLIBCXX_HAVE_/g' \
>            -e 's/PACKAGE/_GLIBCXX_PACKAGE/g' \
>            -e 's/VERSION/_GLIBCXX_VERSION/g' \
>            -e 's/WORDS_/_GLIBCXX_WORDS_/g' \
>            -e 's/_DARWIN_USE_64_BIT_INODE/_GLIBCXX_DARWIN_USE_64_BIT_INODE/g' \
>            -e 's/_FILE_OFFSET_BITS/_GLIBCXX_FILE_OFFSET_BITS/g' \
>            -e 's/_LARGE_FILES/_GLIBCXX_LARGE_FILES/g' \
>            -e 's/ICONV_CONST/_GLIBCXX_ICONV_CONST/g' \
>            -e '/[       ]_GLIBCXX_LONG_DOUBLE_COMPAT[   ]/d' \
>            -e '/[       ]_GLIBCXX_LONG_DOUBLE_ALT128_COMPAT[    ]/d' \
>            < ${CONFIG_HEADER} >> $@ ;\
>so for many macros one needs _GLIBCXX_ prefixes already in configure,
>as can be seen in grep AC_DEFINE.*_GLIBCXX configure.ac acinclude.m4
>But _GLIBCXX_HAVE_POSIX_SEMAPHORE is the only one that shouldn't have
>that prefix because the sed is adding that.
>E.g. on i686-linux, I see
>grep _GLIBCXX__GLIBCXX c++config.h
>#define _GLIBCXX__GLIBCXX_HAVE_POSIX_SEMAPHORE 1
>that proves it is the only broken one.
>
>So, I think the right fix is:
>
>2021-04-21  Jakub Jelinek  <jakub@redhat.com>
>
>	PR libstdc++/100164
>	* acinclude.m4: For POSIX semaphores AC_DEFINE HAVE_POSIX_SEMAPHORE
>	rather than _GLIBCXX_HAVE_POSIX_SEMAPHORE.
>	* configure: Regenerated.
>	* config.h.in: Regenerated.
>
>--- libstdc++-v3/acinclude.m4.jj	2020-12-16 14:42:46.501084530 +0100
>+++ libstdc++-v3/acinclude.m4	2021-04-21 10:51:52.450419650 +0200
>@@ -4097,7 +4097,7 @@ AC_DEFUN([GLIBCXX_CHECK_GTHREADS], [
>       [ac_have_posix_semaphore=no])
>
>   if test $ac_have_posix_semaphore = yes ; then
>-    AC_DEFINE(_GLIBCXX_HAVE_POSIX_SEMAPHORE,
>+    AC_DEFINE(HAVE_POSIX_SEMAPHORE,
> 	      1,
> 	      [Define to 1 if POSIX Semaphores with sem_timedwait are available in <semaphore.h>.])
>   fi

Yes, this is correct. Please push.

Re: GCC 11.1 Release Candidate available from gcc.gnu.org

[Jonathan Wakely]

On 20/04/21 22:12 -0700, Thomas Rodgers wrote:
>@@ -86,6 +88,24 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	}
>     }
> 
>+    _GLIBCXX_ALWAYS_INLINE bool
>+    _M_try_acquire() noexcept
>+    {
>+      for (;;)
>+	{
>+	  auto __err = sem_trywait(&_M_semaphore);
>+	  if (__err && (errno == EINTR))
>+	    continue;
>+	  else if (__err && (errno == EAGAIN))
>+	    return false;
>+	  else if (__err)
>+	    std::terminate();
>+	  else
>+	    break;
>+	}
>+      return true;
>+    }
>+
>     _GLIBCXX_ALWAYS_INLINE void
>     _M_release(std::ptrdiff_t __update) noexcept
>     {

Please just commit this part to trunk and gcc-11, not the macro
renaming (as that's been fixed by Jakub already).

Re: GCC 11.1 Release Candidate available from gcc.gnu.org

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

On 21/04/21 12:38 +0100, Jonathan Wakely wrote:
>On 20/04/21 22:12 -0700, Thomas Rodgers wrote:
>>@@ -86,6 +88,24 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>	}
>>    }
>>
>>+    _GLIBCXX_ALWAYS_INLINE bool
>>+    _M_try_acquire() noexcept
>>+    {
>>+      for (;;)
>>+	{
>>+	  auto __err = sem_trywait(&_M_semaphore);
>>+	  if (__err && (errno == EINTR))
>>+	    continue;
>>+	  else if (__err && (errno == EAGAIN))
>>+	    return false;
>>+	  else if (__err)
>>+	    std::terminate();
>>+	  else
>>+	    break;
>>+	}
>>+      return true;
>>+    }
>>+
>>    _GLIBCXX_ALWAYS_INLINE void
>>    _M_release(std::ptrdiff_t __update) noexcept
>>    {
>
>Please just commit this part to trunk and gcc-11, not the macro
>renaming (as that's been fixed by Jakub already).

I think on trunk I'd prefer to do the attached. WDYT?



[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 9730 bytes --]

commit 07241d79b6720d4f392d5a8ba6e9c21d2801c8c7
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Apr 21 13:04:05 2021

    libstdc++: Streamline <semaphore> implementation [PR 100164]
    
    This adds the missing _M_try_acquire member function so that
    __platform_semaphore works.
    
    Also adjust the <semaphore> implementation so that __platform_semaphore
    is not defined unless we're going to use it, which avoids including
    <semaphore.h> (and polluting he global namespace) when we don't need it.
    
    Also rename the _GLIBCXX_REQUIRE_POSIX_SEMAPHORE macro to
    _GLIBCXX_USE_POSIX_SEMAPHORE for consistency with the similar
    _GLIBCXX_USE_CXX11_ABI macro that can be used to request an alternative
    (ABI-changing) implementation.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/100164
            * include/bits/semaphore_base.h: Only define at most one of
            __platform_semaphore and __atomic_semaphore.
            (__platform_semaphore::_M_try_acquire()): Add missing function.
            * include/std/semaphore: Do not define anything unless one of
            the implementations is available.
            * testsuite/30_threads/semaphore/try_acquire_posix.cc: Define
            macro to request POSIX semaphore implementation. Use standard
            API, not private implementation.

diff --git a/libstdc++-v3/include/bits/semaphore_base.h b/libstdc++-v3/include/bits/semaphore_base.h
index 7e3235d182e..80134d7fc4c 100644
--- a/libstdc++-v3/include/bits/semaphore_base.h
+++ b/libstdc++-v3/include/bits/semaphore_base.h
@@ -1,4 +1,4 @@
-// -*- C++ -*- header.
+// std::counting_semaphore implementation -*- C++ -*- header.
 
 // Copyright (C) 2020-2021 Free Software Foundation, Inc.
 //
@@ -32,25 +32,32 @@
 
 #pragma GCC system_header
 
-#include <bits/atomic_base.h>
-#if __cpp_lib_atomic_wait
-#include <bits/atomic_timed_wait.h>
-#include <ext/numeric_traits.h>
-#endif // __cpp_lib_atomic_wait
-
-#ifdef _GLIBCXX_HAVE_POSIX_SEMAPHORE
-# include <limits.h>
-# include <semaphore.h>
-#endif
-
 #include <chrono>
 #include <type_traits>
 
+// Note: the _GLIBCXX_USE_POSIX_SEMAPHORE macro can be used to force the
+// use of Posix semaphores (sem_t). Doing so however, alters the ABI.
+
+#ifndef _GLIBCXX_HAVE_POSIX_SEMAPHORE
+# undef _GLIBCXX_USE_POSIX_SEMAPHORE
+#endif
+
+#include <bits/atomic_base.h>
+#if __cpp_lib_atomic_wait && ! _GLIBCXX_USE_POSIX_SEMAPHORE
+# include <bits/atomic_timed_wait.h>
+# include <ext/numeric_traits.h>
+#elif _GLIBCXX_HAVE_POSIX_SEMAPHORE
+# include <limits.h>
+# include <semaphore.h>
+# undef _GLIBCXX_USE_POSIX_SEMAPHORE
+# define _GLIBCXX_USE_POSIX_SEMAPHORE 1
+#endif
+
 namespace std _GLIBCXX_VISIBILITY(default)
 {
 _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
-#ifdef _GLIBCXX_HAVE_POSIX_SEMAPHORE
+#if _GLIBCXX_USE_POSIX_SEMAPHORE
   struct __platform_semaphore
   {
     using __clock_t = chrono::system_clock;
@@ -76,23 +83,44 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     {
       for (;;)
 	{
-	  auto __err = sem_wait(&_M_semaphore);
-	  if (__err && (errno == EINTR))
-	    continue;
-	  else if (__err)
-	    std::terminate();
+	  if (auto __err = sem_wait(&_M_semaphore))
+	    {
+	      if (errno == EINTR)
+		continue;
+	      else
+		std::terminate();
+	    }
 	  else
 	    break;
 	}
     }
 
+    _GLIBCXX_ALWAYS_INLINE bool
+    _M_try_acquire() noexcept
+    {
+      for (;;)
+	{
+	  if (auto __err = sem_trywait(&_M_semaphore))
+	    {
+	      if (errno == EINTR)
+		continue;
+	      else if (errno == EAGAIN)
+		return false;
+	      else
+		std::terminate();
+	    }
+	  else
+	    break;
+	}
+      return true;
+    }
+
     _GLIBCXX_ALWAYS_INLINE void
     _M_release(std::ptrdiff_t __update) noexcept
     {
       for(; __update != 0; --__update)
 	{
-	   auto __err = sem_post(&_M_semaphore);
-	   if (__err)
+	   if (auto __err = sem_post(&_M_semaphore))
 	     std::terminate();
 	}
     }
@@ -162,9 +190,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   private:
     sem_t _M_semaphore;
   };
-#endif // _GLIBCXX_HAVE_POSIX_SEMAPHORE
 
-#if __cpp_lib_atomic_wait
+  using __semaphore_impl = __platform_semaphore;
+
+#elif __cpp_lib_atomic_wait
+
   struct __atomic_semaphore
   {
     static constexpr ptrdiff_t _S_max = __gnu_cxx::__int_traits<int>::__max;
@@ -245,19 +275,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   private:
     alignas(__detail::__platform_wait_alignment)
-    __detail::__platform_wait_t _M_counter;
+      __detail::__platform_wait_t _M_counter;
   };
-#endif // __cpp_lib_atomic_wait
 
-// Note: the _GLIBCXX_REQUIRE_POSIX_SEMAPHORE macro can be used to force the
-// use of Posix semaphores (sem_t). Doing so however, alters the ABI.
-#if defined __cpp_lib_atomic_wait && !_GLIBCXX_REQUIRE_POSIX_SEMAPHORE
   using __semaphore_impl = __atomic_semaphore;
-#elif _GLIBCXX_HAVE_POSIX_SEMAPHORE
-  using __semaphore_impl = __platform_semaphore;
-#else
-#  error "No suitable semaphore implementation available"
-#endif
+#endif // __cpp_lib_atomic_wait
 
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace std
diff --git a/libstdc++-v3/include/std/semaphore b/libstdc++-v3/include/std/semaphore
index a1560915d83..43abe8a9495 100644
--- a/libstdc++-v3/include/std/semaphore
+++ b/libstdc++-v3/include/std/semaphore
@@ -34,6 +34,7 @@
 #if __cplusplus > 201703L
 #include <bits/semaphore_base.h>
 
+#if __cpp_lib_atomic_wait || _GLIBCXX_USE_POSIX_SEMAPHORE
 namespace std _GLIBCXX_VISIBILITY(default)
 {
 _GLIBCXX_BEGIN_NAMESPACE_VERSION
@@ -89,5 +90,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace
+#endif // cpp_lib_atomic_wait || _GLIBCXX_USE_POSIX_SEMAPHORE
 #endif // C++20
 #endif // _GLIBCXX_SEMAPHORE
diff --git a/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_posix.cc b/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_posix.cc
index 97e386f7b76..b18fb623ecc 100644
--- a/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_posix.cc
+++ b/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_posix.cc
@@ -20,30 +20,39 @@
 // { dg-require-effective-target pthread }
 // { dg-require-gthreads "" }
 
+// Request the use of POSIX semaphores
+#define _GLIBCXX_USE_POSIX_SEMAPHORE 1
 #include <semaphore>
-#ifdef _GLIBCXX_HAVE_POSIX_SEMAPHORE
+
+#if __cpp_lib_semaphore
+#if _GLIBCXX_USE_POSIX_SEMAPHORE
+static_assert(sizeof(std::counting_semaphore<>) == sizeof(::sem_t));
+#endif
+
 #include <chrono>
 #include <thread>
 #include <atomic>
 #include <testsuite_hooks.h>
 
+using semaphore = std::counting_semaphore<>;
+
 void test01()
 {
   using namespace std::chrono_literals;
-  std::__platform_semaphore s(2);
-  s._M_acquire();
+  semaphore s(2);
+  s.acquire();
 
   auto const dur = 250ms;
   {
     auto const t0 = std::chrono::steady_clock::now();
-    VERIFY( s._M_try_acquire_for(dur) );
+    VERIFY( s.try_acquire_for(dur) );
     auto const diff = std::chrono::steady_clock::now() - t0;
     VERIFY( diff < dur );
   }
 
   {
     auto const t0 = std::chrono::steady_clock::now();
-    VERIFY( !s._M_try_acquire_for(dur) );
+    VERIFY( !s.try_acquire_for(dur) );
     auto const diff = std::chrono::steady_clock::now() - t0;
     VERIFY( diff >= dur );
   }
@@ -52,27 +61,27 @@ void test01()
 void test02()
 {
   using namespace std::chrono_literals;
-  std::__platform_semaphore s(1);
+  semaphore s(1);
   std::atomic<int> a(0), b(0);
   std::thread t([&] {
     a.wait(0);
     auto const dur = 250ms;
-    VERIFY( !s._M_try_acquire_for(dur) );
+    VERIFY( !s.try_acquire_for(dur) );
     b++;
     b.notify_one();
 
     a.wait(1);
-    VERIFY( s._M_try_acquire_for(dur) );
+    VERIFY( s.try_acquire_for(dur) );
     b++;
     b.notify_one();
   });
   t.detach();
 
-  s._M_acquire();
+  s.acquire();
   a++;
   a.notify_one();
   b.wait(0);
-  s._M_release(1);
+  s.release(1);
   a++;
   a.notify_one();
 
@@ -82,14 +91,14 @@ void test02()
 void test03()
 {
   using namespace std::chrono_literals;
-  std::__platform_semaphore s(2);
-  s._M_acquire();
+  semaphore s(2);
+  s.acquire();
 
   auto const dur = 250ms;
   {
     auto const at = std::chrono::system_clock::now() + dur;
     auto const t0 = std::chrono::steady_clock::now();
-    VERIFY( s._M_try_acquire_until(at) );
+    VERIFY( s.try_acquire_until(at) );
     auto const diff = std::chrono::steady_clock::now() - t0;
     VERIFY( diff < dur );
   }
@@ -97,7 +106,7 @@ void test03()
   {
     auto const at = std::chrono::system_clock::now() + dur;
     auto const t0 = std::chrono::steady_clock::now();
-    VERIFY( !s._M_try_acquire_until(at) );
+    VERIFY( !s.try_acquire_until(at) );
     auto const diff = std::chrono::steady_clock::now() - t0;
     VERIFY( diff >= dur );
   }
@@ -106,14 +115,14 @@ void test03()
 void test04()
 {
   using namespace std::chrono_literals;
-  std::__platform_semaphore s(1);
+  semaphore s(1);
   std::atomic<int> a(0), b(0);
   std::thread t([&] {
     a.wait(0);
     auto const dur = 250ms;
     {
       auto const at = std::chrono::system_clock::now() + dur;
-      VERIFY( !s._M_try_acquire_until(at) );
+      VERIFY( !s.try_acquire_until(at) );
 
       b++;
       b.notify_one();
@@ -122,32 +131,31 @@ void test04()
     a.wait(1);
     {
       auto const at = std::chrono::system_clock::now() + dur;
-      VERIFY( s._M_try_acquire_until(at) );
+      VERIFY( s.try_acquire_until(at) );
     }
     b++;
     b.notify_one();
   });
   t.detach();
 
-  s._M_acquire();
+  s.acquire();
   a++;
   a.notify_one();
   b.wait(0);
-  s._M_release(1);
+  s.release(1);
   a++;
   a.notify_one();
 
   b.wait(1);
 }
-#endif
 
 int main()
 {
-#ifdef _GLIBCXX_HAVE_POSIX_SEMAPHORE
   test01();
   test02();
   test03();
   test04();
-#endif
-  return 0;
 }
+#else
+int main() { }
+#endif

Re: GCC 11.1 Release Candidate available from gcc.gnu.org

[Thomas Rodgers]

On 2021-04-21 05:12, Jonathan Wakely wrote:

> On 21/04/21 12:38 +0100, Jonathan Wakely wrote: On 20/04/21 22:12 
> -0700, Thomas Rodgers wrote: @@ -86,6 +88,24 @@ 
> _GLIBCXX_BEGIN_NAMESPACE_VERSION
> }
> }
> 
> +    _GLIBCXX_ALWAYS_INLINE bool
> +    _M_try_acquire() noexcept
> +    {
> +      for (;;)
> +    {
> +      auto __err = sem_trywait(&_M_semaphore);
> +      if (__err && (errno == EINTR))
> +        continue;
> +      else if (__err && (errno == EAGAIN))
> +        return false;
> +      else if (__err)
> +        std::terminate();
> +      else
> +        break;
> +    }
> +      return true;
> +    }
> +
> _GLIBCXX_ALWAYS_INLINE void
> _M_release(std::ptrdiff_t __update) noexcept
> {
> Please just commit this part to trunk and gcc-11, not the macro
> renaming (as that's been fixed by Jakub already).

I think on trunk I'd prefer to do the attached. WDYT?

Looks good to me.

Re: GCC 11.1 Release Candidate available from gcc.gnu.org

[David Edelsohn]

Hi, Jonathan

Thanks for the further investigation.  I definitely encountered the
missing _M_try_acquire in __platform_semaphore.

Thanks, David

On Wed, Apr 21, 2021 at 8:12 AM Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On 21/04/21 12:38 +0100, Jonathan Wakely wrote:
> >On 20/04/21 22:12 -0700, Thomas Rodgers wrote:
> >>@@ -86,6 +88,24 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >>      }
> >>    }
> >>
> >>+    _GLIBCXX_ALWAYS_INLINE bool
> >>+    _M_try_acquire() noexcept
> >>+    {
> >>+      for (;;)
> >>+     {
> >>+       auto __err = sem_trywait(&_M_semaphore);
> >>+       if (__err && (errno == EINTR))
> >>+         continue;
> >>+       else if (__err && (errno == EAGAIN))
> >>+         return false;
> >>+       else if (__err)
> >>+         std::terminate();
> >>+       else
> >>+         break;
> >>+     }
> >>+      return true;
> >>+    }
> >>+
> >>    _GLIBCXX_ALWAYS_INLINE void
> >>    _M_release(std::ptrdiff_t __update) noexcept
> >>    {
> >
> >Please just commit this part to trunk and gcc-11, not the macro
> >renaming (as that's been fixed by Jakub already).
>
> I think on trunk I'd prefer to do the attached. WDYT?
>
>

Re: GCC 11.1 Release Candidate available from gcc.gnu.org

[Jonathan Wakely]

On 21/04/21 13:12 +0100, Jonathan Wakely wrote:
>On 21/04/21 12:38 +0100, Jonathan Wakely wrote:
>>On 20/04/21 22:12 -0700, Thomas Rodgers wrote:
>>>@@ -86,6 +88,24 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>>	}
>>>   }
>>>
>>>+    _GLIBCXX_ALWAYS_INLINE bool
>>>+    _M_try_acquire() noexcept
>>>+    {
>>>+      for (;;)
>>>+	{
>>>+	  auto __err = sem_trywait(&_M_semaphore);
>>>+	  if (__err && (errno == EINTR))
>>>+	    continue;
>>>+	  else if (__err && (errno == EAGAIN))
>>>+	    return false;
>>>+	  else if (__err)
>>>+	    std::terminate();
>>>+	  else
>>>+	    break;
>>>+	}
>>>+      return true;
>>>+    }
>>>+
>>>   _GLIBCXX_ALWAYS_INLINE void
>>>   _M_release(std::ptrdiff_t __update) noexcept
>>>   {
>>
>>Please just commit this part to trunk and gcc-11, not the macro
>>renaming (as that's been fixed by Jakub already).
>
>I think on trunk I'd prefer to do the attached. WDYT?

In fact I think something like this is neded even on gcc-11 branch,
otherwise anything that tries to include <semaphore> without atomics
or sem_t gets hard errors:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100179

And <thread> includes <stop_token> which includes <semaphore>, meaning
<thread> is unusable on those targets.

So I think removing this #error is essential:
  
>-// Note: the _GLIBCXX_REQUIRE_POSIX_SEMAPHORE macro can be used to force the
>-// use of Posix semaphores (sem_t). Doing so however, alters the ABI.
>-#if defined __cpp_lib_atomic_wait && !_GLIBCXX_REQUIRE_POSIX_SEMAPHORE
>   using __semaphore_impl = __atomic_semaphore;
>-#elif _GLIBCXX_HAVE_POSIX_SEMAPHORE
>-  using __semaphore_impl = __platform_semaphore;
>-#else
>-#  error "No suitable semaphore implementation available"
>-#endif
>+#endif // __cpp_lib_atomic_wait

Re: GCC 11.1 Release Candidate available from gcc.gnu.org

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 2228 bytes --]

On 21/04/21 15:30 +0100, Jonathan Wakely wrote:
>On 21/04/21 13:12 +0100, Jonathan Wakely wrote:
>>On 21/04/21 12:38 +0100, Jonathan Wakely wrote:
>>>On 20/04/21 22:12 -0700, Thomas Rodgers wrote:
>>>>@@ -86,6 +88,24 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>>>	}
>>>>  }
>>>>
>>>>+    _GLIBCXX_ALWAYS_INLINE bool
>>>>+    _M_try_acquire() noexcept
>>>>+    {
>>>>+      for (;;)
>>>>+	{
>>>>+	  auto __err = sem_trywait(&_M_semaphore);
>>>>+	  if (__err && (errno == EINTR))
>>>>+	    continue;
>>>>+	  else if (__err && (errno == EAGAIN))
>>>>+	    return false;
>>>>+	  else if (__err)
>>>>+	    std::terminate();
>>>>+	  else
>>>>+	    break;
>>>>+	}
>>>>+      return true;
>>>>+    }
>>>>+
>>>>  _GLIBCXX_ALWAYS_INLINE void
>>>>  _M_release(std::ptrdiff_t __update) noexcept
>>>>  {
>>>
>>>Please just commit this part to trunk and gcc-11, not the macro
>>>renaming (as that's been fixed by Jakub already).
>>
>>I think on trunk I'd prefer to do the attached. WDYT?
>
>In fact I think something like this is neded even on gcc-11 branch,
>otherwise anything that tries to include <semaphore> without atomics
>or sem_t gets hard errors:
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100179
>
>And <thread> includes <stop_token> which includes <semaphore>, meaning
><thread> is unusable on those targets.
>
>So I think removing this #error is essential:
>>-// Note: the _GLIBCXX_REQUIRE_POSIX_SEMAPHORE macro can be used to force the
>>-// use of Posix semaphores (sem_t). Doing so however, alters the ABI.
>>-#if defined __cpp_lib_atomic_wait && !_GLIBCXX_REQUIRE_POSIX_SEMAPHORE
>>  using __semaphore_impl = __atomic_semaphore;
>>-#elif _GLIBCXX_HAVE_POSIX_SEMAPHORE
>>-  using __semaphore_impl = __platform_semaphore;
>>-#else
>>-#  error "No suitable semaphore implementation available"
>>-#endif
>>+#endif // __cpp_lib_atomic_wait

Here's a simpler patch which just removes the #error and renames the
REQUIRE macro to USE. This still dumps the whole of <semaphore.h> and
<limits.h> in the global namespace when <semaphore> is included, but
we'll have to live with that for the 11.1 release.

I'm just finishing testing this on various targets and will push to
trunk. I'd like to backport it to gcc-11 too, to fix PR100179.


[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 2422 bytes --]

commit 50070d8602a07160cece5890899929e9f210244d
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Apr 22 11:10:06 2021

    libstdc++: Remove #error from <semaphore> implementation [PR 100179]
    
    This removes the #error from <bits/semaphore_base.h> for the case where
    neither __atomic_semaphore nor __platform_semaphore is defined.
    
    Also rename the _GLIBCXX_REQUIRE_POSIX_SEMAPHORE macro to
    _GLIBCXX_USE_POSIX_SEMAPHORE for consistency with the similar
    _GLIBCXX_USE_CXX11_ABI macro that can be used to request an alternative
    (ABI-changing) implementation.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/100179
            * include/bits/semaphore_base.h: Remove #error.
            * include/std/semaphore: Do not define anything unless one of
            the implementations is available.

diff --git a/libstdc++-v3/include/bits/semaphore_base.h b/libstdc++-v3/include/bits/semaphore_base.h
index 84b33423fff..4948f0fd0bc 100644
--- a/libstdc++-v3/include/bits/semaphore_base.h
+++ b/libstdc++-v3/include/bits/semaphore_base.h
@@ -263,14 +263,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   };
 #endif // __cpp_lib_atomic_wait
 
-// Note: the _GLIBCXX_REQUIRE_POSIX_SEMAPHORE macro can be used to force the
+// Note: the _GLIBCXX_USE_POSIX_SEMAPHORE macro can be used to force the
 // use of Posix semaphores (sem_t). Doing so however, alters the ABI.
-#if defined __cpp_lib_atomic_wait && !_GLIBCXX_REQUIRE_POSIX_SEMAPHORE
+#if defined __cpp_lib_atomic_wait && !_GLIBCXX_USE_POSIX_SEMAPHORE
   using __semaphore_impl = __atomic_semaphore;
 #elif _GLIBCXX_HAVE_POSIX_SEMAPHORE
   using __semaphore_impl = __platform_semaphore;
-#else
-#  error "No suitable semaphore implementation available"
 #endif
 
 _GLIBCXX_END_NAMESPACE_VERSION
diff --git a/libstdc++-v3/include/std/semaphore b/libstdc++-v3/include/std/semaphore
index a1560915d83..52addca742c 100644
--- a/libstdc++-v3/include/std/semaphore
+++ b/libstdc++-v3/include/std/semaphore
@@ -34,6 +34,7 @@
 #if __cplusplus > 201703L
 #include <bits/semaphore_base.h>
 
+#if __cpp_lib_atomic_wait || _GLIBCXX_HAVE_POSIX_SEMAPHORE
 namespace std _GLIBCXX_VISIBILITY(default)
 {
 _GLIBCXX_BEGIN_NAMESPACE_VERSION
@@ -89,5 +90,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace
+#endif // cpp_lib_atomic_wait || _GLIBCXX_HAVE_POSIX_SEMAPHORE
 #endif // C++20
 #endif // _GLIBCXX_SEMAPHORE

Re: GCC 11.1 Release Candidate available from gcc.gnu.org

[Jakub Jelinek]

On Thu, Apr 22, 2021 at 01:27:40PM +0100, Jonathan Wakely wrote:
> I'm just finishing testing this on various targets and will push to
> trunk. I'd like to backport it to gcc-11 too, to fix PR100179.

Ok for 11.1.

> commit 50070d8602a07160cece5890899929e9f210244d
> Author: Jonathan Wakely <jwakely@redhat.com>
> Date:   Thu Apr 22 11:10:06 2021
> 
>     libstdc++: Remove #error from <semaphore> implementation [PR 100179]
>     
>     This removes the #error from <bits/semaphore_base.h> for the case where
>     neither __atomic_semaphore nor __platform_semaphore is defined.
>     
>     Also rename the _GLIBCXX_REQUIRE_POSIX_SEMAPHORE macro to
>     _GLIBCXX_USE_POSIX_SEMAPHORE for consistency with the similar
>     _GLIBCXX_USE_CXX11_ABI macro that can be used to request an alternative
>     (ABI-changing) implementation.
>     
>     libstdc++-v3/ChangeLog:
>     
>             PR libstdc++/100179
>             * include/bits/semaphore_base.h: Remove #error.
>             * include/std/semaphore: Do not define anything unless one of
>             the implementations is available.

	Jakub