Skip to content

Do not fetch the next instruction when proc_exit is propagated #1975

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 10 commits into from
Closed

Do not fetch the next instruction when proc_exit is propagated #1975

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9f68c4f
fix(wasi-threads): return exit code set by proc_exit
eloparco Feb 20, 2023
f837651
fix: add suspend flags check in AOT mode
eloparco Feb 22, 2023
9c51c31
fix: fix atomic wait return code comparison
eloparco Feb 23, 2023
fcb1f0c
doc: update wasi-threads readme
eloparco Feb 23, 2023
56ee840
fix: add check for thread termination after function call in AOT mode
eloparco Feb 23, 2023
7d5a808
fix: remove assert in signal handler
eloparco Feb 23, 2023
6192719
fix: update assert condition and move termination check
eloparco Feb 23, 2023
1f04944
fix: revert changes in previous commit
eloparco Feb 24, 2023
e4075dc
fix: add option to use atomic wait and poll_oneoff in thread terminat…
eloparco Feb 24, 2023
fcc45c8
fix: clear resources when thread terminated in AOT mode
eloparco Feb 24, 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
1 change: 0 additions & 1 deletion core/iwasm/common/wasm_runtime_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,6 @@ runtime_signal_handler(void *sig_addr)
else if (exec_env_tls->exce_check_guard_page <= (uint8 *)sig_addr
&& (uint8 *)sig_addr
< exec_env_tls->exce_check_guard_page + page_size) {
bh_assert(wasm_get_exception(module_inst));
Copy link
Contributor

@wenyongh wenyongh Feb 23, 2023

Choose a reason for hiding this comment

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

Had better not remove and modify to:

    bh_assert(wasm_get_exception(module_inst)
#if WASM_ENABLE_THREAD_MGR != 0
              || wasm_cluster_is_thread_terminated(exec_env_tls));
#endif
);

And also change the similar place for Windows (this file, L253):
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/common/wasm_runtime_common.c#L253

Copy link
Contributor Author

@eloparco eloparco Feb 23, 2023

Choose a reason for hiding this comment

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

Right, I'm debugging to understand why exec_env_tls->suspend_flags is not set properly once I get there.

Copy link
Contributor Author

@eloparco eloparco Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, the problem was that we don't set the suspend flag for the thread that originally raises the exception. I pushed a fix but I noticed that it's breaking the spec tests. Any clue on what's breaking?

Copy link
Contributor Author

@eloparco eloparco Feb 24, 2023

Choose a reason for hiding this comment

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

The problem is that the assert would be executed also on the thread that originally raises the exception and that thread doesn't have the suspend flag set. I tried setting the suspend flag on that initial thread too but it break things since it forces the initial thread to exit prematurely (after a CHECK_SUSPEND_FLAGS();) while we don't want that.

So I was thinking of just having a

#if WASM_ENABLE_THREAD_MGR == 0
    bh_assert(wasm_get_exception(module_inst));
#endif

Copy link
Contributor

@wenyongh wenyongh Feb 24, 2023

Choose a reason for hiding this comment

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

Not sure how to reproduce the issue? The thread_terminate sample with TEST_TERMINATION_BY_TRAP == 1 and TEST_TERMINATION_IN_MAIN_THREAD == 1?

Copy link
Contributor Author

@eloparco eloparco Feb 24, 2023

Choose a reason for hiding this comment

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

I think we may merge this PR firstly if you are OK and we will submit another PR to fix that issue.

Let's do it, so that I can move to #1985 that hopefully should succeed after we merge this.

Copy link
Contributor

@wenyongh wenyongh Feb 24, 2023

Choose a reason for hiding this comment

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

@eloparco I tried again, and think this is not a best way to resolve the issue. The root cause is that the main thread's "wasi proc exit" exception is cleared by the sub thread after the latter gets the spread exception from main thread and then clear exception for all threads.
It is strange to spread the "wasi proc exit" exception to other threads and then clear it in other threads. I think we can just ignore spreading of this exception and no need to clear it again. I uploaded PR #1988 to fix the issues, I tested the cases and didn't find the issues you mentioned (unreachable exception was thrown and bh_assert failure), could you help review and try? Thanks.

Copy link
Contributor Author

@eloparco eloparco Feb 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can we merge this one and rebase #1988? So that it's easier to review. I'll then try #1988 in a few different cases to see if we don't have regressions.

Copy link
Contributor

@wenyongh wenyongh Feb 24, 2023

Choose a reason for hiding this comment

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

My personal preference is we had better not merge a temporarily PR which doesn't fix the root cause, #1988 should be better and have resolved the issues mentioned in this PR. I tend to merge #1988 directly.

Copy link
Contributor Author

@eloparco eloparco Feb 24, 2023

Choose a reason for hiding this comment

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

Right, it makes sense. Let's close this one once #1988 gets merged

os_longjmp(jmpbuf_node->jmpbuf, 1);
}
}
Expand Down
8 changes: 8 additions & 0 deletions core/iwasm/compilation/aot_emit_function.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -857,6 +857,14 @@ aot_compile_op_call(AOTCompContext *comp_ctx, AOTFuncContext *func_ctx,
goto fail;
}

#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

/* Check whether there was exception thrown when executing
the function */
if (!check_exception_thrown(comp_ctx, func_ctx)) {
Expand Down
11 changes: 10 additions & 1 deletion core/iwasm/compilation/aot_emit_memory.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 { \
Expand Down Expand Up @@ -1344,7 +1345,15 @@ aot_compile_op_atomic_wait(AOTCompContext *comp_ctx, AOTFuncContext *func_ctx,
return false;
}

BUILD_ICMP(LLVMIntSGT, ret_value, I32_ZERO, cmp, "atomic_wait_ret");
#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

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");
Expand Down
12 changes: 10 additions & 2 deletions core/iwasm/interpreter/wasm_interp_classic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3419,6 +3419,10 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module,
ret = wasm_runtime_atomic_wait(
(WASMModuleInstanceCommon *)module, maddr,
(uint64)expect, timeout, false);

#if WASM_ENABLE_THREAD_MGR != 0
CHECK_SUSPEND_FLAGS();
#endif
if (ret == (uint32)-1)
goto got_exception;

Expand All @@ -3439,6 +3443,10 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module,
ret = wasm_runtime_atomic_wait(
(WASMModuleInstanceCommon *)module, maddr, expect,
timeout, true);

#if WASM_ENABLE_THREAD_MGR != 0
CHECK_SUSPEND_FLAGS();
#endif
if (ret == (uint32)-1)
goto got_exception;

Expand Down Expand Up @@ -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();
CHECK_SUSPEND_FLAGS();
#endif
}
HANDLE_OP_END();
}

Expand Down
11 changes: 11 additions & 0 deletions core/iwasm/interpreter/wasm_interp_fast.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3263,6 +3263,10 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module,
ret = wasm_runtime_atomic_wait(
(WASMModuleInstanceCommon *)module, maddr,
(uint64)expect, timeout, false);

#if WASM_ENABLE_THREAD_MGR != 0
CHECK_SUSPEND_FLAGS();
#endif
if (ret == (uint32)-1)
goto got_exception;

Expand All @@ -3283,6 +3287,10 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module,
ret = wasm_runtime_atomic_wait(
(WASMModuleInstanceCommon *)module, maddr, expect,
timeout, true);

#if WASM_ENABLE_THREAD_MGR != 0
CHECK_SUSPEND_FLAGS();
#endif
if (ret == (uint32)-1)
goto got_exception;

Expand Down Expand Up @@ -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();
}

Expand Down
2 changes: 2 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 @@ -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 */
Expand Down
1 change: 1 addition & 0 deletions samples/wasi-threads/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
```