[newlib] Allow locking routine to be retargeted

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

[newlib] Allow locking routine to be retargeted

Commit Message

Comments

Patch