Re: [PATCH 2/3, newlib] Only define static locks in multithreaded mode

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

Hi Craig,

On 02/02/17 15:45, Craig Howland wrote:
> On 02/02/2017 09:32 AM, Thomas Preudhomme wrote:
>> On 01/02/17 08:48, Freddie Chopin wrote:
>>> On Tue, 2017-01-31 at 17:10 +0000, Thomas Preudhomme wrote:
>>>> It also makes sure locking macros in lock.h are noop in single thread
>>>> mode.
>>>
>>> Doesn't it make all the guards in the code superfluous? If the lock
>>> initialization macros are defined to be ";" and lock functions are
>>> defined to be "(void)0", then the guards in the code actually guard
>>> nothing, as things like:
>>>
>>>  #if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
>>>  __LOCK_INIT(static, __dd_hash_mutex);
>>>  #endif
>>>
>>>  #ifndef __SINGLE_THREAD__
>>>         __lock_acquire(__dd_hash_mutex);
>>>  #endif
>>>
>>> will then be equivalent to:
>>>
>>>  #if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
>>>  ;
>>>  #endif
>>>
>>>  #ifndef __SINGLE_THREAD__
>>>         (void)0;
>>>  #endif
>>
>> Yes it does and I started to remove them but noticed that removing the guard
>> made code more inconsistent. The example you give is a nice one because then
>> we would still have a condition with HAVE_DD_LOCK which might induce people to
>> think that's the only case where this line does nothing.
>>
>> In some other case __SINGLE_THREAD__ guards more than just the lock. For
>> instance in newlib/libc/stdio/findfp.c __SINGLE_THREAD__ is used to guard
>> whether some function are defined. This would need to be kept so the only
>> change that could be done is take the lock out of the __SINGLE_THREAD__
>> guarded block which would look weird again.
>>
>> I then wondered whether making the code noop in lock.h was the right approach
>> but decided that provided more safety that the code would not be compiled in
>> in case of missing guard (as you discovered for some of the locks). It also
>> goes with what is currently being done when targeting bare-metal since the
>> lock routines themselves are noop in that case. Only the declarations are not.
>> The one thing that could be done is to make the declaration noop in both
>> cases, thus reducing the complexity.
>>
>> Any thoughts on that?
>>
>> Best regards,
>>
>> Thomas
> Yes, a thought on the general approach.  At this moment in time the locks define
> to being degenerate so that the guards are superfluous.  However, the guards
> make it very clear on reading the "application" code that the locks are not used
> in those cases, without needing to chase down what happens with the underlying
> functions.  In addition, the locks becoming degenerate in the guarded cases
> could possibly change in the future.  So the thought is that while the guards
> could be removed, don't.  This not only keeps the main code more clear in
> intent, it saves the time to take them out, the time to check it, and could also
> possibly save the time putting them back later on if the present degeneration
> method were to be altered.

Another aspect I've just thought of is that if one target a system with its own 
lock (such as Linux) then we cannot be sure they will degenerate in the single 
threaded case.

I hear your argument about making the code more explicit and came to the same 
conclusion while I was removing them. Note that had SINGLE_THREAD been there 
only for the locks, I would have happily removed them because I think it would 
have made the code much cleaner while not causing too much conclusion (that's 
reasonable to expect locks to be noop in the case of single thread). That's not 
the case though so removing the the lock only improves the code size marginally 
while increasing confusion more, for the reason I explained earlier.

Do you have any thoughts though on making the lock declarations noop for both 
single thread and no retargeting cases? Right now my patch differentiate the two 
but I don't see why not just use the same approach for both.

Best regards,

Thomas