From 636333d3b26cba21a7c8ed6afdbc5de5712dd73e Mon Sep 17 00:00:00 2001
From: Neil Jerram <neil@ossau.uklinux.net>
Date: Thu, 15 Jan 2009 22:36:28 +0000
Subject: [PATCH] Don't try to unlock already unlocked heap mutex

For each thread that goes into Guile mode, Guile pushes a cleanup
function, scm_leave_guile_cleanup, whose purpose is to execute
`scm_leave_guile ()' if the thread is terminated while in Guile mode.
The problem is that there are various places - like
scm_pthread_cond_wait, scm_without_guile and scm_std_select - where
the thread temporarily leaves Guile mode (which means unlocking the
heap mutex), and the cleanup function is still in place.  Therefore if
the thread is terminated at these places, the cleanup function ends up
trying to unlock a mutex (the heap mutex) which isn't actually locked.

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

* libguile/threads.c (scm_enter_guile): Set heap_mutex_locked_by_self.
  (scm_leave_guile): Only unlock if heap_mutex_locked_by_self is 1.
  (guilify_self_1): Initialize heap_mutex_locked_by_self.
---
 libguile/threads.c |   18 +++++++++++++++---
 libguile/threads.h |    7 +++++++
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/libguile/threads.c b/libguile/threads.c
index 1d497e7..27aad3d 100644
--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -409,6 +409,7 @@ scm_enter_guile (scm_t_guile_ticket ticket)
   if (t)
     {
       scm_i_pthread_mutex_lock (&t->heap_mutex);
+      t->heap_mutex_locked_by_self = 1;
       resume (t);
     }
 }
@@ -430,7 +431,11 @@ static scm_t_guile_ticket
 scm_leave_guile ()
 {
   scm_i_thread *t = suspend ();
-  scm_i_pthread_mutex_unlock (&t->heap_mutex);
+  if (t->heap_mutex_locked_by_self)
+    {
+      t->heap_mutex_locked_by_self = 0;
+      scm_i_pthread_mutex_unlock (&t->heap_mutex);
+    }
   return (scm_t_guile_ticket) t;
 }
 
@@ -491,6 +496,7 @@ guilify_self_1 (SCM_STACKITEM *base)
     abort ();
 
   scm_i_pthread_mutex_init (&t->heap_mutex, NULL);
+  t->heap_mutex_locked_by_self = 0;
   scm_i_pthread_mutex_init (&t->admin_mutex, NULL);
   t->clear_freelists_p = 0;
   t->gc_running_p = 0;
@@ -505,6 +511,7 @@ guilify_self_1 (SCM_STACKITEM *base)
   scm_i_pthread_setspecific (scm_i_thread_key, t);
 
   scm_i_pthread_mutex_lock (&t->heap_mutex);
+  t->heap_mutex_locked_by_self = 1;
 
   scm_i_pthread_mutex_lock (&thread_admin_mutex);
   t->next_thread = all_threads;
@@ -1992,9 +1999,14 @@ void
 scm_i_thread_sleep_for_gc ()
 {
   scm_i_thread *t = suspend ();
-  t->held_mutex = &t->heap_mutex;
+
+  /* Don't put t->heap_mutex in t->held_mutex here, because if the
+     thread is cancelled during the cond wait, the thread's cleanup
+     function (scm_leave_guile_cleanup) will handle unlocking the
+     heap_mutex, so we don't need to do that again in on_thread_exit.
+  */
   scm_i_pthread_cond_wait (&wake_up_cond, &t->heap_mutex);
-  t->held_mutex = NULL;
+
   resume (t);
 }
 
diff --git a/libguile/threads.h b/libguile/threads.h
index cbff648..66ddb6a 100644
--- a/libguile/threads.h
+++ b/libguile/threads.h
@@ -72,6 +72,13 @@ typedef struct scm_i_thread {
   */
   scm_i_pthread_mutex_t heap_mutex;
 
+  /* Boolean tracking whether the above mutex is currently locked by
+     this thread.  This is equivalent to whether or not the thread is
+     in "Guile mode".  This field doesn't need any protection because
+     it is only ever set or tested by the owning thread.
+  */
+  int heap_mutex_locked_by_self;
+
   /* The freelists of this thread.  Each thread has its own lists so
      that they can all allocate concurrently.
   */
-- 
1.5.6.5