On Tue, 12 Sept 2023 at 11:07, Jonathan Wakely <jwak...@redhat.com> wrote:
> On Tue, 12 Sept 2023 at 08:59, Christophe Lyon > <christophe.l...@linaro.org> wrote: > > > > > > > > On Mon, 11 Sept 2023 at 18:11, Jonathan Wakely <jwak...@redhat.com> > wrote: > >> > >> On Mon, 11 Sept 2023 at 16:40, Christophe Lyon > >> <christophe.l...@linaro.org> wrote: > >> > > >> > > >> > > >> > On Mon, 11 Sept 2023 at 17:22, Jonathan Wakely <jwak...@redhat.com> > wrote: > >> >> > >> >> On Mon, 11 Sept 2023 at 14:57, Christophe Lyon > >> >> <christophe.l...@linaro.org> wrote: > >> >> > > >> >> > > >> >> > > >> >> > On Mon, 11 Sept 2023 at 15:12, Jonathan Wakely <jwak...@redhat.com> > wrote: > >> >> >> > >> >> >> On Mon, 11 Sept 2023 at 13:36, Christophe Lyon > >> >> >> <christophe.l...@linaro.org> wrote: > >> >> >> > > >> >> >> > > >> >> >> > > >> >> >> > On Mon, 11 Sept 2023 at 12:59, Jonathan Wakely < > jwak...@redhat.com> wrote: > >> >> >> >> > >> >> >> >> On Sun, 10 Sept 2023 at 20:31, Christophe Lyon > >> >> >> >> <christophe.l...@linaro.org> 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<count_type> _M_work_count; > +#else > + count_type _M_work_count; > +#endif > mutable execution_context::mutex_type _M_mtx; > queue<function<void()>> _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<memory_resource*>; > #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<T*> 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<memory_resource*>; > #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
