From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by sourceware.org (Postfix) with ESMTP id 4B61A3861936 for ; Tue, 3 Nov 2020 20:13:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 4B61A3861936 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-538-suHuvoxbMrW3z-7Me95_bg-1; Tue, 03 Nov 2020 15:13:42 -0500 X-MC-Unique: suHuvoxbMrW3z-7Me95_bg-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 7E1878014D8; Tue, 3 Nov 2020 20:13:41 +0000 (UTC) Received: from localhost (unknown [10.33.36.7]) by smtp.corp.redhat.com (Postfix) with ESMTP id E8AA75C1D0; Tue, 3 Nov 2020 20:13:40 +0000 (UTC) Date: Tue, 3 Nov 2020 20:13:39 +0000 From: Jonathan Wakely To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: Re: [committed] libstdc++: Rewrite std::call_once to use futexes [PR 66146] Message-ID: <20201103201339.GX503596@redhat.com> References: <20201103184534.GA3112738@redhat.com> MIME-Version: 1.0 In-Reply-To: <20201103184534.GA3112738@redhat.com> X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/mixed; boundary="PWfwoUCx3AFJRUBq" Content-Disposition: inline X-Spam-Status: No, score=-14.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H5, 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: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Nov 2020 20:13:47 -0000 --PWfwoUCx3AFJRUBq Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline On 03/11/20 18:45 +0000, Jonathan Wakely wrote: >The current implementation of std::call_once uses pthread_once, which >only meets the C++ requirements when compiled with support for >exceptions. For most glibc targets and all non-glibc targets, >pthread_once does not work correctly if the init_routine exits via an >exception. The pthread_once_t object is left in the "active" state, and >any later attempts to run another init_routine will block forever. > >This change makes std::call_once work correctly for Linux targets, by >replacing the use of pthread_once with a futex, based on the code from >__cxa_guard_acquire. For both glibc and musl, the Linux implementation >of pthread_once is already based on futexes, and pthread_once_t is just >a typedef for int, so this change does not alter the layout of >std::once_flag. By choosing the values for the int appropriately, the >new code is even ABI compatible. Code that calls the old implementation >of std::call_once will use pthread_once to manipulate the int, while new >code will use the new std::once_flag members to manipulate it, but they >should interoperate correctly. In both cases, the int is initially zero, >has the lowest bit set when there is an active execution, and equals 2 >after a successful returning execution. The difference with the new code >is that exceptional exceptions are correctly detected and the int is >reset to zero. > >The __cxa_guard_acquire code (and musl's pthread_once) use an additional >state to say there are other threads waiting. This allows the futex wake >syscall to be skipped if there is no contention. Glibc doesn't use a >waiter bit, so we have to unconditionally issue the wake in order to be >compatible with code calling the old std::call_once that uses Glibc's >pthread_once. If we know that we're using musl (and musl's pthread_once >doesn't change) it would be possible to set a waiting state and check >for it in std::once_flag::_M_finish(bool), but this patch doesn't do >that. > >This doesn't fix the bug for non-linux targets. A similar approach could >be used for targets where we know the definition of pthread_once_t is a >mutex and an integer. We could make once_flag._M_activate() use >pthread_mutex_lock on the mutex member within the pthread_once_t, and >then only set the integer if the execution finishes, and then unlock the >mutex. That would require careful study of each target's pthread_once >implementation and that work is left for a later date. Something like the attached (completely untested) patch might work so we could stop using pthread_once for the BSDs and Solaris (and so fix the bug with exceptional executions). It's a kluge though (and fragile for Solaris because it assumes the pthread_once_t type will always have the same layout - in the public header it's just got lots of 64-bit integers for padding). I should probably test this some time. In theory something similar could be done for AIX, but its pthread_once_t is another struct full of padding integers, and unlike Solaris we can't look at the code to see how the OS uses those bits. I'm not sure if this can work for Darwin, as its pthread_once_t isn't just an aggregate of a pthread_mutex_t and a flag, I think it contains state used by GCD. That means we can't perform equivalent operations directly just in terms of Pthread APIs. --PWfwoUCx3AFJRUBq Content-Type: text/x-patch; charset=us-ascii Content-Disposition: attachment; filename="patch.txt" diff --git a/libstdc++-v3/config/os/bsd/dragonfly/os_defines.h b/libstdc++-v3/config/os/bsd/dragonfly/os_defines.h index c42c6e661630..0c63a0ee1fdd 100644 --- a/libstdc++-v3/config/os/bsd/dragonfly/os_defines.h +++ b/libstdc++-v3/config/os/bsd/dragonfly/os_defines.h @@ -38,4 +38,8 @@ #define _GLIBCXX_USE_C99_LONG_LONG_CHECK 1 #define _GLIBCXX_USE_C99_LONG_LONG_DYNAMIC (_GLIBCXX_USE_C99_DYNAMIC || !defined __LONG_LONG_SUPPORTED) +// Make std::call_once access pthread_once_t members directly +#define _GLIBCXX_COMPAT_PTHREAD_ONCE_T \ + { int _M_done; pthread_mutex_t _M_mutex; } + #endif diff --git a/libstdc++-v3/config/os/bsd/freebsd/os_defines.h b/libstdc++-v3/config/os/bsd/freebsd/os_defines.h index d896de4254f4..fbae94c0370d 100644 --- a/libstdc++-v3/config/os/bsd/freebsd/os_defines.h +++ b/libstdc++-v3/config/os/bsd/freebsd/os_defines.h @@ -40,4 +40,8 @@ #define _GLIBCXX_USE_C99_FLOAT_TRANSCENDENTALS_CHECK 1 #define _GLIBCXX_USE_C99_FLOAT_TRANSCENDENTALS_DYNAMIC defined _XOPEN_SOURCE +// Make std::call_once access pthread_once_t members directly +#define _GLIBCXX_COMPAT_PTHREAD_ONCE_T \ + { int _M_done; pthread_mutex_t _M_mutex; } + #endif diff --git a/libstdc++-v3/config/os/bsd/netbsd/os_defines.h b/libstdc++-v3/config/os/bsd/netbsd/os_defines.h index d15727e76e0a..0ab2e6528228 100644 --- a/libstdc++-v3/config/os/bsd/netbsd/os_defines.h +++ b/libstdc++-v3/config/os/bsd/netbsd/os_defines.h @@ -30,4 +30,8 @@ #define __ssize_t ssize_t +// Make std::call_once access pthread_once_t members directly +#define _GLIBCXX_COMPAT_PTHREAD_ONCE_T \ + { pthread_mutex_t _M_mutex; int _M_done; } + #endif diff --git a/libstdc++-v3/config/os/bsd/openbsd/os_defines.h b/libstdc++-v3/config/os/bsd/openbsd/os_defines.h index 1761a10e3836..d248bd2cab29 100644 --- a/libstdc++-v3/config/os/bsd/openbsd/os_defines.h +++ b/libstdc++-v3/config/os/bsd/openbsd/os_defines.h @@ -38,4 +38,8 @@ #define _GLIBCXX_USE_C99_FLOAT_TRANSCENDENTALS_DYNAMIC _GLIBCXX_USE_C99_DYNAMIC #define _GLIBCXX_USE_C99_FP_MACROS_DYNAMIC _GLIBCXX_USE_C99_DYNAMIC +// Make std::call_once access pthread_once_t members directly +#define _GLIBCXX_COMPAT_PTHREAD_ONCE_T \ + { int _M_done; pthread_mutex_t _M_mutex; } + #endif diff --git a/libstdc++-v3/config/os/solaris/os_defines.h b/libstdc++-v3/config/os/solaris/os_defines.h index 36e8cb786cbc..58b7a9f309d4 100644 --- a/libstdc++-v3/config/os/solaris/os_defines.h +++ b/libstdc++-v3/config/os/solaris/os_defines.h @@ -35,5 +35,9 @@ #define __CORRECT_ISO_CPP_WCHAR_H_PROTO #endif +// Make std::call_once access pthread_once_t members directly +#define _GLIBCXX_COMPAT_PTHREAD_ONCE_T \ + { pthread_mutex_t _M_mutex; unsigned _M_unused; unsigned _M_done; } + #endif diff --git a/libstdc++-v3/include/std/mutex b/libstdc++-v3/include/std/mutex index 8c7664bff550..6d87834b3bfd 100644 --- a/libstdc++-v3/include/std/mutex +++ b/libstdc++-v3/include/std/mutex @@ -684,6 +684,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION enum _Bits : int { _Init = 0, _Active = 1, _Done = 2 }; int _M_once = _Bits::_Init; +#elif defined _GLIBCXX_COMPAT_PTHREAD_ONCE_T + struct _Once _GLIBCXX_COMPAT_PTHREAD_ONCE_T; + static_assert(sizeof(_Once) == sizeof(__gthread_once_t), "config error"); + static_assert(alignof(_Once) == alignof(__gthread_once_t), "config error"); + union + { + __gthread_once_t _M_native = __GTHREAD_ONCE_INIT; + _Once _M_once; + }; +#else + __gthread_once_t _M_once = __GTHREAD_ONCE_INIT; +#endif // ! GTHREADS // Non-blocking query whether this is known to be a passive execution. bool @@ -706,11 +718,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION once_flag& _M_flag; bool _M_returning = false; }; -#else - __gthread_once_t _M_once = __GTHREAD_ONCE_INIT; struct _Prepare_execution; -#endif // ! GTHREADS template friend void @@ -753,7 +762,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION once_flag::_M_exceptional() noexcept { _M_once = _Bits::_Init; } -#else // ! FUTEX && GTHREADS +#elif defined _GLIBCXX_COMPAT_PTHREAD_ONCE_T + + inline bool + once_flag::_M_passive() const noexcept + { return false; } + +#else /// @cond undocumented # ifdef _GLIBCXX_HAVE_TLS @@ -814,7 +829,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION void call_once(once_flag& __once, _Callable&& __f, _Args&&... __args) { -#if defined _GLIBCXX_HAVE_LINUX_FUTEX || ! defined _GLIBCXX_HAS_GTHREADS +#if defined _GLIBCXX_HAVE_LINUX_FUTEX || ! defined _GLIBCXX_HAS_GTHREADS \ + || _GLIBCXX_COMPAT_PTHREAD_ONCE_T if (__once._M_passive()) return; else if (__once._M_activate()) diff --git a/libstdc++-v3/src/c++11/mutex.cc b/libstdc++-v3/src/c++11/mutex.cc index 286f77f9a454..e63b709749a6 100644 --- a/libstdc++-v3/src/c++11/mutex.cc +++ b/libstdc++-v3/src/c++11/mutex.cc @@ -82,6 +82,26 @@ std::once_flag::_M_finish(bool returning) noexcept } } +#elif defined _GLIBCXX_COMPAT_PTHREAD_ONCE_T + +bool +std::once_flag::_M_activate() +{ + pthread_mutex_lock(&_M_once._M_mutex); + if (_M_once._M_done == 0) + return true; + pthread_mutex_unlock(&_M_once._M_mutex); + return false; +} + +void +std::once_flag::_M_finish(bool returning) noexcept +{ + if (returning) + _M_once._M_done = 1; + pthread_mutex_unlock(&_M_once._M_mutex); +} + #endif // ! FUTEX #ifndef _GLIBCXX_HAVE_TLS --PWfwoUCx3AFJRUBq--