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.
