From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: ludo@gnu.org (Ludovic =?iso-8859-1?Q?Court=E8s?=) Newsgroups: gmane.lisp.guile.devel Subject: Re: Terrific Dead Lock Date: Fri, 14 Mar 2008 11:24:17 +0100 Message-ID: <87y78lioxa.fsf@gnu.org> References: <8763vq6yvv.fsf@gnu.org> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Trace: ger.gmane.org 1205490289 28176 80.91.229.12 (14 Mar 2008 10:24:49 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 14 Mar 2008 10:24:49 +0000 (UTC) To: guile-devel@gnu.org Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Fri Mar 14 11:25:17 2008 Return-path: Envelope-to: guile-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.50) id 1Ja76C-00029K-VO for guile-devel@m.gmane.org; Fri, 14 Mar 2008 11:25:17 +0100 Original-Received: from localhost ([127.0.0.1] helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Ja75d-0003jA-Lu for guile-devel@m.gmane.org; Fri, 14 Mar 2008 06:24:41 -0400 Original-Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Ja75W-0003dY-CU for guile-devel@gnu.org; Fri, 14 Mar 2008 06:24:34 -0400 Original-Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Ja75T-0003b5-Hp for guile-devel@gnu.org; Fri, 14 Mar 2008 06:24:33 -0400 Original-Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Ja75S-0003as-Us for guile-devel@gnu.org; Fri, 14 Mar 2008 06:24:31 -0400 Original-Received: from main.gmane.org ([80.91.229.2] helo=ciao.gmane.org) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1Ja75S-0002Z4-20 for guile-devel@gnu.org; Fri, 14 Mar 2008 06:24:30 -0400 Original-Received: from list by ciao.gmane.org with local (Exim 4.43) id 1Ja75O-000667-D1 for guile-devel@gnu.org; Fri, 14 Mar 2008 10:24:26 +0000 Original-Received: from 193.50.110.120 ([193.50.110.120]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Fri, 14 Mar 2008 10:24:26 +0000 Original-Received: from ludo by 193.50.110.120 with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Fri, 14 Mar 2008 10:24:26 +0000 X-Injected-Via-Gmane: http://gmane.org/ Original-Lines: 329 Original-X-Complaints-To: usenet@ger.gmane.org X-Gmane-NNTP-Posting-Host: 193.50.110.120 X-Revolutionary-Date: 25 =?iso-8859-1?Q?Vent=F4se?= an 216 de la =?iso-8859-1?Q?R=E9volution?= X-PGP-Key-ID: 0xEB1F5364 X-PGP-Key: http://www.laas.fr/~lcourtes/ludovic.asc X-PGP-Fingerprint: 821D 815D 902A 7EAB 5CEE D120 7FBA 3D4F EB1F 5364 X-OS: i686-pc-linux-gnu User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.1 (gnu/linux) Cancel-Lock: sha1:aZcaixUnllYkZoNqFfohZhNCW5Q= X-detected-kernel: by monty-python.gnu.org: Linux 2.6, seldom 2.4 (older, 4) X-BeenThere: guile-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Developers list for Guile, the GNU extensibility library" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Errors-To: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.lisp.guile.devel:7072 Archived-At: --=-=-= Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit 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. --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename*=us-ascii''%2c%2csrcprop-new.diff Content-Description: The patch against srcprops 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; --=-=-=--