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 [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 71DDA3857C43 for ; Tue, 12 Sep 2023 09:07:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 71DDA3857C43 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1694509674; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=dIN3ZKIWtqyHGNns8Mjl+R8C7QeHe1On56lnbz9aBqo=; b=h3DsvhIBPkfHXwRCBy8ReRCB5o55Elygz9TkY3Fp2ICfNJn+2J22jB/wELSaeTuqREJbeX 2dOlgdksgC4v8cUY0b8t1Al54PHyz8d1LxAevXZJ7alVpsL14P+rQiauT0qmYKwOgBSLyJ yyyyKfya8BGBpyU9CKPo2+1hDAichBI= Received: from mail-lj1-f197.google.com (mail-lj1-f197.google.com [209.85.208.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-111-B9iAYl3eNoGO5iOljTLrUw-1; Tue, 12 Sep 2023 05:07:52 -0400 X-MC-Unique: B9iAYl3eNoGO5iOljTLrUw-1 Received: by mail-lj1-f197.google.com with SMTP id 38308e7fff4ca-2bd132e84b2so59240271fa.0 for ; Tue, 12 Sep 2023 02:07:52 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694509671; x=1695114471; 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=dIN3ZKIWtqyHGNns8Mjl+R8C7QeHe1On56lnbz9aBqo=; b=OkwJkeCOdrgloAcWELnPCam63v6nALJwzMPR/g5nCMcRmgqUM8USTRR6ulQwzwoNim zeOIjB8ToGjvQegFXP96ZJipbvfwxDt38737T6YBQWfY9b5y0cntWS/n4RmyDWz3E4v/ HlBoFZETH3MqD2ZTD2s3TYh8GVjpYwdXUo129pTx41qs/au7u+Q9r0yOZdtMVVYnSueu E4343aMEUm+t/hMg1ZlR+1eU39g37czB25P1dbmljLECisIkQERgeIk4yJBg4khtbJ5P UbdVBpSLGx6eci32uX0f3zWlVwX487uADuZqDOcfWdsKPyIXpRTjDlB46WFz8VQopY29 cwuQ== X-Gm-Message-State: AOJu0YzfbWZOnV2I9bd6ywgsPx11i0aWVnFplirmGHKBx3DibV8IY8NG 3SCIMvGTTtEmtOVXDHDQKIXHkyRlapMtFqk4B4SFOykDQ9d1XiwosGBYZ5LKQtCVv1F/eu4UvDF LWCabpyH++XuP6M5lN4a33fC0/hbd3JrM5w== X-Received: by 2002:a2e:7801:0:b0:2b6:9ed0:46f4 with SMTP id t1-20020a2e7801000000b002b69ed046f4mr9521327ljc.23.1694509671159; Tue, 12 Sep 2023 02:07:51 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGm8dLkdgXDdSSMV157dwbZWpo49mYJ/K7b4kmwr/BSWi9kiAWrPBjpF4AhLHIbHRf8bqck7BYWDNZ5ju3QvBA= X-Received: by 2002:a2e:7801:0:b0:2b6:9ed0:46f4 with SMTP id t1-20020a2e7801000000b002b69ed046f4mr9521314ljc.23.1694509670714; Tue, 12 Sep 2023 02:07:50 -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: Jonathan Wakely Date: Tue, 12 Sep 2023 10:07:39 +0100 Message-ID: Subject: Re: [PATCH 2/2] libstdc++: Add dg-require-thread-fence in several tests To: Christophe Lyon Cc: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org, kyrylo.tkachov@arm.com, richard.earnshaw@arm.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 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".