From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.61]) by sourceware.org (Postfix) with ESMTP id 2272538618C3 for ; Mon, 3 Aug 2020 20:19:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 2272538618C3 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-430-5DVQYpIqOyyAI__W4mCafA-1; Mon, 03 Aug 2020 16:19:26 -0400 X-MC-Unique: 5DVQYpIqOyyAI__W4mCafA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 34D0D107BEF6; Mon, 3 Aug 2020 20:19:25 +0000 (UTC) Received: from localhost (unknown [10.33.36.241]) by smtp.corp.redhat.com (Postfix) with ESMTP id D47CE8A19E; Mon, 3 Aug 2020 20:19:24 +0000 (UTC) Date: Mon, 3 Aug 2020 21:19:23 +0100 From: Jonathan Wakely To: Thomas Rodgers Cc: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org, trodgers@redhat.com Subject: Re: [PATCH] Add C++2a synchronization support Message-ID: <20200803201923.GH3400@redhat.com> References: <6F58268B-FA3E-48EC-8108-E16E4A8324ED@appliantology.com> <20200606002956.1512343-1-rodgert@appliantology.com> <20200803140957.GG3400@redhat.com> MIME-Version: 1.0 In-Reply-To: <20200803140957.GG3400@redhat.com> X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline X-Spam-Status: No, score=-14.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 03 Aug 2020 20:19:31 -0000 On 03/08/20 15:09 +0100, Jonathan Wakely wrote: >On 05/06/20 17:29 -0700, Thomas Rodgers wrote: >>diff --git a/libstdc++-v3/include/bits/semaphore_base.h b/libstdc++-v3/include/bits/semaphore_base.h >>new file mode 100644 >>index 00000000000..f0c4235d91c >>--- /dev/null >>+++ b/libstdc++-v3/include/bits/semaphore_base.h >>@@ -0,0 +1,272 @@ >>+// -*- 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 >>+// . >>+ >>+/** @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 >>+#include >>+#include >>+ >>+#if defined _POSIX_SEMAPHORES && __has_include() >>+#define _GLIBCXX_HAVE_POSIX_SEMAPHORE 1 >>+#include >>+#endif >>+ >>+#include >>+#include >>+ >>+namespace std _GLIBCXX_VISIBILITY(default) >>+{ >>+_GLIBCXX_BEGIN_NAMESPACE_VERSION >>+ >>+#ifdef _GLIBCXX_HAVE_POSIX_SEMAPHORE >>+ template >>+ struct __platform_semaphore Why is this a template? It means we'll instantiate identical code for counting_semaphore<3> and counting_semaphore<4>, but they're distinct types that will bloat the binary. Can't we just have a single __platform_semaphore type for all max values, and just make sure we don't instantiate it for values larger than SEM_VALUE_MAX? >>+ { > >I think we want to delete the copy constructor and copy assignment >operator for this type and __atomic_semaphore. > >>+ using __clock_t = chrono::system_clock; >>+ >>+ explicit __platform_semaphore(ptrdiff_t __count) noexcept >>+ { >>+ static_assert( __least_max_value <= SEM_VALUE_MAX, ""); I think it would be useful for __platform_semaphore to define a static constexpr member like: static constexpr ptrdiff_t _S_max = SEM_VALUE_MAX; And then __atomic_semaphore could define: static constexpr _Tp _S_max = __gnu_cxx::__int_traits<_Tp>::__max; And then the alias __semaphore_base could be defined as: #if _GLIBCXX_HAVE_LINUX_FUTEX && ! _GLIBCXX_REQUIRE_POSIX_SEMAPHORE // Use futex if available and user didn't force use of POSIX: using __fast_semaphore_base = __atomic_semaphore<__platform_wait_t>; #elif _GLIBCXX_HAVE_POSIX_SEMAPHORE // Otherwise, use POSIX if available: using __fast_semaphore_base = __platform_semaphore; #else // Otherwise, use atomics: using __fast_semaphore_base = __atomic_semaphore; #endif template using __semaphore_base = conditional_t< __least_max_value <= __fast_semaphore_base::_S_max, __fast_semaphore_base, __atomic_semaphore; Would that make sense? Do the comments above reflect the intent? >>+#ifdef _GLIBCXX_REQUIRE_POSIX_SEMAPHORE > >What is this case for? This macro seems to only exist for use by a >single testcase. > >Is it supposed to be something users can set? If so, it needs to be >documented as ABI-breaking and as implying that counting_semaphore >cannot be use with counts higher than SEM_VALUE_MAX. > >>+ template >>+ using __semaphore_base = __platform_semaphore<__least_max_value>; >>+#else >>+# ifdef _GLIBCXX_HAVE_LINUX_FUTEX >>+ template >>+ using __semaphore_base = conditional_t<( >>+ __least_max_value >= 0 > >This condition is redundant, we already know that counting_semaphore >has checked the value is non-negative. > >>+ && __least_max_value <= __detail::__platform_wait_max_value), >>+ __atomic_semaphore<__detail::__platform_wait_t>, >>+ __atomic_semaphore>; >>+ >>+// __platform_semaphore >>+# elif defined _GLIBCXX_HAVE_POSIX_SEMAPHORE >>+ template >>+ using __semaphore_base = conditional_t<( >>+ __least_max_value >= 0 > >Redundant condition again. > >>+ && __least_max_value <= SEM_VALUE_MAX), >>+ __platform_semaphore<__least_max_value>, >>+ __atomic_semaphore>; >>+# else >>+ template >>+ using __semaphore_base = __atomic_semaphore; >>+# endif >>+#endif >>+ >>+_GLIBCXX_END_NAMESPACE_VERSION >>+} // namespace std >>+ >>+#endif