Skip to content

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.

Sign up for GitHub

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

Closed
eloparco wants to merge 5 commits into from
Closed

Allow interrupting blocking instructions #1930

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3c2f060
fix(wasi-threads): interrupt blocking instructions using signals
eloparco Feb 2, 2023
185cd4d
fix(wasi-threads): fix platform includes
eloparco Feb 3, 2023
f9c0bd1
refactor(wasi-threads): move platform-related code and remove "block …
eloparco Feb 4, 2023
6eecad7
feat(wasi-threads): use blocking operations in thread termination exa…
eloparco Feb 5, 2023
9d6dc29
fix(wasi-threads): handle signal handler being called before os_setjmp
eloparco Feb 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions build-scripts/config_common.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,12 @@ else ()
add_definitions (-DWASM_DISABLE_STACK_HW_BOUND_CHECK=0)
endif ()
endif ()
if (WAMR_DISABLE_BLOCK_INSN_INTERRUPT EQUAL 1)
Comment thread
eloparco marked this conversation as resolved.
add_definitions (-DWASM_DISABLE_BLOCK_INSN_INTERRUPT=1)
message (" Interruption of blocking instructions disabled")
else ()
add_definitions (-DWASM_DISABLE_BLOCK_INSN_INTERRUPT=0)
endif ()
if (WAMR_BUILD_SIMD EQUAL 1)
if (NOT WAMR_BUILD_TARGET MATCHES "RISCV64.*")
add_definitions (-DWASM_ENABLE_SIMD=1)
Expand Down
35 changes: 30 additions & 5 deletions core/iwasm/aot/aot_runtime.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down Expand Up @@ -1385,8 +1388,19 @@ 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,
Comment thread
eloparco marked this conversation as resolved.
Outdated
func_type, NULL, NULL, argv1, argc,
argv);
#ifdef OS_ENABLE_BLOCK_INSN_INTERRUPT

jmpbuf_node_pop = wasm_exec_env_pop_jmpbuf(exec_env);
bh_assert(&jmpbuf_node == jmpbuf_node_pop);
Comment thread
eloparco marked this conversation as resolved.
Outdated
#endif
Comment thread
eloparco marked this conversation as resolved.
Outdated

#if WASM_ENABLE_DUMP_CALL_STACK != 0
if (!ret) {
Expand Down Expand Up @@ -1444,8 +1458,19 @@ 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)
Comment thread
eloparco marked this conversation as resolved.
Outdated
#endif
ret =
invoke_native_internal(exec_env, function->u.func.func_ptr,
func_type, NULL, NULL, argv, argc, argv);
Comment thread
eloparco marked this conversation as resolved.
Outdated
#ifdef OS_ENABLE_BLOCK_INSN_INTERRUPT

jmpbuf_node_pop = wasm_exec_env_pop_jmpbuf(exec_env);
bh_assert(&jmpbuf_node == jmpbuf_node_pop);
#endif

#if WASM_ENABLE_DUMP_CALL_STACK != 0
if (aot_get_exception(module_inst)) {
Expand All @@ -1471,7 +1496,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 =
Expand Down
2 changes: 1 addition & 1 deletion core/iwasm/common/wasm_exec_env.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ wasm_exec_env_set_thread_arg(WASMExecEnv *exec_env, void *thread_arg)
}
#endif

#ifdef OS_ENABLE_HW_BOUND_CHECK
#if defined(OS_ENABLE_HW_BOUND_CHECK) || defined(OS_ENABLE_BLOCK_INSN_INTERRUPT)
void
wasm_exec_env_push_jmpbuf(WASMExecEnv *exec_env, WASMJmpBuf *jmpbuf)
{
Expand Down
8 changes: 5 additions & 3 deletions core/iwasm/common/wasm_exec_env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ typedef struct WASMCurrentEnvStatus WASMCurrentEnvStatus;
#endif
#endif

#ifdef OS_ENABLE_HW_BOUND_CHECK
#if defined(OS_ENABLE_HW_BOUND_CHECK) || defined(OS_ENABLE_BLOCK_INSN_INTERRUPT)
typedef struct WASMJmpBuf {
struct WASMJmpBuf *prev;
korp_jmpbuf jmpbuf;
Expand Down Expand Up @@ -135,8 +135,10 @@ typedef struct WASMExecEnv {
BlockAddr block_addr_cache[BLOCK_ADDR_CACHE_SIZE][BLOCK_ADDR_CONFLICT_SIZE];
#endif

#ifdef OS_ENABLE_HW_BOUND_CHECK
#if defined(OS_ENABLE_HW_BOUND_CHECK) || defined(OS_ENABLE_BLOCK_INSN_INTERRUPT)
WASMJmpBuf *jmpbuf_stack_top;
#endif
#ifdef OS_ENABLE_HW_BOUND_CHECK
/* One guard page for the exception check */
uint8 *exce_check_guard_page;
#endif
Expand Down Expand Up @@ -291,7 +293,7 @@ void
wasm_exec_env_set_thread_arg(WASMExecEnv *exec_env, void *thread_arg);
#endif

#ifdef OS_ENABLE_HW_BOUND_CHECK
#if defined(OS_ENABLE_HW_BOUND_CHECK) || defined(OS_ENABLE_BLOCK_INSN_INTERRUPT)
void
wasm_exec_env_push_jmpbuf(WASMExecEnv *exec_env, WASMJmpBuf *jmpbuf);

Expand Down
42 changes: 39 additions & 3 deletions core/iwasm/common/wasm_runtime_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
{
Expand All @@ -315,7 +319,35 @@ 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
os_interrupt_block_insn_sig_handler(int sig)
{
bh_assert(sig == SIGUSR1);
Comment thread
eloparco marked this conversation as resolved.
Outdated

WASMJmpBuf *jmpbuf_node = exec_env_tls->jmpbuf_stack_top;

@yamt yamt Feb 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jmpbuf_stack_top here can be for another handler.

@eloparco eloparco Feb 6, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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" jmpbuf #1930 (comment), but that should not be a problem unless I'm missing something

@yamt yamt Feb 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

@eloparco eloparco Feb 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

In that case, the jmpbuf would be invalid, as it is in general before being set up by os_setjmp. Isn't that the same for the HW bound check before my PR?

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 wasm_exec_env_push_jmpbuf and os_setjmp as they would use a non-initialized jmpbuf iiuc.

@yamt yamt Feb 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

@eloparco eloparco Feb 7, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we maybe invert the order of wasm_exec_env_push_jmpbuf and os_setjmp? So that the jmpbuf is only pushed after being initialized by os_setjmp.

But there are probably other cases that are not covered, like an exception before os_setjmp. [EDIT] These cases shouldn't be a problem since they're already handled by the normal exception spreading mechanism.

@eloparco eloparco Feb 7, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we maybe invert the order of wasm_exec_env_push_jmpbuf and os_setjmp? So that the jmpbuf is only pushed after being initialized by os_setjmp.

I tried that in the latest commit, but maybe it's safer to use something like static volatile sig_atomic_t canjump; (but with tls) as explained here https://notes.shichao.io/apue/ch10/?

you can even get a signal between wasm_exec_env_push_jmpbuf and os_setjmp.

Apart from this, do you think there are other cases that are not covered? If we get the signal before os_setjmp, the signal handler (for instruction interruption) will return and the thread will stop when starting to execute instructions, using the normal exception propagation mechanism.

bh_assert(jmpbuf_node);

os_longjmp(jmpbuf_node->jmpbuf, 1);
}

bool
os_interrupt_block_insn_init()
{
struct sigaction act;
memset(&act, 0, sizeof(act));
act.sa_handler = os_interrupt_block_insn_sig_handler;
sigfillset(&act.sa_mask);
if (sigaction(SIGUSR1, &act, NULL) < 0) {
LOG_ERROR("failed to set signal handler");
return false;
}

return true;
}
#endif
Comment thread
eloparco marked this conversation as resolved.
Outdated

static bool
wasm_runtime_env_init()
Expand Down Expand Up @@ -348,7 +380,11 @@ wasm_runtime_env_init()
goto fail5;
}
#endif

#ifdef OS_ENABLE_BLOCK_INSN_INTERRUPT
if (!os_interrupt_block_insn_init()) {
goto fail5;
}
#endif
#ifdef OS_ENABLE_HW_BOUND_CHECK
if (!runtime_signal_init()) {
goto fail6;
Expand Down
2 changes: 2 additions & 0 deletions core/iwasm/common/wasm_runtime_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,9 @@ typedef struct WASMSignalInfo {
EXCEPTION_POINTERS *exce_info;
#endif
} WASMSignalInfo;
#endif

#if defined(OS_ENABLE_HW_BOUND_CHECK) || defined(OS_ENABLE_BLOCK_INSN_INTERRUPT)
/* Set exec_env of thread local storage */
void
wasm_runtime_set_exec_env_tls(WASMExecEnv *exec_env);
Expand Down
18 changes: 16 additions & 2 deletions core/iwasm/interpreter/wasm_runtime.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2137,11 +2137,25 @@ wasm_call_function(WASMExecEnv *exec_env, WASMFunctionInstance *function,
{
WASMModuleInstance *module_inst =
(WASMModuleInstance *)exec_env->module_inst;
#ifdef OS_ENABLE_BLOCK_INSN_INTERRUPT
WASMJmpBuf jmpbuf_node = { 0 }, *jmpbuf_node_pop;
#endif

/* set thread handle and stack boundary */
wasm_exec_env_set_thread_info(exec_env);

interp_call_wasm(module_inst, exec_env, function, 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
interp_call_wasm(module_inst, exec_env, function, argc, argv);
#ifdef OS_ENABLE_BLOCK_INSN_INTERRUPT

jmpbuf_node_pop = wasm_exec_env_pop_jmpbuf(exec_env);
bh_assert(&jmpbuf_node == jmpbuf_node_pop);
#endif

return !wasm_get_exception(module_inst) ? true : false;
}

Expand All @@ -2153,7 +2167,7 @@ wasm_create_exec_env_and_call_function(WASMModuleInstance *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 =
Expand Down
4 changes: 4 additions & 0 deletions core/iwasm/libraries/thread-mgr/thread_manager.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1064,6 +1064,10 @@ set_exception_visitor(void *node, void *user_data)
bh_memcpy_s(curr_wasm_inst->cur_exception,
sizeof(curr_wasm_inst->cur_exception),
wasm_inst->cur_exception, sizeof(wasm_inst->cur_exception));

bh_assert(curr_exec_env->handle);
os_thread_signal(curr_exec_env->handle, SIGUSR1);
Comment thread
eloparco marked this conversation as resolved.
Outdated

/* Terminate the thread so it can exit from dead loops */
set_thread_cancel_flags(curr_exec_env);
}
Expand Down
17 changes: 11 additions & 6 deletions core/shared/platform/android/platform_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,19 +63,17 @@ typedef sem_t korp_sem;

#define bh_socket_t int

#if WASM_DISABLE_BLOCK_INSN_INTERRUPT == 0
#define OS_ENABLE_BLOCK_INSN_INTERRUPT
#endif
Comment thread
eloparco marked this conversation as resolved.
Outdated

#if WASM_DISABLE_HW_BOUND_CHECK == 0
#if defined(BUILD_TARGET_X86_64) || defined(BUILD_TARGET_AMD_64) \
|| defined(BUILD_TARGET_AARCH64) || defined(BUILD_TARGET_RISCV64_LP64D) \
|| defined(BUILD_TARGET_RISCV64_LP64)

#include <setjmp.h>

#define OS_ENABLE_HW_BOUND_CHECK

typedef jmp_buf korp_jmpbuf;

#define os_setjmp setjmp
#define os_longjmp longjmp
#define os_alloca alloca

#define os_getpagesize getpagesize
Expand All @@ -99,6 +97,13 @@ os_sigreturn();
#endif /* end of BUILD_TARGET_X86_64/AMD_64/AARCH64/RISCV64 */
#endif /* end of WASM_DISABLE_HW_BOUND_CHECK */

#if defined(OS_ENABLE_BLOCK_INSN_INTERRUPT) || defined(OS_ENABLE_HW_BOUND_CHECK)
#include <setjmp.h>
typedef jmp_buf korp_jmpbuf;
#define os_setjmp setjmp
#define os_longjmp longjmp
#endif

typedef long int __syscall_slong_t;

#if __ANDROID_API__ < 19
Expand Down
6 changes: 6 additions & 0 deletions core/shared/platform/common/posix/posix_thread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,12 @@ os_thread_exit(void *retval)
return pthread_exit(retval);
}

int
os_thread_signal(korp_tid tid, int sig)
{
return pthread_kill(tid, sig);
}

#if defined(os_thread_local_attribute)
static os_thread_local_attribute uint8 *thread_stack_boundary = NULL;
#endif
Expand Down
17 changes: 11 additions & 6 deletions core/shared/platform/darwin/platform_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,19 +66,17 @@ typedef sem_t korp_sem;

#define bh_socket_t int

#if WASM_DISABLE_BLOCK_INSN_INTERRUPT == 0
#define OS_ENABLE_BLOCK_INSN_INTERRUPT
#endif

#if WASM_DISABLE_HW_BOUND_CHECK == 0
#if defined(BUILD_TARGET_X86_64) || defined(BUILD_TARGET_AMD_64) \
|| defined(BUILD_TARGET_AARCH64) || defined(BUILD_TARGET_RISCV64_LP64D) \
|| defined(BUILD_TARGET_RISCV64_LP64)

#include <setjmp.h>

#define OS_ENABLE_HW_BOUND_CHECK

typedef jmp_buf korp_jmpbuf;

#define os_setjmp setjmp
#define os_longjmp longjmp
#define os_alloca alloca

#define os_getpagesize getpagesize
Expand All @@ -102,6 +100,13 @@ os_sigreturn();
#endif /* end of BUILD_TARGET_X86_64/AMD_64/AARCH64/RISCV64 */
#endif /* end of WASM_DISABLE_HW_BOUND_CHECK */

#if defined(OS_ENABLE_BLOCK_INSN_INTERRUPT) || defined(OS_ENABLE_HW_BOUND_CHECK)
#include <setjmp.h>
typedef jmp_buf korp_jmpbuf;
#define os_setjmp setjmp
#define os_longjmp longjmp
#endif

#ifdef __cplusplus
}
#endif
Expand Down
17 changes: 11 additions & 6 deletions core/shared/platform/freebsd/platform_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,19 +65,17 @@ typedef sem_t korp_sem;

#define bh_socket_t int

#if WASM_DISABLE_BLOCK_INSN_INTERRUPT == 0
#define OS_ENABLE_BLOCK_INSN_INTERRUPT
#endif

#if WASM_DISABLE_HW_BOUND_CHECK == 0
#if defined(BUILD_TARGET_X86_64) || defined(BUILD_TARGET_AMD_64) \
|| defined(BUILD_TARGET_AARCH64) || defined(BUILD_TARGET_RISCV64_LP64D) \
|| defined(BUILD_TARGET_RISCV64_LP64)

#include <setjmp.h>

#define OS_ENABLE_HW_BOUND_CHECK

typedef jmp_buf korp_jmpbuf;

#define os_setjmp setjmp
#define os_longjmp longjmp
#define os_alloca alloca

#define os_getpagesize getpagesize
Expand All @@ -101,6 +99,13 @@ os_sigreturn();
#endif /* end of BUILD_TARGET_X86_64/AMD_64/AARCH64/RISCV64 */
#endif /* end of WASM_DISABLE_HW_BOUND_CHECK */

#if defined(OS_ENABLE_BLOCK_INSN_INTERRUPT) || defined(OS_ENABLE_HW_BOUND_CHECK)
#include <setjmp.h>
typedef jmp_buf korp_jmpbuf;
#define os_setjmp setjmp
#define os_longjmp longjmp
#endif

#ifdef __cplusplus
}
#endif
Expand Down
3 changes: 3 additions & 0 deletions core/shared/platform/include/platform_api_extension.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@ int os_thread_detach(korp_tid);
void
os_thread_exit(void *retval);

int
os_thread_signal(korp_tid tid, int sig);

/**
* Initialize current thread environment if current thread
* is created by developer but not runtime
Expand Down
Loading