From 58513220fcf87e161a5e0d3fbadd76c45f27d584 Mon Sep 17 00:00:00 2001 From: Gabor Melis Date: Mon, 22 Jun 2009 12:58:22 +0000 Subject: [PATCH] 1.0.29.32: SCRUB-CONTROL-STACK related changes - remove unused count logic from SCRUB-CONTROL-STACK - fix SCRUB-CONTROL-STACK being uncareful about touching the guard page - threads stopped by gc do a quick scrubbing of the control stack to slightly lessen the probability of uninitialized stack locations pointing to live objects --- src/code/exhaust.lisp | 8 +-- src/code/gc.lisp | 2 + src/code/toplevel.lisp | 99 +------------------------------------- src/compiler/generic/objdef.lisp | 1 + src/runtime/cheneygc.c | 27 +---------- src/runtime/gc-common.c | 74 ++++++++++++++++++++++++++++ src/runtime/gc-internal.h | 2 + src/runtime/interrupt.c | 28 +++++++++-- src/runtime/thread.c | 1 + src/runtime/validate.c | 12 ++--- version.lisp-expr | 2 +- 11 files changed, 115 insertions(+), 141 deletions(-) diff --git a/src/code/exhaust.lisp b/src/code/exhaust.lisp index 98fa6bc..f35ac45 100644 --- a/src/code/exhaust.lisp +++ b/src/code/exhaust.lisp @@ -11,10 +11,4 @@ ;;;; files for more information. (in-package "SB!KERNEL") -(define-alien-routine ("protect_control_stack_guard_page" - %protect-control-stack-guard-page) - sb!alien:void - (protect-p sb!alien:int) - (thread sb!alien:int)) -(defun protect-control-stack-guard-page (n) - (%protect-control-stack-guard-page (if n 1 0) 0)) +(define-alien-routine reset-control-stack-guard-page sb!alien:void) diff --git a/src/code/gc.lisp b/src/code/gc.lisp index c128995..06d38a0 100644 --- a/src/code/gc.lisp +++ b/src/code/gc.lisp @@ -285,6 +285,8 @@ run in any thread.") (when (sub-gc :gen (if full 6 gen)) (post-gc))) +(define-alien-routine scrub-control-stack sb!alien:void) + (defun unsafe-clear-roots () ;; KLUDGE: Do things in an attempt to get rid of extra roots. Unsafe ;; as having these cons more then we have space left leads to huge diff --git a/src/code/toplevel.lisp b/src/code/toplevel.lisp index ba8c00c..419e8a1 100644 --- a/src/code/toplevel.lisp +++ b/src/code/toplevel.lisp @@ -179,103 +179,6 @@ command-line.") (sb!win32:millisleep (truncate (* n 1000))) nil) -;;;; SCRUB-CONTROL-STACK - -(defconstant bytes-per-scrub-unit 2048) - -;;; Zero the unused portion of the control stack so that old objects -;;; are not kept alive because of uninitialized stack variables. - -;;; "To summarize the problem, since not all allocated stack frame -;;; slots are guaranteed to be written by the time you call an another -;;; function or GC, there may be garbage pointers retained in your -;;; dead stack locations. The stack scrubbing only affects the part -;;; of the stack from the SP to the end of the allocated stack." -;;; - ram, on cmucl-imp, Tue, 25 Sep 2001 - -;;; So, as an (admittedly lame) workaround, from time to time we call -;;; scrub-control-stack to zero out all the unused portion. This is -;;; supposed to happen when the stack is mostly empty, so that we have -;;; a chance of clearing more of it: callers are currently (2002.07.18) -;;; REPL and SUB-GC - -(defun scrub-control-stack () - (declare (optimize (speed 3) (safety 0)) - (values (unsigned-byte 20))) ; FIXME: DECLARE VALUES? - - #!-stack-grows-downward-not-upward - (let* ((csp (sap-int (sb!c::control-stack-pointer-sap))) - (initial-offset (logand csp (1- bytes-per-scrub-unit))) - (end-of-stack - (- (sap-int (sb!di::descriptor-sap sb!vm:*control-stack-end*)) - (* 2 sb!c:*backend-page-bytes*)))) - (labels - ((scrub (ptr offset count) - (declare (type system-area-pointer ptr) - (type (unsigned-byte 16) offset) - (type (unsigned-byte 20) count) - (values (unsigned-byte 20))) - (cond ((>= (sap-int ptr) end-of-stack) 0) - ((= offset bytes-per-scrub-unit) - (look (sap+ ptr bytes-per-scrub-unit) 0 count)) - (t - (setf (sap-ref-word ptr offset) 0) - (scrub ptr (+ offset sb!vm:n-word-bytes) count)))) - (look (ptr offset count) - (declare (type system-area-pointer ptr) - (type (unsigned-byte 16) offset) - (type (unsigned-byte 20) count) - (values (unsigned-byte 20))) - (cond ((>= (sap-int ptr) end-of-stack) 0) - ((= offset bytes-per-scrub-unit) - count) - ((zerop (sap-ref-word ptr offset)) - (look ptr (+ offset sb!vm:n-word-bytes) count)) - (t - (scrub ptr offset (+ count sb!vm:n-word-bytes)))))) - (declare (type sb!vm::word csp)) - (scrub (int-sap (- csp initial-offset)) - (* (floor initial-offset sb!vm:n-word-bytes) sb!vm:n-word-bytes) - 0))) - - #!+stack-grows-downward-not-upward - (let* ((csp (sap-int (sb!c::control-stack-pointer-sap))) - (end-of-stack (+ (sap-int - (sb!di::descriptor-sap sb!vm:*control-stack-start*)) - (* 2 sb!c:*backend-page-bytes*))) - (initial-offset (logand csp (1- bytes-per-scrub-unit)))) - (labels - ((scrub (ptr offset count) - (declare (type system-area-pointer ptr) - (type (unsigned-byte 16) offset) - (type (unsigned-byte 20) count) - (values (unsigned-byte 20))) - (let ((loc (int-sap (- (sap-int ptr) (+ offset sb!vm:n-word-bytes))))) - (cond ((< (sap-int loc) end-of-stack) 0) - ((= offset bytes-per-scrub-unit) - (look (int-sap (- (sap-int ptr) bytes-per-scrub-unit)) - 0 count)) - (t ;; need to fix bug in %SET-STACK-REF - (setf (sap-ref-word loc 0) 0) - (scrub ptr (+ offset sb!vm:n-word-bytes) count))))) - (look (ptr offset count) - (declare (type system-area-pointer ptr) - (type (unsigned-byte 16) offset) - (type (unsigned-byte 20) count) - (values (unsigned-byte 20))) - (let ((loc (int-sap (- (sap-int ptr) offset)))) - (cond ((< (sap-int loc) end-of-stack) 0) - ((= offset bytes-per-scrub-unit) - count) - ((zerop (sb!kernel::get-lisp-obj-address (stack-ref loc 0))) - (look ptr (+ offset sb!vm:n-word-bytes) count)) - (t - (scrub ptr offset (+ count sb!vm:n-word-bytes))))))) - (declare (type sb!vm::word csp)) - (scrub (int-sap (+ csp initial-offset)) - (* (floor initial-offset sb!vm:n-word-bytes) sb!vm:n-word-bytes) - 0)))) - ;;;; the default toplevel function (defvar / nil @@ -609,7 +512,7 @@ that provides the REPL for the system. Assumes that *STANDARD-INPUT* and ;; should have unwound enough stack by the time we get ;; here that this is now possible. #!-win32 - (sb!kernel::protect-control-stack-guard-page 1) + (sb!kernel::reset-control-stack-guard-page) (funcall repl-fun noprint) (critically-unreachable "after REPL"))))))))) diff --git a/src/compiler/generic/objdef.lisp b/src/compiler/generic/objdef.lisp index dd22370..b4d231b 100644 --- a/src/compiler/generic/objdef.lisp +++ b/src/compiler/generic/objdef.lisp @@ -396,6 +396,7 @@ (binding-stack-pointer :c-type "lispobj *" :length #!+alpha 2 #!-alpha 1) (control-stack-start :c-type "lispobj *" :length #!+alpha 2 #!-alpha 1) (control-stack-end :c-type "lispobj *" :length #!+alpha 2 #!-alpha 1) + (control-stack-guard-page-protected) (alien-stack-start :c-type "lispobj *" :length #!+alpha 2 #!-alpha 1) (alien-stack-pointer :c-type "lispobj *" :length #!+alpha 2 #!-alpha 1) #!+gencgc (alloc-region :c-type "struct alloc_region" :length 5) diff --git a/src/runtime/cheneygc.c b/src/runtime/cheneygc.c index 62bd6de..cf79fc3 100644 --- a/src/runtime/cheneygc.c +++ b/src/runtime/cheneygc.c @@ -61,31 +61,6 @@ tv_diff(struct timeval *x, struct timeval *y) } #endif -#define BYTES_ZERO_BEFORE_END (1<<12) - -/* FIXME do we need this? Doesn't it duplicate lisp code in - * scrub-control-stack? */ - -static void -zero_stack(void) -{ - lispobj *ptr = current_control_stack_pointer; - search: - do { - if (*ptr) - goto fill; - ptr++; - } while (((unsigned long)ptr) & (BYTES_ZERO_BEFORE_END-1)); - return; - fill: - do { - *ptr++ = 0; - } while (((unsigned long)ptr) & (BYTES_ZERO_BEFORE_END-1)); - - goto search; -} - - void * gc_general_alloc(long bytes, int page_type_flag, int quick_p) { lispobj *new=new_space_free_pointer; @@ -258,7 +233,7 @@ collect_garbage(generation_index_t ignore) #ifdef PRINTNOISE printf("Zeroing empty part of control stack ...\n"); #endif - zero_stack(); + scrub_control_stack(); set_auto_gc_trigger(size_retained+bytes_consed_between_gcs); thread_sigmask(SIG_SETMASK, &old, 0); diff --git a/src/runtime/gc-common.c b/src/runtime/gc-common.c index d38c9b5..a9f69b6 100644 --- a/src/runtime/gc-common.c +++ b/src/runtime/gc-common.c @@ -2471,3 +2471,77 @@ maybe_gc(os_context_t *context) FSHOW((stderr, "/maybe_gc: returning\n")); return (gc_happened != NIL); } + +#define BYTES_ZERO_BEFORE_END (1<<12) + +/* There used to be a similar function called SCRUB-CONTROL-STACK in + * Lisp and another called zero_stack() in cheneygc.c, but since it's + * shorter to express in, and more often called from C, I keep only + * the C one after fixing it. -- MG 2009-03-25 */ + +/* Zero the unused portion of the control stack so that old objects + * are not kept alive because of uninitialized stack variables. + * + * "To summarize the problem, since not all allocated stack frame + * slots are guaranteed to be written by the time you call an another + * function or GC, there may be garbage pointers retained in your dead + * stack locations. The stack scrubbing only affects the part of the + * stack from the SP to the end of the allocated stack." - ram, on + * cmucl-imp, Tue, 25 Sep 2001 + * + * So, as an (admittedly lame) workaround, from time to time we call + * scrub-control-stack to zero out all the unused portion. This is + * supposed to happen when the stack is mostly empty, so that we have + * a chance of clearing more of it: callers are currently (2002.07.18) + * REPL, SUB-GC and sig_stop_for_gc_handler. */ + +/* Take care not to tread on the guard page and the hard guard page as + * it would be unkind to sig_stop_for_gc_handler. Touching the return + * guard page is not dangerous. For this to work the guard page must + * be zeroed when protected. */ + +/* FIXME: I think there is no guarantee that once + * BYTES_ZERO_BEFORE_END bytes are zero the rest are also zero. This + * may be what the "lame" adjective in the above comment is for. In + * this case, exact gc may lose badly. */ +void +scrub_control_stack(void) +{ + struct thread *th = arch_os_get_current_thread(); + os_vm_address_t guard_page_address = CONTROL_STACK_GUARD_PAGE(th); + os_vm_address_t hard_guard_page_address = CONTROL_STACK_HARD_GUARD_PAGE(th); + lispobj *sp; +#ifdef LISP_FEATURE_C_STACK_IS_CONTROL_STACK + sp = (lispobj *)&sp - 1; +#else + sp = current_control_stack_pointer; +#endif + scrub: + if ((((os_vm_address_t)sp < (hard_guard_page_address + os_vm_page_size)) && + ((os_vm_address_t)sp >= hard_guard_page_address)) || + (((os_vm_address_t)sp < (guard_page_address + os_vm_page_size)) && + ((os_vm_address_t)sp >= guard_page_address) && + (th->control_stack_guard_page_protected != NIL))) + return; +#ifdef LISP_FEATURE_STACK_GROWS_DOWNWARD_NOT_UPWARD + do { + *sp = 0; + } while (((unsigned long)sp--) & (BYTES_ZERO_BEFORE_END - 1)); + if ((os_vm_address_t)sp < (hard_guard_page_address + os_vm_page_size)) + return; + do { + if (*sp) + goto scrub; + } while (((unsigned long)sp--) & (BYTES_ZERO_BEFORE_END - 1)); +#else + do { + *sp = 0; + } while (((unsigned long)++sp) & (BYTES_ZERO_BEFORE_END - 1)); + if ((os_vm_address_t)sp >= hard_guard_page_address) + return; + do { + if (*sp) + goto scrub; + } while (((unsigned long)++sp) & (BYTES_ZERO_BEFORE_END - 1)); +#endif +} diff --git a/src/runtime/gc-internal.h b/src/runtime/gc-internal.h index c1e0834..3d43fab 100644 --- a/src/runtime/gc-internal.h +++ b/src/runtime/gc-internal.h @@ -129,6 +129,8 @@ lispobj *search_dynamic_space(void *pointer); lispobj *gc_search_space(lispobj *start, size_t words, lispobj *pointer); +extern void scrub_control_stack(); + #include "fixnump.h" #ifdef LISP_FEATURE_GENCGC diff --git a/src/runtime/interrupt.c b/src/runtime/interrupt.c index 4f33522..294fc64 100644 --- a/src/runtime/interrupt.c +++ b/src/runtime/interrupt.c @@ -1242,6 +1242,12 @@ sig_stop_for_gc_handler(int signal, siginfo_t *info, os_context_t *context) set_thread_state(thread,STATE_SUSPENDED); FSHOW_SIGNAL((stderr,"suspended\n")); + /* While waiting for gc to finish occupy ourselves with zeroing + * the unused portion of the control stack to reduce conservatism. + * On hypothetic platforms with threads and exact gc it is + * actually a must. */ + scrub_control_stack(); + wait_for_thread_state_change(thread, STATE_SUSPENDED); FSHOW_SIGNAL((stderr,"resumed\n")); @@ -1479,6 +1485,19 @@ undefined_alien_function(void) funcall0(StaticSymbolFunction(UNDEFINED_ALIEN_FUNCTION_ERROR)); } +/* Called from the REPL, too. */ +void reset_control_stack_guard_page(void) +{ + struct thread *th=arch_os_get_current_thread(); + if (th->control_stack_guard_page_protected == NIL) { + memset(CONTROL_STACK_GUARD_PAGE(th), 0, os_vm_page_size); + protect_control_stack_guard_page(1, NULL); + protect_control_stack_return_guard_page(0, NULL); + th->control_stack_guard_page_protected = T; + fprintf(stderr, "INFO: Control stack guard page reprotected\n"); + } +} + boolean handle_guard_page_triggered(os_context_t *context,os_vm_address_t addr) { @@ -1494,8 +1513,11 @@ handle_guard_page_triggered(os_context_t *context,os_vm_address_t addr) * protection so the error handler has some headroom, protect the * previous page so that we can catch returns from the guard page * and restore it. */ + if (th->control_stack_guard_page_protected == NIL) + lose("control_stack_guard_page_protected NIL"); protect_control_stack_guard_page(0, NULL); protect_control_stack_return_guard_page(1, NULL); + th->control_stack_guard_page_protected = NIL; fprintf(stderr, "INFO: Control stack guard page unprotected\n"); #ifdef LISP_FEATURE_C_STACK_IS_CONTROL_STACK @@ -1513,9 +1535,9 @@ handle_guard_page_triggered(os_context_t *context,os_vm_address_t addr) * unprotect this one. This works even if we somehow missed * the return-guard-page, and hit it on our way to new * exhaustion instead. */ - protect_control_stack_guard_page(1, NULL); - protect_control_stack_return_guard_page(0, NULL); - fprintf(stderr, "INFO: Control stack guard page reprotected\n"); + if (th->control_stack_guard_page_protected != NIL) + lose("control_stack_guard_page_protected not NIL"); + reset_control_stack_guard_page(); return 1; } else if(addr >= BINDING_STACK_HARD_GUARD_PAGE(th) && diff --git a/src/runtime/thread.c b/src/runtime/thread.c index 08627d6..53ae855 100644 --- a/src/runtime/thread.c +++ b/src/runtime/thread.c @@ -413,6 +413,7 @@ create_thread_struct(lispobj initial_function) { th->binding_stack_start= (lispobj*)((void*)th->control_stack_start+thread_control_stack_size); th->control_stack_end = th->binding_stack_start; + th->control_stack_guard_page_protected = T; th->alien_stack_start= (lispobj*)((void*)th->binding_stack_start+BINDING_STACK_SIZE); th->binding_stack_pointer=th->binding_stack_start; diff --git a/src/runtime/validate.c b/src/runtime/validate.c index 2afaa68..d00c391 100644 --- a/src/runtime/validate.c +++ b/src/runtime/validate.c @@ -89,12 +89,12 @@ protect_page(void *page, int protect_p, os_vm_prot_t flags) { flags : OS_VM_PROT_ALL); } -#define DEF_PROTECT_PAGE(name,page_name,flags) \ - void \ - protect_##name(int protect_p, struct thread *thread) { \ - if (!thread) \ - thread = arch_os_get_current_thread(); \ - protect_page(page_name(thread), protect_p, flags); \ +#define DEF_PROTECT_PAGE(name,page_name,flags) \ + void \ + protect_##name(int protect_p, struct thread *thread) { \ + if (!thread) \ + thread = arch_os_get_current_thread(); \ + protect_page(page_name(thread), protect_p, flags); \ } DEF_PROTECT_PAGE(control_stack_hard_guard_page, diff --git a/version.lisp-expr b/version.lisp-expr index 8428885..a7f8d85 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.29.31" +"1.0.29.32" -- 1.7.10.4