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".
