Add internal tests for WASI threads #1963

eloparco · 2023-02-15T23:25:40Z

Add internal tests for WASI threads. These tests are run in addition to the ones in the proposal https://github.com/WebAssembly/wasi-threads/tree/main/test/testsuite.

The purpose is to test additional and more complex scenarios.

.github/workflows/compilation_on_android_ubuntu.yml

tests/wamr-test-suites/test_wamr.sh

core/iwasm/libraries/lib-wasi-threads/test/build.sh

wenyongh · 2023-03-04T08:43:28Z

CI reported error:
https://github.com/bytecodealliance/wasm-micro-runtime/actions/runs/4324003969/jobs/7548386927#step:13:232

Test nonmain_proc_exit_sleep failed
  [exit_code] 33 == 250
STDOUT:

STDERR:
double free or corruption (fasttop)

Any idea about that?

eloparco · 2023-03-06T00:06:09Z

Any idea about that?

I rebased the PR on top of main. The problem seems to be caused by the exec_env being freed by the thread after finishing but still used by the main thread for the join operation. Apart from that, there are some edge cases that still cause problems for the tests in this PR.
Instead, after #1995, WASi tests from the proposal run fine without race conditions.

For the tests in this PR, I mainly see two kind of issues:

some remaining race conditions generated in the thread manager and shared memory handling
many other data races coming from wasm load/store operations, like

wasm-micro-runtime/core/iwasm/interpreter/wasm_interp_classic.c

Line 1840 in e6a0184

PUSH_I32(LOAD_I32(maddr));

in race condition with both

wasm-micro-runtime/core/iwasm/interpreter/wasm_interp_classic.c

Line 3663 in e6a0184

STORE_U32(maddr, sval);

and

wasm-micro-runtime/core/iwasm/interpreter/wasm_interp_classic.c

Line 2013 in e6a0184

STORE_U32(maddr, frame_sp[1]);

My guess is that they're caused by the internal implementation of some functions (like the pthread ones for sync primitives) in wasi-libc. For instance, I don't see any data race here https://github.com/eloparco/wasm-micro-runtime/blob/eloparco/test-wasi-threads/core/iwasm/libraries/lib-wasi-threads/test/global_atomic.c, but I see store/load data races in this other test https://github.com/eloparco/wasm-micro-runtime/blob/eloparco/test-wasi-threads/core/iwasm/libraries/lib-wasi-threads/test/global_lock.c, where the only difference is in the usage of a lock instead of atomic variable.

The investigation is still on.

wenyongh · 2023-03-06T02:34:41Z

Any idea about that?

I rebased the PR on top of main. The problem seems to be caused by the exec_env being freed by the thread after finishing but still used by the main thread for the join operation. Apart from that, there are some edge cases that still cause problems for the tests in this PR. Instead, after #1995, WASi tests from the proposal run fine without race conditions.

Yes, seems that the "double free or corruption (fasttop)" error had been reported by CI when running nonmain_proc_exit_sleep and nonmain_trap_wait, and I found it ever occurred when running thread_termination.wasm in samples/wasi-threads. But I manually tested them again and cannot reproduce the issues, instead, I found that nonmain_proc_exit_wait sometimes got hang.

For the tests in this PR, I mainly see two kind of issues:

some remaining race conditions generated in the thread manager and shared memory handling

many other data races coming from wasm load/store operations, like

wasm-micro-runtime/core/iwasm/interpreter/wasm_interp_classic.c

Line 1840 in e6a0184

PUSH_I32(LOAD_I32(maddr));

in race condition with both

wasm-micro-runtime/core/iwasm/interpreter/wasm_interp_classic.c

Line 3663 in e6a0184

STORE_U32(maddr, sval);

and

wasm-micro-runtime/core/iwasm/interpreter/wasm_interp_classic.c

Line 2013 in e6a0184

STORE_U32(maddr, frame_sp[1]);

My guess is that they're caused by the internal implementation of some functions (like the pthread ones for sync primitives) in wasi-libc. For instance, I don't see any data race here https://github.com/eloparco/wasm-micro-runtime/blob/eloparco/test-wasi-threads/core/iwasm/libraries/lib-wasi-threads/test/global_atomic.c, but I see store/load data races in this other test https://github.com/eloparco/wasm-micro-runtime/blob/eloparco/test-wasi-threads/core/iwasm/libraries/lib-wasi-threads/test/global_lock.c, where the only difference is in the usage of a lock instead of atomic variable.

The investigation is still on.

OK, seems the first issue is caused by runtime and the second is caused by wasi-libc itself.

wenyongh · 2023-03-07T01:10:38Z

Maybe you can try whether it fixes some issues found?

It didn't fix the data race that I was looking at, I still see it between these two calls

wasm-micro-runtime/core/iwasm/common/wasm_memory.c

Line 218 in 04616d3

free_func(ptr);

wasm-micro-runtime/core/iwasm/libraries/thread-mgr/thread_manager.c

Line 870 in 04616d3

if (!clusters_have_exec_env(exec_env) || exec_env->thread_is_detached) {

In addition, from time to time, I still see (even before your PR) problems related to unlocking an unlocked mutex, usage of invalid mutexes and heap-use-after-free.

If you want to try to reproduce, add the thread sanitizer here

wasm-micro-runtime/samples/wasi-threads/CMakeLists.txt

Line 79 in 04616d3
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -fsanitize=thread")
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fsanitize=thread")
and then run the tests like
/wasm-micro-runtime/samples/wasi-threads/build $ ./iwasm -v=5 ../../../core/iwasm/libraries/lib-wasi-threads/test/nonmain_trap_wait.wasm; echo $?
Usually I have I have a script to run all the tests in sequence and I may have to run it multiple times to get useful warnings from the sanitizer.

I won't have much time in the next couple of weeks, so feel free to take a look if you want.

OK, thanks, I will try looking into it.

wenyongh · 2023-03-07T14:06:24Z

@eloparco I found some multi-threading related issues and uploaded PR #2013 to fix them, I think it should be able to fix some of the issues you reported, could you help review it and have a try?
BTW: The PR will lead to compile error in Windows when thread manager is enabled, I will fix it later.

eloparco · 2023-03-07T23:15:05Z

@wenyongh Great! I just tried the changes, and they seem to fix the remaining race conditions I was seeing.
The only one that I spot a couple of times is with

wasm-micro-runtime/core/iwasm/common/wasm_runtime_common.c

Line 3147 in 7a3d2bf

return wasi_ctx->exit_code;

wasm-micro-runtime/core/iwasm/libraries/libc-wasi/libc_wasi_wrapper.c

Line 1095 in 7a3d2bf

wasi_ctx->exit_code = rval;

But that should be fairly easy to fix.

I tried many runs in classic interpreter mode, I still need to check with AOT.

wenyongh · 2023-03-08T03:27:24Z

@wenyongh Great! I just tried the changes, and they seem to fix the remaining race conditions I was seeing. The only one that I spot a couple of times is with

wasm-micro-runtime/core/iwasm/common/wasm_runtime_common.c

Line 3147 in 7a3d2bf

return wasi_ctx->exit_code;

wasm-micro-runtime/core/iwasm/libraries/libc-wasi/libc_wasi_wrapper.c

Line 1095 in 7a3d2bf

wasi_ctx->exit_code = rval;

But that should be fairly easy to fix.
I tried many runs in classic interpreter mode, I still need to check with AOT.

OK, I have merged that PR, maybe you can rebase this PR with main branch, and if CIs run OK, I will merge it.

loganek · 2023-03-08T09:10:58Z

.github/workflows/compilation_on_android_ubuntu.yml

+      - name: build wasi-libc (needed for wasi-threads)
+        if: matrix.test_option == '$WASI_TEST_OPTIONS'
+        run: |
+          mkdir wasi-libc


Could we upgrade to wasi-sdk 20, so we don't need to build libc from git?

Sure, I'm addressing that in a separate PR

Do you think I should update it everywhere in the CI or only for the parts where WASI threads are used?
eloparco@4b8c49e

Had better update here only since we claimed in the document that we support the wasi-sdk 19.0+, we had better use it by default in the CI files.
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/doc/build_wasm_app.md

I tried it here eloparco@006534c. It works on MacOS but not on Ubuntu 20.04, as we observed in the past (because of the glibc version installed there).

https://github.com/eloparco/wasm-micro-runtime/actions/runs/4379797818/jobs/7666263974
https://github.com/eloparco/wasm-micro-runtime/actions/runs/4379796629/jobs/7666151109

We can use wasi-sdk-20 pre-release in case of Ubuntu 22.04 and keep using wasi-libc for wasi-sdk-19 on Ubuntu 20.04, making sure that in case of wasi-sdk-19 we specify WASI_SYSROOT. What do you think?

Maybe I should create a PR anyway and we can discuss there.

It's good, at least we don't need to build wasi-libc in Ubuntu 22.04. And we can update CI again when the wasi-sdk-20 is actually released. Not sure whether the glibc version issue on Ubuntu 20.04 will be resolved in wasi-sdk-20.0? Or should we file an issue in wasi-sdk community, or upload a PR to help resolve the issue?

g0djan · 2023-03-08T11:27:31Z

#2016

Fixed a data race when deleting wait_info in wasm_shared_memory, spotted with main_proc_exit_wait.c from this PR

eloparco · 2023-03-08T18:32:38Z

OK, I have merged that PR, maybe you can rebase this PR with main branch, and if CIs run OK, I will merge it.

Yes, let's do that so it's easier to use the tests without applying patches or cherry-picking. They seem to succeed now (I tried many runs) and the data races seem gone (apart from the ones related to load/store operations).
I tried only in classic interpreter mode for now, I'll have to check in AOT mode.

wenyongh · 2023-03-09T01:04:38Z

OK, I have merged that PR, maybe you can rebase this PR with main branch, and if CIs run OK, I will merge it.

Yes, let's do that so it's easier to use the tests without applying patches or cherry-picking. They seem to succeed now (I tried many runs) and the data races seem gone (apart from the ones related to load/store operations). I tried only in classic interpreter mode for now, I'll have to check in AOT mode.

OK, I merged it.
BTW, @loganek added a comment to suggest to use the wasi-sdk-20.0 instead in CI, please submit another PR to do it if you want.

wenyongh · 2023-03-09T01:27:03Z

CI run failure:

./test_wamr.sh -m x86_32 -P $WASI_TEST_OPTIONS -t classic-interp
...
Test nonmain_proc_exit_sleep failed
  [exit_code] 33 == 0
STDOUT:

STDERR:

Add internal tests for WASI threads (bytecodealliance#1963)

eloparco · 2023-03-10T00:16:14Z

CI run failure:

Locally, in classic interpreter mode, I tried multiple runs of test_wamr.sh and it was always succeeding. I tried multiple runs of nonmain_proc_exit_sleep with the thread sanitizer but I couldn't find anything (apart from the usual race conditions in load/store operations).

I didn't try with x86_32 though since I don't have the necessary packages installed, do you think the problem could be x86_32-specific?

wenyongh · 2023-03-10T02:07:21Z

Locally, in classic interpreter mode, I tried multiple runs of test_wamr.sh and it was always succeeding. I tried multiple runs of nonmain_proc_exit_sleep with the thread sanitizer but I couldn't find anything (apart from the usual race conditions in load/store operations).

I didn't try with x86_32 though since I don't have the necessary packages installed, do you think the problem could be x86_32-specific?

I am not sure, I tested it many times with both iwasm x86_64 and x86_32, but also cannot reproduce the issue.

But for update_shared_data_and_alloc_heap.wasm in core/iwasm/libraries/lib-wasi-threads/test, seems it is only related to x86_32 when calling malloc function:

Failed to spawn a new thread
Exception: out of bounds memory access

I didn't reproduce it with iwasm x86_64, but it easily occurs with iwasm x86_32.

eloparco · 2023-03-10T02:23:49Z

But for update_shared_data_and_alloc_heap.wasm in core/iwasm/libraries/lib-wasi-threads/test, seems it is only related to x86_32 when calling malloc function:

Ok, that may be caused by the fact that the malloc implementation in wasi-libc is not thread-safe iirc and no lock is being used in the test.

wenyongh · 2023-03-10T02:40:38Z

OK, could we add lock to malloc for this case? And not sure whether there will be a thread-safe malloc in future version of wasi-libc?

eloparco · 2023-03-11T01:24:31Z

OK, could we add lock to malloc for this case? And not sure whether there will be a thread-safe malloc in future version of wasi-libc?

Done #2022

Fix a data race for test main_proc_exit_wait.c from #1963. And fix atomic_wait logic that was wrong before: - a thread 1 started executing wasm instruction wasm_atomic_wait but hasn't reached waiting on condition variable - a main thread calls proc_exit and notifies all the threads that reached waiting on condition variable Which leads to thread 1 hang on waiting on condition variable after that Now it's atomically checked whether proc_exit was already called.

Add internal tests for WASI threads. These tests are run in addition to the ones in the proposal: https://github.com/WebAssembly/wasi-threads/tree/main/test/testsuite. The purpose is to test additional and more complex scenarios.

…odealliance#2016) Fix a data race for test main_proc_exit_wait.c from bytecodealliance#1963. And fix atomic_wait logic that was wrong before: - a thread 1 started executing wasm instruction wasm_atomic_wait but hasn't reached waiting on condition variable - a main thread calls proc_exit and notifies all the threads that reached waiting on condition variable Which leads to thread 1 hang on waiting on condition variable after that Now it's atomically checked whether proc_exit was already called.

eloparco force-pushed the eloparco/test-wasi-threads branch 2 times, most recently from afa80d0 to 99b8f88 Compare February 18, 2023 21:38

eloparco changed the base branch from dev/wasi_threads to main February 18, 2023 21:39

eloparco force-pushed the eloparco/test-wasi-threads branch from 99b8f88 to bfeeb62 Compare February 18, 2023 22:32

eloparco mentioned this pull request Feb 19, 2023

Fix data races #1971

Merged

eloparco force-pushed the eloparco/test-wasi-threads branch 7 times, most recently from baa0c58 to 878efac Compare February 23, 2023 02:15

wenyongh reviewed Feb 23, 2023

View reviewed changes

.github/workflows/compilation_on_android_ubuntu.yml Show resolved Hide resolved

tests/wamr-test-suites/test_wamr.sh Outdated Show resolved Hide resolved

eloparco changed the title ~~Add tests for WASI threads~~ Add internal tests for WASI threads Feb 23, 2023

eloparco force-pushed the eloparco/test-wasi-threads branch 4 times, most recently from 3f38ec1 to 69242fc Compare February 24, 2023 01:41

eloparco marked this pull request as ready for review February 24, 2023 02:00

eloparco force-pushed the eloparco/test-wasi-threads branch from 6127a8f to 986db41 Compare February 26, 2023 17:34

wenyongh mentioned this pull request Feb 27, 2023

Update WASI thread proposal tests in CI #1985

Merged

eloparco force-pushed the eloparco/test-wasi-threads branch 2 times, most recently from c4b1654 to 120c146 Compare February 27, 2023 13:52

eloparco mentioned this pull request Mar 2, 2023

Add/reorganize locks for thread synchronization #1995

Merged

eloparco force-pushed the eloparco/test-wasi-threads branch from 83a8f2c to ccce28b Compare March 3, 2023 01:50

wenyongh reviewed Mar 3, 2023

View reviewed changes

core/iwasm/libraries/lib-wasi-threads/test/build.sh Show resolved Hide resolved

eloparco force-pushed the eloparco/test-wasi-threads branch from 6e50016 to 584da41 Compare March 5, 2023 23:37

This was referenced Mar 7, 2023

Enhance/Fix sample socket-api and workload #2006

Merged

feat: added tests for socket apis #1900

Merged

loganek reviewed Mar 8, 2023

View reviewed changes

g0djan mentioned this pull request Mar 8, 2023

Fix wait_info data race for deletion + fix atomic_wait logic #2016

Merged

eloparco added 7 commits March 8, 2023 18:13

test(wasi-threads): add tests for wasi threads

12b2d84

feat(wasi-threads): add test execution in ci

4b2ae05

feat: add tests for thread termination

f8121f8

fix: remove prints from different threads causing data races

1ccc5c5

chore: move wasi thread test folder

e6ddf59

fix: use barrier instead of semaphore in thread termination tests

3dc9a6f

chore: add copyright

7aafc8d

eloparco force-pushed the eloparco/test-wasi-threads branch from 95e9f1e to 7aafc8d Compare March 8, 2023 18:14

wenyongh merged commit 128c0ea into bytecodealliance:main Mar 9, 2023

wenyongh added a commit to wenyongh/wasm-micro-runtime that referenced this pull request Mar 9, 2023

Merge pull request #727 from bytecodealliance/main

1484aaf

Add internal tests for WASI threads (bytecodealliance#1963)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add internal tests for WASI threads #1963

Add internal tests for WASI threads #1963

eloparco commented Feb 15, 2023 •

edited

Loading

wenyongh commented Mar 4, 2023

eloparco commented Mar 6, 2023 •

edited

Loading

wenyongh commented Mar 6, 2023

wenyongh commented Mar 7, 2023

wenyongh commented Mar 7, 2023

eloparco commented Mar 7, 2023 •

edited

Loading

wenyongh commented Mar 8, 2023

loganek Mar 8, 2023

eloparco Mar 8, 2023

eloparco Mar 8, 2023 •

edited

Loading

wenyongh Mar 9, 2023

eloparco Mar 10, 2023

wenyongh Mar 10, 2023

g0djan commented Mar 8, 2023

eloparco commented Mar 8, 2023

wenyongh commented Mar 9, 2023

wenyongh commented Mar 9, 2023

eloparco commented Mar 10, 2023

wenyongh commented Mar 10, 2023

eloparco commented Mar 10, 2023

wenyongh commented Mar 10, 2023

eloparco commented Mar 11, 2023

Add internal tests for WASI threads #1963

Add internal tests for WASI threads #1963

Conversation

eloparco commented Feb 15, 2023 • edited Loading

wenyongh commented Mar 4, 2023

eloparco commented Mar 6, 2023 • edited Loading

wenyongh commented Mar 6, 2023

wenyongh commented Mar 7, 2023

wenyongh commented Mar 7, 2023

eloparco commented Mar 7, 2023 • edited Loading

wenyongh commented Mar 8, 2023

loganek Mar 8, 2023

Choose a reason for hiding this comment

eloparco Mar 8, 2023

Choose a reason for hiding this comment

eloparco Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

wenyongh Mar 9, 2023

Choose a reason for hiding this comment

eloparco Mar 10, 2023

Choose a reason for hiding this comment

wenyongh Mar 10, 2023

Choose a reason for hiding this comment

g0djan commented Mar 8, 2023

eloparco commented Mar 8, 2023

wenyongh commented Mar 9, 2023

wenyongh commented Mar 9, 2023

eloparco commented Mar 10, 2023

wenyongh commented Mar 10, 2023

eloparco commented Mar 10, 2023

wenyongh commented Mar 10, 2023

eloparco commented Mar 11, 2023

eloparco commented Feb 15, 2023 •

edited

Loading

eloparco commented Mar 6, 2023 •

edited

Loading

eloparco commented Mar 7, 2023 •

edited

Loading

eloparco Mar 8, 2023 •

edited

Loading