Re: [PATCH, newlib] Allow locking routine to be retargeted

[Thomas Preudhomme <thomas.preudhomme@foss.arm.com>]

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.

Best regards,

Thomas