catching scm_without_guile badness

catching scm_without_guile badness

[Andy Wingo]

Hi,

I just spent a couple days tracking down a bug in my code that was due
to calling scm_without_guile when I wasn't actually in Guile. This
exhibited itself as a deadlock at some point in the future when a
scm_without_guile that was lower on the stack exited, it tried to take
the heap lock when the first scm_without_guile had already,
unnecessarily taken it.

The solution of course is to not call scm_without_guile when you're not
in guile. (My problem was actually that I had failed to call
scm_with_guile when re-entering into some scheme code.) The following
patch aborts if suspend() is called, but the thread was not in guile
mode. Does it make sense to yall? I guess we should benchmark it, it's a
pretty hot spot.

Cheers,

Andy

diff --git a/libguile/threads.c b/libguile/threads.c
index 609fc99..e0d7e74 100644
--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -383,6 +383,9 @@ suspend (void)
 {
   scm_i_thread *t = SCM_I_CURRENT_THREAD;
 
+  if (t->top)
+    abort ();
+
   /* record top of stack for the GC */
   t->top = SCM_STACK_PTR (&t);
   /* save registers. */
@@ -459,6 +462,7 @@ guilify_self_1 (SCM_STACKITEM *base)
 
   scm_i_pthread_setspecific (scm_i_thread_key, t);
 
+  t->top = NULL;  
   scm_i_pthread_mutex_lock (&t->heap_mutex);
 
   scm_i_pthread_mutex_lock (&thread_admin_mutex);


-- 
http://wingolog.org/

Re: catching scm_without_guile badness

[Neil Jerram]

2008/7/31 Andy Wingo <wingo@pobox.com>:
> Hi,
>
> I just spent a couple days tracking down a bug in my code that was due
> to calling scm_without_guile when I wasn't actually in Guile. This
> exhibited itself as a deadlock at some point in the future when a
> scm_without_guile that was lower on the stack exited, it tried to take
> the heap lock when the first scm_without_guile had already,
> unnecessarily taken it.
>
> The solution of course is to not call scm_without_guile when you're not
> in guile. (My problem was actually that I had failed to call
> scm_with_guile when re-entering into some scheme code.) The following
> patch aborts if suspend() is called, but the thread was not in guile
> mode. Does it make sense to yall?

Well it does to me.  Assuming you now (or soon) have repo access, can
you proceed with committing this yourself?  (Both master and 1.8
branch, I would say.)

> I guess we should benchmark it, it's a
> pretty hot spot.

I don't think so.  I don't think one pointer reference will be
significant next to mutex locking.

Regards,
       Neil

Re: catching scm_without_guile badness

[Ludovic Courtès]

Hi Guilers!

"Neil Jerram" <neiljerram@googlemail.com> writes:

> 2008/7/31 Andy Wingo <wingo@pobox.com>:
>> Hi,
>>
>> I just spent a couple days tracking down a bug in my code that was due
>> to calling scm_without_guile when I wasn't actually in Guile. This
>> exhibited itself as a deadlock at some point in the future when a
>> scm_without_guile that was lower on the stack exited, it tried to take
>> the heap lock when the first scm_without_guile had already,
>> unnecessarily taken it.
>>
>> The solution of course is to not call scm_without_guile when you're not
>> in guile. (My problem was actually that I had failed to call
>> scm_with_guile when re-entering into some scheme code.) The following
>> patch aborts if suspend() is called, but the thread was not in guile
>> mode. Does it make sense to yall?
>
> Well it does to me.

+1.

>> I guess we should benchmark it, it's a
>> pretty hot spot.
>
> I don't think so.  I don't think one pointer reference will be
> significant next to mutex locking.

Indeed (not to mention the `setjmp ()' call).

Thanks,
Ludovic.