[PATCH, newlib] Allow locking routine to be retargeted

[Thomas Preudhomme]

And here's the patch. Tested with full and partial retargeting as well as no 
retargeting. Newlib testsuite shows no regression either.

Best regards,

Thomas

On 13/01/17 11:25, Thomas Preudhomme wrote:
>
>
> On 12/01/17 16:55, Thomas Preudhomme wrote:
>> Hi Freddie,
>>
>> On 12/01/17 16:20, Freddie Chopin wrote:
>>> On Thu, 2017-01-12 at 10:51 +0000, Thomas Preudhomme wrote:
>>>> My bad. I've rebased it on latest master branch. Please find the
>>>> patch attached.
>>>
>>> Thanks! Now it builds fine (;
>>>
>>> In my very limited testing it seems to work very nicely and the idea of
>>> using "struct _lock;" is very clever, as it allows me to do the
>>> retargeting in a very clean way:
>>>
>>> --- >8 --- >8 --- >8 --- >8 --- >8 --- >8 --- >8 --- >8 ---
>>>
>>> struct _lock : public distortos::Mutex
>>> {
>>>     using Mutex::Mutex;
>>> };
>>>
>>> _lock _lock___sinit_lock {distortos::Mutex::Type::recursive,
>>> distortos::Mutex::Protocol::priorityInheritance};
>>> _lock _lock___sfp_lock {distortos::Mutex::Type::recursive,
>>> distortos::Mutex::Protocol::priorityInheritance};
>>> _lock _lock___atexit_lock {distortos::Mutex::Type::recursive,
>>> distortos::Mutex::Protocol::priorityInheritance};
>>> _lock _lock___malloc_lock_object {distortos::Mutex::Type::recursive,
>>> distortos::Mutex::Protocol::priorityInheritance};
>>>
>>> void __retarget_lock_init (_LOCK_T *lock)
>>> {
>>>     *lock = new _lock {distortos::Mutex::Type::normal,
>>> distortos::Mutex::Protocol::priorityInheritance};
>>> }
>>>
>>> void __retarget_lock_close(_LOCK_T lock)
>>> {
>>>     delete lock;
>>> }
>>>
>>> void __retarget_lock_acquire (_LOCK_T lock)
>>> {
>>>     lock->lock();
>>> }
>>>
>>> void __retarget_lock_release (_LOCK_T lock)
>>> {
>>>     lock->unlock();
>>> }
>>>
>>> ...
>>>
>>> --- >8 --- >8 --- >8 --- >8 --- >8 --- >8 --- >8 --- >8 ---
>>>
>>> But I still see a big issue with the weak objects that you provided in
>>> lock.c... The problem here is that now the user can redefine just 3 of
>>> them, so the last one will be taken from the weak definition that you
>>> provided. There will be no error/warning during compilation or linking.
>>> This will of course fail terribly during run-time due to mismatched
>>> types... While this may seem a bit unlikely at the first glance, the
>>> possible scenario that would lead to that is:
>>> 1. today newlib adds this very cool feature and adds weak objects for
>>> all used locks.
>>> 2. after some time RTOS developers start to support it and also provide
>>> their own versions of the locks needed by newlib.
>>> 3. after some more time newlib adds another lock somewhere, along with
>>> the object.
>>> At this moment all users of code created during step 2 linked with
>>> newlib version from step 3 will result in undefined behaviour whenever
>>> the new lock is used...
>>>
>>> You also missed some of the locks that are used by newlib:
>>> newlib/libc/posix/telldir.c:__LOCK_INIT(static, dd_hash_lock);
>>> newlib/libc/time/tzlock.c:__LOCK_INIT(static, __tz_lock_object);
>>> newlib/libc/stdlib/quick_exit.c:__LOCK_INIT(static, atexit_mutex);
>>> newlib/libc/stdlib/envlock.c:__LOCK_INIT_RECURSIVE(static, __env_lock_object);
>>>
>>> Maybe it would be somehow possible to bind all of the objects (and
>>> maybe even functions) together, so that when user provides just one
>>> definition, all other weak definitions will be immediately disabled?
>>> This way the user would either have to provide all or none, without an
>>> option to only provide some of the objects/functions. This is my only
>>> concern - I have nothing against providing weak definitions, but the
>>> possibility of overriding only some of the objects is a bit disturbing.
>>
>> That's a very valid concern indeed. Thanks a lot for this feedback. I'll think
>> about it and see if I can come up with a solution.
>
> Actually the dummy function and lock do not have to be weakly defined to be able
> to be retargeted. Members of a static library only gets pulled if they solve an
> undefined reference. Otherwise, they are ignored.
>
> All we need is for the retargeted functions and locks provided by a platform to
> be linked before newlib. Then if all functions and locks are retargeted the
> lock.o member with all dummy definitions in newlib static library is ignored,
> otherwise it would be pulled in during the link and ld would give a linking
> error because some of the lock and function would be defined twice.
>
> How does that sounds?
>
> Best regards,
>
> Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: newlib_retargetable_locking_routine.patch
Type: text/x-patch
Size: 22962 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/newlib/attachments/20170113/f29151fe/attachment.bin>