Re: Locks and threads

[Neil Jerram]

Neil Jerram <address@hidden> writes:

> - first to address problems reported by helgrind (since I think we
>   should take advantage of external tools before adding debug code to
>   Guile internally)

Here's another lock ordering fix.  (For 1.8.x at least, I haven't
checked if it's relevant to master yet.)

1.8.x is then helgrind-clean (apart from one unimportant-looking
termination issue), so next it's on to the real problem, threadsafe
define.

        Neil

>From 588edc4cd1bfea7d51c9aed463e8119482e7a3f0 Mon Sep 17 00:00:00 2001
From: Neil Jerram <address@hidden>
Date: Wed, 4 Mar 2009 23:45:11 +0000
Subject: [PATCH] Lock ordering: don't allocate when in critical section 
(scm_sigaction_for_thread)

This fixes the following helgrind report.

Thread #1: lock order "0x4114748 before 0x4331084" violated
   at 0x40234F7: pthread_mutex_lock (hg_intercepts.c:408)
   by 0x407A55F: deval (eval.c:4077)
   by 0x407A071: scm_dapply (eval.c:5012)
   by 0x407888A: scm_apply (eval.c:4811)
   by 0x407E20C: scm_call_0 (eval.c:4666)
   by 0x406BDF7: scm_dynamic_wind (dynwind.c:111)
   by 0x407620C: ceval (eval.c:4571)
   by 0x407E8D9: scm_primitive_eval_x (eval.c:5921)
   by 0x407E934: scm_eval_x (eval.c:5956)
   by 0x40B9140: scm_shell (script.c:737)
   by 0x4095AC5: invoke_main_func (init.c:367)
   by 0x4065A81: c_body (continuations.c:349)
  Required order was established by acquisition of lock at 0x4114748
   at 0x40234F7: pthread_mutex_lock (hg_intercepts.c:408)
   by 0x40B7BE1: scm_sigaction_for_thread (scmsigs.c:339)
   by 0x40911DE: scm_gsubr_apply (gsubr.c:223)
   by 0x4079E36: scm_dapply (eval.c:4930)
   by 0x407AB68: deval (eval.c:4378)
   by 0x407A071: scm_dapply (eval.c:5012)
   by 0x407888A: scm_apply (eval.c:4811)
   by 0x407E1D0: scm_call_1 (eval.c:4672)
   by 0x407FEEE: scm_map (eval.c:5489)
   by 0x407BDCC: deval (eval.c:4367)
   by 0x407D197: deval (eval.c:3698)
   by 0x407A071: scm_dapply (eval.c:5012)
  followed by a later acquisition of lock at 0x4331084
   at 0x40234F7: pthread_mutex_lock (hg_intercepts.c:408)
   by 0x40DC397: scm_enter_guile (threads.c:377)
   by 0x40DC8F4: scm_pthread_mutex_lock (threads.c:1485)
   by 0x4083FAA: scm_gc_for_newcell (gc.c:484)
   by 0x40A9781: scm_cons (inline.h:115)
   by 0x4079867: scm_dapply (eval.c:4850)
   by 0x407888A: scm_apply (eval.c:4811)
   by 0x40796D6: scm_call_3 (eval.c:4684)
   by 0x409A7C2: module_variable (modules.c:302)
   by 0x409A7EE: module_variable (modules.c:312)
   by 0x409A962: scm_sym2var (modules.c:466)
   by 0x40738F4: scm_lookupcar1 (eval.c:2874)

* libguile/scmsigs.c (close_1): Renamed `handler_to_async'; also
  handle #f case and wrapping the async in a cons, if necessary.

  (install_handler): Pass in async instead of constructing it; combine
  two branches into one.

  (scm_sigaction_for_thread): Allocate async upfront instead of inside
  the critical section, and pass it to install_handler calls.  Leave
  critical section before signaling out-of-range error.
---
 libguile/scmsigs.c |   53 ++++++++++++++++++++++++++++-----------------------
 1 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/libguile/scmsigs.c b/libguile/scmsigs.c
index 9433273..e15bbf3 100644
--- a/libguile/scmsigs.c
+++ b/libguile/scmsigs.c
@@ -108,10 +108,20 @@ static SIGRETTYPE (*orig_handlers[NSIG])(int);
 #endif
 
 static SCM
-close_1 (SCM proc, SCM arg)
+handler_to_async (SCM handler, int signum)
 {
-  return scm_primitive_eval_x (scm_list_3 (scm_sym_lambda, SCM_EOL,
-                                          scm_list_2 (proc, arg)));
+  if (scm_is_false (handler))
+    return SCM_BOOL_F;
+  else
+    {
+      SCM async = scm_primitive_eval_x (scm_list_3 (scm_sym_lambda, SCM_EOL,
+                                                   scm_list_2 (handler,
+                                                               scm_from_int 
(signum))));
+#if !SCM_USE_PTHREAD_THREADS
+      async = scm_cons (async, SCM_BOOL_F);
+#endif
+      return async;
+    }
 }
 
 #if SCM_USE_PTHREAD_THREADS
@@ -239,23 +249,10 @@ ensure_signal_delivery_thread ()
 #endif /* !SCM_USE_PTHREAD_THREADS */
 
 static void
-install_handler (int signum, SCM thread, SCM handler)
+install_handler (int signum, SCM thread, SCM handler, SCM async)
 {
-  if (scm_is_false (handler))
-    {
-      SCM_SIMPLE_VECTOR_SET (*signal_handlers, signum, SCM_BOOL_F);
-      SCM_SIMPLE_VECTOR_SET (signal_handler_asyncs, signum, SCM_BOOL_F);
-    }
-  else
-    {
-      SCM async = close_1 (handler, scm_from_int (signum));
-#if !SCM_USE_PTHREAD_THREADS
-      async = scm_cons (async, SCM_BOOL_F);
-#endif
-      SCM_SIMPLE_VECTOR_SET (*signal_handlers, signum, handler);
-      SCM_SIMPLE_VECTOR_SET (signal_handler_asyncs, signum, async);
-    }
-
+  SCM_SIMPLE_VECTOR_SET (*signal_handlers, signum, handler);
+  SCM_SIMPLE_VECTOR_SET (signal_handler_asyncs, signum, async);
   SCM_SIMPLE_VECTOR_SET (signal_handler_threads, signum, thread);
 }
 
@@ -308,6 +305,7 @@ SCM_DEFINE (scm_sigaction_for_thread, "sigaction", 1, 3, 0,
   int save_handler = 0;
       
   SCM old_handler;
+  SCM async;
 
   csig = scm_to_signed_integer (signum, 0, NSIG-1);
 
@@ -334,6 +332,10 @@ SCM_DEFINE (scm_sigaction_for_thread, "sigaction", 1, 3, 0,
        SCM_MISC_ERROR ("thread has already exited", SCM_EOL);
     }
 
+  /* Allocate upfront, as we can't do it inside the critical
+     section. */
+  async = handler_to_async (handler, csig);
+
   ensure_signal_delivery_thread ();
 
   SCM_CRITICAL_SECTION_START;
@@ -351,10 +353,13 @@ SCM_DEFINE (scm_sigaction_for_thread, "sigaction", 1, 3, 
0,
 #else
          chandler = (SIGRETTYPE (*) (int)) handler_int;
 #endif
-         install_handler (csig, SCM_BOOL_F, SCM_BOOL_F);
+         install_handler (csig, SCM_BOOL_F, SCM_BOOL_F, async);
        }
       else
-       SCM_OUT_OF_RANGE (2, handler);
+       {
+         SCM_CRITICAL_SECTION_END;
+         SCM_OUT_OF_RANGE (2, handler);
+       }
     }
   else if (scm_is_false (handler))
     {
@@ -366,7 +371,7 @@ SCM_DEFINE (scm_sigaction_for_thread, "sigaction", 1, 3, 0,
        {
          action = orig_handlers[csig];
          orig_handlers[csig].sa_handler = SIG_ERR;
-         install_handler (csig, SCM_BOOL_F, SCM_BOOL_F);
+         install_handler (csig, SCM_BOOL_F, SCM_BOOL_F, async);
        }
 #else
       if (orig_handlers[csig] == SIG_ERR)
@@ -375,7 +380,7 @@ SCM_DEFINE (scm_sigaction_for_thread, "sigaction", 1, 3, 0,
        {
          chandler = orig_handlers[csig];
          orig_handlers[csig] = SIG_ERR;
-         install_handler (csig, SCM_BOOL_F, SCM_BOOL_F);
+         install_handler (csig, SCM_BOOL_F, SCM_BOOL_F, async);
        }
 #endif
     }
@@ -391,7 +396,7 @@ SCM_DEFINE (scm_sigaction_for_thread, "sigaction", 1, 3, 0,
       if (orig_handlers[csig] == SIG_ERR)
        save_handler = 1;
 #endif
-      install_handler (csig, thread, handler);
+      install_handler (csig, thread, handler, async);
     }
 
   /* XXX - Silently ignore setting handlers for `program error signals'
-- 
1.5.6.5