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

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

[Thomas Preudhomme]

[-- Attachment #1: Type: text/plain, Size: 557 bytes --]

Hi,

Newlib build system defines __SINGLE_THREAD__ to allow concurrency code
to be only compiled when newlib is configured for multithread. One such
example are locks which become useless in single thread mode. Although
most static locks are indeed guarded by !defined(__SINGLE_THREAD__),
some are not.

This commit adds these missing guards to __dd_hash_mutex,
__atexit_recursive_mutex, __at_quick_exit_mutex and __arc4random_mutex.
It also makes sure locking macros in lock.h are noop in single thread
mode.

Is this ok for master?

Best regards,

Thomas

[-- Attachment #2: guard_static_lock_single_thread.patch --]
[-- Type: text/x-patch, Size: 5314 bytes --]

diff --git a/newlib/libc/include/sys/lock.h b/newlib/libc/include/sys/lock.h
index 9075e35c9179968031010432515e3a845ff6ca8d..a406ec7fd71f10b81bdfb0ca2476fa5ea50e0c0b 100644
--- a/newlib/libc/include/sys/lock.h
+++ b/newlib/libc/include/sys/lock.h
@@ -8,8 +8,14 @@ typedef int _LOCK_RECURSIVE_T;
  
 #include <_ansi.h>
 
+#ifdef __SINGLE_THREAD__
+#define __LOCK_INIT(class,lock) ;
+#define __LOCK_INIT_RECURSIVE(class,lock) ;
+#else
 #define __LOCK_INIT(class,lock) static int lock = 0;
 #define __LOCK_INIT_RECURSIVE(class,lock) static int lock = 0;
+#endif
+
 #define __lock_init(lock) (_CAST_VOID 0)
 #define __lock_init_recursive(lock) (_CAST_VOID 0)
 #define __lock_close(lock) (_CAST_VOID 0)
diff --git a/newlib/libc/posix/telldir.c b/newlib/libc/posix/telldir.c
index 0a8eac024b4955cd70b192307938358bb1cb2236..959e3b7bb78860f775e0de2f9d52ff2eb6b67411 100644
--- a/newlib/libc/posix/telldir.c
+++ b/newlib/libc/posix/telldir.c
@@ -70,7 +70,7 @@ struct ddloc {
 static long	dd_loccnt = 1;	/* Index of entry for sequential readdir's */
 static struct	ddloc *dd_hash[NDIRHASH];   /* Hash list heads for ddlocs */
 
-#ifdef HAVE_DD_LOCK
+#if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
 __LOCK_INIT(static, __dd_hash_mutex);
 #endif
 
@@ -92,8 +92,10 @@ _DEFUN(telldir, (dirp),
 
 #ifdef HAVE_DD_LOCK
 	__lock_acquire_recursive(dirp->dd_lock);
+#ifndef __SINGLE_THREAD__
 	__lock_acquire(__dd_hash_mutex);
 #endif
+#endif
 	index = dd_loccnt++;
 	lp->loc_index = index;
 	lp->loc_seek = dirp->dd_seek;
@@ -102,7 +104,9 @@ _DEFUN(telldir, (dirp),
 	lp->loc_next = dd_hash[LOCHASH(index)];
 	dd_hash[LOCHASH(index)] = lp;
 #ifdef HAVE_DD_LOCK
+#ifndef __SINGLE_THREAD__
 	__lock_release(__dd_hash_mutex);
+#endif
 	__lock_release_recursive(dirp->dd_lock);
 #endif
 	return (index);
@@ -123,7 +127,7 @@ _DEFUN(_seekdir, (dirp, loc),
 	register struct ddloc **prevlp;
 	struct dirent *dp;
 
-#ifdef HAVE_DD_LOCK
+#if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
 	__lock_acquire(__dd_hash_mutex);
 #endif
 	if (loc != 0) {
@@ -136,7 +140,7 @@ _DEFUN(_seekdir, (dirp, loc),
 			lp = lp->loc_next;
 		}
 		if (lp == NULL) {
-#ifdef HAVE_DD_LOCK
+#if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
 			__lock_release(__dd_hash_mutex);
 #endif
 			return;
@@ -162,7 +166,7 @@ found:
 		dirp->dd_seek = 0;
 		dirp->dd_loc = 0;
 	}
-#ifdef HAVE_DD_LOCK
+#if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
 	__lock_release(__dd_hash_mutex);
 #endif
 }
@@ -174,7 +178,7 @@ _DEFUN(_cleanupdir, (dirp),
 {
 	int i;
 
-#ifdef HAVE_DD_LOCK
+#if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
 	__lock_acquire(__dd_hash_mutex);
 #endif
 	for (i = 0; i < NDIRHASH; ++i) {
@@ -199,7 +203,7 @@ _DEFUN(_cleanupdir, (dirp),
 		}
 		dd_hash[i] = head.loc_next;
 	}
-#ifdef HAVE_DD_LOCK
+#if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
 	__lock_release(__dd_hash_mutex);
 #endif
 
diff --git a/newlib/libc/stdlib/__call_atexit.c b/newlib/libc/stdlib/__call_atexit.c
index 7d5e0d0464b4b9ff74fcb003d965a3c2da020699..6a809cc4d5bd09905d010fdc21708a537166971d 100644
--- a/newlib/libc/stdlib/__call_atexit.c
+++ b/newlib/libc/stdlib/__call_atexit.c
@@ -11,7 +11,9 @@
 /* Make this a weak reference to avoid pulling in free.  */
 void free(void *) _ATTRIBUTE((__weak__));
 
+#ifndef __SINGLE_THREAD__
 __LOCK_INIT_RECURSIVE(, __atexit_recursive_mutex);
+#endif
 
 #ifdef _REENT_GLOBAL_ATEXIT
 struct _atexit *_global_atexit = _NULL;
diff --git a/newlib/libc/stdlib/arc4random.h b/newlib/libc/stdlib/arc4random.h
index 3c5fe235338270b1af48190cefa9278ea3af917f..4b9855305f5cbce00dd96ee58ed32d1ad48aa5cf 100644
--- a/newlib/libc/stdlib/arc4random.h
+++ b/newlib/libc/stdlib/arc4random.h
@@ -47,7 +47,9 @@
 
 #endif /* _ARC4_LOCK_INIT */
 
+#ifndef __SINGLE_THREAD__
 _ARC4_LOCK_INIT
+#endif
 
 #ifdef _ARC4RANDOM_DATA
 _ARC4RANDOM_DATA
diff --git a/newlib/libc/stdlib/arc4random.c b/newlib/libc/stdlib/arc4random.c
index 75cdff3bc4b5d4245cce4130df64aebc956c853f..3cccc3ed46a5e9e756d4e216cb7686589eb60993 100644
--- a/newlib/libc/stdlib/arc4random.c
+++ b/newlib/libc/stdlib/arc4random.c
@@ -180,16 +180,24 @@ arc4random(void)
 {
 	uint32_t val;
 
+#ifndef __SINGLE_THREAD__
 	_ARC4_LOCK();
+#endif
 	_rs_random_u32(&val);
+#ifndef __SINGLE_THREAD__
 	_ARC4_UNLOCK();
+#endif
 	return val;
 }
 
 void
 arc4random_buf(void *buf, size_t n)
 {
+#ifndef __SINGLE_THREAD__
 	_ARC4_LOCK();
+#endif
 	_rs_random_buf(buf, n);
+#ifndef __SINGLE_THREAD__
 	_ARC4_UNLOCK();
+#endif
 }
diff --git a/newlib/libc/stdlib/quick_exit.c b/newlib/libc/stdlib/quick_exit.c
index aaa5f9f7f1df12dace7d87271291aa87cd2fb3b1..5ab2609bf04d508a228049a52aea5e6ec7216d54 100644
--- a/newlib/libc/stdlib/quick_exit.c
+++ b/newlib/libc/stdlib/quick_exit.c
@@ -44,7 +44,9 @@ struct quick_exit_handler {
 /**
  * Lock protecting the handlers list.
  */
+#ifndef __SINGLE_THREAD__
 __LOCK_INIT(static, __at_quick_exit_mutex);
+#endif
 /**
  * Stack of cleanup handlers.  These will be invoked in reverse order when
  */
@@ -60,10 +62,14 @@ at_quick_exit(void (*func)(void))
 	if (NULL == h)
 		return (1);
 	h->cleanup = func;
+#ifndef __SINGLE_THREAD__
 	__lock_acquire(__at_quick_exit_mutex);
+#endif
 	h->next = handlers;
 	handlers = h;
+#ifndef __SINGLE_THREAD__
 	__lock_release(__at_quick_exit_mutex);
+#endif
 	return (0);
 }
 

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

[Thomas Preudhomme]

[-- Attachment #1: Type: text/plain, Size: 685 bytes --]

And in git format-patch now.

Best regards,

Thomas

On 31/01/17 17:10, Thomas Preudhomme wrote:
> Hi,
>
> Newlib build system defines __SINGLE_THREAD__ to allow concurrency code
> to be only compiled when newlib is configured for multithread. One such
> example are locks which become useless in single thread mode. Although
> most static locks are indeed guarded by !defined(__SINGLE_THREAD__),
> some are not.
>
> This commit adds these missing guards to __dd_hash_mutex,
> __atexit_recursive_mutex, __at_quick_exit_mutex and __arc4random_mutex.
> It also makes sure locking macros in lock.h are noop in single thread
> mode.
>
> Is this ok for master?
>
> Best regards,
>
> Thomas

[-- Attachment #2: guard_static_lock_single_thread.patch --]
[-- Type: text/x-patch, Size: 6013 bytes --]

From 5788cf9f4ad372a541f654e2919a2a5f9709ec1c Mon Sep 17 00:00:00 2001
From: Thomas Preud'homme <thomas.preudhomme@arm.com>
Date: Mon, 30 Jan 2017 11:23:00 +0000
Subject: [PATCH 1/2] Only define static locks in multithreaded mode

Newlib build system defines __SINGLE_THREAD__ to allow concurrency code
to be only compiled when newlib is configured for multithread. One such
example are locks which become useless in single thread mode. Although
most static locks are indeed guarded by !defined(__SINGLE_THREAD__),
some are not.

This commit adds these missing guards to __dd_hash_mutex,
__atexit_recursive_mutex, __at_quick_exit_mutex and __arc4random_mutex.
It also makes sure locking macros in lock.h are noop in single thread
mode.
---
 newlib/libc/include/sys/lock.h     |  6 ++++++
 newlib/libc/posix/telldir.c        | 16 ++++++++++------
 newlib/libc/stdlib/__call_atexit.c |  2 ++
 newlib/libc/stdlib/arc4random.c    |  8 ++++++++
 newlib/libc/stdlib/arc4random.h    |  2 ++
 newlib/libc/stdlib/quick_exit.c    |  6 ++++++
 6 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/newlib/libc/include/sys/lock.h b/newlib/libc/include/sys/lock.h
index 9075e35..a406ec7 100644
--- a/newlib/libc/include/sys/lock.h
+++ b/newlib/libc/include/sys/lock.h
@@ -8,8 +8,14 @@ typedef int _LOCK_RECURSIVE_T;
  
 #include <_ansi.h>
 
+#ifdef __SINGLE_THREAD__
+#define __LOCK_INIT(class,lock) ;
+#define __LOCK_INIT_RECURSIVE(class,lock) ;
+#else
 #define __LOCK_INIT(class,lock) static int lock = 0;
 #define __LOCK_INIT_RECURSIVE(class,lock) static int lock = 0;
+#endif
+
 #define __lock_init(lock) (_CAST_VOID 0)
 #define __lock_init_recursive(lock) (_CAST_VOID 0)
 #define __lock_close(lock) (_CAST_VOID 0)
diff --git a/newlib/libc/posix/telldir.c b/newlib/libc/posix/telldir.c
index 0a8eac0..959e3b7 100644
--- a/newlib/libc/posix/telldir.c
+++ b/newlib/libc/posix/telldir.c
@@ -70,7 +70,7 @@ struct ddloc {
 static long	dd_loccnt = 1;	/* Index of entry for sequential readdir's */
 static struct	ddloc *dd_hash[NDIRHASH];   /* Hash list heads for ddlocs */
 
-#ifdef HAVE_DD_LOCK
+#if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
 __LOCK_INIT(static, __dd_hash_mutex);
 #endif
 
@@ -92,8 +92,10 @@ _DEFUN(telldir, (dirp),
 
 #ifdef HAVE_DD_LOCK
 	__lock_acquire_recursive(dirp->dd_lock);
+#ifndef __SINGLE_THREAD__
 	__lock_acquire(__dd_hash_mutex);
 #endif
+#endif
 	index = dd_loccnt++;
 	lp->loc_index = index;
 	lp->loc_seek = dirp->dd_seek;
@@ -102,7 +104,9 @@ _DEFUN(telldir, (dirp),
 	lp->loc_next = dd_hash[LOCHASH(index)];
 	dd_hash[LOCHASH(index)] = lp;
 #ifdef HAVE_DD_LOCK
+#ifndef __SINGLE_THREAD__
 	__lock_release(__dd_hash_mutex);
+#endif
 	__lock_release_recursive(dirp->dd_lock);
 #endif
 	return (index);
@@ -123,7 +127,7 @@ _DEFUN(_seekdir, (dirp, loc),
 	register struct ddloc **prevlp;
 	struct dirent *dp;
 
-#ifdef HAVE_DD_LOCK
+#if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
 	__lock_acquire(__dd_hash_mutex);
 #endif
 	if (loc != 0) {
@@ -136,7 +140,7 @@ _DEFUN(_seekdir, (dirp, loc),
 			lp = lp->loc_next;
 		}
 		if (lp == NULL) {
-#ifdef HAVE_DD_LOCK
+#if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
 			__lock_release(__dd_hash_mutex);
 #endif
 			return;
@@ -162,7 +166,7 @@ found:
 		dirp->dd_seek = 0;
 		dirp->dd_loc = 0;
 	}
-#ifdef HAVE_DD_LOCK
+#if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
 	__lock_release(__dd_hash_mutex);
 #endif
 }
@@ -174,7 +178,7 @@ _DEFUN(_cleanupdir, (dirp),
 {
 	int i;
 
-#ifdef HAVE_DD_LOCK
+#if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
 	__lock_acquire(__dd_hash_mutex);
 #endif
 	for (i = 0; i < NDIRHASH; ++i) {
@@ -199,7 +203,7 @@ _DEFUN(_cleanupdir, (dirp),
 		}
 		dd_hash[i] = head.loc_next;
 	}
-#ifdef HAVE_DD_LOCK
+#if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
 	__lock_release(__dd_hash_mutex);
 #endif
 
diff --git a/newlib/libc/stdlib/__call_atexit.c b/newlib/libc/stdlib/__call_atexit.c
index 7d5e0d0..6a809cc 100644
--- a/newlib/libc/stdlib/__call_atexit.c
+++ b/newlib/libc/stdlib/__call_atexit.c
@@ -11,7 +11,9 @@
 /* Make this a weak reference to avoid pulling in free.  */
 void free(void *) _ATTRIBUTE((__weak__));
 
+#ifndef __SINGLE_THREAD__
 __LOCK_INIT_RECURSIVE(, __atexit_recursive_mutex);
+#endif
 
 #ifdef _REENT_GLOBAL_ATEXIT
 struct _atexit *_global_atexit = _NULL;
diff --git a/newlib/libc/stdlib/arc4random.c b/newlib/libc/stdlib/arc4random.c
index 75cdff3..3cccc3e 100644
--- a/newlib/libc/stdlib/arc4random.c
+++ b/newlib/libc/stdlib/arc4random.c
@@ -180,16 +180,24 @@ arc4random(void)
 {
 	uint32_t val;
 
+#ifndef __SINGLE_THREAD__
 	_ARC4_LOCK();
+#endif
 	_rs_random_u32(&val);
+#ifndef __SINGLE_THREAD__
 	_ARC4_UNLOCK();
+#endif
 	return val;
 }
 
 void
 arc4random_buf(void *buf, size_t n)
 {
+#ifndef __SINGLE_THREAD__
 	_ARC4_LOCK();
+#endif
 	_rs_random_buf(buf, n);
+#ifndef __SINGLE_THREAD__
 	_ARC4_UNLOCK();
+#endif
 }
diff --git a/newlib/libc/stdlib/arc4random.h b/newlib/libc/stdlib/arc4random.h
index 3c5fe23..4b98553 100644
--- a/newlib/libc/stdlib/arc4random.h
+++ b/newlib/libc/stdlib/arc4random.h
@@ -47,7 +47,9 @@
 
 #endif /* _ARC4_LOCK_INIT */
 
+#ifndef __SINGLE_THREAD__
 _ARC4_LOCK_INIT
+#endif
 
 #ifdef _ARC4RANDOM_DATA
 _ARC4RANDOM_DATA
diff --git a/newlib/libc/stdlib/quick_exit.c b/newlib/libc/stdlib/quick_exit.c
index aaa5f9f..5ab2609 100644
--- a/newlib/libc/stdlib/quick_exit.c
+++ b/newlib/libc/stdlib/quick_exit.c
@@ -44,7 +44,9 @@ struct quick_exit_handler {
 /**
  * Lock protecting the handlers list.
  */
+#ifndef __SINGLE_THREAD__
 __LOCK_INIT(static, __at_quick_exit_mutex);
+#endif
 /**
  * Stack of cleanup handlers.  These will be invoked in reverse order when
  */
@@ -60,10 +62,14 @@ at_quick_exit(void (*func)(void))
 	if (NULL == h)
 		return (1);
 	h->cleanup = func;
+#ifndef __SINGLE_THREAD__
 	__lock_acquire(__at_quick_exit_mutex);
+#endif
 	h->next = handlers;
 	handlers = h;
+#ifndef __SINGLE_THREAD__
 	__lock_release(__at_quick_exit_mutex);
+#endif
 	return (0);
 }
 
-- 
1.9.1

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

[Freddie Chopin]

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

Regards,
FCh

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

[Thomas Preudhomme]

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

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

[Craig Howland]

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

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

[Thomas Preudhomme]

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

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

[Thomas Preudhomme]

[-- Attachment #1: Type: text/plain, Size: 903 bytes --]

Hi,

I've removed the changes to lock.h as requested. Please find attached an updated 
patch in git format-patch format.

Best regards,

Thomas

On 31/01/17 17:21, Thomas Preudhomme wrote:
> And in git format-patch now.
>
> Best regards,
>
> Thomas
>
> On 31/01/17 17:10, Thomas Preudhomme wrote:
>> Hi,
>>
>> Newlib build system defines __SINGLE_THREAD__ to allow concurrency code
>> to be only compiled when newlib is configured for multithread. One such
>> example are locks which become useless in single thread mode. Although
>> most static locks are indeed guarded by !defined(__SINGLE_THREAD__),
>> some are not.
>>
>> This commit adds these missing guards to __dd_hash_mutex,
>> __atexit_recursive_mutex, __at_quick_exit_mutex and __arc4random_mutex.
>> It also makes sure locking macros in lock.h are noop in single thread
>> mode.
>>
>> Is this ok for master?
>>
>> Best regards,
>>
>> Thomas

[-- Attachment #2: guard_static_lock_single_thread.patch --]
[-- Type: text/x-patch, Size: 5333 bytes --]

From 90852a60b67b62f3ae28d7f56fc9a6e56c7872e3 Mon Sep 17 00:00:00 2001
From: Thomas Preud'homme <thomas.preudhomme@arm.com>
Date: Mon, 30 Jan 2017 11:23:00 +0000
Subject: [PATCH 1/2] Only define static locks in multithreaded mode

Newlib build system defines __SINGLE_THREAD__ to allow concurrency code
to be only compiled when newlib is configured for multithread. One such
example are locks which become useless in single thread mode. Although
most static locks are indeed guarded by !defined(__SINGLE_THREAD__),
some are not.

This commit adds these missing guards to __dd_hash_mutex,
__atexit_recursive_mutex, __at_quick_exit_mutex and __arc4random_mutex.
It also makes sure locking macros in lock.h are noop in single thread
mode.
---
 newlib/libc/posix/telldir.c        | 16 ++++++++++------
 newlib/libc/stdlib/__call_atexit.c |  2 ++
 newlib/libc/stdlib/arc4random.c    |  8 ++++++++
 newlib/libc/stdlib/arc4random.h    |  2 ++
 newlib/libc/stdlib/quick_exit.c    |  6 ++++++
 5 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/newlib/libc/posix/telldir.c b/newlib/libc/posix/telldir.c
index 0a8eac0..959e3b7 100644
--- a/newlib/libc/posix/telldir.c
+++ b/newlib/libc/posix/telldir.c
@@ -70,7 +70,7 @@ struct ddloc {
 static long	dd_loccnt = 1;	/* Index of entry for sequential readdir's */
 static struct	ddloc *dd_hash[NDIRHASH];   /* Hash list heads for ddlocs */
 
-#ifdef HAVE_DD_LOCK
+#if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
 __LOCK_INIT(static, __dd_hash_mutex);
 #endif
 
@@ -92,8 +92,10 @@ _DEFUN(telldir, (dirp),
 
 #ifdef HAVE_DD_LOCK
 	__lock_acquire_recursive(dirp->dd_lock);
+#ifndef __SINGLE_THREAD__
 	__lock_acquire(__dd_hash_mutex);
 #endif
+#endif
 	index = dd_loccnt++;
 	lp->loc_index = index;
 	lp->loc_seek = dirp->dd_seek;
@@ -102,7 +104,9 @@ _DEFUN(telldir, (dirp),
 	lp->loc_next = dd_hash[LOCHASH(index)];
 	dd_hash[LOCHASH(index)] = lp;
 #ifdef HAVE_DD_LOCK
+#ifndef __SINGLE_THREAD__
 	__lock_release(__dd_hash_mutex);
+#endif
 	__lock_release_recursive(dirp->dd_lock);
 #endif
 	return (index);
@@ -123,7 +127,7 @@ _DEFUN(_seekdir, (dirp, loc),
 	register struct ddloc **prevlp;
 	struct dirent *dp;
 
-#ifdef HAVE_DD_LOCK
+#if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
 	__lock_acquire(__dd_hash_mutex);
 #endif
 	if (loc != 0) {
@@ -136,7 +140,7 @@ _DEFUN(_seekdir, (dirp, loc),
 			lp = lp->loc_next;
 		}
 		if (lp == NULL) {
-#ifdef HAVE_DD_LOCK
+#if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
 			__lock_release(__dd_hash_mutex);
 #endif
 			return;
@@ -162,7 +166,7 @@ found:
 		dirp->dd_seek = 0;
 		dirp->dd_loc = 0;
 	}
-#ifdef HAVE_DD_LOCK
+#if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
 	__lock_release(__dd_hash_mutex);
 #endif
 }
@@ -174,7 +178,7 @@ _DEFUN(_cleanupdir, (dirp),
 {
 	int i;
 
-#ifdef HAVE_DD_LOCK
+#if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
 	__lock_acquire(__dd_hash_mutex);
 #endif
 	for (i = 0; i < NDIRHASH; ++i) {
@@ -199,7 +203,7 @@ _DEFUN(_cleanupdir, (dirp),
 		}
 		dd_hash[i] = head.loc_next;
 	}
-#ifdef HAVE_DD_LOCK
+#if !defined(__SINGLE_THREAD__) && defined(HAVE_DD_LOCK)
 	__lock_release(__dd_hash_mutex);
 #endif
 
diff --git a/newlib/libc/stdlib/__call_atexit.c b/newlib/libc/stdlib/__call_atexit.c
index 7d5e0d0..6a809cc 100644
--- a/newlib/libc/stdlib/__call_atexit.c
+++ b/newlib/libc/stdlib/__call_atexit.c
@@ -11,7 +11,9 @@
 /* Make this a weak reference to avoid pulling in free.  */
 void free(void *) _ATTRIBUTE((__weak__));
 
+#ifndef __SINGLE_THREAD__
 __LOCK_INIT_RECURSIVE(, __atexit_recursive_mutex);
+#endif
 
 #ifdef _REENT_GLOBAL_ATEXIT
 struct _atexit *_global_atexit = _NULL;
diff --git a/newlib/libc/stdlib/arc4random.c b/newlib/libc/stdlib/arc4random.c
index 75cdff3..3cccc3e 100644
--- a/newlib/libc/stdlib/arc4random.c
+++ b/newlib/libc/stdlib/arc4random.c
@@ -180,16 +180,24 @@ arc4random(void)
 {
 	uint32_t val;
 
+#ifndef __SINGLE_THREAD__
 	_ARC4_LOCK();
+#endif
 	_rs_random_u32(&val);
+#ifndef __SINGLE_THREAD__
 	_ARC4_UNLOCK();
+#endif
 	return val;
 }
 
 void
 arc4random_buf(void *buf, size_t n)
 {
+#ifndef __SINGLE_THREAD__
 	_ARC4_LOCK();
+#endif
 	_rs_random_buf(buf, n);
+#ifndef __SINGLE_THREAD__
 	_ARC4_UNLOCK();
+#endif
 }
diff --git a/newlib/libc/stdlib/arc4random.h b/newlib/libc/stdlib/arc4random.h
index 3c5fe23..4b98553 100644
--- a/newlib/libc/stdlib/arc4random.h
+++ b/newlib/libc/stdlib/arc4random.h
@@ -47,7 +47,9 @@
 
 #endif /* _ARC4_LOCK_INIT */
 
+#ifndef __SINGLE_THREAD__
 _ARC4_LOCK_INIT
+#endif
 
 #ifdef _ARC4RANDOM_DATA
 _ARC4RANDOM_DATA
diff --git a/newlib/libc/stdlib/quick_exit.c b/newlib/libc/stdlib/quick_exit.c
index aaa5f9f..5ab2609 100644
--- a/newlib/libc/stdlib/quick_exit.c
+++ b/newlib/libc/stdlib/quick_exit.c
@@ -44,7 +44,9 @@ struct quick_exit_handler {
 /**
  * Lock protecting the handlers list.
  */
+#ifndef __SINGLE_THREAD__
 __LOCK_INIT(static, __at_quick_exit_mutex);
+#endif
 /**
  * Stack of cleanup handlers.  These will be invoked in reverse order when
  */
@@ -60,10 +62,14 @@ at_quick_exit(void (*func)(void))
 	if (NULL == h)
 		return (1);
 	h->cleanup = func;
+#ifndef __SINGLE_THREAD__
 	__lock_acquire(__at_quick_exit_mutex);
+#endif
 	h->next = handlers;
 	handlers = h;
+#ifndef __SINGLE_THREAD__
 	__lock_release(__at_quick_exit_mutex);
+#endif
 	return (0);
 }
 
-- 
1.9.1

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

[Jeff Johnston]

Patch checked in.

-- JEff J.

----- Original Message -----
> Hi,
> 
> I've removed the changes to lock.h as requested. Please find attached an
> updated
> patch in git format-patch format.
> 
> Best regards,
> 
> Thomas
> 
> On 31/01/17 17:21, Thomas Preudhomme wrote:
> > And in git format-patch now.
> >
> > Best regards,
> >
> > Thomas
> >
> > On 31/01/17 17:10, Thomas Preudhomme wrote:
> >> Hi,
> >>
> >> Newlib build system defines __SINGLE_THREAD__ to allow concurrency code
> >> to be only compiled when newlib is configured for multithread. One such
> >> example are locks which become useless in single thread mode. Although
> >> most static locks are indeed guarded by !defined(__SINGLE_THREAD__),
> >> some are not.
> >>
> >> This commit adds these missing guards to __dd_hash_mutex,
> >> __atexit_recursive_mutex, __at_quick_exit_mutex and __arc4random_mutex.
> >> It also makes sure locking macros in lock.h are noop in single thread
> >> mode.
> >>
> >> Is this ok for master?
> >>
> >> Best regards,
> >>
> >> Thomas
>