On Tue, 17 Nov 2020 at 16:41, Richard Earnshaw (lists) via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On 17/11/2020 15:18, Bernd Edlinger wrote:
> > On 11/17/20 1:44 PM, Richard Earnshaw (lists) wrote:
> >> On 03/11/2020 15:08, Bernd Edlinger wrote:
> >>> Hi,
> >>>
> >>> this fixes a problem with a missing symbol __sync_synchronize
> >>> which happens when newlib is used together with libstdc++ for
> >>> the non-threaded simulator target arm-none-eabi.
> >>>
> >>> There are several questions on stackoverflow about this issue.
> >>>
> >>> I would like to add a weak symbol for this target, since this
> >>> is only a default implementation and not meant to override a
> >>> possibly more sophisticated synchronization function from the
> >>> c-runtime.
> >>>
> >>>
> >>> Regression tested successfully on arm-none-eabi with newlib-3.3.0.
> >>>
> >>> Is it OK for trunk?
> >>>
> >>>
> >>> Thanks
> >>> Bernd.
> >>>
> >>
> >> I seem to recall that this was a deliberate decision - you can't guess
> >> this correctly, at least when trying to build portable code - you just
> >> have to know which runtime you will be using.
> >>
> >
> > Therefore I suggest to use the weak attribute. It is on purpose not
> > implementing all of the atomics.
> >
> > The use case, is a C++ program which initializes a local static variable.
> >
> > $ cat test.cc
> > #include <string>
> > main(int argc, char **argv)
> > {
> > static std::string x = "test";
> > return 0;
> > }
> >
> > compiles to this:
> > sub sp, sp, #20
> > str r0, [fp, #-24]
> > str r1, [fp, #-28]
> > ldr r3, .L14
> > ldrb r4, [r3]
> > bl __sync_synchronize
> > and r3, r4, #255
> > and r3, r3, #1
> > cmp r3, #0
> > moveq r3, #1
> > movne r3, #0
> > and r3, r3, #255
> > cmp r3, #0
> > beq .L8
> > ldr r0, .L14
> > bl __cxa_guard_acquire
> > mov r3, r0
> >
> > so __sync_synchronize is not defined in newlib since the target (arm-sim)
> > is known to be not multi-threaded,
> > but __cxa_guard_acquire is also not a thread safe function,
> > because __GTHREADS is not defined by libgcc, since it is known
> > at configure time, that the target does not support threads.
> > So libstdc++ does not try to use a mutex or any atomics either,
> > because it is not compiled with __GTHREADS.
> >
> > I can further narrow down the patch by only defining this function when
> > __GTHREADS is not defined, to make it more clear.
> >
> >
> >> I think Ramana had some changes in the works at one point to address
> >> (some) of this, but I'm not sure what happened to them. Ramana?
> >>
> >>
> >> +#if defined (__ARM_ARCH_6__) || defined (__ARM_ARCH_6J__) \
> >> + || defined (__ARM_ARCH_6K__) || defined (__ARM_ARCH_6T2__) \
> >> + || defined (__ARM_ARCH_6Z__) || defined (__ARM_ARCH_6ZK__) \
> >> + || defined (__ARM_ARCH_7__) || defined (__ARM_ARCH_7A__)
> >> +#if defined (__ARM_ARCH_7__) || defined (__ARM_ARCH_7A__)
> >>
> >> Ug, no! Use the ACLE macros to avoid this sort of mess.
> >>
> >
> > Ah, thanks, copy-paste from freebsd-atomic.c :)
> >
> >
> > I've attached the updated patch.
> > Is it OK?
> >
> >
> > Thanks
> > Bernd.
> >
>
> libgcc is *still* the wrong place for this. It belongs in the system
> library (eg newlib, or glibc, or whatever), which knows about the system
> it's running on. (Sorry, I should have said this before, but I've
> context-switched this out since it's been a long time since it came up).
>
> This hack will just lead to silent code failure of the worst kind
> (non-reproducable, racy) at runtime.
>
I haven't fully re-read the thread, but I think this is related to an
old discussion,
not very well archived in:
https://gcc.gnu.org/pipermail/gcc-patches/2016-November/462299.html
There's a pointer to a newlib patch from Ramana.
> R.
