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 experimental/net/timer/waitable/dest.cc (from _ZNSt12experimental3net2v110io_context9_M_do_oneENSt6chrono8durationIxSt5ratioILx1ELx1000EEEE) experimental/net/timer/waitable/ops.cc not sure why? experimental/polymorphic_allocator/construct_pair.cc (from load, line 835 of atomic_base.h) 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 [...] Thanks, Christophe
