From: Neil Jerram <neil@ossau.uklinux.net>
To: Guile Development <guile-devel@gnu.org>
Cc: Linas Vepstas <linasvepstas@gmail.com>
Subject: Re: Locks and threads
Date: Wed, 04 Mar 2009 23:49:20 +0000 [thread overview]
Message-ID: <87vdqovofz.fsf@arudy.ossau.uklinux.net> (raw)
In-Reply-To: <87mycsd2rj.fsf@arudy.ossau.uklinux.net> (Neil Jerram's message of "Wed\, 11 Feb 2009 22\:31\:28 +0000")
[-- Attachment #1: Type: text/plain, Size: 474 bytes --]
Neil Jerram <neil@ossau.uklinux.net> writes:
> - first to address problems reported by helgrind (since I think we
> should take advantage of external tools before adding debug code to
> Guile internally)
Here's another lock ordering fix. (For 1.8.x at least, I haven't
checked if it's relevant to master yet.)
1.8.x is then helgrind-clean (apart from one unimportant-looking
termination issue), so next it's on to the real problem, threadsafe
define.
Neil
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Lock-ordering-don-t-allocate-when-in-critical-secti.patch --]
[-- Type: text/x-diff, Size: 6292 bytes --]
From 588edc4cd1bfea7d51c9aed463e8119482e7a3f0 Mon Sep 17 00:00:00 2001
From: Neil Jerram <neil@ossau.uklinux.net>
Date: Wed, 4 Mar 2009 23:45:11 +0000
Subject: [PATCH] Lock ordering: don't allocate when in critical section (scm_sigaction_for_thread)
This fixes the following helgrind report.
Thread #1: lock order "0x4114748 before 0x4331084" violated
at 0x40234F7: pthread_mutex_lock (hg_intercepts.c:408)
by 0x407A55F: deval (eval.c:4077)
by 0x407A071: scm_dapply (eval.c:5012)
by 0x407888A: scm_apply (eval.c:4811)
by 0x407E20C: scm_call_0 (eval.c:4666)
by 0x406BDF7: scm_dynamic_wind (dynwind.c:111)
by 0x407620C: ceval (eval.c:4571)
by 0x407E8D9: scm_primitive_eval_x (eval.c:5921)
by 0x407E934: scm_eval_x (eval.c:5956)
by 0x40B9140: scm_shell (script.c:737)
by 0x4095AC5: invoke_main_func (init.c:367)
by 0x4065A81: c_body (continuations.c:349)
Required order was established by acquisition of lock at 0x4114748
at 0x40234F7: pthread_mutex_lock (hg_intercepts.c:408)
by 0x40B7BE1: scm_sigaction_for_thread (scmsigs.c:339)
by 0x40911DE: scm_gsubr_apply (gsubr.c:223)
by 0x4079E36: scm_dapply (eval.c:4930)
by 0x407AB68: deval (eval.c:4378)
by 0x407A071: scm_dapply (eval.c:5012)
by 0x407888A: scm_apply (eval.c:4811)
by 0x407E1D0: scm_call_1 (eval.c:4672)
by 0x407FEEE: scm_map (eval.c:5489)
by 0x407BDCC: deval (eval.c:4367)
by 0x407D197: deval (eval.c:3698)
by 0x407A071: scm_dapply (eval.c:5012)
followed by a later acquisition of lock at 0x4331084
at 0x40234F7: pthread_mutex_lock (hg_intercepts.c:408)
by 0x40DC397: scm_enter_guile (threads.c:377)
by 0x40DC8F4: scm_pthread_mutex_lock (threads.c:1485)
by 0x4083FAA: scm_gc_for_newcell (gc.c:484)
by 0x40A9781: scm_cons (inline.h:115)
by 0x4079867: scm_dapply (eval.c:4850)
by 0x407888A: scm_apply (eval.c:4811)
by 0x40796D6: scm_call_3 (eval.c:4684)
by 0x409A7C2: module_variable (modules.c:302)
by 0x409A7EE: module_variable (modules.c:312)
by 0x409A962: scm_sym2var (modules.c:466)
by 0x40738F4: scm_lookupcar1 (eval.c:2874)
* libguile/scmsigs.c (close_1): Renamed `handler_to_async'; also
handle #f case and wrapping the async in a cons, if necessary.
(install_handler): Pass in async instead of constructing it; combine
two branches into one.
(scm_sigaction_for_thread): Allocate async upfront instead of inside
the critical section, and pass it to install_handler calls. Leave
critical section before signaling out-of-range error.
---
libguile/scmsigs.c | 53 ++++++++++++++++++++++++++++-----------------------
1 files changed, 29 insertions(+), 24 deletions(-)
diff --git a/libguile/scmsigs.c b/libguile/scmsigs.c
index 9433273..e15bbf3 100644
--- a/libguile/scmsigs.c
+++ b/libguile/scmsigs.c
@@ -108,10 +108,20 @@ static SIGRETTYPE (*orig_handlers[NSIG])(int);
#endif
static SCM
-close_1 (SCM proc, SCM arg)
+handler_to_async (SCM handler, int signum)
{
- return scm_primitive_eval_x (scm_list_3 (scm_sym_lambda, SCM_EOL,
- scm_list_2 (proc, arg)));
+ if (scm_is_false (handler))
+ return SCM_BOOL_F;
+ else
+ {
+ SCM async = scm_primitive_eval_x (scm_list_3 (scm_sym_lambda, SCM_EOL,
+ scm_list_2 (handler,
+ scm_from_int (signum))));
+#if !SCM_USE_PTHREAD_THREADS
+ async = scm_cons (async, SCM_BOOL_F);
+#endif
+ return async;
+ }
}
#if SCM_USE_PTHREAD_THREADS
@@ -239,23 +249,10 @@ ensure_signal_delivery_thread ()
#endif /* !SCM_USE_PTHREAD_THREADS */
static void
-install_handler (int signum, SCM thread, SCM handler)
+install_handler (int signum, SCM thread, SCM handler, SCM async)
{
- if (scm_is_false (handler))
- {
- SCM_SIMPLE_VECTOR_SET (*signal_handlers, signum, SCM_BOOL_F);
- SCM_SIMPLE_VECTOR_SET (signal_handler_asyncs, signum, SCM_BOOL_F);
- }
- else
- {
- SCM async = close_1 (handler, scm_from_int (signum));
-#if !SCM_USE_PTHREAD_THREADS
- async = scm_cons (async, SCM_BOOL_F);
-#endif
- SCM_SIMPLE_VECTOR_SET (*signal_handlers, signum, handler);
- SCM_SIMPLE_VECTOR_SET (signal_handler_asyncs, signum, async);
- }
-
+ SCM_SIMPLE_VECTOR_SET (*signal_handlers, signum, handler);
+ SCM_SIMPLE_VECTOR_SET (signal_handler_asyncs, signum, async);
SCM_SIMPLE_VECTOR_SET (signal_handler_threads, signum, thread);
}
@@ -308,6 +305,7 @@ SCM_DEFINE (scm_sigaction_for_thread, "sigaction", 1, 3, 0,
int save_handler = 0;
SCM old_handler;
+ SCM async;
csig = scm_to_signed_integer (signum, 0, NSIG-1);
@@ -334,6 +332,10 @@ SCM_DEFINE (scm_sigaction_for_thread, "sigaction", 1, 3, 0,
SCM_MISC_ERROR ("thread has already exited", SCM_EOL);
}
+ /* Allocate upfront, as we can't do it inside the critical
+ section. */
+ async = handler_to_async (handler, csig);
+
ensure_signal_delivery_thread ();
SCM_CRITICAL_SECTION_START;
@@ -351,10 +353,13 @@ SCM_DEFINE (scm_sigaction_for_thread, "sigaction", 1, 3, 0,
#else
chandler = (SIGRETTYPE (*) (int)) handler_int;
#endif
- install_handler (csig, SCM_BOOL_F, SCM_BOOL_F);
+ install_handler (csig, SCM_BOOL_F, SCM_BOOL_F, async);
}
else
- SCM_OUT_OF_RANGE (2, handler);
+ {
+ SCM_CRITICAL_SECTION_END;
+ SCM_OUT_OF_RANGE (2, handler);
+ }
}
else if (scm_is_false (handler))
{
@@ -366,7 +371,7 @@ SCM_DEFINE (scm_sigaction_for_thread, "sigaction", 1, 3, 0,
{
action = orig_handlers[csig];
orig_handlers[csig].sa_handler = SIG_ERR;
- install_handler (csig, SCM_BOOL_F, SCM_BOOL_F);
+ install_handler (csig, SCM_BOOL_F, SCM_BOOL_F, async);
}
#else
if (orig_handlers[csig] == SIG_ERR)
@@ -375,7 +380,7 @@ SCM_DEFINE (scm_sigaction_for_thread, "sigaction", 1, 3, 0,
{
chandler = orig_handlers[csig];
orig_handlers[csig] = SIG_ERR;
- install_handler (csig, SCM_BOOL_F, SCM_BOOL_F);
+ install_handler (csig, SCM_BOOL_F, SCM_BOOL_F, async);
}
#endif
}
@@ -391,7 +396,7 @@ SCM_DEFINE (scm_sigaction_for_thread, "sigaction", 1, 3, 0,
if (orig_handlers[csig] == SIG_ERR)
save_handler = 1;
#endif
- install_handler (csig, thread, handler);
+ install_handler (csig, thread, handler, async);
}
/* XXX - Silently ignore setting handlers for `program error signals'
--
1.5.6.5
next prev parent reply other threads:[~2009-03-04 23:49 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-02-11 22:31 Locks and threads Neil Jerram
2009-02-11 23:05 ` Neil Jerram
2009-02-11 23:32 ` Ludovic Courtès
2009-02-11 23:30 ` Linas Vepstas
2009-02-11 23:53 ` Neil Jerram
2009-02-12 0:18 ` Linas Vepstas
2009-02-12 20:51 ` Ludovic Courtès
2009-02-11 23:30 ` Ludovic Courtès
2009-02-12 12:55 ` Greg Troxel
2009-02-12 18:00 ` Ken Raeburn
2009-02-12 21:14 ` Ludovic Courtès
2009-02-14 1:25 ` Ken Raeburn
2009-02-14 16:09 ` Ludovic Courtès
2009-03-05 20:41 ` Neil Jerram
2009-03-04 23:49 ` Neil Jerram [this message]
2009-03-05 3:54 ` Linas Vepstas
2009-03-05 19:46 ` Neil Jerram
2009-03-05 20:05 ` Neil Jerram
2009-03-05 20:40 ` Linas Vepstas
2009-03-05 20:49 ` Neil Jerram
2009-03-05 20:57 ` Linas Vepstas
2009-03-05 21:25 ` Neil Jerram
2009-03-05 21:56 ` Linas Vepstas
2009-03-06 11:01 ` Andy Wingo
2009-03-06 12:36 ` Linas Vepstas
2009-03-06 22:05 ` Ludovic Courtès
2009-03-08 22:04 ` Neil Jerram
2009-03-25 19:00 ` Neil Jerram
2009-03-25 22:08 ` Ludovic Courtès
2009-03-05 21:35 ` Ludovic Courtès
2009-03-10 23:57 ` Neil Jerram
2009-03-12 0:07 ` Neil Jerram
2009-03-12 0:53 ` Neil Jerram
2009-03-12 1:29 ` Linas Vepstas
2009-03-12 3:09 ` Clinton Ebadi
2009-03-25 22:13 ` Neil Jerram
2009-03-25 22:34 ` Linas Vepstas
2009-03-12 22:13 ` Andy Wingo
2009-03-13 19:13 ` Neil Jerram
2009-03-25 23:19 ` Neil Jerram
2009-03-26 3:40 ` Linas Vepstas
2009-03-26 8:02 ` Neil Jerram
2009-03-26 18:39 ` Linas Vepstas
2009-03-26 9:10 ` Ludovic Courtès
2009-03-26 22:01 ` Neil Jerram
2009-03-26 23:12 ` Ludovic Courtès
2009-03-26 22:51 ` Neil Jerram
2009-03-27 3:15 ` Linas Vepstas
2009-03-14 14:23 ` Ludovic Courtès
2009-03-16 22:57 ` Andy Wingo
2009-03-25 18:57 ` Neil Jerram
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/guile/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87vdqovofz.fsf@arudy.ossau.uklinux.net \
--to=neil@ossau.uklinux.net \
--cc=guile-devel@gnu.org \
--cc=linasvepstas@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).