Trouble joining with threads from C

Trouble joining with threads from C

[Mark H Weaver]

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

Apologies in advance if I'm making a mistake here, but can someone
explain to me why the minimal attached example code fails to join with
its trivial spawned thread?

I might guess that it's because spawn_thread() in threads.c calls
scm_i_pthread_detach.  So does launch_thread().  Yet the documentation
of scm_join_thread seems to indicate that you ought to be able to join
with a thread created by scm_spawn_thread.

    Thanks,
      Mark



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Minimal example code --]
[-- Type: text/x-csrc, Size: 481 bytes --]

#include <libguile.h>

static SCM
thread_main (void *data)
{
  return SCM_BOOL_F;
}

static SCM
thread_handler (void *data, SCM key, SCM args)
{
  return SCM_BOOL_F;
}

static void *
inner_main (void *data)
{
  SCM thread = scm_spawn_thread (thread_main, 0, thread_handler, 0);
  SCM timeout = scm_from_unsigned_integer (time (NULL) + 20);
  scm_join_thread_timed (thread, timeout, SCM_BOOL_T);
}

int
main (int argc, char **argv)
{
  scm_with_guile (inner_main, 0);
  return 0;
}

Re: Trouble joining with threads from C

[Mark H Weaver]

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

Having further investigated, I'm convinced that this is a bug.

Attached are two minimal C programs.  Both create threads that do
nothing but sleep for 2 seconds and then exit.  The parent tries to join
with the child thread, with a timeout of 10 seconds.

The only difference is that test1 uses scm_spawn_thread, and test2 uses
scm_call_with_new_thread.  test1 always times out, and test2 works
properly.

Having looked at the code in threads.c, I'm surprised that either of
them work.  Both scm_spawn_thread and scm_call_with_new_thread arrange
for pthread_detach to be called in the child (in spawn_thread and
launch_thread, respectively).  This is supposed to make the child
un-joinable.

I've attached not only test1.c and test2.c, but also the diff
between them.  I compiled them as follows:

  gcc -o test1 test1.c $(pkg-config --cflags --libs guile-2.0)
  gcc -o test2 test2.c $(pkg-config --cflags --libs guile-2.0)

Any ideas?

   Thanks,
     Mark



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: test1.c, which uses scm_spawn_thread --]
[-- Type: text/x-csrc, Size: 920 bytes --]

#include <libguile.h>

static SCM
thread_main (void *data)
{
  /* scm_sleep (scm_from_signed_integer (20));  /* This should cause a timeout */
  /* scm_sleep (SCM_BOOL_T);                    /* This should throw an exception */
  scm_sleep (scm_from_signed_integer (2));      /* This should finish */
  return scm_from_latin1_string ("finished");
}

static SCM
thread_handler (void *data, SCM key, SCM args)
{
  return scm_from_latin1_string ("exception thrown");
}

static void *
inner_main (void *data)
{
  SCM thread = scm_spawn_thread (thread_main, 0, thread_handler, 0);
  SCM timeout = scm_sum (scm_current_time(), scm_from_signed_integer (10));
  SCM result = scm_join_thread_timed (thread, timeout, scm_from_latin1_string ("timed out"));
  scm_simple_format (SCM_BOOL_T, scm_from_latin1_string ("~A\n"), scm_list_1 (result));
}

int
main (int argc, char **argv)
{
  scm_with_guile (inner_main, 0);
  return 0;
}

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: test2.c, which uses scm_call_with_new_thread --]
[-- Type: text/x-csrc, Size: 1098 bytes --]

#include <libguile.h>

static SCM
thread_main (void)
{
  /* scm_sleep (scm_from_signed_integer (20));  /* This should cause a timeout */
  /* scm_sleep (SCM_BOOL_T);                    /* This should throw an exception */
  scm_sleep (scm_from_signed_integer (2));      /* This should finish */
  return scm_from_latin1_string ("finished");
}

static SCM
thread_handler (SCM handler, SCM key, SCM args)
{
  return scm_from_latin1_string ("exception thrown");
}

static void *
inner_main (void *data)
{
  SCM thread_main_proc = scm_c_make_gsubr ("thread-main", 0, 0, 0, thread_main);
  SCM thread_handler_proc = scm_c_make_gsubr ("thread-handler", 2, 0, 1, thread_handler);
  SCM thread = scm_call_with_new_thread (thread_main_proc, thread_handler_proc);
  SCM timeout = scm_sum (scm_current_time(), scm_from_signed_integer (10));
  SCM result = scm_join_thread_timed (thread, timeout, scm_from_latin1_string ("timed out"));
  scm_simple_format (SCM_BOOL_T, scm_from_latin1_string ("~A\n"), scm_list_1 (result));
}

int
main (int argc, char **argv)
{
  scm_with_guile (inner_main, 0);
  return 0;
}

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: Diff between test1.c and test2.c --]
[-- Type: text/x-diff, Size: 1207 bytes --]

--- test1.c	2011-03-09 01:33:36.000000000 -0500
+++ test2.c	2011-03-09 01:34:18.000000000 -0500
@@ -1,7 +1,7 @@
 #include <libguile.h>
 
 static SCM
-thread_main (void *data)
+thread_main (void)
 {
   /* scm_sleep (scm_from_signed_integer (20));  /* This should cause a timeout */
   /* scm_sleep (SCM_BOOL_T);                    /* This should throw an exception */
@@ -10,7 +10,7 @@
 }
 
 static SCM
-thread_handler (void *data, SCM key, SCM args)
+thread_handler (SCM handler, SCM key, SCM args)
 {
   return scm_from_latin1_string ("exception thrown");
 }
@@ -18,7 +18,9 @@
 static void *
 inner_main (void *data)
 {
+  SCM thread_main_proc = scm_c_make_gsubr ("thread-main", 0, 0, 0, thread_main);
+  SCM thread_handler_proc = scm_c_make_gsubr ("thread-handler", 2, 0, 1, thread_handler);
-  SCM thread = scm_spawn_thread (thread_main, 0, thread_handler, 0);
+  SCM thread = scm_call_with_new_thread (thread_main_proc, thread_handler_proc);
   SCM timeout = scm_sum (scm_current_time(), scm_from_signed_integer (10));
   SCM result = scm_join_thread_timed (thread, timeout, scm_from_latin1_string ("timed out"));
   scm_simple_format (SCM_BOOL_T, scm_from_latin1_string ("~A\n"), scm_list_1 (result));

Re: Trouble joining with threads from C

[Andy Wingo]

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

Hi Mark,

On Wed 09 Mar 2011 07:48, Mark H Weaver <mhw@netris.org> writes:

> Attached are two minimal C programs.  Both create threads that do
> nothing but sleep for 2 seconds and then exit.  The parent tries to join
> with the child thread, with a timeout of 10 seconds.
>
> The only difference is that test1 uses scm_spawn_thread, and test2 uses
> scm_call_with_new_thread.  test1 always times out, and test2 works
> properly.

Indeed, a bug.

> Having looked at the code in threads.c, I'm surprised that either of
> them work.  Both scm_spawn_thread and scm_call_with_new_thread arrange
> for pthread_detach to be called in the child (in spawn_thread and
> launch_thread, respectively).  This is supposed to make the child
> un-joinable.

scm_join_thread_timed doesn't use pthread_join.  I don't recall exactly
why, but its implementation does look OK, at least at first glance.

No, the issue is elsewhere, that the thread-exit handlers were not being
called, which makes me trust our pthreads support much less than before,
if this bug could exist for so long.  I have fixed this in Git with the
attached patch, which fixes your test as well.

Would you mind submitting (and committing :) a test case?  Adding it to
test-suite/standalone is probably the thing to do.

Regards,

Andy


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-fix-thread-cleanup.patch --]
[-- Type: text/x-patch, Size: 6719 bytes --]

From 4732679c36305b886cbb6bd3473154731535cb0b Mon Sep 17 00:00:00 2001
From: Andy Wingo <wingo@pobox.com>
Date: Fri, 18 Mar 2011 13:18:47 +0100
Subject: [PATCH] fix thread cleanup

* libguile/threads.h: Always declare a scm_i_thread_key, for cleanup
  purposes, in the BUILDING_LIBGUILE case.

* libguile/threads.c (scm_i_thread_key): Init with a cleanup handler, so
  any guile-specific info for a thread can be cleaned up reliably.
  (guilify_self_1): Always set the thread key.
  (do_thread_exit_trampoline, on_thread_exit): Enter guile-mode for the
  guile-mode cleanup handler, and trampoline through a
  gc_call_with_stack_base for reasons explained in the code.
  (init_thread_key, scm_i_init_thread_for_guile): Always init the key.
  (scm_i_with_guile_and_parent): No need for pthread_cancel cleanup
  handlers, as the pthread key destructor will take care of that for
  us.
  (really_launch): Remove needless pthread_exit call with incorrect
  comment.
---
 libguile/threads.c |   71 ++++++++++++++++++++++++++++-----------------------
 libguile/threads.h |    5 +++-
 2 files changed, 43 insertions(+), 33 deletions(-)

diff --git a/libguile/threads.c b/libguile/threads.c
index e7347ad..e2e17ac 100644
--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -343,6 +343,12 @@ unblock_from_queue (SCM queue)
 /* Getting into and out of guile mode.
  */
 
+/* Key used to attach a cleanup handler to a given thread.  Also, if
+   thread-local storage is unavailable, this key is used to retrieve the
+   current thread with `pthread_getspecific ()'.  */
+scm_i_pthread_key_t scm_i_thread_key;
+
+
 #ifdef SCM_HAVE_THREAD_STORAGE_CLASS
 
 /* When thread-local storage (TLS) is available, a pointer to the
@@ -352,17 +358,7 @@ unblock_from_queue (SCM queue)
    represent.  */
 SCM_THREAD_LOCAL scm_i_thread *scm_i_current_thread = NULL;
 
-# define SET_CURRENT_THREAD(_t)  scm_i_current_thread = (_t)
-
-#else /* !SCM_HAVE_THREAD_STORAGE_CLASS */
-
-/* Key used to retrieve the current thread with `pthread_getspecific ()'.  */
-scm_i_pthread_key_t scm_i_thread_key;
-
-# define SET_CURRENT_THREAD(_t)				\
-  scm_i_pthread_setspecific (scm_i_thread_key, (_t))
-
-#endif /* !SCM_HAVE_THREAD_STORAGE_CLASS */
+#endif /* SCM_HAVE_THREAD_STORAGE_CLASS */
 
 
 static scm_i_pthread_mutex_t thread_admin_mutex = SCM_I_PTHREAD_MUTEX_INITIALIZER;
@@ -428,7 +424,12 @@ guilify_self_1 (SCM_STACKITEM *base)
   t->exited = 0;
   t->guile_mode = 0;
 
-  SET_CURRENT_THREAD (t);
+  scm_i_pthread_setspecific (scm_i_thread_key, t);
+
+#ifdef SCM_HAVE_THREAD_STORAGE_CLASS
+  /* Cache the current thread in TLS for faster lookup.  */
+  scm_i_current_thread = t;
+#endif
 
   scm_i_pthread_mutex_lock (&thread_admin_mutex);
   t->next_thread = all_threads;
@@ -537,6 +538,22 @@ do_thread_exit (void *v)
   return NULL;
 }
 
+static void *
+do_thread_exit_trampoline (struct GC_stack_base *sb, void *v)
+{
+  void *ret;
+  int registered;
+
+  registered = GC_register_my_thread (sb);
+
+  ret = scm_with_guile (do_thread_exit, v);
+
+  if (registered == GC_SUCCESS)
+    GC_unregister_my_thread ();
+
+  return ret;
+}
+
 static void
 on_thread_exit (void *v)
 {
@@ -551,19 +568,20 @@ on_thread_exit (void *v)
       t->held_mutex = NULL;
     }
 
-  SET_CURRENT_THREAD (v);
+  /* Reinstate the current thread for purposes of scm_with_guile
+     guile-mode cleanup handlers.  Only really needed in the non-TLS
+     case but it doesn't hurt to be consistent.  */
+  scm_i_pthread_setspecific (scm_i_thread_key, t);
 
   /* Ensure the signal handling thread has been launched, because we might be
      shutting it down.  */
   scm_i_ensure_signal_delivery_thread ();
 
   /* Unblocking the joining threads needs to happen in guile mode
-     since the queue is a SCM data structure.  */
-
-  /* Note: Since `do_thread_exit ()' uses allocates memory via `libgc', we
-     assume the GC is usable at this point, and notably that thread-local
-     storage (TLS) hasn't been deallocated yet.  */
-  do_thread_exit (v);
+     since the queue is a SCM data structure.   Trampoline through
+     GC_call_with_stack_base so that the GC works even if it already
+     cleaned up for this thread.  */
+  GC_call_with_stack_base (do_thread_exit_trampoline, v);
 
   /* Removing ourself from the list of all threads needs to happen in
      non-guile mode since all SCM values on our stack become
@@ -590,21 +608,17 @@ on_thread_exit (void *v)
 
   scm_i_pthread_mutex_unlock (&thread_admin_mutex);
 
-  SET_CURRENT_THREAD (NULL);
+  scm_i_pthread_setspecific (scm_i_thread_key, NULL);
 }
 
-#ifndef SCM_HAVE_THREAD_STORAGE_CLASS
-
 static scm_i_pthread_once_t init_thread_key_once = SCM_I_PTHREAD_ONCE_INIT;
 
 static void
 init_thread_key (void)
 {
-  scm_i_pthread_key_create (&scm_i_thread_key, NULL);
+  scm_i_pthread_key_create (&scm_i_thread_key, on_thread_exit);
 }
 
-#endif
-
 /* Perform any initializations necessary to make the current thread
    known to Guile (via SCM_I_CURRENT_THREAD), initializing Guile itself,
    if necessary.
@@ -625,9 +639,7 @@ init_thread_key (void)
 static int
 scm_i_init_thread_for_guile (SCM_STACKITEM *base, SCM parent)
 {
-#ifndef SCM_HAVE_THREAD_STORAGE_CLASS
   scm_i_pthread_once (&init_thread_key_once, init_thread_key);
-#endif
 
   if (SCM_I_CURRENT_THREAD)
     {
@@ -790,9 +802,7 @@ scm_i_with_guile_and_parent (void *(*func)(void *), void *data, SCM parent)
       /* We are in Guile mode.  */
       assert (t->guile_mode);
 
-      scm_i_pthread_cleanup_push (scm_leave_guile_cleanup, NULL);
       res = scm_c_with_continuation_barrier (func, data);
-      scm_i_pthread_cleanup_pop (0);
 
       /* Leave Guile mode.  */
       t->guile_mode = 0;
@@ -880,9 +890,6 @@ really_launch (void *d)
   else
     t->result = scm_catch (SCM_BOOL_T, thunk, handler);
 
-  /* Trigger a call to `on_thread_exit ()'.  */
-  pthread_exit (NULL);
-
   return 0;
 }
 
diff --git a/libguile/threads.h b/libguile/threads.h
index b5e3c21..475af32 100644
--- a/libguile/threads.h
+++ b/libguile/threads.h
@@ -192,6 +192,10 @@ SCM_API void scm_dynwind_critical_section (SCM mutex);
 
 #ifdef BUILDING_LIBGUILE
 
+/* Though we don't need the key for SCM_I_CURRENT_THREAD if we have TLS,
+   we do use it for cleanup purposes.  */
+SCM_INTERNAL scm_i_pthread_key_t scm_i_thread_key;
+
 # ifdef SCM_HAVE_THREAD_STORAGE_CLASS
 
 SCM_INTERNAL SCM_THREAD_LOCAL scm_i_thread *scm_i_current_thread;
@@ -199,7 +203,6 @@ SCM_INTERNAL SCM_THREAD_LOCAL scm_i_thread *scm_i_current_thread;
 
 # else /* !SCM_HAVE_THREAD_STORAGE_CLASS */
 
-SCM_INTERNAL scm_i_pthread_key_t scm_i_thread_key;
 #  define SCM_I_CURRENT_THREAD						\
     ((scm_i_thread *) scm_i_pthread_getspecific (scm_i_thread_key))
 
-- 
1.7.3.4


[-- Attachment #3: Type: text/plain, Size: 25 bytes --]

-- 
http://wingolog.org/

Re: Trouble joining with threads from C

[Ludovic Courtès]

Hi!

Thanks for the brave threading debugging.  :-)

Andy Wingo <wingo@pobox.com> writes:

> No, the issue is elsewhere, that the thread-exit handlers were not being
> called

I just tried with 60582b7c2a495957012f9a20cd8691dc6307a850 and
‘on_thread_exit’ /is/ called after something like
‘(call-with-new-thread (lambda () #t))’.

Am I missing something?

> Would you mind submitting (and committing :) a test case?  Adding it to
> test-suite/standalone is probably the thing to do.

Yes, that’d be nice (even better in a single commit so we see the fix
and test are related.)

Thanks,
Ludo’.

Re: Trouble joining with threads from C

[Ludovic Courtès]

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

ludo@gnu.org (Ludovic Courtès) writes:

> Hi!

Howdy!

> Andy Wingo <wingo@pobox.com> writes:
>
>> No, the issue is elsewhere, that the thread-exit handlers were not being
>> called
>
> I just tried with 60582b7c2a495957012f9a20cd8691dc6307a850 and
> ‘on_thread_exit’ /is/ called after something like
> ‘(call-with-new-thread (lambda () #t))’.

The thread-exit handlers were not being called *for threads launched by
scm_spawn_thread*.

The patch below (against 60582b7c2a495957012f9a20cd8691dc6307a850) fixes it.

Now to see what's happened since then...

Ludo'.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: the patch --]
[-- Type: text/x-patch, Size: 2977 bytes --]

diff --git a/libguile/threads.c b/libguile/threads.c
index e7347ad..1c90517 100644
--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -980,7 +980,11 @@ spawn_thread (void *d)
 {
   spawn_data *data = (spawn_data *)d;
   scm_i_pthread_detach (scm_i_pthread_self ());
+
+  scm_i_pthread_cleanup_push (scm_leave_guile_cleanup, NULL);
   scm_i_with_guile_and_parent (really_spawn, d, data->parent);
+  scm_i_pthread_cleanup_pop (1);
+
   return NULL;
 }
 
diff --git a/test-suite/standalone/Makefile.am b/test-suite/standalone/Makefile.am
index b21edd2..b1f16f3 100644
--- a/test-suite/standalone/Makefile.am
+++ b/test-suite/standalone/Makefile.am
@@ -198,6 +198,11 @@ test_scm_with_guile_LDADD = $(LIBGUILE_LDADD)
 check_PROGRAMS += test-scm-with-guile
 TESTS += test-scm-with-guile
 
+test_scm_spawn_thread_CFLAGS = ${test_cflags}
+test_scm_spawn_thread_LDADD = $(LIBGUILE_LDADD)
+check_PROGRAMS += test-scm-spawn-thread
+TESTS += test-scm-spawn-thread
+
 else
 
 EXTRA_DIST += test-with-guile-module.c test-scm-with-guile.c
diff --git a/test-suite/standalone/test-scm-spawn-thread.c b/test-suite/standalone/test-scm-spawn-thread.c
new file mode 100644
index 0000000..b632ab0
--- /dev/null
+++ b/test-suite/standalone/test-scm-spawn-thread.c
@@ -0,0 +1,62 @@
+/* Copyright (C) 2011 Free Software Foundation, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 3 of
+ * the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+/* Test whether a thread created with `scm_spawn_thread' can be joined.
+   See <http://thread.gmane.org/gmane.lisp.guile.devel/11804> for the
+   original report.  */
+
+#ifdef HAVE_CONFIG_H
+# include <config.h>
+#endif
+
+#include <libguile.h>
+
+#include <time.h>
+#include <stdlib.h>
+
+static SCM
+thread_main (void *data)
+{
+  return SCM_BOOL_T;
+}
+
+static SCM
+thread_handler (void *data, SCM key, SCM args)
+{
+  return SCM_BOOL_T;
+}
+
+static void *
+inner_main (void *data)
+{
+  SCM thread, timeout;
+
+  thread = scm_spawn_thread (thread_main, 0, thread_handler, 0);
+  timeout = scm_from_unsigned_integer (time (NULL) + 10);
+  return (void *) scm_join_thread_timed (thread, timeout, SCM_BOOL_F);
+}
+
+\f
+int
+main (int argc, char **argv)
+{
+  SCM result;
+
+  result = PTR2SCM (scm_with_guile (inner_main, 0));
+  return scm_is_true (result) ? EXIT_SUCCESS : EXIT_FAILURE;
+}

Re: Trouble joining with threads from C

[Ludovic Courtès]

Hello,

ludo@gnu.org (Ludovic Courtès) writes:

> ludo@gnu.org (Ludovic Courtès) writes:
>> Andy Wingo <wingo@pobox.com> writes:
>>
>>> No, the issue is elsewhere, that the thread-exit handlers were not being
>>> called
>>
>> I just tried with 60582b7c2a495957012f9a20cd8691dc6307a850 and
>> ‘on_thread_exit’ /is/ called after something like
>> ‘(call-with-new-thread (lambda () #t))’.
>
> The thread-exit handlers were not being called *for threads launched by
> scm_spawn_thread*.
>
> The patch below (against 60582b7c2a495957012f9a20cd8691dc6307a850) fixes it.

After reviewing f60a7648d5926555c7760364a6fbb7dc0cf60720 (which
addressed the same issue), I lean towards reverting it and instead
applying the patch I just sent.

The problem I see with f60a7648d5926555c7760364a6fbb7dc0cf60720 is that
it re-introduces a pthread_key, even when using TLS, and make things a
bit complex IMO.

Thoughts?

Besides, reverting it may fix --without-threads builds.

Thanks,
Ludo’.

Re: Trouble joining with threads from C

[Andy Wingo]

Hi Ludovic,

On Wed 13 Apr 2011 23:34, ludo@gnu.org (Ludovic Courtès) writes:

> After reviewing f60a7648d5926555c7760364a6fbb7dc0cf60720 (which
> addressed the same issue), I lean towards reverting it and instead
> applying the patch I just sent.
>
> The problem I see with f60a7648d5926555c7760364a6fbb7dc0cf60720 is that
> it re-introduces a pthread_key, even when using TLS, and make things a
> bit complex IMO.

The issue is that threads in Guile are not always spawned by Guile.
It's true that of the two cases in which Guile spawned a thread, one of
them wasn't getting the cleanup handlers called, and your patch fixes
that; but that ignores the case of threads that are spawned by a user's
program.

For example in the following program:

    void* thread (void*)
    {
      scm_with_guile (do_something, NULL);
      scm_with_guile (do_something_else, NULL);
      return NULL;
    }

    int main ()
    {
      pthread_t thr;
      pthread_create (&thr, NULL, thread, NULL);
      pthread_join (thr, NULL);
      return 0;
    }

When do you propose that the cleanup handlers for the thread be called?

As far as I understand things, reliably cleaning up after the thread
*requires* the use of pthread_key with a destructor.  It is the only way
to attach a cleanup callback to a thread.  It's true that the key is not
used when TLS is available, except for its destructor capabilities, but
it is not more complicated than before.

I think they context that you are missing here is bug 32436.

It does appear that I have introduced some bugs here: Mark's
after-gc-hook not being called, and a failure to build
--without-threads.  For that I apologize.  But these bugs are fixable.
Consider bug 32436 for a moment: you can't fix that with
pthread_cleanup_push.

Does this explanation help?

Let me know,

Andy
-- 
http://wingolog.org/

Re: Trouble joining with threads from C

[Ludovic Courtès]

Hi Andy!

Thanks for your explanations.

Andy Wingo <wingo@pobox.com> writes:

> On Wed 13 Apr 2011 23:34, ludo@gnu.org (Ludovic Courtès) writes:
>
>> After reviewing f60a7648d5926555c7760364a6fbb7dc0cf60720 (which
>> addressed the same issue), I lean towards reverting it and instead
>> applying the patch I just sent.
>>
>> The problem I see with f60a7648d5926555c7760364a6fbb7dc0cf60720 is that
>> it re-introduces a pthread_key, even when using TLS, and make things a
>> bit complex IMO.
>
> The issue is that threads in Guile are not always spawned by Guile.
> It's true that of the two cases in which Guile spawned a thread, one of
> them wasn't getting the cleanup handlers called, and your patch fixes
> that; but that ignores the case of threads that are spawned by a user's
> program.

Right, I was just focusing on Mark’s case, blissfully ignoring #32436.

> For example in the following program:
>
>     void* thread (void*)
>     {
>       scm_with_guile (do_something, NULL);
>       scm_with_guile (do_something_else, NULL);
>       return NULL;
>     }
>
>     int main ()
>     {
>       pthread_t thr;
>       pthread_create (&thr, NULL, thread, NULL);
>       pthread_join (thr, NULL);
>       return 0;
>     }
>
> When do you propose that the cleanup handlers for the thread be called?

I’ll followup on this later, but currently it seems impossible to call
GC_INIT from a thread other than the initial thread [0].

Thanks,
Ludo’.

[0] http://article.gmane.org/gmane.comp.programming.garbage-collection.boehmgc/4490

Re: Trouble joining with threads from C

[Andy Wingo]

Heya Ludo,

On Fri 15 Apr 2011 23:44, ludo@gnu.org (Ludovic Courtès) writes:

>> For example in the following program:
>>
>>     void* thread (void*)
>>     {
>>       scm_with_guile (do_something, NULL);
>>       scm_with_guile (do_something_else, NULL);
>>       return NULL;
>>     }
>>
>>     int main ()
>>     {
>>       pthread_t thr;
>>       pthread_create (&thr, NULL, thread, NULL);
>>       pthread_join (thr, NULL);
>>       return 0;
>>     }
>>
>> When do you propose that the cleanup handlers for the thread be called?
>
> I’ll followup on this later, but currently it seems impossible to call
> GC_INIT from a thread other than the initial thread [0].

Interesting; I had it working at some point, in the context of that
other bug;  anyway, the same considerations hold if you scm_init_guile
in the main thread first.

Cheers,

Andy
-- 
http://wingolog.org/

Re: Trouble joining with threads from C

[Ludovic Courtès]

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

Hello!

Andy Wingo <wingo@pobox.com> writes:

> When do you propose that the cleanup handlers for the thread be called?
>
> As far as I understand things, reliably cleaning up after the thread
> *requires* the use of pthread_key with a destructor.  It is the only way
> to attach a cleanup callback to a thread.  It's true that the key is not
> used when TLS is available, except for its destructor capabilities, but
> it is not more complicated than before.
>
> I think they context that you are missing here is bug 32436.
>
> It does appear that I have introduced some bugs here: Mark's
> after-gc-hook not being called, and a failure to build
> --without-threads.  For that I apologize.  But these bugs are fixable.
> Consider bug 32436 for a moment: you can't fix that with
> pthread_cleanup_push.

Attached are 3 tests: one for Mark’s bug, and two for #32436.

Currently they all pass, but ‘test-scm-spawn-thread’ hits a libgc
assertion failure (“Duplicate large block deallocation”) once every 5
runs or so:

--8<---------------cut here---------------start------------->8---
(gdb) thread apply all bt

Thread 3 (Thread 0x7ffff582e700 (LWP 15207)):
#0  0x00007ffff62b220b in memset () from /nix/store/vxycd107wjbhcj720hzkw2px7s7kr724-glibc-2.12.2/lib/libc.so.6
#1  0x00007ffff6e8c6d8 in GC_generic_malloc (lb=524288, k=<value optimized out>) at ../malloc.c:193
#2  0x00007ffff7b36aad in make_vm () at vm.c:501
#3  0x00007ffff7b36b8e in scm_the_vm () at vm.c:578
#4  0x00007ffff7ac3190 in scm_call_4 (proc=0x778ab0, arg1=<value optimized out>, arg2=<value optimized out>, arg3=<value optimized out>, arg4=<value optimized out>) at eval.c:476
#5  0x00007ffff7aba283 in scm_i_with_continuation_barrier (body=0x7ffff7ab9a90 <c_body>, body_data=0x7ffff582dd80, handler=0x7ffff7ab9e60 <c_handler>, handler_data=0x7ffff582dd80,
    pre_unwind_handler=<value optimized out>, pre_unwind_handler_data=<value optimized out>) at continuations.c:450
#6  0x00007ffff7aba335 in scm_c_with_continuation_barrier (func=<value optimized out>, data=<value optimized out>) at continuations.c:546
#7  0x00007ffff7b3146a in with_guile_and_parent (base=0x7ffff582dde0, data=0x7ffff582de00) at threads.c:856
#8  0x00007ffff6e916b5 in GC_call_with_stack_base (fn=<value optimized out>, arg=<value optimized out>) at ../misc.c:1505
#9  0x00007ffff7b3109c in scm_i_with_guile_and_parent (d=<value optimized out>) at threads.c:899
#10 spawn_thread (d=<value optimized out>) at threads.c:1056
#11 0x00007ffff6e96791 in GC_inner_start_routine (sb=<value optimized out>, arg=<value optimized out>) at ../pthread_start.c:61
#12 0x00007ffff6e916b5 in GC_call_with_stack_base (fn=<value optimized out>, arg=<value optimized out>) at ../misc.c:1505
#13 0x00007ffff6c61cec in start_thread () from /nix/store/vxycd107wjbhcj720hzkw2px7s7kr724-glibc-2.12.2/lib/libpthread.so.0
#14 0x00007ffff63041ed in clone () from /nix/store/vxycd107wjbhcj720hzkw2px7s7kr724-glibc-2.12.2/lib/libc.so.6

Thread 2 (Thread 0x7ffff602f700 (LWP 15206)):
#0  0x00007ffff626585e in raise () from /nix/store/vxycd107wjbhcj720hzkw2px7s7kr724-glibc-2.12.2/lib/libc.so.6
#1  0x00007ffff6266d16 in abort () from /nix/store/vxycd107wjbhcj720hzkw2px7s7kr724-glibc-2.12.2/lib/libc.so.6
#2  0x00007ffff6e90d9f in GC_abort (msg=0x7ffff6e98b50 "Duplicate large block deallocation") at ../misc.c:1409
#3  0x00007ffff6e85b3c in GC_freehblk (hbp=0x980000) at ../allchblk.c:852
#4  0x00007ffff6e977cb in GC_pthread_create (new_thread=0x7ffff602ee38, attr=0x0, start_routine=0x7ffff7b31060 <spawn_thread>, arg=0x7ffff602edb0) at ../pthread_support.c:1552
#5  0x00007ffff7b31cbc in scm_spawn_thread (body=<value optimized out>, body_data=<value optimized out>, handler=0x7ffff7b33f20 <scm_handle_by_message>, handler_data=0x7ffff7b72868) at threads.c:1078
#6  0x00007ffff7b0e1cc in start_signal_delivery_thread () at scmsigs.c:193
#7  0x00007ffff6c67c83 in pthread_once () from /nix/store/vxycd107wjbhcj720hzkw2px7s7kr724-glibc-2.12.2/lib/libpthread.so.0
#8  0x00007ffff7b3115d in on_thread_exit (v=0x96a600) at threads.c:692
#9  0x00007ffff6c61b29 in __nptl_deallocate_tsd () from /nix/store/vxycd107wjbhcj720hzkw2px7s7kr724-glibc-2.12.2/lib/libpthread.so.0
#10 0x00007ffff6c61cfa in start_thread () from /nix/store/vxycd107wjbhcj720hzkw2px7s7kr724-glibc-2.12.2/lib/libpthread.so.0
#11 0x00007ffff63041ed in clone () from /nix/store/vxycd107wjbhcj720hzkw2px7s7kr724-glibc-2.12.2/lib/libc.so.6

Thread 1 (Thread 0x7ffff7ff3700 (LWP 15205)):
#0  0x00007ffff6c6692b in pthread_cond_timedwait@@GLIBC_2.3.2 () from /nix/store/vxycd107wjbhcj720hzkw2px7s7kr724-glibc-2.12.2/lib/libpthread.so.0
#1  0x00007ffff7b31e76 in scm_pthread_cond_timedwait (cond=0x662e88, mutex=0x96a628, wt=0x7fffffffcca0) at threads.c:1916
#2  0x00007ffff7b31fca in block_self (queue=0x981fc0, sleep_object=<value optimized out>, mutex=0x96a628, waittime=0x7fffffffcca0) at threads.c:431
#3  0x00007ffff7b321f9 in scm_join_thread_timed (thread=0x981ff0, timeout=<value optimized out>, timeoutval=<value optimized out>) at threads.c:1205
#4  0x00007ffff7ab9a9a in c_body (d=0x7fffffffcf30) at continuations.c:512
#5  0x00007ffff7b497b0 in vm_regular_engine (vm=0x6f38c0, program=0x7ffff7b72f72, argv=0x6f70a8, nargs=1) at vm-i-system.c:956
#6  0x00007ffff7ac31a3 in scm_call_4 (proc=0x778ab0, arg1=<value optimized out>, arg2=<value optimized out>, arg3=<value optimized out>, arg4=<value optimized out>) at eval.c:476
#7  0x00007ffff7aba283 in scm_i_with_continuation_barrier (body=0x7ffff7ab9a90 <c_body>, body_data=0x7fffffffcf30, handler=0x7ffff7ab9e60 <c_handler>, handler_data=0x7fffffffcf30,
    pre_unwind_handler=<value optimized out>, pre_unwind_handler_data=<value optimized out>) at continuations.c:450
#8  0x00007ffff7aba335 in scm_c_with_continuation_barrier (func=<value optimized out>, data=<value optimized out>) at continuations.c:546
#9  0x00007ffff7b3146a in with_guile_and_parent (base=0x7fffffffcf90, data=0x7fffffffcfb0) at threads.c:856
#10 0x00007ffff6e916b5 in GC_call_with_stack_base (fn=<value optimized out>, arg=<value optimized out>) at ../misc.c:1505
#11 0x00007ffff7b31598 in scm_i_with_guile_and_parent (func=<value optimized out>, data=<value optimized out>) at threads.c:899
#12 scm_with_guile (func=<value optimized out>, data=<value optimized out>) at threads.c:905
#13 0x00000000004008c0 in main (argc=<value optimized out>, argv=<value optimized out>) at test-scm-spawn-thread.c:60
--8<---------------cut here---------------end--------------->8---

This happens when calling ‘GC_pthread_create’ from a pthread key
destructor, which appears to be problematic [0].

I haven’t been able to fix it or otherwise work around it though, so
help welcome!  :-)

I think this is not a showstopper for 2.0.1.

Thanks,
Ludo’.

[0] http://thread.gmane.org/gmane.comp.programming.garbage-collection.boehmgc/4502


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: the tests --]
[-- Type: text/x-patch, Size: 8887 bytes --]

From 257ee9f4449c82c4cf9d3cd9951e48df549667fb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Fri, 22 Apr 2011 12:29:50 +0200
Subject: [PATCH] Add pthread-related tests.

* test-suite/standalone/Makefile.am (test_scm_spawn_thread_CFLAGS,
  test_scm_spawn_thread_LDADD, test_pthread_create_CFLAGS,
  test_pthread_create_LDADD, test_pthread_create_secondary_CFLAGS,
  test_pthread_create_secondary_LDADD): New variables.
  (check_PROGRAMS)[BUILD_PTHREAD_SUPPORT]: Add `test-scm-spawn-thread',
  `test-pthread_create', `test-pthread_create-secondary'.
  (TESTS)[BUILD_PTHREAD_SUPPORT]: Likewise.

* test-suite/standalone/test-scm-spawn-thread.c,
  test-suite/standalone/test-pthread-create.c,
  test-suite/standalone/test-pthread-create-secondary.c: New files.
---
 .gitignore                                         |    3 +
 test-suite/standalone/Makefile.am                  |   15 ++++
 .../standalone/test-pthread-create-secondary.c     |   85 ++++++++++++++++++++
 test-suite/standalone/test-pthread-create.c        |   63 +++++++++++++++
 test-suite/standalone/test-scm-spawn-thread.c      |   62 ++++++++++++++
 5 files changed, 228 insertions(+), 0 deletions(-)
 create mode 100644 test-suite/standalone/test-pthread-create-secondary.c
 create mode 100644 test-suite/standalone/test-pthread-create.c
 create mode 100644 test-suite/standalone/test-scm-spawn-thread.c

diff --git a/.gitignore b/.gitignore
index f24e589..a0eeeca 100644
--- a/.gitignore
+++ b/.gitignore
@@ -139,3 +139,6 @@ INSTALL
 /.sc-start-*
 /lib/math.h
 /lib/sys/time.h
+/test-suite/standalone/test-scm-spawn-thread
+/test-suite/standalone/test-pthread-create
+/test-suite/standalone/test-pthread-create-secondary
diff --git a/test-suite/standalone/Makefile.am b/test-suite/standalone/Makefile.am
index b21edd2..cf1fc4f 100644
--- a/test-suite/standalone/Makefile.am
+++ b/test-suite/standalone/Makefile.am
@@ -198,6 +198,21 @@ test_scm_with_guile_LDADD = $(LIBGUILE_LDADD)
 check_PROGRAMS += test-scm-with-guile
 TESTS += test-scm-with-guile
 
+test_scm_spawn_thread_CFLAGS = ${test_cflags}
+test_scm_spawn_thread_LDADD = $(LIBGUILE_LDADD)
+check_PROGRAMS += test-scm-spawn-thread
+TESTS += test-scm-spawn-thread
+
+test_pthread_create_CFLAGS = ${test_cflags}
+test_pthread_create_LDADD = $(LIBGUILE_LDADD)
+check_PROGRAMS += test-pthread-create
+TESTS += test-pthread-create
+
+test_pthread_create_secondary_CFLAGS = ${test_cflags} $(BDW_GC_CFLAGS)
+test_pthread_create_secondary_LDADD = $(LIBGUILE_LDADD)
+check_PROGRAMS += test-pthread-create-secondary
+TESTS += test-pthread-create-secondary
+
 else
 
 EXTRA_DIST += test-with-guile-module.c test-scm-with-guile.c
diff --git a/test-suite/standalone/test-pthread-create-secondary.c b/test-suite/standalone/test-pthread-create-secondary.c
new file mode 100644
index 0000000..fe39c2a
--- /dev/null
+++ b/test-suite/standalone/test-pthread-create-secondary.c
@@ -0,0 +1,85 @@
+/* Copyright (C) 2011 Free Software Foundation, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 3 of
+ * the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+/* Test whether threads created with `pthread_create' work, and whether
+   a secondary thread can call `scm_with_guile'. (bug #32436).  */
+
+#ifdef HAVE_CONFIG_H
+# include <config.h>
+#endif
+
+#include <pthread.h>
+#include <stdlib.h>
+#include <libguile.h>
+
+#include <gc/gc_version.h>
+
+
+/* Up to GC 7.2alpha5, calling `GC_INIT' from a secondary thread would
+   lead to a segfault.  This was fixed in BDW-GC on 2011-04-16 by Ivan
+   Maidanski.  See <http://thread.gmane.org/gmane.lisp.guile.bugs/5340>
+   for details.  */
+
+#if (GC_VERSION_MAJOR > 7)					\
+  || ((GC_VERSION_MAJOR == 7) && (GC_VERSION_MINOR > 2))	\
+  || ((GC_VERSION_MAJOR == 7) && (GC_VERSION_MINOR == 2)	\
+      && (GC_ALPHA_VERSION > 5))
+
+static void *
+do_something (void *arg)
+{
+  scm_list_copy (scm_make_list (scm_from_int (1234), SCM_BOOL_T));
+  scm_gc ();
+  return NULL;
+}
+
+static void *
+thread (void *arg)
+{
+  scm_with_guile (do_something, NULL);
+  return NULL;
+}
+
+\f
+int
+main (int argc, char *argv[])
+{
+  int i;
+
+  for (i = 0; i < 77; i++)
+    {
+      pthread_t thr;
+
+      pthread_create (&thr, NULL, thread, NULL);
+      pthread_join (thr, NULL);
+    }
+
+  return EXIT_SUCCESS;
+}
+
+\f
+#else /* GC < 7.2 */
+
+int
+main (int argc, char *argv[])
+{
+  /* Skip.  */
+  return 77;
+}
+
+#endif /* GC < 7.2 */
diff --git a/test-suite/standalone/test-pthread-create.c b/test-suite/standalone/test-pthread-create.c
new file mode 100644
index 0000000..8d9617b
--- /dev/null
+++ b/test-suite/standalone/test-pthread-create.c
@@ -0,0 +1,63 @@
+/* Copyright (C) 2011 Free Software Foundation, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 3 of
+ * the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+/* Test whether threads created with `pthread_create' work (bug #32436)
+   when then main thread is the one that initializes Guile.  */
+
+#ifdef HAVE_CONFIG_H
+# include <config.h>
+#endif
+
+#include <pthread.h>
+#include <stdlib.h>
+#include <libguile.h>
+
+static void *
+do_something (void *arg)
+{
+  scm_list_copy (scm_make_list (scm_from_int (1234), SCM_BOOL_T));
+  scm_gc ();
+  return NULL;
+}
+
+static void *
+thread (void *arg)
+{
+  scm_with_guile (do_something, NULL);
+  return NULL;
+}
+
+\f
+int
+main (int argc, char *argv[])
+{
+  int i;
+  pthread_t thr;
+
+  scm_init_guile ();
+
+  do_something (NULL);
+
+  for (i = 0; i < 77; i++)
+    {
+      pthread_create (&thr, NULL, thread, NULL);
+      pthread_join (thr, NULL);
+    }
+
+  return EXIT_SUCCESS;
+}
diff --git a/test-suite/standalone/test-scm-spawn-thread.c b/test-suite/standalone/test-scm-spawn-thread.c
new file mode 100644
index 0000000..b632ab0
--- /dev/null
+++ b/test-suite/standalone/test-scm-spawn-thread.c
@@ -0,0 +1,62 @@
+/* Copyright (C) 2011 Free Software Foundation, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 3 of
+ * the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+/* Test whether a thread created with `scm_spawn_thread' can be joined.
+   See <http://thread.gmane.org/gmane.lisp.guile.devel/11804> for the
+   original report.  */
+
+#ifdef HAVE_CONFIG_H
+# include <config.h>
+#endif
+
+#include <libguile.h>
+
+#include <time.h>
+#include <stdlib.h>
+
+static SCM
+thread_main (void *data)
+{
+  return SCM_BOOL_T;
+}
+
+static SCM
+thread_handler (void *data, SCM key, SCM args)
+{
+  return SCM_BOOL_T;
+}
+
+static void *
+inner_main (void *data)
+{
+  SCM thread, timeout;
+
+  thread = scm_spawn_thread (thread_main, 0, thread_handler, 0);
+  timeout = scm_from_unsigned_integer (time (NULL) + 10);
+  return (void *) scm_join_thread_timed (thread, timeout, SCM_BOOL_F);
+}
+
+\f
+int
+main (int argc, char **argv)
+{
+  SCM result;
+
+  result = PTR2SCM (scm_with_guile (inner_main, 0));
+  return scm_is_true (result) ? EXIT_SUCCESS : EXIT_FAILURE;
+}
-- 
1.7.4.1

Re: Trouble joining with threads from C

[Andy Wingo]

On Mon 25 Apr 2011 15:53, ludo@gnu.org (Ludovic Courtès) writes:

> Attached are 3 tests: one for Mark’s bug, and two for #32436.

o/~ did you ever know that you're my heeee-roooo o/~

> Currently they all pass, but ‘test-scm-spawn-thread’ hits a libgc
> assertion failure (“Duplicate large block deallocation”) once every 5
> runs or so:

I guess we disable this one then?

That code always seemed a little fishy to me:

  /* Ensure the signal handling thread has been launched, because we might be
     shutting it down.  */
  scm_i_ensure_signal_delivery_thread ();

Perhaps we can avoid spawning a thread in the key destructor.

Andy
-- 
http://wingolog.org/

Re: Trouble joining with threads from C

[Ludovic Courtès]

Hello!

Andy Wingo <wingo@pobox.com> writes:

> On Mon 25 Apr 2011 15:53, ludo@gnu.org (Ludovic Courtès) writes:

[...]

>> Currently they all pass, but ‘test-scm-spawn-thread’ hits a libgc
>> assertion failure (“Duplicate large block deallocation”) once every 5
>> runs or so:
>
> I guess we disable this one then?
>
> That code always seemed a little fishy to me:
>
>   /* Ensure the signal handling thread has been launched, because we might be
>      shutting it down.  */
>   scm_i_ensure_signal_delivery_thread ();
>
> Perhaps we can avoid spawning a thread in the key destructor.

I apparently found a fix: calling ‘scm_i_ensure_signal_delivery_thread’
in Guile mode.

I *think* the stack of ‘GC_pthread_create’ wasn’t scanned, so the area
pointed to by its variable ‘si’ would be reclaimed before the function
completes, leading to the “duplicate deallocation” error.

Pfffeww.

Thanks,
Ludo’.