Re: Locks and threads

[Neil Jerram]

Neil Jerram <address@hidden> writes:

> next it's on to the real problem, threadsafe define.

I've been running Linas's define-race test program, and hitting the
`throw from within critical section' quite a lot.

We've already discussed this a couple of times:
http://www.mail-archive.com/address@hidden/msg04613.html
http://www.mail-archive.com/address@hidden/msg03016.html

Andy proposed just removing the check for 1.8.x, but I would prefer to
keep it if possible, as I think there probably still are genuine
problems where the check could help.  What about the patch below?

Thanks,
        Neil

>From 2daf18f36c6428cbc883c0eb1a381ffbba59614b Mon Sep 17 00:00:00 2001
From: Neil Jerram <address@hidden>
Date: Tue, 10 Mar 2009 23:55:31 +0000
Subject: [PATCH] Fix spurious `throw from within critical section' errors

The crux of this problem was that the thread doing a throw, and so
checking scm_i_critical_section_level, was different from the thread
that was in a critical section.

* libguile/async.h (scm_i_critical_section_level): Removed, replaced
  by per-thread critical_section_level.
  (SCM_CRITICAL_SECTION_START, SCM_CRITICAL_SECTION_END): Use
  per-thread critical_section_level.

* libguile/continuations.c (scm_dynthrow): Check per-thread
  critical_section_level.

* libguile/threads.c (guilify_self_1): Init per-thread
  critical_section_level.
  (scm_i_critical_section_level): Removed.

* libguile/threads.h (scm_i_thread): New critical_section_level field.

* libguile/throw.c (scm_ithrow): Check per-thread critical_section_level.
---
 libguile/async.h         |    8 +++-----
 libguile/continuations.c |    2 +-
 libguile/threads.c       |    2 +-
 libguile/threads.h       |    3 +++
 libguile/throw.c         |    2 +-
 5 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/libguile/async.h b/libguile/async.h
index a81a98d..3c1725f 100644
--- a/libguile/async.h
+++ b/libguile/async.h
@@ -58,20 +58,18 @@ void scm_dynwind_unblock_asyncs (void);
    the manual.
 */
 
-/* Defined in threads.c.  scm_i_critical_section_level is only used
-   for error checking and will go away eventually. */
+/* Defined in threads.c. */
 extern scm_i_pthread_mutex_t scm_i_critical_section_mutex;
-extern int scm_i_critical_section_level;
 
 #define SCM_CRITICAL_SECTION_START \
   do { \
     scm_i_pthread_mutex_lock (&scm_i_critical_section_mutex);\
     SCM_I_CURRENT_THREAD->block_asyncs++; \
-    scm_i_critical_section_level++; \
+    SCM_I_CURRENT_THREAD->critical_section_level++; \
   } while (0)
 #define SCM_CRITICAL_SECTION_END \
   do { \
-    scm_i_critical_section_level--; \
+    SCM_I_CURRENT_THREAD->critical_section_level--; \
     SCM_I_CURRENT_THREAD->block_asyncs--; \
     scm_i_pthread_mutex_unlock (&scm_i_critical_section_mutex); \
     scm_async_click ();        \
diff --git a/libguile/continuations.c b/libguile/continuations.c
index 74bb911..69d2569 100644
--- a/libguile/continuations.c
+++ b/libguile/continuations.c
@@ -256,7 +256,7 @@ scm_dynthrow (SCM cont, SCM val)
   SCM_STACKITEM *dst = thread->continuation_base;
   SCM_STACKITEM stack_top_element;
 
-  if (scm_i_critical_section_level)
+  if (thread->critical_section_level)
     {
       fprintf (stderr, "continuation invoked from within critical section.\n");
       abort ();
diff --git a/libguile/threads.c b/libguile/threads.c
index 05e01e3..fc3e607 100644
--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -422,6 +422,7 @@ guilify_self_1 (SCM_STACKITEM *base)
   t->active_asyncs = SCM_EOL;
   t->block_asyncs = 1;
   t->pending_asyncs = 1;
+  t->critical_section_level = 0;
   t->last_debug_frame = NULL;
   t->base = base;
 #ifdef __ia64__
@@ -1667,7 +1668,6 @@ scm_i_thread_sleep_for_gc ()
 /* This mutex is used by SCM_CRITICAL_SECTION_START/END.
  */
 scm_i_pthread_mutex_t scm_i_critical_section_mutex;
-int scm_i_critical_section_level = 0;
 
 static SCM dynwind_critical_section_mutex;
 
diff --git a/libguile/threads.h b/libguile/threads.h
index 9417b88..2b0e067 100644
--- a/libguile/threads.h
+++ b/libguile/threads.h
@@ -113,6 +113,9 @@ typedef struct scm_i_thread {
   scm_t_contregs *pending_rbs_continuation;
 #endif
 
+  /* Whether this thread is in a critical section. */
+  int critical_section_level;
+
 } scm_i_thread;
 
 #define SCM_I_IS_THREAD(x)    SCM_SMOB_PREDICATE (scm_tc16_thread, x)
diff --git a/libguile/throw.c b/libguile/throw.c
index 7e2803f..92c5a1a 100644
--- a/libguile/throw.c
+++ b/libguile/throw.c
@@ -692,7 +692,7 @@ scm_ithrow (SCM key, SCM args, int noreturn SCM_UNUSED)
   SCM dynpair = SCM_UNDEFINED;
   SCM winds;
 
-  if (scm_i_critical_section_level)
+  if (SCM_I_CURRENT_THREAD->critical_section_level)
     {
       fprintf (stderr, "throw from within critical section.\n");
       abort ();
-- 
1.5.6.5