bugGuile - Bugs: bug #24867, `define' should be thread-safe

 
 

bug #24867: `define' should be thread-safe

Submitter:  Ludovic Courtès <civodul>
Submitted:  Tue 18 Nov 2008 10:18:28 AM UTC
Votes: 60
 
Category:  None Severity:  3 - Normal
Item Group:  None Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Open
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 26 Dec 2013 12:08:38 AM UTC, comment #13: 

To answer my own question:
-- compile test case below, as:
cc define-race.c -lpthread -lguile-2.0 -I/usr/include/guile/2.0

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff5d44700 (LWP 7601)]
0x00007ffff7243bc9 in GC_push_all_eager () from /usr/lib/libgc.so.1
(gdb) bt
#0  0x00007ffff7243bc9 in GC_push_all_eager () from /usr/lib/libgc.so.1
#1  0x00007ffff724bee1 in GC_push_all_stacks () from /usr/lib/libgc.so.1
#2  0x00007ffff7244733 in GC_mark_some () from /usr/lib/libgc.so.1
#3  0x00007ffff723b802 in GC_stopped_mark () from /usr/lib/libgc.so.1
#4  0x00007ffff723bf0a in GC_try_to_collect_inner () from /usr/lib/libgc.so.1
#5  0x00007ffff72465d0 in GC_init_inner () from /usr/lib/libgc.so.1
#6  0x00007ffff78af793 in ?? () from /usr/lib/libguile-2.0.so.22
#7  0x00007ffff78be9a8 in ?? () from /usr/lib/libguile-2.0.so.22
#8  0x00007ffff79118f0 in ?? () from /usr/lib/libguile-2.0.so.22
#9  0x00007ffff7911927 in ?? () from /usr/lib/libguile-2.0.so.22
#10 0x00007ffff72461e5 in GC_call_with_stack_base () from /usr/lib/libgc.so.1
#11 0x00007ffff7911ae8 in scm_with_guile () from /usr/lib/libguile-2.0.so.22
#12 0x0000000000400b2e in definer ()
#13 0x00007ffff7bc4e9a in start_thread (arg=0x7ffff5d44700) at pthread_create.c:308
#14 0x00007ffff75793fd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:112
#15 0x0000000000000000 in ?? ()


This is with
$ guile --version
guile (GNU Guile) 2.0.5-deb+1-1

installed on an Ubuntu Precise system

The corresponding GC seems to be froom a package called
libgc1c2 - conservative garbage collector for C and C++
which installs libgc.so.1.0.3
The last entry in the ChangeLog is
libgc (1:7.1-8ubuntu0.12.04.1) precise-security; urgency=low

so this seems to be the bdwgc version 7.1

 Linas Vepstas <linas>
Wed 25 Dec 2013 11:15:25 PM UTC, comment #12: 

Sooo .. is this bug fixed in guile-2.0.5 or 2.0.9?

Because I'm currently staring at the five-year-old work-around for this bug, wondering if I should remove it or not. It's definitely a bottle-neck for my app (natural language processing for opencog).

Some of my guile scripts run for minutes, which means, as long as guile remains single-threaded, I can't use it while a script is running. This makes development incredibly painful...

 Linas Vepstas <linas>
  Spam posted by jackbenny
  Spam posted by jackbenny
  Spam posted by jackbenny
  Spam posted by jackbenny
  Spam posted by kourtney001
Tue 23 Dec 2008 07:10:58 PM UTC, comment #6: 

I've attached a patch to lock the entry and exit to scm_sym2var().  However, it doesn't fix the problem (at all).

-- There are still deadlocks in garbage collection, (but different from the previously reported one !?)
-- There are still crashes (with same stack trace as previously posted)
-- There are still races.



(file #17129)

 Linas Vepstas <linas>
Tue 23 Dec 2008 06:28:45 PM UTC, comment #5: 

1) Easier said than done, because:

1a) the mutex needs to be recursive, since sym2var evaluates code in boot9.scm which can cause sym2var to run again. The core problem is that the mechanism for specifying recursive mutexes seems to be somewhat OS-dependent (and possibly some OS'es don't support recursive mutexes??) and so a portability wrapper might be needed. :-(

1b) There's still a strange deadlock somehow; am debugging.

3) Fine-grained usually means speedy. if there was some per-module C struct, then the mutex could be put in there. (I don't know of any, but I don't know guile internals).  The alternative would be somehow grabbing a lock in the boot9.scm code, but I don't see how, without making some symbol lookup (i.e. race).

Linas Vepstas <linas>
Tue 23 Dec 2008 03:21:21 PM UTC, comment #4: 

Thanks for the patch and test case!

A few remarks:

  1. `scm_sym2var ()' must be changed to acquire the mutex before performing actual lookup.  Thus, `scm_define ()' itself doesn't need to acquire it.  I think this should be enough to fix the bug.  It shouldn't slow things down too much, thanks to memoization.

  2. The mutex can be declared as `static'.

  3. In 1.9, we should perhaps avoid `scm_i_define_mutex' and use a per-module mutex (which isn't possible in 1.8 since it would break ABI).  OTOH, is it really necessary to have such a fine grain?

Can you provide an updated patch with ChangeLog-style entry?

Ludo'.

 Ludovic Courtès <civodul>
Group administrator
Tue 23 Dec 2008 03:06:41 AM UTC, comment #3: 

Note that the attached test case shows 4 distinct behaviours:

1) runs fine, exiting normally, w/o any errors

2) Spewing messages such as:
ERROR: Unbound variable: x2-347
ERROR: Unbound variable: x2-347
ERROR: Unbound variable: x2-347
ERROR: Unbound variable: define
ERROR: Unbound variable: x1-2525
ERROR: Unbound variable: x1-2525
ERROR: Unbound variable: x1-2525
ERROR: Unbound variable: x1-2525
ERROR: Unbound variable: x1-2525
ERROR: Unbound variable: x1-2525

  the numbers being different each time, of course, but then exiting normally.

3) Deadlocking in garbage collection, with all four threads
   stuck in
   #5  0xf7f01f6b in scm_gc_for_newcell (freelist=0xf7f8c8ec,
    free_cells=0x90c810c) at gc.c:484

  (this may be preceeded by the prints above)

4) Segfault:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xf74c5b90 (LWP 30649)]
0xf7f426ad in scm_assert_smob_type (tag=639, val=0x0) at smob.c:63
63   if (!SCM_SMOB_PREDICATE (tag, val))
(gdb) bt
#0  0xf7f426ad in scm_assert_smob_type (tag=639, val=0x0) at smob.c:63
#1  0xf7f0b4c5 in scm_make_dynamic_state (parent=0x0) at fluids.c:508
#2  0xf7f62bdf in guilify_self_2 (parent=0x0) at threads.c:508
#3  0xf7f64992 in scm_i_init_thread_for_guile (base=0xf74c5388, parent=0x0)
    at threads.c:611
#4  0xf7f649c5 in scm_i_with_guile_and_parent (
    func=0x8048754 <guile_mode_definer>, data=0xffaf0cbc, parent=0x0)
    at threads.c:743
#5  0xf7f64ace in scm_with_guile (func=0x8048754 <guile_mode_definer>,
    data=0xffaf0cbc) at threads.c:732
#6  0x080488a8 in definer ()
#7  0xf7fa14fb in start_thread () from /lib/tls/i686/cmov/libpthread.so.0
#8  0xf7e45e5e in clone () from /lib/tls/i686/cmov/libc.so.6


I'm guessing that the use of immutable vlists, as proposed by Ludo, for use in the module-obarray per mail discussion, would solve all of the above. 

Linas Vepstas <linas>
Tue 23 Dec 2008 02:36:11 AM UTC, comment #2: 


The following patch protects the update of the module hash tables
to be thread-safe. This is a partial solution to the bug reported
in https://savannah.gnu.org/bugs/?24867 This is not a full solution,
because other threads might still be reading the hash tables while
they are being updated, and thus may obtain stale/bad data.

Signed-off-by: Linas Vepstas <linasvepstas@gmail.com>

---
 libguile/modules.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Index: guile-1.8.6/libguile/modules.c
===================================================================
--- guile-1.8.6.orig/libguile/modules.c 2008-12-22 18:38:41.000000000 -0600
+++ guile-1.8.6/libguile/modules.c 2008-12-22 20:22:19.000000000 -0600
@@ -555,11 +555,16 @@ scm_c_define (const char *name, SCM valu
   return scm_define (scm_from_locale_symbol (name), value);
 }
 
+scm_i_pthread_mutex_t scm_i_define_mutex;
+
 SCM
 scm_define (SCM sym, SCM value)
 {
-  SCM var =
+  SCM var;
+  scm_pthread_mutex_lock(&scm_i_define_mutex);
+  var =
     scm_sym2var (sym, scm_current_module_lookup_closure (), SCM_BOOL_T);
+  scm_i_pthread_mutex_unlock(&scm_i_define_mutex);
   SCM_VARIABLE_SET (var, value);
   return var;
 }
@@ -651,6 +656,8 @@ void
 scm_init_modules ()
 {
 #include "libguile/modules.x"
+  scm_i_pthread_mutex_init (&scm_i_define_mutex, NULL);
+
   module_make_local_var_x_var = scm_c_define ("module-make-local-var!",
      SCM_UNDEFINED);
   scm_tc16_eval_closure = scm_make_smob_type ("eval-closure", 0);


(file #17118)

 Linas Vepstas <linas>
Tue 18 Nov 2008 01:27:30 PM UTC, comment #1: 

Gmane appears to be broken.  Alternate URL: http://lists.gnu.org/archive/html/guile-devel/2008-11/threads.html#00055 .

 Ludovic Courtès <civodul>
Group administrator
Tue 18 Nov 2008 10:18:28 AM UTC, original submission:  

`define' uses a hash table behind the scenes.  The hash table itself is not thread-safe, but `define' should be thread-safe, that is, by synchronizing accesses to the underlying hash table.

See http://thread.gmane.org/gmane.lisp.guile.devel/7851/focus=7858 for the original report.

Thanks,
Ludo'.

 Ludovic Courtès <civodul>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #17129:  symvar-race.patch added by linas (2KiB - text/x-diff - patch to lock sym2var ... except this patch doesn't fix the problem)
file #17119:  define-race.c added by linas (2KiB - text/x-csrc - test case to exhibit the bug)
file #17118:  define-race.patch added by linas (1KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jackbenny (Posted a comment)
  • -email is unavailable- added by kourtney001 (Posted a comment)
  • -email is unavailable- added by linas (Voted in favor of this item)
  • -email is unavailable- added by linas (Updated the item)
  • -email is unavailable- added by civodul (Submitted the item)
  • -email is unavailable- added by civodul
  • -email is unavailable- added by civodul

    •  

    There are 60 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-12-23 linas Attached File- Added symvar-race.patch, #17129
    2008-12-23 linas Attached File- Added define-race.c, #17119
        Carbon-Copy- Added linas
    2008-12-23 linas Attached File- Added define-race.patch, #17118
    2008-11-18 civodul Carbon-Copy- Added -email is unavailable-
        Carbon-Copy- Added -email is unavailable-

    Back to the top

    Copyright © 2024 Free Software Foundation, Inc.
    Verbatim copying and distribution of this entire article is permitted in any medium, provided this notice is preserved.
    The Levitating, Meditating, Flute-playing Gnu logo is a GNU GPL'ed image provided by the Nevrax Design Team.
    Page source code

    Powered by Savane 3.14-f13d.
    Corresponding source code