[prev in list] [next in list] [prev in thread] [next in thread]
List: sbcl-commits
Subject: [Sbcl-commits] master: Finish removing dubious safepoint logic (follow-up to 1cb0a8e3)
From: Douglas Katzman via Sbcl-commits <sbcl-commits () lists ! sourceforge ! net>
Date: 2020-09-29 6:29:05
Message-ID: 1601360945.438349.20885 () sfp-scm-1 ! v30 ! lw ! sourceforge ! com
[Download RAW message or body]
The branch "master" has been updated in SBCL:
via 9630920270ebe386df7dfafb7afd21366063a7b9 (commit)
from f8901f982a0e0f8e17b8a76ab0e68159c34b19d3 (commit)
- Log -----------------------------------------------------------------
commit 9630920270ebe386df7dfafb7afd21366063a7b9
Author: Douglas Katzman <dougk@google.com>
Date: Tue Sep 29 01:55:24 2020 -0400
Finish removing dubious safepoint logic (follow-up to 1cb0a8e3)
The sb-sprof trick had two subtle flaws because a safepoint in a nested context
does not imply that it is safe to GC. It would be wrong for GC to close the
normal per-thread boxed region if it was in the midst of doing an allocation.
And that's if you can figure out how to avoid self-deadlock on free_pages_lock
without resorting back to using pseudo-atomic.
---
contrib/sb-sprof/interface.lisp | 3 +--
src/code/target-signal.lisp | 31 ++++++++++++++++---------------
src/compiler/generic/objdef.lisp | 2 --
src/compiler/sparc/macros.lisp | 2 --
src/runtime/gencgc.c | 3 ---
src/runtime/interrupt.c | 11 +----------
src/runtime/interrupt.h | 1 -
src/runtime/thread.c | 9 ---------
8 files changed, 18 insertions(+), 44 deletions(-)
diff --git a/contrib/sb-sprof/interface.lisp b/contrib/sb-sprof/interface.lisp
index 497cfab60..ff71aa3b3 100644
--- a/contrib/sb-sprof/interface.lisp
+++ b/contrib/sb-sprof/interface.lisp
@@ -194,8 +194,7 @@ The following keyword args are recognized:
(enable-call-counting)
(setf *profiled-threads* threads)
(sb-sys:enable-interrupt sb-unix:sigprof
- #'sigprof-handler
- :synchronous t)
+ #'sigprof-handler)
(ecase mode
(:alloc
(let ((alloc-signal (1- alloc-interval)))
diff --git a/src/code/target-signal.lisp b/src/code/target-signal.lisp
index eef33734a..b48eab104 100644
--- a/src/code/target-signal.lisp
+++ b/src/code/target-signal.lisp
@@ -62,7 +62,9 @@
;;; FIXME: terrible name. doesn't actually "enable" in the sense of unmasking.
(defun enable-interrupt (signal handler &key synchronous)
(declare (type (or function (member :default :ignore)) handler))
- (%install-handler signal handler synchronous)
+ (when synchronous
+ (error ":SYNCHRONOUS is broken and should not be used"))
+ (%install-handler signal handler)
;; This used to return the previously installed handler, if any.
;; It no longer does, but the old handler can be obtained via SAP-REF-LISPOBJ
;; on 'lisp_sig_handlers'. The reason for not returning it is that the value was
@@ -72,7 +74,7 @@
;; feasible, is definitely not what was intended.
nil)
-(defun %install-handler (signal handler synchronous)
+(defun %install-handler (signal handler)
(flet ((run-handler (signo info-sap context-sap)
#-(or c-stack-is-control-stack sb-safepoint) ;; able to do that in interrupt_handle_now()
(unblock-gc-signals)
@@ -82,14 +84,13 @@
;; 0 and 1 probably coincide with SIG_DFL and SIG_IGN, but those
;; constants are opaque. We use our own explicit translation
;; of them in the C install_handler() argument convention.
- (with-alien ((%sigaction (function void int unsigned int) :extern "install_handler"))
+ (with-alien ((%sigaction (function void int unsigned) :extern "install_handler"))
(alien-funcall %sigaction
signal
(case handler
(:default 0)
(:ignore 1)
- (t (sb-kernel:get-lisp-obj-address #'run-handler)))
- (if synchronous 1 0)))))
+ (t (sb-kernel:get-lisp-obj-address #'run-handler)))))))
nil)
;;;; Support for signal handlers which aren't.
@@ -218,19 +219,19 @@
(defun sb-kernel:signal-cold-init-or-reinit ()
"Enable all the default signals that Lisp knows how to deal with."
- (%install-handler sigint #'sigint-handler nil)
- (%install-handler sigterm #'sigterm-handler nil)
- (%install-handler sigill #'sigill-handler t)
- #-(or linux android haiku) (%install-handler sigemt #'sigemt-handler nil)
- (%install-handler sigfpe #'sb-vm:sigfpe-handler t)
+ (%install-handler sigint #'sigint-handler)
+ (%install-handler sigterm #'sigterm-handler)
+ (%install-handler sigill #'sigill-handler)
+ #-(or linux android haiku) (%install-handler sigemt #'sigemt-handler)
+ (%install-handler sigfpe #'sb-vm:sigfpe-handler)
(if (/= (extern-alien "install_sig_memory_fault_handler" int) 0)
- (%install-handler sigbus #'sigbus-handler t)
+ (%install-handler sigbus #'sigbus-handler)
(write-string ";;;; SIGBUS handler not installed
" sb-sys:*stderr*))
- #-(or linux android) (%install-handler sigsys #'sigsys-handler t)
- #-sb-wtimer (%install-handler sigalrm #'sigalrm-handler nil)
- #-sb-thruption (%install-handler sigurg #'sigurg-handler nil)
- (%install-handler sigchld #'sigchld-handler nil)
+ #-(or linux android) (%install-handler sigsys #'sigsys-handler)
+ #-sb-wtimer (%install-handler sigalrm #'sigalrm-handler)
+ #-sb-thruption (%install-handler sigurg #'sigurg-handler)
+ (%install-handler sigchld #'sigchld-handler)
;; Undo the effect of block_blockable_signals() from right at the top of sbcl_main()
;; and (if pertinent) blocking stop-for-GC somewhere thereafter.
(dx-let ((mask (make-array sb-unix::sizeof-sigset_t :element-type '(unsigned-byte 8)
diff --git a/src/compiler/generic/objdef.lisp b/src/compiler/generic/objdef.lisp
index 792cddf49..d89679581 100644
--- a/src/compiler/generic/objdef.lisp
+++ b/src/compiler/generic/objdef.lisp
@@ -569,8 +569,6 @@ during backtrace.
(control-stack-pointer :c-type "lispobj *")
#+mach-exception-handler
(mach-port-name :c-type "mach_port_name_t")
- #+(and sb-safepoint-strictly (not win32))
- (sprof-alloc-region :c-type "struct alloc_region" :length 4)
;; If we need the header slots, but they can't precede this structure
;; for technical reasons having to do with no writable memory being there,
;; then stuff them at the end, for lack of any place better.
diff --git a/src/compiler/sparc/macros.lisp b/src/compiler/sparc/macros.lisp
index 0d8bfe345..4742ca0d6 100644
--- a/src/compiler/sparc/macros.lisp
+++ b/src/compiler/sparc/macros.lisp
@@ -227,8 +227,6 @@
;; the branch delay slot to write back the free-pointer
;; (on overflow restore it the trap handler to a good value),
;; and fold the lowtag addition into the size subtraction.
- ;; (Possibly not ok for the sprof_alloc_region,
- ;; but sb-sprof didn't work on sparc prior to this change)
(storew result-tn null-tn 0 (- nil-value boxed-region))
;; Compute the base pointer and add lowtag.
(cond ((integerp size)
diff --git a/src/runtime/gencgc.c b/src/runtime/gencgc.c
index 22d43e076..54c869b39 100644
--- a/src/runtime/gencgc.c
+++ b/src/runtime/gencgc.c
@@ -3921,9 +3921,6 @@ collect_garbage(generation_index_t last_gen)
struct thread *th;
for_each_thread(th) {
ensure_region_closed(&th->alloc_region, BOXED_PAGE_FLAG);
-#if defined(LISP_FEATURE_SB_SAFEPOINT_STRICTLY) && !defined(LISP_FEATURE_WIN32)
- ensure_region_closed(&th->sprof_alloc_region, BOXED_PAGE_FLAG);
-#endif
}
gc_close_all_regions();
diff --git a/src/runtime/interrupt.c b/src/runtime/interrupt.c
index aa838b311..50289ea75 100644
--- a/src/runtime/interrupt.c
+++ b/src/runtime/interrupt.c
@@ -1876,9 +1876,7 @@ ll_install_handler (int signal, interrupt_handler_t handler)
#endif
/* This is called from Lisp. */
-void
-install_handler(int signal, lispobj handler,
- int __attribute__((unused)) synchronous)
+void install_handler(int signal, lispobj handler)
{
#ifndef LISP_FEATURE_WIN32
struct sigaction sa;
@@ -1898,13 +1896,6 @@ install_handler(int signal, lispobj handler,
lisp_sig_handlers[signal] = NIL;
return;
}
-#ifdef LISP_FEATURE_SB_SAFEPOINT_STRICTLY
- if (signal == SIGPROF)
- sa.sa_sigaction = sigprof_handler_trampoline;
- else if (!synchronous)
- sa.sa_sigaction = spawn_signal_thread_handler;
- else
-#endif
if (sigismember(&deferrable_sigset, signal))
sa.sa_sigaction = maybe_now_maybe_later;
else
diff --git a/src/runtime/interrupt.h b/src/runtime/interrupt.h
index 0fbb2aed9..b80c08ef7 100644
--- a/src/runtime/interrupt.h
+++ b/src/runtime/interrupt.h
@@ -122,7 +122,6 @@ extern void sig_stop_for_gc_handler(int, siginfo_t*, os_context_t*);
#endif
typedef void (*interrupt_handler_t)(int, siginfo_t *, os_context_t *);
extern void ll_install_handler(int signal, interrupt_handler_t handler);
-extern void install_handler(int signal, lispobj handler, int synchronous);
/* The void* casting here avoids having to mess with the various types
* of function argument lists possible for signal handlers:
diff --git a/src/runtime/thread.c b/src/runtime/thread.c
index f366b18d4..3dd0853ca 100644
--- a/src/runtime/thread.c
+++ b/src/runtime/thread.c
@@ -379,9 +379,6 @@ unregister_thread(struct thread *th,
block_blockable_signals(0);
ensure_region_closed(&th->alloc_region, BOXED_PAGE_FLAG);
-#if defined(LISP_FEATURE_SB_SAFEPOINT_STRICTLY) && !defined(LISP_FEATURE_WIN32)
- ensure_region_closed(&th->sprof_alloc_region, BOXED_PAGE_FLAG);
-#endif
pop_gcing_safety(&scribble->safety);
lock_ret = thread_mutex_lock(&all_threads_lock);
gc_assert(lock_ret == 0);
@@ -408,9 +405,6 @@ unregister_thread(struct thread *th,
* There's no reason for that, so closing of regions should be done
* sooner to eliminate an ordering constraint. */
ensure_region_closed(&th->alloc_region, BOXED_PAGE_FLAG);
-#if defined(LISP_FEATURE_SB_SAFEPOINT_STRICTLY) && !defined(LISP_FEATURE_WIN32)
- ensure_region_closed(&th->sprof_alloc_region, BOXED_PAGE_FLAG);
-#endif
unlink_thread(th);
thread_mutex_unlock(&all_threads_lock);
gc_assert(lock_ret == 0);
@@ -973,9 +967,6 @@ alloc_thread_struct(void* spaces, lispobj start_routine) {
#ifdef LISP_FEATURE_GENCGC
gc_init_region(&th->alloc_region);
-# if defined(LISP_FEATURE_SB_SAFEPOINT_STRICTLY) && !defined(LISP_FEATURE_WIN32)
- gc_init_region(&th->sprof_alloc_region);
-# endif
#endif
#ifdef LISP_FEATURE_SB_THREAD
/* This parallels the same logic in globals.c for the
-----------------------------------------------------------------------
hooks/post-receive
--
SBCL
_______________________________________________
Sbcl-commits mailing list
Sbcl-commits@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sbcl-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic