handle a return from wasi _start function correctly #2529

yamt · 2023-09-04T10:48:20Z

this fixes a few test cases in wasi-threads testsuite like wasi_threads_return_main_block.

also, move the special handling for wasi proc exit to a more appropriate place.

this fixes a few test cases in wasi-threads testsuite like wasi_threads_return_main_block. also, move the special handling for wasi proc exit to a more appropriate place.

yamt · 2023-09-04T10:53:58Z

extracted from #2516

wenyongh · 2023-09-06T05:08:24Z

core/iwasm/common/wasm_application.c

+        if (ret) {
+            wasm_runtime_set_exception(module_inst, wasi_proc_exit_exception);
+            /* exit_code is zero-initialized */
+            ret = false;


Not sure why need to set wasi_proc_exit_exception here? If ret is true, there should be no exception thrown and wasi_exit_code was already initialized as 0, seems we can do nothing?

to propagate it to other threads.

But the main thread will wait until all other threads exit in wasm_exec_env_destroy, it doesn't meet the "wasi proc exit" exception, seems no need to propagate "wasi proc exit" for other threads to exit early? Or other threads may not finish their jobs normally, also other threads may fail to throw exception if they continue to run?

a return from _start should behave as exit(0) because it's what wasi-libc expects.

exit(0) should terminate other threads.

wasm_runtime_set_exception here is just a convenient way to terminate other threads.

That's what I my concern is, suppose the sub thread is doing an important job, e.g. do some calculation, output important data to a file, can we terminate it from the main thread? Is it better to wait until the sub threads exit? In fact, there is API wasm_cluster_wait_for_all_except_self in thread manager.

Not sure whether there is reference about "a return from _start should behave as exit(0)" in wasi-libc?

@loganek What's your opinion?

i'm not sure if i understand your concern.
the importance of the job is something apps should consider. not runtime.

Not sure whether there is reference about "a return from _start should behave as exit(0)" in wasi-libc?

a return from main() is an equivalent of exit(). (C standard)

wasi-libc maps a return from main() to a return from _start when the exit code is 0. https://github.com/WebAssembly/wasi-libc/blob/ec4566beae84e54952637f0bf61bee4b4cacc087/libc-bottom-half/crt/crt1-command.c#L48-L52

I am not very sure whether it is a good behavior that the main thread terminates all others thread instead of waiting for them. But if you want to do that, can we just call the API wasm_cluster_terminate_all_except_self of thread manager:

#if WASM_ENABLE_THREAD_MGR != 0 if (ret) { WASMCluster *cluster = wasm_exec_env_get_cluster(exec_env); wasm_cluster_terminate_all_except_self(cluster, exec_env); } #endif

It also sets the terminate flag for other threads.

i chose to use wasm_runtime_set_exception because:

what i want to do here is to mimic wasi proc_exit, which uses wasm_runtime_set_exception.

wasm_cluster_terminate_all_except_self seems unused (thus untested) and broken. (cluster->lock vs cluster_list_lock locking order issue.)

we don't need to join threads here

OK, if to use wasm_runtime_set_exception, could you please add comments to mention somewhat like we want to mimic wasi proc_exit and to terminate other threads? It is not very easy to understand the code :)
And could you wrap the code with #if WASM_ENABLE_THREAD_MGR != 0 .. #endif, it is only for multi-threading?

OK, if to use wasm_runtime_set_exception, could you please add comments to mention somewhat like we want to mimic wasi proc_exit and to terminate other threads? It is not very easy to understand the code :) And could you wrap the code with #if WASM_ENABLE_THREAD_MGR != 0 .. #endif, it is only for multi-threading?

i prefer less #ifdef in general. but ok. done.

wenyongh · 2023-09-06T05:14:14Z

core/iwasm/common/wasm_runtime_common.c

-            }
-            return false;
+        if (new_argv != argv) {
+            wasm_runtime_free(new_argv);


clear_wasi_proc_exit_exception is only applied after calling wasi _start function now, is it a general handling of wasi? Will developer call other functions in wasi mode and then wasm_proc_exit occurs?

i guess it's a gray area.
proc_exit is supposed to terminate the "process".
the concept of "process" only makes sense in the context of a wasi command.

Sounds reasonable, maybe we can apply the changes first, and modify if needed in the future.

lum1n0us · 2023-09-06T05:51:26Z

core/iwasm/common/wasm_application.c

+        }
+        /* report wasm proc exit as a success */
+        WASMModuleInstance *inst = (WASMModuleInstance *)module_inst;
+        if (!ret && strstr(inst->cur_exception, wasi_proc_exit_exception)) {


It seems both L117 and L115 make if in L121 always true.

it's false if (ret == false and cur_exception == some other exception).

lum1n0us · 2023-09-07T06:06:57Z

core/iwasm/common/wasm_application.c

       may cause exception thrown. */
    if ((func = wasm_runtime_lookup_wasi_start_function(module_inst))) {
-        return wasm_runtime_call_wasm(exec_env, func, 0, NULL);
+        const char *wasi_proc_exit_exception = "wasi proc exit";


Is it better if we use a global variable or macro to represent the string "wasi proc exit"？ It is a very import constant string now。

ideally we should stop using strings i guess.

also, make some code conditional on WASM_ENABLE_THREAD_MGR

lum1n0us · 2023-09-07T07:12:55Z

LGTM

wenyongh

LGTM

eloparco · 2023-09-07T07:42:34Z

I know we already had this conversation with @yamt, but with these changes, should we update trap_after_main_thread_finishes.c to have 0 as exit code and to replace the trap here with an assert? And even change the name of the tests and put some comments to describe the expected behavior. I could do it as a separate PR if it makes sense.

Or we drop the test completely since it has some overlapping with https://github.com/WebAssembly/wasi-threads/blob/main/test/testsuite/wasi_threads_exit_main_block.wat.

Because the test had another goal initially, to make sure the spawned threads kept running and its trap error code was propagated to the runtime. Now the implemented behavior is different instead. So I think we should update the test or drop it (it doesn't make much sense to me to keep it as it is).

yamt · 2023-09-07T08:28:51Z

I know we already had this conversation with @yamt, but with these changes, should we update trap_after_main_thread_finishes.c to have 0 as exit code and to replace the trap here with an assert? And even change the name of the tests and put some comments to describe the expected behavior. I could do it as a separate PR if it makes sense.

i'm not sure if i understand what updates you are suggesting.
it already has 0 as exit code.
the trap and assert are not that different as they are both wasm unreachable.

Or we drop the test completely since it has some overlapping with https://github.com/WebAssembly/wasi-threads/blob/main/test/testsuite/wasi_threads_exit_main_block.wat.

it's also fine for me.

wenyongh · 2023-09-07T12:34:46Z

@eloparco I understand that the main thread already returns 0 and no need to change it. Changing buildin_trap to assert is that buildin_trap will throw exception in spawned thread, while assert (assume we use assert(true statement)) won't throw exception and the spawned thread exits normally.

I think it is a good case and had better not change it. A possible issue is that after main function returns and runtime set_exception("wasi proc exit") to the spawned thread, and then before the spawned thread handles the exception and terminate flag, it runs into the builtin_trap, throws unreachable exception and ends first. That means the main thread returns 0 while sub thread throws exception (and it may spread to the main thread then), not sure whether we should handle it? @yamt

yamt · 2023-09-07T23:50:37Z

A possible issue is that after main function returns and runtime set_exception("wasi proc exit") to the spawned thread, and then before the spawned thread handles the exception and terminate flag, it runs into the builtin_trap, throws unreachable exception and ends first. That means the main thread returns 0 while sub thread throws exception (and it may spread to the main thread then), not sure whether we should handle it? @yamt

wrt racing multiple traps/exit, i plan to prevent wasm_runtime_set_exception from overwriting existing trap/exit. (not within this PR though)

wenyongh · 2023-09-08T03:46:56Z

A possible issue is that after main function returns and runtime set_exception("wasi proc exit") to the spawned thread, and then before the spawned thread handles the exception and terminate flag, it runs into the builtin_trap, throws unreachable exception and ends first. That means the main thread returns 0 while sub thread throws exception (and it may spread to the main thread then), not sure whether we should handle it? @yamt

wrt racing multiple traps/exit, i plan to prevent wasm_runtime_set_exception from overwriting existing trap/exit. (not within this PR though)

Got it, great, thanks!

On posix-like platforms, the rest of wasi-threads tests should pass after the recent changes including the following PRs: bytecodealliance#2516 bytecodealliance#2524 bytecodealliance#2529

On posix-like platforms, the rest of wasi-threads tests should pass after the recent changes including the following PRs: #2516, #2524, #2529, #2571, #2576 and #2582.

…#2529) This fixes a few test cases in wasi-threads testsuite like wasi_threads_return_main_block. And also move the special handling for "wasi proc exit" to a more appropriate place.

On posix-like platforms, the rest of wasi-threads tests should pass after the recent changes including the following PRs: bytecodealliance#2516, bytecodealliance#2524, bytecodealliance#2529, bytecodealliance#2571, bytecodealliance#2576 and bytecodealliance#2582.

handle a return from wasi _start function correctly

935f055

this fixes a few test cases in wasi-threads testsuite like wasi_threads_return_main_block. also, move the special handling for wasi proc exit to a more appropriate place.

yamt changed the title ~~handle a return frow wasi _start function correctly~~ handle a return from wasi _start function correctly Sep 4, 2023

yamt force-pushed the wasi-start-return-as-exit branch from 9b77116 to 935f055 Compare September 4, 2023 10:48

yamt mentioned this pull request Sep 4, 2023

Revert "Return error when exception was raised after main thread fini… #2524

Merged

wenyongh reviewed Sep 6, 2023

View reviewed changes

lum1n0us reviewed Sep 6, 2023

View reviewed changes

lum1n0us reviewed Sep 7, 2023

View reviewed changes

add comments

cd56892

also, make some code conditional on WASM_ENABLE_THREAD_MGR

wenyongh reviewed Sep 7, 2023

View reviewed changes

wenyongh merged commit 534a8cf into bytecodealliance:main Sep 8, 2023

yamt mentioned this pull request Sep 20, 2023

test_wamr.sh: bump wasi-testsuite version #2568

Merged

nchamzn mentioned this pull request Jan 18, 2024

Handling "Exception: wasi proc exit" #3050

Open

handle a return from wasi _start function correctly #2529

handle a return from wasi _start function correctly #2529

Uh oh!

Conversation

yamt commented Sep 4, 2023

Uh oh!

yamt commented Sep 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lum1n0us commented Sep 7, 2023

Uh oh!

wenyongh left a comment

Choose a reason for hiding this comment

Uh oh!

eloparco commented Sep 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yamt commented Sep 7, 2023

Uh oh!

wenyongh commented Sep 7, 2023

Uh oh!

yamt commented Sep 7, 2023

Uh oh!

wenyongh commented Sep 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eloparco commented Sep 7, 2023 •

edited

Loading