From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x52c.google.com (mail-ed1-x52c.google.com [IPv6:2a00:1450:4864:20::52c]) by sourceware.org (Postfix) with ESMTPS id A459A3858D39 for ; Wed, 13 Sep 2023 15:37:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A459A3858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-ed1-x52c.google.com with SMTP id 4fb4d7f45d1cf-52f3ba561d9so2433729a12.1 for ; Wed, 13 Sep 2023 08:37:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1694619450; x=1695224250; darn=gcc.gnu.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=LOlsngZG3SKftPTVAO7pDUZAx9pAnIUGEHB9Xs3LjAw=; b=FGrLcZCMEU0paULNsTjGF9VvZZ/S0w6wnPXUS9plKeA0MN2zebz+H1E0C+oQ0AcIGi l7Vf8p1Jq7w26I5gxMHDt3h6kf3nVh0yL0YkbVp3QnwVd1jugTBLsDEWGylEeckZR0cv FYuObNxuQVWuYhWo1983S+5sevJdBG9hU/RyRuaoAtR04Kwa0m9Z9/cXeymW0BhgGmRl nYA5MkyQ73ha4KplP9RslCohAnpXpJ6eypTrRCY/axRe1oW88TMJPLlSG351M/RfHNIE 0mAAIg4DxylDmvfz/1aOOPRuGhYtjbV/r4FcVXdq+LB+1Sr/aVUVoqLE9ikljAiILOsU Ftpg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694619450; x=1695224250; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=LOlsngZG3SKftPTVAO7pDUZAx9pAnIUGEHB9Xs3LjAw=; b=eGLwlxyY/bSGDnMJCPb3102VwBb2iFZgP38U9P+Z1MMvY/EWVNgIAoC3+Ar+JIL/mt 4HQlxmWWDohQpjtpH+Lac9T5bq9ceAcyvlYH+c3HCITFgkBNBdPcPaVU0NFW63NMVz7K kX2hdVOkl0QyPfts1GHHurRjWSaidkV5usivaZHWWLEfMpBMm+8lPMaPAzyrcK38Cdau TDF0iJ39hbPN2f7Wu8lwOhGB5ac9l2+Lo6nmYROKFu2jFd75fP5r9YvWT1GkvkJdXsfC 6GPV3P2w3vnluSj1lgQ5lU+AyYE8kTnWn+HAQVbqDHdVCqjToxt/N8KyoIQhtcFr8d3Z Azig== X-Gm-Message-State: AOJu0Yzydb5vTlxNUOvzWdWckaWGdwlByxx0O7kRvyhsDc8TxUYtlstC FmiN023FKZxV+ynoYVl8YvkvVXhLDY9ciVEfb7mUCQ== X-Google-Smtp-Source: AGHT+IFpgcLsabGYsmvrJ1eGokw+N4iLVivW+JacREGTLd+vRmIOkNydWTKN9x/+xzee+XJKBpPjHjE1cpTFU/ldBpk= X-Received: by 2002:aa7:d992:0:b0:523:37f0:2d12 with SMTP id u18-20020aa7d992000000b0052337f02d12mr4486511eds.17.1694619450085; Wed, 13 Sep 2023 08:37:30 -0700 (PDT) MIME-Version: 1.0 References: <20230910193045.3549775-1-christophe.lyon@linaro.org> <20230910193045.3549775-2-christophe.lyon@linaro.org> In-Reply-To: From: Christophe Lyon Date: Wed, 13 Sep 2023 17:37:21 +0200 Message-ID: Subject: Re: [PATCH 2/2] libstdc++: Add dg-require-thread-fence in several tests To: Jonathan Wakely Cc: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org, kyrylo.tkachov@arm.com, richard.earnshaw@arm.com Content-Type: multipart/alternative; boundary="0000000000005a334d06053f5671" X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,HTML_MESSAGE,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --0000000000005a334d06053f5671 Content-Type: text/plain; charset="UTF-8" On Tue, 12 Sept 2023 at 11:07, Jonathan Wakely wrote: > On Tue, 12 Sept 2023 at 08:59, Christophe Lyon > wrote: > > > > > > > > On Mon, 11 Sept 2023 at 18:11, Jonathan Wakely > wrote: > >> > >> On Mon, 11 Sept 2023 at 16:40, Christophe Lyon > >> wrote: > >> > > >> > > >> > > >> > On Mon, 11 Sept 2023 at 17:22, Jonathan Wakely > wrote: > >> >> > >> >> On Mon, 11 Sept 2023 at 14:57, Christophe Lyon > >> >> wrote: > >> >> > > >> >> > > >> >> > > >> >> > On Mon, 11 Sept 2023 at 15:12, Jonathan Wakely > wrote: > >> >> >> > >> >> >> On Mon, 11 Sept 2023 at 13:36, Christophe Lyon > >> >> >> wrote: > >> >> >> > > >> >> >> > > >> >> >> > > >> >> >> > On Mon, 11 Sept 2023 at 12:59, Jonathan Wakely < > jwakely@redhat.com> wrote: > >> >> >> >> > >> >> >> >> On Sun, 10 Sept 2023 at 20:31, Christophe Lyon > >> >> >> >> wrote: > >> >> >> >> > > >> >> >> >> > Some targets like arm-eabi with newlib and default settings > rely on > >> >> >> >> > __sync_synchronize() to ensure synchronization. Newlib does > not > >> >> >> >> > implement it by default, to make users aware they have to > take special > >> >> >> >> > care. > >> >> >> >> > > >> >> >> >> > This makes a few tests fail to link. > >> >> >> >> > >> >> >> >> Does this mean those features are unusable on the target, or > just that > >> >> >> >> users need to provide their own __sync_synchronize to use them? > >> >> >> > > >> >> >> > > >> >> >> > IIUC the user is expected to provide them. > >> >> >> > Looks like we discussed this in the past :-) > >> >> >> > In > https://gcc.gnu.org/legacy-ml/gcc-patches/2016-10/msg01632.html, > >> >> >> > see the pointer to Ramana's comment: > https://gcc.gnu.org/ml/gcc-patches/2015-05/msg02751.html > >> >> >> > >> >> >> Oh yes, thanks for the reminder! > >> >> >> > >> >> >> > > >> >> >> > The default arch for arm-eabi is armv4t which is very old. > >> >> >> > When running the testsuite with something more recent (either > as default by configuring GCC --with-arch=XXX or by forcing -march/-mcpu > via dejagnu's target-board), the compiler generates barrier instructions > and there are no such errors. > >> >> >> > >> >> >> Ah yes, that's fine then. > >> >> >> > >> >> >> > For instance, here is a log with the defaults: > >> >> >> > > https://git.linaro.org/toolchain/ci/base-artifacts/tcwg_gnu_embed_check_gcc/master-arm_eabi.git/tree/00-sumfiles?h=linaro-local/ci/tcwg_gnu_embed_check_gcc/master-arm_eabi > >> >> >> > and a log when we target cortex-m0 which is still a very small > cpu but has barriers: > >> >> >> > > https://git.linaro.org/toolchain/ci/base-artifacts/tcwg_gnu_embed_check_gcc/master-thumb_m0_eabi.git/tree/00-sumfiles?h=linaro-local/ci/tcwg_gnu_embed_check_gcc/master-thumb_m0_eabi > >> >> >> > > >> >> >> > I somehow wanted to get rid of such errors with the default > configuration.... > >> >> >> > >> >> >> Yep, that makes sense, and we'll still be testing them for newer > >> >> >> arches on the target, so it's not completely disabling those > parts of > >> >> >> the testsuite. > >> >> >> > >> >> >> But I'm still curious why some of those tests need this change. I > >> >> >> think the ones I noted below are probably failing for some other > >> >> >> reasons. > >> >> >> > >> >> > Just looked at 23_containers/span/back_assert_neg.cc, the linker > says it needs > >> >> > arm-eabi/libstdc++-v3/src/.libs/libstdc++.a(debug.o) to resolve > >> >> > ./back_assert_neg-back_assert_neg.o > (std::__glibcxx_assert_fail(char const*, int, char const*, char const*)) > >> >> > and indeed debug.o has a reference to __sync_synchronize > >> >> > >> >> Aha, that's just because I put __glibcxx_assert_fail in debug.o, but > >> >> there are no dependencies on anything else in that file, including > the > >> >> _M_detach member function that uses atomics. > >> > > >> > indeed > >> > > >> > > >> >> > >> >> This would also be solved by -Wl,--gc-sections :-) > >> > > >> > :-) > >> > > >> >> > >> >> I think it would be better to move __glibcxx_assert_fail to a new > >> >> file, so that it doesn't make every assertion unnecessarily depend on > >> >> __sync_synchronize. I'll do that now. > >> > > >> > Sounds like a good idea, thanks. > >> > >> Done now at r14-3846-g4a2766ed00a479 > >> > > >> >> > >> >> We could also make the atomics in debug.o conditional, so that debug > >> >> mode doesn't depend on __sync_synchronize for single-threaded > targets. > >> >> Does the arm4t arch have pthreads support in newlib? I didn't bother > >> > > >> > No ( grep _GLIBCXX_HAS_GTHREADS > $objdir/arm-eabi/libstdc++-v3/include/arm-eabi/bits/c++config returns: > >> > /* #undef _GLIBCXX_HAS_GTHREADS */ > >> > > >> >> making the use of atomics conditional, because performance is not > >> >> really a priority for debug mode bookkeeping. But the problem here > >> >> isn't just a slight performance overhead of atomics, it's that they > >> >> aren't even supported for arm4t. > >> > > >> > OK thanks. > >> > > >> > So finally, this uncovered at least a "bug" that > __glibcxx_assert_fail should be in a dedicated object file :-) > >> > > >> > I'll revisit my patch once you have moved __glibcxx_assert_fail > >> > >> That's done (at r14-3845-gc7db9000fa7cac) and there should be no more > >> __sync_synchronize from src/c++11/debug.o at all now (at > >> r14-3846-g4a2766ed00a479). With that second change, it would have been > >> OK for __glibcxx_assert_fail to stay in that file, but it's not really > >> related so it's probably better for it to be in a separate file > >> anyway. > >> > >> That should remove the need for most of your patch! > >> > > > > Hi! > > > > I've looked at the remaining undefined references to __sync_synchronize > after your commits: > > 29_atomics/atomic/compare_exchange_padding.cc (from a.load()) > > 29_atomics/atomic/cons/value_init.cc (from a.load()) > > 29_atomics/atomic_float/value_init.cc (from a.load()) > > 29_atomics/atomic_float/1.cc no problem (is_always_lock_free is false?) > > 29_atomics/atomic_integral/cons/value_init.cc (from a.load()) > > 29_atomics/atomic_ref/compare_exchange_padding.cc (from a.store()) > > 29_atomics/atomic_ref/generic.cc > > 29_atomics/atomic_ref/integral.cc > > 29_atomics/atomic_ref/pointer.cc > > These all make sense. > > > experimental/net/timer/waitable/dest.cc (from > _ZNSt12experimental3net2v110io_context9_M_do_oneENSt6chrono8durationIxSt5ratioILx1ELx1000EEEE) > > experimental/net/timer/waitable/ops.cc not sure why? > > I think we can make those uses of atomics conditional like this > > --- a/libstdc++-v3/include/experimental/io_context > +++ b/libstdc++-v3/include/experimental/io_context > @@ -562,7 +562,11 @@ inline namespace v1 > } > }; > > +#ifdef _GLIBCXX_HAS_GTHREADS > atomic _M_work_count; > +#else > + count_type _M_work_count; > +#endif > mutable execution_context::mutex_type _M_mtx; > queue> _M_op; > bool _M_stopped = false; > > > > > > experimental/polymorphic_allocator/construct_pair.cc (from load, line > 835 of atomic_base.h) > > Curious. This comes from lines 168 and 173 in src/c++17/memory_resource.cc > The logic there is: > > #if ATOMIC_POINTER_LOCK_FREE == 2 > using atomic_mem_res = atomic; > #elif defined(_GLIBCXX_HAS_GTHREADS) > // Emulate the interface of std::atomic but using a mutex. > struct atomic_mem_res { > memory_resource* load(memory_order); > memory_resource* exchange(memory_resource*, memory_order); > }; > #else > // Emulate the interface of std::atomic with no atomicity or > synchronization. > struct atomic_mem_res { > memory_resource* load(memory_order); > memory_resource* exchange(memory_resource*, memory_order); > }; > #endif > > So we use an atomic if that's always lock free, even for > single-threaded. It didn't occur to me that a target would have > lock-free pointer-size atomics, but trying to use them would give a > linker error. > > Maybe it should be: > > #ifndef _GLIBCXX_HAS_GTHREADS > // single-threaded struct atomic_mem_res > #elif ATOMIC_POINTER_LOCK_FREE == 2 > using atomic_mem_res = atomic; > #else > // mutex-based struct atomic_mem_res > #endif > > > I've noticed several undefined references to > __glibcxx_backtrace_create_state too > > 19_diagnostics/stacktrace/current.cc > > 19_diagnostics/stacktrace/entry.cc > > 19_diagnostics/stacktrace/stacktrace.cc > > Odd. These were changed in r14-3812-gb96b554592c5cb to link to > libstdc++exp.a instead of libstdc++_libbacktrace.a, and > __glibcxx_backtrace_create_state should be part of libstdc++exp.a now. > If the target doesn't support libbacktrace then the symbols will be > missing from libstdc++exp.a, but then the test should fail to match > the effective target "stacktrace". > > Strange, it looks like these libs were not correctly rebuilt after I rebased to have your patches. I've rebuilt from scratch and these undefined references are not present indeed. Thanks, Christophe --0000000000005a334d06053f5671--