diff mbox series

libgcc: Add a weak stub for __sync_synchronize

Message ID AM6PR03MB5170ECC2BE5C2B0F3C276433E4110@AM6PR03MB5170.eurprd03.prod.outlook.com
State New
Headers show
Series libgcc: Add a weak stub for __sync_synchronize | expand

Commit Message

Bernd Edlinger Nov. 3, 2020, 3:08 p.m. UTC 
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.

Comments

Bernd Edlinger Nov. 17, 2020, 5:43 a.m. UTC | #1 
Ping...

I'd like to ping for this patch:
https://gcc.gnu.org/pipermail/gcc-patches/2020-November/557886.html

Thanks
Bernd.

On 11/3/20 4:08 PM, 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.
>
Richard Earnshaw (lists) Nov. 17, 2020, 12:44 p.m. UTC | #2 
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.

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.

R.
Bernd Edlinger Nov. 17, 2020, 3:18 p.m. UTC | #3 
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.
Richard Earnshaw (lists) Nov. 17, 2020, 3:41 p.m. UTC | #4 
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.

R.
Christophe Lyon Nov. 17, 2020, 3:51 p.m. UTC | #5 
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.
Bernd Edlinger Nov. 17, 2020, 5:17 p.m. UTC | #6 
On 11/17/20 4:41 PM, Richard Earnshaw (lists) wrote:
> 
> 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).
> 

No problem.  I just saw it from the other end.

It is odd that this problem does not go away even if gcc is configured
with --disable-threads, which should be the default for arm-none-eabi
anyway.

If we assume a threaded environment then it is still libgcc
which does not define __GTHREADS in libgcc/gthr.h, and libstdc++'s
__cxa_guard_acquire is not making use of functions like __gthread_mutex_lock.
But that appears to be this way by design.

Of course the race is not fixed if you ask newlib to implement just this
__sync_synchronize function.

> This hack will just lead to silent code failure of the worst kind
> (non-reproducable, racy) at runtime.
> 

So in a arm-none-eabi system with armv6 or higher where the intrinsic
__sync_synchronize is not a library call but an instruction
we have exactly this worst kind scenario, already.

It is however possible that the default of -fthreadsafe_statics
is inappropriate for --disable-threads ?


Bernd.


> R.
>
diff mbox series

Patch

From f8a3df552f4b98308096659c66b4c8ea68580f25 Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Mon, 2 Nov 2020 11:43:44 +0100
Subject: [PATCH] libgcc: Add a weak stub for __sync_synchronize

This patch adds a default implementation for __sync_synchronize,
which prevents many unresolved symbol errors on arm-none-eabi.
This happens often in C++ programs even without any threading.

libgcc:
2020-11-02  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* config.host: Use t-eabi for arm-none-eabi.
	* config/arm/t-eabi: New.
	* config/arm/eabi-sync.c: New.
---
 libgcc/config.host            |  2 +-
 libgcc/config/arm/eabi-sync.c | 39 +++++++++++++++++++++++++++++++++++++++
 libgcc/config/arm/t-eabi      |  1 +
 3 files changed, 41 insertions(+), 1 deletion(-)
 create mode 100644 libgcc/config/arm/eabi-sync.c
 create mode 100644 libgcc/config/arm/t-eabi

diff --git a/libgcc/config.host b/libgcc/config.host
index fd8e55e..e25abf4 100644
--- a/libgcc/config.host
+++ b/libgcc/config.host
@@ -495,7 +495,7 @@  arm*-*-eabi* | arm*-*-symbianelf* | arm*-*-rtems*)
 	tm_file="$tm_file arm/bpabi-lib.h"
 	case ${host} in
 	arm*-*-eabi* | arm*-*-rtems*)
-	  tmake_file="${tmake_file} arm/t-bpabi t-crtfm"
+	  tmake_file="${tmake_file} arm/t-bpabi t-crtfm arm/t-eabi"
 	  extra_parts="crtbegin.o crtend.o crti.o crtn.o"
 	  ;;
 	arm*-*-symbianelf*)
diff --git a/libgcc/config/arm/eabi-sync.c b/libgcc/config/arm/eabi-sync.c
new file mode 100644
index 0000000..bffdd4a
--- /dev/null
+++ b/libgcc/config/arm/eabi-sync.c
@@ -0,0 +1,39 @@ 
+/* Copyright (C) 2020 Free Software Foundation, Inc.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+Software Foundation; either version 3, or (at your option) any later
+version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+for more details.
+
+Under Section 7 of GPL version 3, you are granted additional
+permissions described in the GCC Runtime Library Exception, version
+3.1, as published by the Free Software Foundation.
+
+You should have received a copy of the GNU General Public License and
+a copy of the GCC Runtime Library Exception along with this program;
+see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
+<http://www.gnu.org/licenses/>.  */
+
+void __attribute__ ((weak))
+__sync_synchronize (void)
+{
+#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__)
+    __asm __volatile ("dmb" : : : "memory");
+#else
+    __asm __volatile ("mcr p15, 0, r0, c7, c10, 5" : : : "memory");
+#endif
+#else
+    __asm __volatile ("nop" : : : "memory");
+#endif
+}
diff --git a/libgcc/config/arm/t-eabi b/libgcc/config/arm/t-eabi
new file mode 100644
index 0000000..556032f
--- /dev/null
+++ b/libgcc/config/arm/t-eabi
@@ -0,0 +1 @@ 
+LIB2ADD_ST += $(srcdir)/config/arm/eabi-sync.c
-- 
1.9.1