Re: signals and _REENT

[Jeff Johnston <jjohnstn at redhat dot com>]

Modified patch attached.

-- Jeff J.

----- Original Message -----
From: "Jeff Johnston" <jjohnstn@redhat.com>
To: "Joel Sherrill" <joel.sherrill@OARcorp.com>
Cc: "Newlib Mailing List" <newlib@sourceware.org>
Sent: Thursday, February 26, 2015 6:45:02 PM
Subject: Re: signals and _REENT

----- Original Message -----
> From: "Joel Sherrill" <joel.sherrill@OARcorp.com>
> To: "Jeff Johnston" <jjohnstn@redhat.com>, "Freddie Chopin" <freddie_chopin@op.pl>
> Cc: newlib@sourceware.org
> Sent: Thursday, February 26, 2015 5:35:25 PM
> Subject: Re: signals and _REENT
> 
> 
> 
> On February 26, 2015 5:31:53 PM EST, Jeff Johnston <jjohnstn@redhat.com>
> wrote:
> >----- Original Message -----
> >> From: "Freddie Chopin" <freddie_chopin@op.pl>
> >> To: newlib@sourceware.org
> >> Sent: Thursday, February 26, 2015 4:55:00 PM
> >> Subject: Re: signals and _REENT
> >> 
> >> On 02/26/2015 10:41 PM, Jeff Johnston wrote:
> >> > Actually I did miss something.  Since we are using the global list,
> >> > we have to thread protect it with locking and unlocking.  The
> >normal way
> >> > would be
> >> > to add two new locks: __sigfunc_lock_acquire() and
> >__sigfunc_lock_release()
> >> > which
> >> > are needed if not single threaded.  This will cause some breakage
> >for
> >> > builds that
> >> > use threads until they define these locks.  Perhaps a single global
> >reent
> >> > lock might
> >> > be helpful.
> >> 
> >> In that case maybe it would be a good idea to "reuse" the lock that
> >is
> >> used with _GLOBAL_REENT in stdio? Or do you think separate lock for
> >> signals and separate lock for stdio would be a better option (smaller
> >> chance of congestion)?
> >> 
> >
> >I had thought of that.  I'm ok with using the _newlib_sfp_lock_start
> >and
> >_newlib_sfp_lock_end macros as they are defined in stdio/local.h.  If
> >no one
> >objects, I can do that at least for now.
> 
> How many locks does newlib require? Is there a per thread factor?
> 

Not sure what you asking.  

After thinking about it, I should have just done
the same thing we do for locking the environ functions and atexit functions so
that no-one has to add anything.  I'll work on it and re-post my patch.

-- Jeff J.
 

Index: signal.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/signal/signal.c,v
retrieving revision 1.6
diff -u -p -r1.6 signal.c
--- signal.c	31 Oct 2007 22:22:31 -0000	1.6
+++ signal.c	27 Feb 2015 22:38:36 -0000
@@ -98,6 +98,9 @@ int _dummy_simulated_signal;
 #include <stdlib.h>
 #include <reent.h>
 #include <_syslist.h>
+#include <sys/lock.h>
+
+__LOCK_INIT_RECURSIVE(, __sigfunc_lock);
 
 int
 _DEFUN (_init_signal_r, (ptr),
@@ -105,16 +108,29 @@ _DEFUN (_init_signal_r, (ptr),
 {
   int i;
 
-  if (ptr->_sig_func == NULL)
+#ifndef __SINGLE_THREAD__
+  __lock_acquire_recursive(__sigfunc_lock);
+#endif
+
+  if (_GLOBAL_REENT->_sig_func == NULL)
     {
-      ptr->_sig_func = (_sig_func_ptr *)_malloc_r (ptr, sizeof (_sig_func_ptr) * NSIG);
-      if (ptr->_sig_func == NULL)
+      _GLOBAL_REENT->_sig_func = (_sig_func_ptr *)_malloc_r (ptr, sizeof (_sig_func_ptr) * NSIG);
+      if (_GLOBAL_REENT->_sig_func == NULL) {
+
+#ifndef __SINGLE_THREAD__
+	__lock_release_recursive(__sigfunc_lock);
+#endif
 	return -1;
+      }
 
       for (i = 0; i < NSIG; i++)
-	ptr->_sig_func[i] = SIG_DFL;
+	_GLOBAL_REENT->_sig_func[i] = SIG_DFL;
     }
 
+#ifndef __SINGLE_THREAD__
+  __lock_release_recursive(__sigfunc_lock);
+#endif
+
   return 0;
 }
 
@@ -132,11 +148,25 @@ _DEFUN (_signal_r, (ptr, sig, func),
       return SIG_ERR;
     }
 
-  if (ptr->_sig_func == NULL && _init_signal_r (ptr) != 0)
+#ifndef __SINGLE_THREAD__
+  __lock_acquire_recursive(__sigfunc_lock);
+#endif
+
+  if (_GLOBAL_REENT->_sig_func == NULL && _init_signal_r (ptr) != 0) {
+
+#ifndef __SINGLE_THREAD__
+  __lock_release_recursive(__sigfunc_lock);
+#endif
+
     return SIG_ERR;
+  }
   
-  old_func = ptr->_sig_func[sig];
-  ptr->_sig_func[sig] = func;
+  old_func = _GLOBAL_REENT->_sig_func[sig];
+  _GLOBAL_REENT->_sig_func[sig] = func;
+
+#ifndef __SINGLE_THREAD__
+  __lock_release_recursive(__sigfunc_lock);
+#endif
 
   return old_func;
 }
@@ -154,10 +184,18 @@ _DEFUN (_raise_r, (ptr, sig),
       return -1;
     }
 
-  if (ptr->_sig_func == NULL)
+#ifndef __SINGLE_THREAD__
+  __lock_acquire_recursive(__sigfunc_lock);
+#endif
+
+  if (_GLOBAL_REENT->_sig_func == NULL)
     func = SIG_DFL;
   else
-    func = ptr->_sig_func[sig];
+    func = _GLOBAL_REENT->_sig_func[sig];
+
+#ifndef __SINGLE_THREAD__
+  __lock_release_recursive(__sigfunc_lock);
+#endif
 
   if (func == SIG_DFL)
     return _kill_r (ptr, _getpid_r (ptr), sig);
@@ -170,7 +208,17 @@ _DEFUN (_raise_r, (ptr, sig),
     }
   else
     {
-      ptr->_sig_func[sig] = SIG_DFL;
+
+#ifndef __SINGLE_THREAD__
+      __lock_acquire_recursive(__sigfunc_lock);
+#endif
+
+      _GLOBAL_REENT->_sig_func[sig] = SIG_DFL;
+
+#ifndef __SINGLE_THREAD__
+      __lock_release_recursive(__sigfunc_lock);
+#endif
+
       func (sig);
       return 0;
     }
@@ -188,10 +236,23 @@ _DEFUN (__sigtramp_r, (ptr, sig),
       return -1;
     }
 
-  if (ptr->_sig_func == NULL && _init_signal_r (ptr) != 0)
+#ifndef __SINGLE_THREAD__
+      __lock_acquire_recursive(__sigfunc_lock);
+#endif
+
+  if (_GLOBAL_REENT->_sig_func == NULL && _init_signal_r (ptr) != 0) {
+#ifndef __SINGLE_THREAD__
+    __lock_release_recursive(__sigfunc_lock);
+#endif
     return -1;
+  }
+
+  func = _GLOBAL_REENT->_sig_func[sig];
+
+#ifndef __SINGLE_THREAD__
+  __lock_release_recursive(__sigfunc_lock);
+#endif
 
-  func = ptr->_sig_func[sig];
   if (func == SIG_DFL)
     return 1;
   else if (func == SIG_ERR)
@@ -200,7 +261,14 @@ _DEFUN (__sigtramp_r, (ptr, sig),
     return 3;
   else
     {
-      ptr->_sig_func[sig] = SIG_DFL;
+#ifndef __SINGLE_THREAD__
+      __lock_acquire_recursive(__sigfunc_lock);
+#endif
+      _GLOBAL_REENT->_sig_func[sig] = SIG_DFL;
+#ifndef __SINGLE_THREAD__
+      __lock_release_recursive(__sigfunc_lock);
+#endif
+
       func (sig);
       return 0;
     }