From 4f9b5cd4268d3e28ac54748c4871e07f38acfd4c Mon Sep 17 00:00:00 2001 From: Gabor Melis Date: Mon, 16 Feb 2009 21:53:25 +0000 Subject: [PATCH] 1.0.25.34: gc trigger improvements - Make sure that gc never runs user code (after gc hooks and finalizers) if interrupts are disabled and they cannot be enabled (i.e. *ALLOW-WITH-INTERRUPTS* is NIL). - clean up maybe_gc: we know that in the interrupted context gc signals are unblocked (see "check that gc signals are unblocked" commit) so it's safe to simply enable them and call SUB-GC --- NEWS | 1 + src/code/gc.lisp | 47 +++++++++++++++++++----------- src/compiler/generic/parms.lisp | 1 + src/runtime/bsd-os.c | 9 ++++++ src/runtime/gc-common.c | 60 +++++++++++++++++++++------------------ src/runtime/interrupt.c | 58 ++++++++++++++++++++++++++++--------- src/runtime/interrupt.h | 24 +++++++++------- version.lisp-expr | 2 +- 8 files changed, 132 insertions(+), 70 deletions(-) diff --git a/NEWS b/NEWS index efefafa..0226ff5 100644 --- a/NEWS +++ b/NEWS @@ -12,6 +12,7 @@ changes in sbcl-1.0.26 relative to 1.0.25: * optimization: faster WITHOUT-GCING * bug fix: real-time signals are not used anymore, so no more hanging when the system wide real-time signal queue gets full. + * bug fix: finalizers, gc hooks never run in a WITHOUT-INTERRUPTS changes in sbcl-1.0.25 relative to 1.0.24: * incompatible change: SB-INTROSPECT:FUNCTION-ARGLIST is deprecated, to be diff --git a/src/code/gc.lisp b/src/code/gc.lisp index f3b9615..32aa109 100644 --- a/src/code/gc.lisp +++ b/src/code/gc.lisp @@ -197,7 +197,8 @@ run in any thread.") (defun sub-gc (&key (gen 0)) (cond (*gc-inhibit* - (setf *gc-pending* t)) + (setf *gc-pending* t) + nil) (t (without-interrupts (setf *gc-pending* :in-progress) @@ -247,21 +248,32 @@ run in any thread.") ;; explicitly for a pending gc before interrupts are ;; enabled again. (maybe-handle-pending-gc)) - ;; Outside the mutex, interrupts enabled: these may cause - ;; another GC. FIXME: it can potentially exceed maximum - ;; interrupt nesting by triggering GCs. - ;; - ;; Can that be avoided by having the finalizers and hooks - ;; run only from the outermost SUB-GC? - ;; - ;; KLUDGE: Don't run the hooks in GC's triggered by dying - ;; threads, so that user-code never runs with - ;; (thread-alive-p *current-thread*) => nil - ;; The long-term solution will be to keep a separate thread - ;; for finalizers and after-gc hooks. - (when (sb!thread:thread-alive-p sb!thread:*current-thread*) - (run-pending-finalizers) - (call-hooks "after-GC" *after-gc-hooks* :on-error :warn))))) + t))) + +(defun post-gc () + ;; Outside the mutex, interrupts may be enabled: these may cause + ;; another GC. FIXME: it can potentially exceed maximum interrupt + ;; nesting by triggering GCs. + ;; + ;; Can that be avoided by having the finalizers and hooks run only + ;; from the outermost SUB-GC? If the nested GCs happen in interrupt + ;; handlers that's not enough. + ;; + ;; KLUDGE: Don't run the hooks in GC's if: + ;; + ;; A) this thread is dying, so that user-code never runs with + ;; (thread-alive-p *current-thread*) => nil + ;; + ;; B) interrupts are disabled somewhere up the call chain since we + ;; don't want to run user code in such a case. + ;; + ;; The long-term solution will be to keep a separate thread for + ;; finalizers and after-gc hooks. + (when (sb!thread:thread-alive-p sb!thread:*current-thread*) + (when *allow-with-interrupts* + (with-interrupts + (run-pending-finalizers) + (call-hooks "after-GC" *after-gc-hooks* :on-error :warn))))) ;;; This is the user-advertised garbage collection function. (defun gc (&key (gen 0) (full nil) &allow-other-keys) @@ -271,7 +283,8 @@ run in any thread.") #!+(and sb-doc (not gencgc)) "Initiate a garbage collection. GEN may be provided for compatibility with generational garbage collectors, but is ignored in this implementation." - (sub-gc :gen (if full 6 gen))) + (when (sub-gc :gen (if full 6 gen)) + (post-gc))) (defun unsafe-clear-roots () ;; KLUDGE: Do things in an attempt to get rid of extra roots. Unsafe diff --git a/src/compiler/generic/parms.lisp b/src/compiler/generic/parms.lisp index c2691bd..623aa51 100644 --- a/src/compiler/generic/parms.lisp +++ b/src/compiler/generic/parms.lisp @@ -14,6 +14,7 @@ (defparameter *c-callable-static-symbols* '(sub-gc + sb!kernel::post-gc sb!kernel::internal-error sb!kernel::control-stack-exhausted-error sb!kernel::heap-exhausted-error diff --git a/src/runtime/bsd-os.c b/src/runtime/bsd-os.c index bf71ab9..e9dacd5 100644 --- a/src/runtime/bsd-os.c +++ b/src/runtime/bsd-os.c @@ -222,11 +222,20 @@ memory_fault_handler(int signal, siginfo_t *siginfo, void *void_context #ifdef LISP_FEATURE_C_STACK_IS_CONTROL_STACK lisp_memory_fault_error(context, fault_addr); #else + + /* this disabled section is what used to be here: */ +#if 0 /* FIXME: never returns 0 */ if (!maybe_gc(context)) { interrupt_handle_now(signal, siginfo, context); } #endif + /* FIXME: Nowadays, maybe_gc does return 1 to indicate + * that GC did happen, but I'm keeping the code as it + * was. */ + maybe_gc(context); + interrupt_handle_now(signal, siginfo, context); +#endif } } diff --git a/src/runtime/gc-common.c b/src/runtime/gc-common.c index 0be3fbf..0da8a1f 100644 --- a/src/runtime/gc-common.c +++ b/src/runtime/gc-common.c @@ -2406,9 +2406,8 @@ gc_search_space(lispobj *start, size_t words, lispobj *pointer) boolean maybe_gc(os_context_t *context) { -#ifndef LISP_FEATURE_WIN32 + lispobj gc_happened; struct thread *thread = arch_os_get_current_thread(); -#endif fake_foreign_function_call(context); /* SUB-GC may return without GCing if *GC-INHIBIT* is set, in @@ -2434,32 +2433,37 @@ maybe_gc(os_context_t *context) * outer context. */ #ifndef LISP_FEATURE_WIN32 - if(SymbolValue(INTERRUPTS_ENABLED,thread)!=NIL) { - sigset_t *context_sigmask = os_context_sigmask_addr(context); -#ifdef LISP_FEATURE_SB_THREAD - /* What if the context we'd like to restore has GC signals - * blocked? Just skip the GC: we can't set GC_PENDING, because - * that would block the next attempt, and we don't know when - * we'd next check for it -- and it's hard to be sure that - * unblocking would be safe. - * - * FIXME: This is not actually much better: we may already have - * GC_PENDING set, and presumably our caller assumes that we will - * clear it. Perhaps we should, even though we don't actually GC? */ - if (sigismember(context_sigmask,SIG_STOP_FOR_GC)) { - undo_fake_foreign_function_call(context); - return 1; - } -#endif - thread_sigmask(SIG_SETMASK, context_sigmask, 0); + check_gc_signals_unblocked_in_sigset_or_lose + (os_context_sigmask_addr(context)); + unblock_gc_signals(); +#endif + FSHOW((stderr, "/maybe_gc: calling SUB_GC\n")); + /* FIXME: Nothing must go wrong during GC else we end up running + * the debugger, error handlers, and user code in general in a + * potentially unsafe place. With deferrables blocked due to + * fake_foreign_function_call + unblock_gc_signals(), we'll not + * have a pleasant time either. Running out of the control stack + * or the heap in SUB-GC are ways to lose. Of course, deferrables + * cannot be unblocked because there may be a pending handler, or + * we may even be in a WITHOUT-INTERRUPTS. */ + gc_happened = funcall0(StaticSymbolFunction(SUB_GC)); + FSHOW((stderr, "/maybe_gc: gc_happened=%s\n", + (gc_happened == NIL) ? "NIL" : "T")); + if ((gc_happened != NIL) && + /* See if interrupts are enabled or it's possible to enable + * them. POST-GC has a similar check, but we don't want to + * unlock deferrables in that case, because if there is a + * pending interrupt we'd lose, but more to point the POST-GC + * runs user code in a pretty much arbitrary place so it + * better not be in WITHOUT-INTERRUPTS. */ + ((SymbolValue(INTERRUPTS_ENABLED,thread) != NIL) || + (SymbolValue(ALLOW_WITH_INTERRUPTS,thread) != NIL))) { + FSHOW((stderr, "/maybe_gc: calling POST_GC\n")); + if (!interrupt_handler_pending_p()) + unblock_deferrable_signals(); + funcall0(StaticSymbolFunction(POST_GC)); } - else - unblock_gc_signals(); -#endif - /* SIG_STOP_FOR_GC needs to be enabled before we can call lisp: - * otherwise two threads racing here may deadlock: the other will - * wait on the GC lock, and the other cannot stop the first one... */ - funcall0(StaticSymbolFunction(SUB_GC)); undo_fake_foreign_function_call(context); - return 1; + FSHOW((stderr, "/maybe_gc: returning\n")); + return (gc_happened != NIL); } diff --git a/src/runtime/interrupt.c b/src/runtime/interrupt.c index f63dbf9..d44e53c 100644 --- a/src/runtime/interrupt.c +++ b/src/runtime/interrupt.c @@ -114,6 +114,12 @@ void sigaddset_blockable(sigset_t *sigset) { sigaddset_deferrable(sigset); + sigaddset_gc(sigset); +} + +void +sigaddset_gc(sigset_t *sigset) +{ #ifdef LISP_FEATURE_SB_THREAD sigaddset(sigset,SIG_STOP_FOR_GC); #endif @@ -122,6 +128,7 @@ sigaddset_blockable(sigset_t *sigset) /* initialized in interrupt_init */ sigset_t deferrable_sigset; sigset_t blockable_sigset; +sigset_t gc_sigset; #endif void @@ -162,11 +169,9 @@ void check_blockables_blocked_or_lose(void) { #if !defined(LISP_FEATURE_WIN32) - /* Get the current sigmask, by blocking the empty set. */ - sigset_t empty,current; + sigset_t current; int i; - sigemptyset(&empty); - thread_sigmask(SIG_BLOCK, &empty, ¤t); + fill_current_sigmask(¤t); for(i = 1; i < NSIG; i++) { if (sigismember(&blockable_sigset, i) && !sigismember(¤t, i)) lose("blockable signal %d not blocked\n",i); @@ -175,16 +180,24 @@ check_blockables_blocked_or_lose(void) } void -unblock_gc_signals(void) +check_gc_signals_unblocked_in_sigset_or_lose(sigset_t *sigset) { -#ifdef LISP_FEATURE_SB_THREAD - sigset_t new; - sigemptyset(&new); -#if defined(SIG_RESUME_FROM_GC) - sigaddset(&new,SIG_RESUME_FROM_GC); +#if !defined(LISP_FEATURE_WIN32) + int i; + for(i = 1; i < NSIG; i++) { + if (sigismember(&gc_sigset, i) && sigismember(sigset, i)) + lose("gc signal %d blocked\n",i); + } #endif - sigaddset(&new,SIG_STOP_FOR_GC); - thread_sigmask(SIG_UNBLOCK,&new,0); +} + +void +check_gc_signals_unblocked_or_lose(void) +{ +#if !defined(LISP_FEATURE_WIN32) + sigset_t current; + fill_current_sigmask(¤t); + check_gc_signals_unblocked_in_sigset_or_lose(¤t); #endif } @@ -227,11 +240,11 @@ check_interrupt_context_or_lose(os_context_t *context) #if defined(LISP_FEATURE_GENCGC) && !defined(LISP_FEATURE_PPC) #if 0 int interrupts_enabled = (SymbolValue(INTERRUPTS_ENABLED,thread) != NIL); -#endif int gc_inhibit = (SymbolValue(GC_INHIBIT,thread) != NIL); int gc_pending = (SymbolValue(GC_PENDING,thread) == T); int pseudo_atomic_interrupted = get_pseudo_atomic_interrupted(thread); int in_race_p = in_leaving_without_gcing_race_p(thread); +#endif /* In the time window between leaving the *INTERRUPTS-ENABLED* NIL * section and trapping, a SIG_STOP_FOR_GC would see the next * check fail, for this reason sig_stop_for_gc handler does not @@ -304,6 +317,14 @@ unblock_deferrable_signals(void) #endif } +void +unblock_gc_signals(void) +{ +#if defined(LISP_FEATURE_SB_THREAD) && !defined(LISP_FEATURE_WIN32) + thread_sigmask(SIG_UNBLOCK,&gc_sigset,0); +#endif +} + /* * utility routines used by various signal handlers @@ -499,6 +520,14 @@ interrupt_internal_error(os_context_t *context, boolean continuable) arch_skip_instruction(context); } +boolean +interrupt_handler_pending_p(void) +{ + struct thread *thread = arch_os_get_current_thread(); + struct interrupt_data *data = thread->interrupt_data; + return (data->pending_handler != 0); +} + void interrupt_handle_pending(os_context_t *context) { @@ -1382,8 +1411,10 @@ interrupt_init(void) see_if_sigaction_nodefer_works(); sigemptyset(&deferrable_sigset); sigemptyset(&blockable_sigset); + sigemptyset(&gc_sigset); sigaddset_deferrable(&deferrable_sigset); sigaddset_blockable(&blockable_sigset); + sigaddset_gc(&gc_sigset); /* Set up high level handler information. */ for (i = 0; i < NSIG; i++) { @@ -1478,4 +1509,3 @@ handle_trap(os_context_t *context, int trap) unhandled_trap_error(context); } } - diff --git a/src/runtime/interrupt.h b/src/runtime/interrupt.h index 210579b..e2b24c1 100644 --- a/src/runtime/interrupt.h +++ b/src/runtime/interrupt.h @@ -26,13 +26,25 @@ /* FIXME: do not rely on NSIG being a multiple of 8 */ #define REAL_SIGSET_SIZE_BYTES ((NSIG/8)) +/* Set all deferrable signals into *s. */ +extern void sigaddset_deferrable(sigset_t *s); +/* Set all blockable signals into *s. */ +extern void sigaddset_blockable(sigset_t *s); +/* Set all gc signals into *s. */ +extern void sigaddset_gc(sigset_t *s); + extern sigset_t deferrable_sigset; extern sigset_t blockable_sigset; +extern sigset_t gc_sigset; + +extern void block_blockable_signals(void); +extern void unblock_deferrable_signals(void); +extern void unblock_gc_signals(void); extern void check_deferrables_blocked_or_lose(void); extern void check_blockables_blocked_or_lose(void); extern void check_gc_signals_unblocked_or_lose(void); -extern void unblock_gc_signals(void); +extern void check_gc_signals_unblocked_in_sigset_or_lose(sigset_t *sigset); static inline void sigcopyset(sigset_t *new, sigset_t *old) @@ -83,7 +95,7 @@ struct interrupt_data { sigset_t pending_mask; }; - +extern boolean interrupt_handler_pending_p(void); extern void interrupt_init(void); extern void fake_foreign_function_call(os_context_t* context); extern void undo_fake_foreign_function_call(os_context_t* context); @@ -113,14 +125,6 @@ extern unsigned long install_handler(int signal, extern union interrupt_handler interrupt_handlers[NSIG]; -/* Set all deferrable signals into *s. */ -extern void sigaddset_deferrable(sigset_t *s); -/* Set all blockable signals into *s. */ -extern void sigaddset_blockable(sigset_t *s); - -extern void block_blockable_signals(void); -extern void unblock_deferrable_signals(void); - /* The void* casting here avoids having to mess with the various types * of function argument lists possible for signal handlers: * SA_SIGACTION handlers have one signature, and the default old-style diff --git a/version.lisp-expr b/version.lisp-expr index 85ff558..9c15bfe 100644 --- a/version.lisp-expr +++ b/version.lisp-expr @@ -17,4 +17,4 @@ ;;; checkins which aren't released. (And occasionally for internal ;;; versions, especially for internal versions off the main CVS ;;; branch, it gets hairier, e.g. "0.pre7.14.flaky4.13".) -"1.0.25.33" +"1.0.25.34" -- 1.7.10.4