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!
