From 38c67b3f4815a628ce0f9bc22186bface4678089 Mon Sep 17 00:00:00 2001 From: Wenyong Huang Date: Fri, 24 Feb 2023 20:05:39 +0800 Subject: [PATCH] thread-mgr: Fix spread "wasi proc exit" exception and atomic.wait issues (#1988) Raising "wasi proc exit" exception, spreading it to other threads and then clearing it in all threads may result in unexpected behavior: the sub thread may end first, handle the "wasi proc exit" exception and clear exceptions of other threads, including the main thread. And when main thread's exception is cleared, it may continue to run and throw "unreachable" exception. This also leads to some assertion failed. Ignore exception spreading for "wasi proc exit" and don't clear exception of other threads to resolve the issue. And add suspend flag check after atomic wait since the atomic wait may be notified by other thread when exception occurs. --- core/iwasm/common/wasm_runtime_common.c | 6 ++-- core/iwasm/compilation/aot_emit_function.c | 16 ++++++++++ core/iwasm/compilation/aot_emit_memory.c | 11 ++++++- core/iwasm/interpreter/wasm_interp_classic.c | 14 +++++++-- core/iwasm/interpreter/wasm_interp_fast.c | 11 +++++++ .../libraries/thread-mgr/thread_manager.c | 15 ++++++--- samples/wasi-threads/README.md | 1 + .../wasm-apps/thread_termination.c | 31 +++++++++++++------ 8 files changed, 85 insertions(+), 20 deletions(-) diff --git a/core/iwasm/common/wasm_runtime_common.c b/core/iwasm/common/wasm_runtime_common.c index 4aa3d7ed..0b09a9db 100644 --- a/core/iwasm/common/wasm_runtime_common.c +++ b/core/iwasm/common/wasm_runtime_common.c @@ -1880,8 +1880,10 @@ clear_wasi_proc_exit_exception(WASMModuleInstanceCommon *module_inst_comm) if (exception && !strcmp(exception, "Exception: wasi proc exit")) { /* The "wasi proc exit" exception is thrown by native lib to let wasm app exit, which is a normal behavior, we clear - the exception here. */ - wasm_set_exception(module_inst, NULL); + the exception here. And just clear the exception of current + thread, don't call `wasm_set_exception(module_inst, NULL)` + which will clear the exception of all threads. */ + module_inst->cur_exception[0] = '\0'; return true; } return false; diff --git a/core/iwasm/compilation/aot_emit_function.c b/core/iwasm/compilation/aot_emit_function.c index 4ac62a9e..9ba8baa2 100644 --- a/core/iwasm/compilation/aot_emit_function.c +++ b/core/iwasm/compilation/aot_emit_function.c @@ -999,6 +999,14 @@ aot_compile_op_call(AOTCompContext *comp_ctx, AOTFuncContext *func_ctx, } #endif +#if WASM_ENABLE_THREAD_MGR != 0 + /* Insert suspend check point */ + if (comp_ctx->enable_thread_mgr) { + if (!check_suspend_flags(comp_ctx, func_ctx)) + goto fail; + } +#endif + ret = true; fail: if (param_types) @@ -1645,6 +1653,14 @@ aot_compile_op_call_indirect(AOTCompContext *comp_ctx, AOTFuncContext *func_ctx, } #endif +#if WASM_ENABLE_THREAD_MGR != 0 + /* Insert suspend check point */ + if (comp_ctx->enable_thread_mgr) { + if (!check_suspend_flags(comp_ctx, func_ctx)) + goto fail; + } +#endif + ret = true; fail: diff --git a/core/iwasm/compilation/aot_emit_memory.c b/core/iwasm/compilation/aot_emit_memory.c index 7224d9f5..26d9f922 100644 --- a/core/iwasm/compilation/aot_emit_memory.c +++ b/core/iwasm/compilation/aot_emit_memory.c @@ -7,6 +7,7 @@ #include "aot_emit_exception.h" #include "../aot/aot_runtime.h" #include "aot_intrinsic.h" +#include "aot_emit_control.h" #define BUILD_ICMP(op, left, right, res, name) \ do { \ @@ -1344,7 +1345,7 @@ aot_compile_op_atomic_wait(AOTCompContext *comp_ctx, AOTFuncContext *func_ctx, return false; } - BUILD_ICMP(LLVMIntSGT, ret_value, I32_ZERO, cmp, "atomic_wait_ret"); + BUILD_ICMP(LLVMIntNE, ret_value, I32_NEG_ONE, cmp, "atomic_wait_ret"); ADD_BASIC_BLOCK(wait_fail, "atomic_wait_fail"); ADD_BASIC_BLOCK(wait_success, "wait_success"); @@ -1368,6 +1369,14 @@ aot_compile_op_atomic_wait(AOTCompContext *comp_ctx, AOTFuncContext *func_ctx, PUSH_I32(ret_value); +#if WASM_ENABLE_THREAD_MGR != 0 + /* Insert suspend check point */ + if (comp_ctx->enable_thread_mgr) { + if (!check_suspend_flags(comp_ctx, func_ctx)) + return false; + } +#endif + return true; fail: return false; diff --git a/core/iwasm/interpreter/wasm_interp_classic.c b/core/iwasm/interpreter/wasm_interp_classic.c index 7b193f63..ac6763c6 100644 --- a/core/iwasm/interpreter/wasm_interp_classic.c +++ b/core/iwasm/interpreter/wasm_interp_classic.c @@ -3422,6 +3422,10 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module, if (ret == (uint32)-1) goto got_exception; +#if WASM_ENABLE_THREAD_MGR != 0 + CHECK_SUSPEND_FLAGS(); +#endif + PUSH_I32(ret); break; } @@ -3442,6 +3446,10 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module, if (ret == (uint32)-1) goto got_exception; +#if WASM_ENABLE_THREAD_MGR != 0 + CHECK_SUSPEND_FLAGS(); +#endif + PUSH_I32(ret); break; } @@ -3894,10 +3902,10 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module, PUSH_CSP(LABEL_TYPE_FUNCTION, 0, cell_num, frame_ip_end - 1); wasm_exec_env_set_cur_frame(exec_env, frame); -#if WASM_ENABLE_THREAD_MGR != 0 - CHECK_SUSPEND_FLAGS(); -#endif } +#if WASM_ENABLE_THREAD_MGR != 0 + CHECK_SUSPEND_FLAGS(); +#endif HANDLE_OP_END(); } diff --git a/core/iwasm/interpreter/wasm_interp_fast.c b/core/iwasm/interpreter/wasm_interp_fast.c index be924646..7a8f474c 100644 --- a/core/iwasm/interpreter/wasm_interp_fast.c +++ b/core/iwasm/interpreter/wasm_interp_fast.c @@ -3266,6 +3266,10 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module, if (ret == (uint32)-1) goto got_exception; +#if WASM_ENABLE_THREAD_MGR != 0 + CHECK_SUSPEND_FLAGS(); +#endif + PUSH_I32(ret); break; } @@ -3286,6 +3290,10 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module, if (ret == (uint32)-1) goto got_exception; +#if WASM_ENABLE_THREAD_MGR != 0 + CHECK_SUSPEND_FLAGS(); +#endif + PUSH_I32(ret); break; } @@ -3826,6 +3834,9 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module, wasm_exec_env_set_cur_frame(exec_env, (WASMRuntimeFrame *)frame); } +#if WASM_ENABLE_THREAD_MGR != 0 + CHECK_SUSPEND_FLAGS(); +#endif HANDLE_OP_END(); } diff --git a/core/iwasm/libraries/thread-mgr/thread_manager.c b/core/iwasm/libraries/thread-mgr/thread_manager.c index 4d66beb6..79048c04 100644 --- a/core/iwasm/libraries/thread-mgr/thread_manager.c +++ b/core/iwasm/libraries/thread-mgr/thread_manager.c @@ -225,7 +225,9 @@ wasm_cluster_create(WASMExecEnv *exec_env) /* Prepare the aux stack top and size for every thread */ if (!wasm_exec_env_get_aux_stack(exec_env, &aux_stack_start, &aux_stack_size)) { +#if WASM_ENABLE_LIB_WASI_THREADS == 0 LOG_VERBOSE("No aux stack info for this module, can't create thread"); +#endif /* If the module don't have aux stack info, don't throw error here, but remain stack_tops and stack_segment_occupied as NULL */ @@ -1131,9 +1133,14 @@ set_exception_visitor(void *node, void *user_data) WASMModuleInstance *curr_wasm_inst = (WASMModuleInstance *)get_module_inst(curr_exec_env); - bh_memcpy_s(curr_wasm_inst->cur_exception, - sizeof(curr_wasm_inst->cur_exception), - wasm_inst->cur_exception, sizeof(wasm_inst->cur_exception)); + /* Only spread non "wasi proc exit" exception */ + if (!strstr(wasm_inst->cur_exception, "wasi proc exit")) { + bh_memcpy_s(curr_wasm_inst->cur_exception, + sizeof(curr_wasm_inst->cur_exception), + wasm_inst->cur_exception, + sizeof(wasm_inst->cur_exception)); + } + /* Terminate the thread so it can exit from dead loops */ set_thread_cancel_flags(curr_exec_env); } @@ -1203,4 +1210,4 @@ bool wasm_cluster_is_thread_terminated(WASMExecEnv *exec_env) { return (exec_env->suspend_flags.flags & 0x01) ? true : false; -} \ No newline at end of file +} diff --git a/samples/wasi-threads/README.md b/samples/wasi-threads/README.md index 499162b6..ca8d166d 100644 --- a/samples/wasi-threads/README.md +++ b/samples/wasi-threads/README.md @@ -26,6 +26,7 @@ $ ./iwasm wasm-apps/exception_propagation.wasm ## Run samples in AOT mode ```shell $ ../../../wamr-compiler/build/wamrc \ + --enable-multi-thread \ -o wasm-apps/no_pthread.aot wasm-apps/no_pthread.wasm $ ./iwasm wasm-apps/no_pthread.aot ``` diff --git a/samples/wasi-threads/wasm-apps/thread_termination.c b/samples/wasi-threads/wasm-apps/thread_termination.c index dfc228eb..355bb3f4 100644 --- a/samples/wasi-threads/wasm-apps/thread_termination.c +++ b/samples/wasi-threads/wasm-apps/thread_termination.c @@ -15,8 +15,14 @@ #include "wasi_thread_start.h" -#define TEST_TERMINATION_BY_TRAP 0 // Otherwise test `proc_exit` termination -#define TEST_TERMINATION_IN_MAIN_THREAD 1 +#define BUSY_WAIT 0 +#define ATOMIC_WAIT 1 +#define POLL_ONEOFF 2 + +/* Change parameters here to modify the sample behavior */ +#define TEST_TERMINATION_BY_TRAP 0 /* Otherwise `proc_exit` termination */ +#define TEST_TERMINATION_IN_MAIN_THREAD 1 /* Otherwise in spawn thread */ +#define LONG_TASK_IMPL ATOMIC_WAIT #define TIMEOUT_SECONDS 10 #define NUM_THREADS 3 @@ -30,23 +36,28 @@ typedef struct { void run_long_task() { - // Busy waiting to be interruptible by trap or `proc_exit` +#if LONG_TASK_IMPL == BUSY_WAIT for (int i = 0; i < TIMEOUT_SECONDS; i++) sleep(1); +#elif LONG_TASK_IMPL == ATOMIC_WAIT + __builtin_wasm_memory_atomic_wait32(0, 0, -1); +#else + sleep(TIMEOUT_SECONDS); +#endif } void start_job() { sem_post(&sem); - run_long_task(); // Wait to be interrupted + run_long_task(); /* Wait to be interrupted */ assert(false && "Unreachable"); } void terminate_process() { - // Wait for all other threads (including main thread) to be ready + /* Wait for all other threads (including main thread) to be ready */ printf("Waiting before terminating\n"); for (int i = 0; i < NUM_THREADS; i++) sem_wait(&sem); @@ -55,7 +66,7 @@ terminate_process() #if TEST_TERMINATION_BY_TRAP == 1 __builtin_trap(); #else - __wasi_proc_exit(1); + __wasi_proc_exit(33); #endif } @@ -86,14 +97,14 @@ main(int argc, char **argv) } for (i = 0; i < NUM_THREADS; i++) { - // No graceful memory free to simplify the example + /* No graceful memory free to simplify the example */ if (!start_args_init(&data[i].base)) { printf("Failed to allocate thread's stack\n"); return EXIT_FAILURE; } } -// Create a thread that forces termination through trap or `proc_exit` + /* Create a thread that forces termination through trap or `proc_exit` */ #if TEST_TERMINATION_IN_MAIN_THREAD == 1 data[0].throw_exception = false; #else @@ -105,7 +116,7 @@ main(int argc, char **argv) return EXIT_FAILURE; } - // Create two additional threads to test exception propagation + /* Create two additional threads to test exception propagation */ data[1].throw_exception = false; thread_id = __wasi_thread_spawn(&data[1]); if (thread_id < 0) { @@ -128,4 +139,4 @@ main(int argc, char **argv) start_job(); #endif /* TEST_TERMINATION_IN_MAIN_THREAD */ return EXIT_SUCCESS; -} \ No newline at end of file +}