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 A4610385800E for ; Fri, 27 Nov 2020 12:26:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A4610385800E 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-526-hHjYXCaNOASDfYEzzXG__w-1; Fri, 27 Nov 2020 07:26:31 -0500 X-MC-Unique: hHjYXCaNOASDfYEzzXG__w-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 A73B01E7D3; Fri, 27 Nov 2020 12:26:30 +0000 (UTC) Received: from localhost (unknown [10.33.37.15]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4563D6085D; Fri, 27 Nov 2020 12:26:30 +0000 (UTC) Date: Fri, 27 Nov 2020 12:26:29 +0000 From: Jonathan Wakely To: Stephan Bergmann Cc: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: Re: [committed] libstdc++: Use __libc_single_threaded for locale initialization Message-ID: <20201127122629.GS1312820@redhat.com> References: <20201124145952.GA1857606@redhat.com> <20201127111510.GR1312820@redhat.com> MIME-Version: 1.0 In-Reply-To: <20201127111510.GR1312820@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: multipart/mixed; boundary="z4IKABJTiQIqPwmW" Content-Disposition: inline X-Spam-Status: No, score=-14.0 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_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: 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: Fri, 27 Nov 2020 12:26:45 -0000 --z4IKABJTiQIqPwmW Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline On 27/11/20 11:15 +0000, Jonathan Wakely wrote: >On 27/11/20 09:43 +0100, Stephan Bergmann via Libstdc++ wrote: >>On 24/11/2020 15:59, Jonathan Wakely via Gcc-patches wrote: >>>Most initialization of locales and facets happens before main() during >>>startup, when the program is likely to only have one thread. By using >>>the new __gnu_cxx::__is_single_threaded() function instead of checking >>>__gthread_active_p() we can avoid using pthread_once or atomics for the >>>common case. >>> >>>That said, I'm not sure why we don't just use a local static variable >>>instead, as __cxa_guard_acquire() already optimizes for the >>>single-threaded case: >>> >>> static const bool init = (_S_initialize_once(), true); >>> >>>I'll revisit that for GCC 12. >>> >>>libstdc++-v3/ChangeLog: >>> >>> * src/c++98/locale.cc (locale::facet::_S_get_c_locale()) >>> (locale::id::_M_id() const): Use __is_single_threaded. >>> * src/c++98/locale_init.cc (locale::_S_initialize()): >>> Likewise. >>> >>>Tested powerpc64le-linux. Committed to trunk. >> >>I now started to get weird crashes when running LibreOffice test >>code at least with Clang -fsanitize=address and latest libstdc++, >>where the Clang ASan machinery SEGVs while it wants to report some >>malloc/free issue. That goes away when reverting this commit, and I >>think the root cause is that locale::facet::_S_initialize_once() now >>gets called twice: First during __cxx_global_var_init when the >>process is still single threaded (so >> >> if (!__gnu_cxx::__is_single_threaded()) >> >>in locale::facet::_S_get_c_locale, reading __libc_single_threaded, >>is false, whereas >> >> if (__gthread_active_p()) >> >>would have been true even if the process still only had a single >>thread). And again after the process has spawned further threads >>via pthread_create (flipping __libc_single_threaded) and calls >>std::ostringstream() -> ... std::locale() -> ..., at which point >> >> if (!__gnu_cxx::__is_single_threaded()) >> >>in locale::facet::_S_get_c_locale is true now. > >Gah, yes, that's broken. Sorry. I'm reverting that part and testing >it now. > >I still think we should just use local static variables here and let >the runtime do the initialization and optimize it for single-threaded, >but I'll leave that for GCC 12. There might be some reason that wasn't >done originally, maybe so the library could be built with >-fno-threadsafe-statics (which I don't think we should support) or >maybe just because it predates the threadsafe-static code working >well. Fixed with this patch, tested powerp64le-linux (glibc 2.32). Pushed to trunk. Thanks for the testing and bug report, as always. --z4IKABJTiQIqPwmW Content-Type: text/x-patch; charset=us-ascii Content-Disposition: attachment; filename="patch.txt" commit 0d7d69ca4a8c05d883e07ee42058c9c6b0c72370 Author: Jonathan Wakely Date: Fri Nov 27 11:00:15 2020 libstdc++: Partially revert r11-5314 The changes in r11-5314 are broken, because it means we don't use __gthread_once for the first few initializations, but after the program becomes multi-threaded we will repeat the initialization, using __gthread_once once this time. This leads to memory errors. The use of __is_single_threaded() in locale::id::_M_id() is OK, because the side effects are the same either way. libstdc++-v3/ChangeLog: * src/c++98/locale.cc (locale::facet::_S_get_c_locale()): Revert change to use __is_single_threaded. * src/c++98/locale_init.cc (locale::_S_initialize()): Likewise. diff --git a/libstdc++-v3/src/c++98/locale.cc b/libstdc++-v3/src/c++98/locale.cc index 9b3fc3515152..4c1612cc5dca 100644 --- a/libstdc++-v3/src/c++98/locale.cc +++ b/libstdc++-v3/src/c++98/locale.cc @@ -214,7 +214,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION locale::facet::_S_get_c_locale() { #ifdef __GTHREADS - if (!__gnu_cxx::__is_single_threaded()) + if (__gthread_active_p()) __gthread_once(&_S_once, _S_initialize_once); else #endif diff --git a/libstdc++-v3/src/c++98/locale_init.cc b/libstdc++-v3/src/c++98/locale_init.cc index fc8416ba01a6..c3841ccbd3c9 100644 --- a/libstdc++-v3/src/c++98/locale_init.cc +++ b/libstdc++-v3/src/c++98/locale_init.cc @@ -320,7 +320,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION locale::_S_initialize() { #ifdef __GTHREADS - if (!__gnu_cxx::__is_single_threaded()) + if (__gthread_active_p()) __gthread_once(&_S_once, _S_initialize_once); #endif if (!_S_classic) --z4IKABJTiQIqPwmW--