-
Notifications
You must be signed in to change notification settings - Fork 714
Allow interrupting blocking instructions #1930
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
Changes from 2 commits
3c2f060
185cd4d
f9c0bd1
6eecad7
9d6dc29
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 |
---|---|---|
|
@@ -1325,6 +1325,9 @@ aot_call_function(WASMExecEnv *exec_env, AOTFunctionInstance *function, | |
uint32 result_count = func_type->result_count; | ||
uint32 ext_ret_count = result_count > 1 ? result_count - 1 : 0; | ||
bool ret; | ||
#ifdef OS_ENABLE_BLOCK_INSN_INTERRUPT | ||
WASMJmpBuf jmpbuf_node = { 0 }, *jmpbuf_node_pop; | ||
#endif | ||
|
||
if (argc < func_type->param_cell_num) { | ||
char buf[108]; | ||
|
@@ -1385,8 +1388,26 @@ aot_call_function(WASMExecEnv *exec_env, AOTFunctionInstance *function, | |
} | ||
#endif | ||
|
||
ret = invoke_native_internal(exec_env, function->u.func.func_ptr, | ||
func_type, NULL, NULL, argv1, argc, argv); | ||
#ifdef OS_ENABLE_BLOCK_INSN_INTERRUPT | ||
wasm_exec_env_push_jmpbuf(exec_env, &jmpbuf_node); | ||
wasm_runtime_set_exec_env_tls(exec_env); | ||
if (os_setjmp(jmpbuf_node.jmpbuf) == 0) { | ||
#endif | ||
ret = invoke_native_internal(exec_env, function->u.func.func_ptr, | ||
eloparco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
func_type, NULL, NULL, argv1, argc, | ||
argv); | ||
#ifdef OS_ENABLE_BLOCK_INSN_INTERRUPT | ||
} | ||
else { | ||
ret = false; | ||
} | ||
|
||
jmpbuf_node_pop = wasm_exec_env_pop_jmpbuf(exec_env); | ||
bh_assert(&jmpbuf_node == jmpbuf_node_pop); | ||
eloparco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!exec_env->jmpbuf_stack_top) { | ||
wasm_runtime_set_exec_env_tls(NULL); | ||
} | ||
#endif | ||
eloparco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
#if WASM_ENABLE_DUMP_CALL_STACK != 0 | ||
if (!ret) { | ||
|
@@ -1444,8 +1465,26 @@ aot_call_function(WASMExecEnv *exec_env, AOTFunctionInstance *function, | |
} | ||
#endif | ||
|
||
ret = invoke_native_internal(exec_env, function->u.func.func_ptr, | ||
func_type, NULL, NULL, argv, argc, argv); | ||
#ifdef OS_ENABLE_BLOCK_INSN_INTERRUPT | ||
wasm_exec_env_push_jmpbuf(exec_env, &jmpbuf_node); | ||
wasm_runtime_set_exec_env_tls(exec_env); | ||
if (os_setjmp(jmpbuf_node.jmpbuf) == 0) { | ||
eloparco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#endif | ||
ret = | ||
invoke_native_internal(exec_env, function->u.func.func_ptr, | ||
func_type, NULL, NULL, argv, argc, argv); | ||
eloparco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#ifdef OS_ENABLE_BLOCK_INSN_INTERRUPT | ||
} | ||
else { | ||
ret = false; | ||
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. who frees eg. "argv1" in wasm_runtime_invoke_native on a termination? while OS_ENABLE_HW_BOUND_CHECK seems to have the same problem, 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, this is a potential issue, maybe there are two ways to resolve it: (1) avoid allocating memory in wasm_runtime_invoke_native, just define a large enough buffer for argv, (2) add a list in exec_env, add the allocating the memory to the list, and free it later. 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. (2) needs to block the signal for OS_ENABLE_BLOCK_INSN_INTERRUPT. 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. Are you referring to the call at L1396? Isn't it freed at L1458 as before my changes? 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 made a try with valgrind using interpreter and AOT, with or without HW bound check and no memory leakage is detected. Unfortunately, in case of AOT I see many warnings about invalid memory accesses that may explain the weird behavior that I reported in another comment. 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.
At this point, we can probably just have the feature disabled by default until it's well tested.
The tricky part is that it's not always possible to just block signals before the malloc and unblock after the free, because the blocking instruction can be in the middle. Apart from Not sure what's the best solution there, maybe the approach of keeping a list 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. Yes, suggest to add a list of allocated memories after setjmp in the exec_env. 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. 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. 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. Sorry, the source branch and dest branch are invalid, closed it and submitted #1948 instead |
||
} | ||
|
||
jmpbuf_node_pop = wasm_exec_env_pop_jmpbuf(exec_env); | ||
bh_assert(&jmpbuf_node == jmpbuf_node_pop); | ||
if (!exec_env->jmpbuf_stack_top) { | ||
wasm_runtime_set_exec_env_tls(NULL); | ||
} | ||
#endif | ||
|
||
#if WASM_ENABLE_DUMP_CALL_STACK != 0 | ||
if (aot_get_exception(module_inst)) { | ||
|
@@ -1471,7 +1510,7 @@ aot_create_exec_env_and_call_function(AOTModuleInstance *module_inst, | |
WASMExecEnv *exec_env = NULL, *existing_exec_env = NULL; | ||
bool ret; | ||
|
||
#if defined(OS_ENABLE_HW_BOUND_CHECK) | ||
#if defined(OS_ENABLE_HW_BOUND_CHECK) || defined(OS_ENABLE_BLOCK_INSN_INTERRUPT) | ||
existing_exec_env = exec_env = wasm_runtime_get_exec_env_tls(); | ||
#elif WASM_ENABLE_THREAD_MGR != 0 | ||
existing_exec_env = exec_env = | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,12 +128,14 @@ runtime_malloc(uint64 size, WASMModuleInstanceCommon *module_inst, | |
static JitCompOptions jit_options = { 0 }; | ||
#endif | ||
|
||
#ifdef OS_ENABLE_HW_BOUND_CHECK | ||
#if defined(OS_ENABLE_HW_BOUND_CHECK) || defined(OS_ENABLE_BLOCK_INSN_INTERRUPT) | ||
/* The exec_env of thread local storage, set before calling function | ||
and used in signal handler, as we cannot get it from the argument | ||
of signal handler */ | ||
static os_thread_local_attribute WASMExecEnv *exec_env_tls = NULL; | ||
#endif | ||
|
||
#ifdef OS_ENABLE_HW_BOUND_CHECK | ||
#ifndef BH_PLATFORM_WINDOWS | ||
static void | ||
runtime_signal_handler(void *sig_addr) | ||
|
@@ -303,7 +305,9 @@ runtime_signal_destroy() | |
#endif | ||
os_thread_signal_destroy(); | ||
} | ||
#endif /* end of OS_ENABLE_HW_BOUND_CHECK */ | ||
|
||
#if defined(OS_ENABLE_HW_BOUND_CHECK) || defined(OS_ENABLE_BLOCK_INSN_INTERRUPT) | ||
void | ||
wasm_runtime_set_exec_env_tls(WASMExecEnv *exec_env) | ||
{ | ||
|
@@ -315,7 +319,20 @@ wasm_runtime_get_exec_env_tls() | |
{ | ||
return exec_env_tls; | ||
} | ||
#endif /* end of OS_ENABLE_HW_BOUND_CHECK */ | ||
#endif | ||
|
||
#ifdef OS_ENABLE_BLOCK_INSN_INTERRUPT | ||
static void | ||
interrupt_block_insn_sig_handler(int sig) | ||
{ | ||
bh_assert(sig == SIGUSR1); | ||
eloparco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
WASMJmpBuf *jmpbuf_node = exec_env_tls->jmpbuf_stack_top; | ||
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. jmpbuf_stack_top here can be for another handler. 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, if HW bound check is enabled, it will get a "HW bound" 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. you can even get a signal between wasm_exec_env_push_jmpbuf and os_setjmp. 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.
In that case, the I wonder if we have to mask/un-mask signals to avoid the signal handler being called in the middle. We want to avoid signal handlers being called between 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. it isn't a problem for hw bound check because the signal for it is basically synchronous to the thread execution. ie. the thread itself can control where it can happen. 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. Can we maybe invert the order of But there are probably other cases that are not covered, like an exception before 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 tried that in the latest commit, but maybe it's safer to use something like
Apart from this, do you think there are other cases that are not covered? If we get the signal before |
||
bh_assert(jmpbuf_node); | ||
|
||
os_longjmp(jmpbuf_node->jmpbuf, 1); | ||
} | ||
#endif /* OS_ENABLE_BLOCK_INSN_INTERRUPT */ | ||
|
||
static bool | ||
wasm_runtime_env_init() | ||
|
@@ -348,7 +365,11 @@ wasm_runtime_env_init() | |
goto fail5; | ||
} | ||
#endif | ||
|
||
#ifdef OS_ENABLE_BLOCK_INSN_INTERRUPT | ||
if (!os_interrupt_block_insn_init(interrupt_block_insn_sig_handler)) { | ||
goto fail6; | ||
eloparco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
#endif | ||
#ifdef OS_ENABLE_HW_BOUND_CHECK | ||
if (!runtime_signal_init()) { | ||
goto fail6; | ||
|
@@ -404,8 +425,10 @@ wasm_runtime_env_init() | |
fail7: | ||
#endif | ||
#endif | ||
#if defined(OS_ENABLE_HW_BOUND_CHECK) || defined(OS_ENABLE_BLOCK_INSN_INTERRUPT) | ||
#ifdef OS_ENABLE_HW_BOUND_CHECK | ||
runtime_signal_destroy(); | ||
#endif | ||
fail6: | ||
#endif | ||
#if (WASM_ENABLE_WAMR_COMPILER == 0) && (WASM_ENABLE_THREAD_MGR != 0) | ||
|
Uh oh!
There was an error while loading. Please reload this page.