-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Free the alternate signal stack when the main OCaml code / an OCaml thread stops #10726
Changes from all commits
04118b0
b70fac2
14038f2
9f270eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -288,6 +288,36 @@ void caml_init_signals(void) | |
#endif | ||
} | ||
|
||
/* Termination of signal stuff */ | ||
|
||
#if defined(TARGET_power) || defined(TARGET_s390x) \ | ||
|| defined(HAS_STACK_OVERFLOW_DETECTION) | ||
static void set_signal_default(int signum) | ||
{ | ||
struct sigaction act; | ||
sigemptyset(&act.sa_mask); | ||
act.sa_handler = SIG_DFL; | ||
act.sa_flags = 0; | ||
sigaction(signum, &act, NULL); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be better here to be using the storing the old action from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's because of library / pooled uses of libasmrun that it's important not to leave OCaml's signal handlers installed. However, I'm not sure about restoring the old action instead of going back to the default action. If the application has SIGSEGV, etc, handlers installed before starting the OCaml runtime, I assume these handlers have a purpose, and this purpose is broken when the OCaml runtime is started. So, I think it's pretty safe to assume that the special signals (SIGSEGV, etc) have default behavior before the OCaml runtime is started. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense - I guess we could therefore document that in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea of documenting what happens with signal handling. See commit df89f2a in this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks great, modulo a pedantic nit (which I seem to have managed to comment on the commit, rather than the PR!) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pedantic nit addressed. I moved the corresponding change to a separate commit. |
||
} | ||
#endif | ||
|
||
void caml_terminate_signals(void) | ||
{ | ||
#if defined(TARGET_power) | ||
set_signal_default(SIGTRAP); | ||
#endif | ||
|
||
#if defined(TARGET_s390x) | ||
set_signal_default(SIGFPE); | ||
#endif | ||
|
||
#ifdef HAS_STACK_OVERFLOW_DETECTION | ||
set_signal_default(SIGSEGV); | ||
caml_stop_stack_overflow_detection(); | ||
#endif | ||
} | ||
|
||
/* Allocate and select an alternate stack for handling signals, | ||
especially SIGSEGV signals. | ||
Each thread needs its own alternate stack. | ||
|
@@ -308,3 +338,19 @@ CAMLexport int caml_setup_stack_overflow_detection(void) | |
return 0; | ||
#endif | ||
} | ||
|
||
CAMLexport int caml_stop_stack_overflow_detection(void) | ||
{ | ||
#ifdef HAS_STACK_OVERFLOW_DETECTION | ||
stack_t oldstk, stk; | ||
stk.ss_flags = SS_DISABLE; | ||
if (sigaltstack(&stk, &oldstk) == -1) return -1; | ||
/* If caml_setup_stack_overflow_detection failed, we are not using | ||
an alternate signal stack. SS_DISABLE will be set in oldstk, | ||
and there is nothing to free in this case. */ | ||
if (! (oldstk.ss_flags & SS_DISABLE)) free(oldstk.ss_sp); | ||
return 0; | ||
#else | ||
return 0; | ||
#endif | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,7 @@ | |
#include "caml/mlvalues.h" | ||
#include "caml/osdeps.h" | ||
#include "caml/printexc.h" | ||
#include "caml/signals.h" | ||
#include "caml/stack.h" | ||
#include "caml/startup_aux.h" | ||
#include "caml/sys.h" | ||
|
@@ -91,7 +92,6 @@ struct longjmp_buffer caml_termination_jmpbuf; | |
void (*caml_termination_hook)(void *) = NULL; | ||
|
||
extern value caml_start_program (caml_domain_state*); | ||
extern void caml_init_signals (void); | ||
#ifdef _WIN32 | ||
extern void caml_win32_overflow_detection (void); | ||
#endif | ||
|
@@ -106,6 +106,7 @@ extern void caml_install_invalid_parameter_handler(); | |
value caml_startup_common(char_os **argv, int pooling) | ||
{ | ||
char_os * exe_name, * proc_self_exe; | ||
value res; | ||
char tos; | ||
|
||
/* Initialize the domain */ | ||
|
@@ -152,10 +153,13 @@ value caml_startup_common(char_os **argv, int pooling) | |
exe_name = caml_search_exe_in_path(exe_name); | ||
caml_sys_init(exe_name, argv); | ||
if (sigsetjmp(caml_termination_jmpbuf.buf, 0)) { | ||
caml_terminate_signals(); | ||
if (caml_termination_hook != NULL) caml_termination_hook(NULL); | ||
return Val_unit; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't you also call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well spotted! This code path corresponds to the initial thread calling Come to think of it, there's also |
||
} | ||
return caml_start_program(Caml_state); | ||
res = caml_start_program(Caml_state); | ||
caml_terminate_signals(); | ||
return res; | ||
} | ||
|
||
value caml_startup_exn(char_os **argv) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,2 @@ | ||
# ocamlyacc doesn't clean memory on exit | ||
leak:ocamlyacc | ||
# Alternate signal stacks are currently never freed (see #10266) | ||
leak:caml_setup_stack_overflow_detection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the consistent use of the term "stuff" from
caml_init_signals
🙂There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consistent use of precise terminology is important :-)