1.0.25.34: gc trigger improvements
authorGabor Melis <mega@hotpop.com>
Mon, 16 Feb 2009 21:53:25 +0000 (21:53 +0000)
committerGabor Melis <mega@hotpop.com>
Mon, 16 Feb 2009 21:53:25 +0000 (21:53 +0000)
- 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
src/code/gc.lisp
src/compiler/generic/parms.lisp
src/runtime/bsd-os.c
src/runtime/gc-common.c
src/runtime/interrupt.c
src/runtime/interrupt.h
version.lisp-expr

diff --git a/NEWS b/NEWS
index efefafa..0226ff5 100644 (file)
--- 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
index f3b9615..32aa109 100644 (file)
@@ -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
index c2691bd..623aa51 100644 (file)
@@ -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
index bf71ab9..e9dacd5 100644 (file)
@@ -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
         }
 }
 
index 0be3fbf..0da8a1f 100644 (file)
@@ -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);
 }
index f63dbf9..d44e53c 100644 (file)
@@ -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, &current);
+    fill_current_sigmask(&current);
     for(i = 1; i < NSIG; i++) {
         if (sigismember(&blockable_sigset, i) && !sigismember(&current, 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(&current);
+    check_gc_signals_unblocked_in_sigset_or_lose(&current);
 #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
+}
+
 \f
 /*
  * 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);
     }
 }
-
index 210579b..e2b24c1 100644 (file)
 /* 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
index 85ff558..9c15bfe 100644 (file)
@@ -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"