Terrific Dead Lock

Terrific Dead Lock

[Ludovic Courtès]

Hello,

I'm experiencing a dead lock while running the test suite (in a NixOS
build), and I don't remember ever seeing it before.  Sorry for the long
copy/paste, but it helped me understand the problem as I was writing
this message.

Here we go:

(gdb) info threads 
* 3 Thread 0x40b70b90 (LWP 6675)  0xffffe410 in ?? ()
  2 Thread 0x416d3b90 (LWP 6853)  0xffffe410 in ?? ()
  1 Thread 0x402da8d0 (LWP 5049)  0xffffe410 in ?? ()

(gdb) thread 1
[Switching to thread 1 (Thread 0x402da8d0 (LWP 5049))]#0  0xffffe410 in ?? ()
(gdb) bt
#0  0xffffe410 in ?? ()
#1  0xbfbc3e58 in ?? ()
#2  0x00000002 in ?? ()
#3  0x00000080 in ?? ()
#4  0x401912b9 in __lll_lock_wait () from /nix/store/zahfcxzylmadvaj865j5xmm1dsvs03r7-glibc-2.7/lib/libpthread.so.0
#5  0x4018c9d6 in _L_lock_95 () from /nix/store/zahfcxzylmadvaj865j5xmm1dsvs03r7-glibc-2.7/lib/libpthread.so.0
#6  0x4018c3ba in pthread_mutex_lock () from /nix/store/zahfcxzylmadvaj865j5xmm1dsvs03r7-glibc-2.7/lib/libpthread.so.0
#7  0x400bb6fb in scm_i_thread_put_to_sleep () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#8  0x40069159 in scm_i_gc () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#9  0x4006afbe in increase_mtrigger () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#10 0x4009d8be in scm_make_srcprops () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#11 0x400977d9 in scm_read_sexp () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#12 0x4009672f in scm_read_expression () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#13 0x40097622 in scm_read_sexp () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#14 0x4009672f in scm_read_expression () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#15 0x4009769e in scm_read_sexp () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#16 0x4009672f in scm_read_expression () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#17 0x4009769e in scm_read_sexp () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#18 0x4009672f in scm_read_expression () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#19 0x4007d8da in scm_primitive_load () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#20 0x40062ed3 in ceval () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#21 0x4004dc2b in scm_start_stack () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#22 0x4004e3a1 in scm_m_start_stack () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#23 0x4005cb71 in scm_apply () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#24 0x40061a15 in ceval () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#25 0x400617bd in scm_call_0 () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#26 0x400664ad in apply_thunk () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#27 0x4006668e in scm_c_with_fluid () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#28 0x400666e5 in scm_with_fluid () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#29 0x40062093 in ceval () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#30 0x400617bd in scm_call_0 () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#31 0x40051e98 in scm_dynamic_wind () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#32 0x40062093 in ceval () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#33 0x400617bd in scm_call_0 () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#34 0x400664ad in apply_thunk () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#35 0x4006668e in scm_c_with_fluid () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#36 0x400666e5 in scm_with_fluid () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#37 0x40062093 in ceval () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#38 0x40064bb6 in call_closure_1 () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#39 0x4005d48e in scm_for_each () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#40 0x40062eba in ceval () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#41 0x40063156 in ceval () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#42 0x40063a79 in ceval () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#43 0x400648da in scm_primitive_eval_x () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#44 0x40064935 in scm_eval_x () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#45 0x4009a021 in scm_shell () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#46 0x4007a546 in invoke_main_func () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#47 0x4004c492 in c_body () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#48 0x400bdbd9 in scm_c_catch () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#49 0x4004ca02 in scm_i_with_continuation_barrier () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#50 0x4004cae3 in scm_c_with_continuation_barrier () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#51 0x400bcd79 in scm_i_with_guile_and_parent () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#52 0x400bce6e in scm_with_guile () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#53 0x4007a4df in scm_boot_guile () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#54 0x08048a06 in main ()

(gdb) thread 2
[Switching to thread 2 (Thread 0x416d3b90 (LWP 6853))]#0  0xffffe410 in ?? ()
(gdb) bt
#0  0xffffe410 in ?? ()
#1  0x416d31a8 in ?? ()
#2  0x00000002 in ?? ()
#3  0x00000080 in ?? ()
#4  0x401912b9 in __lll_lock_wait () from /nix/store/zahfcxzylmadvaj865j5xmm1dsvs03r7-glibc-2.7/lib/libpthread.so.0
#5  0x4018c9e4 in _L_lock_236 () from /nix/store/zahfcxzylmadvaj865j5xmm1dsvs03r7-glibc-2.7/lib/libpthread.so.0
#6  0x4018c43b in pthread_mutex_lock () from /nix/store/zahfcxzylmadvaj865j5xmm1dsvs03r7-glibc-2.7/lib/libpthread.so.0
#7  0x400bdbed in scm_c_catch () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#8  0x4004ca02 in scm_i_with_continuation_barrier () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#9  0x4004cae3 in scm_c_with_continuation_barrier () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#10 0x400bcd79 in scm_i_with_guile_and_parent () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#11 0x400bce6e in scm_with_guile () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#12 0x400bcec3 in on_thread_exit () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#13 0x40189dc0 in __nptl_deallocate_tsd () from /nix/store/zahfcxzylmadvaj865j5xmm1dsvs03r7-glibc-2.7/lib/libpthread.so.0
#14 0x4018a189 in start_thread () from /nix/store/zahfcxzylmadvaj865j5xmm1dsvs03r7-glibc-2.7/lib/libpthread.so.0
#15 0x40264dae in clone () from /nix/store/zahfcxzylmadvaj865j5xmm1dsvs03r7-glibc-2.7/lib/libc.so.6

(gdb) thread 3
[Switching to thread 3 (Thread 0x40b70b90 (LWP 6675))]#0  0xffffe410 in ?? ()
(gdb) bt
#0  0xffffe410 in ?? ()
#1  0x40b6ff78 in ?? ()
#2  0x00000001 in ?? ()
#3  0x40b7005b in ?? ()
#4  0x401916cb in read () from /nix/store/zahfcxzylmadvaj865j5xmm1dsvs03r7-glibc-2.7/lib/libpthread.so.0
#5  0x400988f3 in do_read_without_guile () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#6  0x400bb7cc in scm_without_guile () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#7  0x40098855 in signal_delivery_thread () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#8  0x400bdbd9 in scm_c_catch () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#9  0x400bdde9 in scm_internal_catch () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#10 0x400bca4d in really_spawn () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#11 0x4004c492 in c_body () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#12 0x400bdbd9 in scm_c_catch () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#13 0x4004ca02 in scm_i_with_continuation_barrier () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#14 0x4004cae3 in scm_c_with_continuation_barrier () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#15 0x400bcd79 in scm_i_with_guile_and_parent () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#16 0x400bcddf in spawn_thread () from /tmp/nix-5221-14/guile-1.8.4/libguile/.libs/libguile.so.17
#17 0x4018a17b in start_thread () from /nix/store/zahfcxzylmadvaj865j5xmm1dsvs03r7-glibc-2.7/lib/libpthread.so.0
#18 0x40264dae in clone () from /nix/store/zahfcxzylmadvaj865j5xmm1dsvs03r7-glibc-2.7/lib/libc.so.6

All this happens apparently while reading `unif.test' (which comes right
after `time.test'):

$ sudo tail -n 3 /tmp/nix-5221-14/guile-1.8.4/check-guile.log 
PASS: time.test: strptime: in another thread after error
PASS: time.test: strptime: GNU %s format: gmtoff on GMT
PASS: time.test: strptime: GNU %s format: gmtoff on EST+5


To summarize:

  * Thread 2 is exiting.  It holds THREAD_ADMIN_MUTEX (it acquired it at
    the beginning of `do_thread_exit ()') and is waiting on
    SCM_I_CRITICAL_SECTION_MUTEX in `scm_c_catch ()'.

  * Thread 1 is reading, actually GC'ing.  It's trying to acquire
    THREAD_ADMIN_MUTEX in `scm_i_thread_put_to_sleep ()'.  It holds
    SCM_I_CRITICAL_SECTION_MUTEX from `scm_make_srcprops ()'.
    
One might wonder: why the heck does `scm_make_srcprops ()' enter a
critical section?  Could it just use a private mutex to protect accesses
to `srcprops_freelist'?

Han-Wen's reimplementation of it in HEAD (2007-01-19) doesn't use a
critical section, nor a mutex, but is thread-safe AFAIUI.

Two possibilities to fix it:

  1. Copy `srcprop.[ch]' and `eval.c' bits from HEAD to 1.8.  After all,
     it's probably solid enough (I use almost only HEAD).  See [0] for
     an overview of the initial patch.  It doesn't break the public API
     nor the ABI, but it (re)moves stuff from the `srcprop.h'.

  2. Remove the critical section from 1.8 and synchronize accesses to
     `srcprops_freelist' with a private mutex, assuming that's a correct
     fix.

I'd be in favor of the first approach.

Comments?

Thanks,
Ludovic.

[0] http://thread.gmane.org/gmane.lisp.guile.devel/6439

Re: Terrific Dead Lock

[Ludovic Courtès]

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

Hi,

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

> Two possibilities to fix it:
>
>   1. Copy `srcprop.[ch]' and `eval.c' bits from HEAD to 1.8.  After all,
>      it's probably solid enough (I use almost only HEAD).  See [0] for
>      an overview of the initial patch.  It doesn't break the public API
>      nor the ABI, but it (re)moves stuff from the `srcprop.h'.
>
>   2. Remove the critical section from 1.8 and synchronize accesses to
>      `srcprops_freelist' with a private mutex, assuming that's a correct
>      fix.

The second approach is actually wrong: we'd need to acquire a mutex in
`srcprops_free ()' to synchronize accesses to `srcprops_freelist', but
we can't do it since `srcprops_free ()' is called during GC---which is
why there was a critical section in the first place I suppose.

Thus the first approach seems unavoidable.  Attached is the exact patch.
It removes non-public macros and data types from `srcprop.h', which is
acceptable IMO.

OK to apply?

Thanks,
Ludovic.


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

Index: libguile/eval.c
===================================================================
RCS file: /sources/guile/guile/guile-core/libguile/eval.c,v
retrieving revision 1.405.2.13
diff -u -r1.405.2.13 eval.c
--- libguile/eval.c	10 Mar 2008 22:13:33 -0000	1.405.2.13
+++ libguile/eval.c	13 Mar 2008 22:42:30 -0000
@@ -3039,7 +3039,7 @@
 do { \
   SCM_SET_ARGSREADY (debug);\
   if (scm_check_apply_p && SCM_TRAPS_P)\
-    if (SCM_APPLY_FRAME_P || (SCM_TRACE_P && PROCTRACEP (proc)))\
+    if (SCM_APPLY_FRAME_P || (SCM_TRACE_P && SCM_PROCTRACEP (proc)))\
       {\
 	SCM tmp, tail = scm_from_bool(SCM_TRACED_FRAME_P (debug)); \
 	SCM_SET_TRACED_FRAME (debug); \
Index: libguile/srcprop.c
===================================================================
RCS file: /sources/guile/guile/guile-core/libguile/srcprop.c,v
retrieving revision 1.73.2.1
diff -u -r1.73.2.1 srcprop.c
--- libguile/srcprop.c	12 Feb 2006 13:42:51 -0000	1.73.2.1
+++ libguile/srcprop.c	13 Mar 2008 22:42:30 -0000
@@ -37,7 +37,7 @@
 /* {Source Properties}
  *
  * Properties of source list expressions.
- * Five of these have special meaning and optimized storage:
+ * Five of these have special meaning:
  *
  * filename    string   The name of the source file.
  * copy        list     A copy of the list expression.
@@ -55,29 +55,47 @@
 SCM_GLOBAL_SYMBOL (scm_sym_column, "column");
 SCM_GLOBAL_SYMBOL (scm_sym_breakpoint, "breakpoint");
 
-scm_t_bits scm_tc16_srcprops;
-static scm_t_srcprops_chunk *srcprops_chunklist = 0;
-static scm_t_srcprops *srcprops_freelist = 0;
 
 
+/*
+ *  Source properties are stored as double cells with the
+ *  following layout:
+  
+ * car = tag
+ * cbr = pos
+ * ccr = copy
+ * cdr = plist 
+ */
+
+#define SRCPROPSP(p) (SCM_SMOB_PREDICATE (scm_tc16_srcprops, (p)))
+#define SRCPROPBRK(p) (SCM_SMOB_FLAGS (p) & SCM_SOURCE_PROPERTY_FLAG_BREAK)
+#define SRCPROPPOS(p) (SCM_CELL_WORD(p,1))
+#define SRCPROPLINE(p) (SRCPROPPOS(p) >> 12)
+#define SRCPROPCOL(p) (SRCPROPPOS(p) & 0x0fffL)
+#define SRCPROPCOPY(p) (SCM_CELL_OBJECT(p,2))
+#define SRCPROPPLIST(p) (SCM_CELL_OBJECT_3(p))
+#define SETSRCPROPBRK(p) \
+ (SCM_SET_SMOB_FLAGS ((p), \
+                      SCM_SMOB_FLAGS (p) | SCM_SOURCE_PROPERTY_FLAG_BREAK))
+#define CLEARSRCPROPBRK(p)  \
+ (SCM_SET_SMOB_FLAGS ((p), \
+                      SCM_SMOB_FLAGS (p) & ~SCM_SOURCE_PROPERTY_FLAG_BREAK))
+#define SRCPROPMAKPOS(l, c) (((l) << 12) + (c))
+#define SETSRCPROPPOS(p, l, c) (SCM_SET_CELL_WORD(p,1, SRCPROPMAKPOS (l, c)))
+#define SETSRCPROPLINE(p, l) SETSRCPROPPOS (p, l, SRCPROPCOL (p))
+#define SETSRCPROPCOL(p, c) SETSRCPROPPOS (p, SRCPROPLINE (p), c)
+
+
+
+scm_t_bits scm_tc16_srcprops;
+
 static SCM
 srcprops_mark (SCM obj)
 {
-  scm_gc_mark (SRCPROPFNAME (obj));
   scm_gc_mark (SRCPROPCOPY (obj));
   return SRCPROPPLIST (obj);
 }
 
-
-static size_t
-srcprops_free (SCM obj)
-{
-  *((scm_t_srcprops **) SCM_SMOB_DATA (obj)) = srcprops_freelist;
-  srcprops_freelist = (scm_t_srcprops *) SCM_SMOB_DATA (obj);
-  return 0; /* srcprops_chunks are not freed until leaving guile */
-}
-
-
 static int
 srcprops_print (SCM obj, SCM port, scm_print_state *pstate)
 {
@@ -99,38 +117,45 @@
 }
 
 
+/*
+ * We remember the last file name settings, so we can share that plist
+ * entry.  This works because scm_set_source_property_x does not use
+ * assoc-set! for modifying the plist.
+ *
+ * This variable contains a protected cons, whose cdr is the cached
+ * plist
+ */
+static SCM scm_last_plist_filename;
+
 SCM
 scm_make_srcprops (long line, int col, SCM filename, SCM copy, SCM plist)
 {
-  register scm_t_srcprops *ptr;
-  SCM_CRITICAL_SECTION_START;
-  if ((ptr = srcprops_freelist) != NULL)
-    srcprops_freelist = *(scm_t_srcprops **)ptr;
-  else
+  if (!SCM_UNBNDP (filename))
     {
-      size_t i;
-      scm_t_srcprops_chunk *mem;
-      size_t n = sizeof (scm_t_srcprops_chunk)
-	            + sizeof (scm_t_srcprops) * (SRCPROPS_CHUNKSIZE - 1);
-      SCM_SYSCALL (mem = (scm_t_srcprops_chunk *) scm_malloc (n));
-      if (mem == NULL)
-	scm_memory_error ("srcprops");
-      scm_gc_register_collectable_memory (mem, n, "srcprops");
-      
-      mem->next = srcprops_chunklist;
-      srcprops_chunklist = mem;
-      ptr = &mem->srcprops[0];
-      for (i = 1; i < SRCPROPS_CHUNKSIZE - 1; ++i)
-	*(scm_t_srcprops **)&ptr[i] = &ptr[i + 1];
-      *(scm_t_srcprops **)&ptr[SRCPROPS_CHUNKSIZE - 1] = 0;
-      srcprops_freelist = (scm_t_srcprops *) &ptr[1];
+      SCM old_plist = plist;
+
+      /*
+	have to extract the acons, and operate on that, for
+	thread safety.
+       */
+      SCM last_acons = SCM_CDR (scm_last_plist_filename);
+      if (old_plist == SCM_EOL
+	  && SCM_CDAR (last_acons) == filename)
+	{
+	  plist = last_acons;
+	}
+      else
+	{
+	  plist = scm_acons (scm_sym_filename, filename, plist);
+	  if (old_plist == SCM_EOL)
+	    SCM_SETCDR (scm_last_plist_filename, plist);
+	}
     }
-  ptr->pos = SRCPROPMAKPOS (line, col);
-  ptr->fname = filename;
-  ptr->copy = copy;
-  ptr->plist = plist;
-  SCM_CRITICAL_SECTION_END;
-  SCM_RETURN_NEWSMOB (scm_tc16_srcprops, ptr);
+  
+  SCM_RETURN_NEWSMOB3 (scm_tc16_srcprops,
+		       SRCPROPMAKPOS (line, col),
+		       copy,
+		       plist);
 }
 
 
@@ -140,8 +165,6 @@
   SCM plist = SRCPROPPLIST (obj);
   if (!SCM_UNBNDP (SRCPROPCOPY (obj)))
     plist = scm_acons (scm_sym_copy, SRCPROPCOPY (obj), plist);
-  if (!SCM_UNBNDP (SRCPROPFNAME (obj)))
-    plist = scm_acons (scm_sym_filename, SRCPROPFNAME (obj), plist);
   plist = scm_acons (scm_sym_column, scm_from_int (SRCPROPCOL (obj)), plist);
   plist = scm_acons (scm_sym_line, scm_from_int (SRCPROPLINE (obj)), plist);
   plist = scm_acons (scm_sym_breakpoint, scm_from_bool (SRCPROPBRK (obj)), plist);
@@ -206,7 +229,6 @@
   if      (scm_is_eq (scm_sym_breakpoint, key)) p = scm_from_bool (SRCPROPBRK (p));
   else if (scm_is_eq (scm_sym_line,       key)) p = scm_from_int (SRCPROPLINE (p));
   else if (scm_is_eq (scm_sym_column,     key)) p = scm_from_int (SRCPROPCOL (p));
-  else if (scm_is_eq (scm_sym_filename,   key)) p = SRCPROPFNAME (p);
   else if (scm_is_eq (scm_sym_copy,       key)) p = SRCPROPCOPY (p);
   else
     {
@@ -277,13 +299,6 @@
 		      scm_make_srcprops (0, scm_to_int (datum),
 					 SCM_UNDEFINED, SCM_UNDEFINED, p));
     }
-  else if (scm_is_eq (scm_sym_filename, key))
-    {
-      if (SRCPROPSP (p))
-	SRCPROPFNAME (p) = datum;
-      else
-	SCM_WHASHSET (scm_source_whash, h, scm_make_srcprops (0, 0, datum, SCM_UNDEFINED, p));
-    }
   else if (scm_is_eq (scm_sym_copy, key))
     {
       if (SRCPROPSP (p))
@@ -308,29 +323,18 @@
 {
   scm_tc16_srcprops = scm_make_smob_type ("srcprops", 0);
   scm_set_smob_mark (scm_tc16_srcprops, srcprops_mark);
-  scm_set_smob_free (scm_tc16_srcprops, srcprops_free);
   scm_set_smob_print (scm_tc16_srcprops, srcprops_print);
 
   scm_source_whash = scm_make_weak_key_hash_table (scm_from_int (2047));
   scm_c_define ("source-whash", scm_source_whash);
 
+  scm_last_plist_filename
+    = scm_permanent_object (scm_cons (SCM_EOL,
+				      scm_acons (SCM_EOL, SCM_EOL, SCM_EOL)));
+
 #include "libguile/srcprop.x"
 }
 
-void
-scm_finish_srcprop ()
-{
-  register scm_t_srcprops_chunk *ptr = srcprops_chunklist, *next;
-  size_t n= sizeof (scm_t_srcprops_chunk)
-    + sizeof (scm_t_srcprops) * (SRCPROPS_CHUNKSIZE - 1);
-  while (ptr)
-    {
-      next = ptr->next;
-      scm_gc_unregister_collectable_memory (ptr, n, "srcprops");
-      free ((char *) ptr);
-      ptr = next;
-    }
-}
 
 /*
   Local Variables:
Index: libguile/srcprop.h
===================================================================
RCS file: /sources/guile/guile/guile-core/libguile/srcprop.h,v
retrieving revision 1.36.2.1
diff -u -r1.36.2.1 srcprop.h
--- libguile/srcprop.h	12 Feb 2006 13:42:51 -0000	1.36.2.1
+++ libguile/srcprop.h	13 Mar 2008 22:42:30 -0000
@@ -49,46 +49,10 @@
 
 /* {Source properties}
  */
-
-SCM_API scm_t_bits scm_tc16_srcprops;
-
-typedef struct scm_t_srcprops
-{
-  unsigned long pos;
-  SCM fname;
-  SCM copy;
-  SCM plist;
-} scm_t_srcprops;
-
-#define SRCPROPS_CHUNKSIZE 2047 /* Number of srcprops per chunk */
-typedef struct scm_t_srcprops_chunk
-{
-  struct scm_t_srcprops_chunk *next;
-  scm_t_srcprops srcprops[1];
-} scm_t_srcprops_chunk;
-
+#define SCM_PROCTRACEP(x) (scm_is_true (scm_procedure_property (x, scm_sym_trace)))
 #define SCM_SOURCE_PROPERTY_FLAG_BREAK 1
 
-#define SRCPROPSP(p) (SCM_SMOB_PREDICATE (scm_tc16_srcprops, (p)))
-#define SRCPROPBRK(p) (SCM_SMOB_FLAGS (p) & SCM_SOURCE_PROPERTY_FLAG_BREAK)
-#define SRCPROPPOS(p) ((scm_t_srcprops *) SCM_SMOB_DATA (p))->pos
-#define SRCPROPLINE(p) (SRCPROPPOS(p) >> 12)
-#define SRCPROPCOL(p) (SRCPROPPOS(p) & 0x0fffL)
-#define SRCPROPFNAME(p) ((scm_t_srcprops *) SCM_SMOB_DATA (p))->fname
-#define SRCPROPCOPY(p) ((scm_t_srcprops *) SCM_SMOB_DATA (p))->copy
-#define SRCPROPPLIST(p) ((scm_t_srcprops *) SCM_SMOB_DATA (p))->plist
-#define SETSRCPROPBRK(p) \
- (SCM_SET_SMOB_FLAGS ((p), \
-                      SCM_SMOB_FLAGS (p) | SCM_SOURCE_PROPERTY_FLAG_BREAK))
-#define CLEARSRCPROPBRK(p)  \
- (SCM_SET_SMOB_FLAGS ((p), \
-                      SCM_SMOB_FLAGS (p) & ~SCM_SOURCE_PROPERTY_FLAG_BREAK))
-#define SRCPROPMAKPOS(l, c) (((l) << 12) + (c))
-#define SETSRCPROPPOS(p, l, c) (SRCPROPPOS (p) = SRCPROPMAKPOS (l, c))
-#define SETSRCPROPLINE(p, l) SETSRCPROPPOS (p, l, SRCPROPCOL (p))
-#define SETSRCPROPCOL(p, c) SETSRCPROPPOS (p, SRCPROPLINE (p), c)
-
-#define PROCTRACEP(x) (scm_is_true (scm_procedure_property (x, scm_sym_trace)))
+SCM_API scm_t_bits scm_tc16_srcprops;
 
 SCM_API SCM scm_sym_filename;
 SCM_API SCM scm_sym_copy;

Re: Terrific Dead Lock

[Neil Jerram]

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

> Hi,
>
> ludo@gnu.org (Ludovic Courtès) writes:
>
>> Two possibilities to fix it:
>>
>>   1. Copy `srcprop.[ch]' and `eval.c' bits from HEAD to 1.8.  After all,
>>      it's probably solid enough (I use almost only HEAD).  See [0] for
>>      an overview of the initial patch.  It doesn't break the public API
>>      nor the ABI, but it (re)moves stuff from the `srcprop.h'.
>>
>>   2. Remove the critical section from 1.8 and synchronize accesses to
>>      `srcprops_freelist' with a private mutex, assuming that's a correct
>>      fix.

I prefer (1).

> The second approach is actually wrong: we'd need to acquire a mutex in
> `srcprops_free ()' to synchronize accesses to `srcprops_freelist', but
> we can't do it since `srcprops_free ()' is called during GC---which is
> why there was a critical section in the first place I suppose.
>
> Thus the first approach seems unavoidable.  Attached is the exact patch.
> It removes non-public macros and data types from `srcprop.h', which is
> acceptable IMO.
>
> OK to apply?

Basically yes, but two thoughts:

- Can I just take a couple more days to review the srcprops changes in
  detail?  It's important for my debugging work, and I recall having
  some concern when Han-Wen implemented this...  Looking at the diffs
  quickly again now, I can't see any reason for that concern, but I'd
  like to be sure.

- Although I agree in principle that all those macros shouldn't be in
  srcprop.h, do we really need to make that change now?  I think we can
  just apply Han-Wen's changes to the macros in srcprop.h, without
  moving them to srcprop.c.

Regards,
        Neil

Re: Terrific Dead Lock

[Ludovic Courtès]

Hi,

Neil Jerram <neil@ossau.uklinux.net> writes:

> Basically yes, but two thoughts:
>
> - Can I just take a couple more days to review the srcprops changes in
>   detail?  It's important for my debugging work, and I recall having
>   some concern when Han-Wen implemented this...  Looking at the diffs
>   quickly again now, I can't see any reason for that concern, but I'd
>   like to be sure.

Sure.  (I browsed this thread recently and I think these were mainly
efficiency concerns.)

> - Although I agree in principle that all those macros shouldn't be in
>   srcprop.h, do we really need to make that change now?  I think we can
>   just apply Han-Wen's changes to the macros in srcprop.h, without
>   moving them to srcprop.c.

I dislike the idea of having to create another version of `srcprop' just
for the sake of leaving private macros publicly accessible, but maybe
I'm just too lazy or radical.

FWIW, http://www.google.com/codesearch?hl=en&q=+SRCPROPSP suggests that
Guile may be the only user of this macro.

Thanks,
Ludovic.

Re: Terrific Dead Lock

[Ludovic Courtès]

Hi,

Neil Jerram <neil@ossau.uklinux.net> writes:

> Basically yes, but two thoughts:
>
> - Can I just take a couple more days to review the srcprops changes in
>   detail?  It's important for my debugging work, and I recall having
>   some concern when Han-Wen implemented this...  Looking at the diffs
>   quickly again now, I can't see any reason for that concern, but I'd
>   like to be sure.
>
> - Although I agree in principle that all those macros shouldn't be in
>   srcprop.h, do we really need to make that change now?  I think we can
>   just apply Han-Wen's changes to the macros in srcprop.h, without
>   moving them to srcprop.c.

Are you OK to apply the patch?

Thanks,
Ludovic.

Re: Terrific Dead Lock

[Neil Jerram]

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

> Hi,
>
> Neil Jerram <neil@ossau.uklinux.net> writes:
>
>> Basically yes, but two thoughts:
>>
>> - Can I just take a couple more days to review the srcprops changes in
>>   detail?  It's important for my debugging work, and I recall having
>>   some concern when Han-Wen implemented this...  Looking at the diffs
>>   quickly again now, I can't see any reason for that concern, but I'd
>>   like to be sure.
>>
>> - Although I agree in principle that all those macros shouldn't be in
>>   srcprop.h, do we really need to make that change now?  I think we can
>>   just apply Han-Wen's changes to the macros in srcprop.h, without
>>   moving them to srcprop.c.
>
> Are you OK to apply the patch?

Yes, fine.  Sorry for forgetting about this.

(I wonder if SCM_CDR (scm_last_plist_filename) on one thread is
guaranteed to give a sane value, if there is another thread
simultaneously doing SCM_SETCDR (scm_last_plist_filename) - but I'm
not an expert in this kind of thing.)

Regards,
      Neil

Re: Terrific Dead Lock

[Ludovic Courtès]

Hi,

Neil Jerram <neil@ossau.uklinux.net> writes:

> Yes, fine.  Sorry for forgetting about this.

Thanks, applied.

> (I wonder if SCM_CDR (scm_last_plist_filename) on one thread is
> guaranteed to give a sane value, if there is another thread
> simultaneously doing SCM_SETCDR (scm_last_plist_filename) - but I'm
> not an expert in this kind of thing.)

Yes, I suppose that's the assumption, and it's probably a reasonable one
since `SCM_SETCDR' boils down to a single memory write.

Thanks,
Ludovic.

Re: Terrific Dead Lock

[Neil Jerram]

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

> Hi,
>
> Neil Jerram <neil@ossau.uklinux.net> writes:
>
>> Yes, fine.  Sorry for forgetting about this.
>
> Thanks, applied.
>
>> (I wonder if SCM_CDR (scm_last_plist_filename) on one thread is
>> guaranteed to give a sane value, if there is another thread
>> simultaneously doing SCM_SETCDR (scm_last_plist_filename) - but I'm
>> not an expert in this kind of thing.)
>
> Yes, I suppose that's the assumption, and it's probably a reasonable one
> since `SCM_SETCDR' boils down to a single memory write.

Exactly.  If there is any risk here, I feel happy to live with it for
now.

        Neil