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

[Craig Howland <howland at LGSInnovations dot com>]

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.
Craig