WebAssembly / wasi-threads

145 stars 8 forks source link

test: unable to use `build.sh` #36

Open abrown opened 1 year ago

abrown commented 1 year ago

When I run build.sh, I see the following errors:

$ ./build.sh 
Compiling testsuite/thread_spawn-simple.c
testsuite/thread_spawn-simple.c:3:10: fatal error: 'wasi/api.h' file not found
#include <wasi/api.h>
         ^~~~~~~~~~~~
1 error generated.
testsuite/wasi_thread_spawn.S:5:2: error: unknown directive
 .export_name wasi_thread_start, wasi_thread_start
 ^
testsuite/wasi_thread_spawn.S:7:2: error: unknown directive
 .globaltype __stack_pointer, i32
 ^
testsuite/wasi_thread_spawn.S:8:2: error: unknown directive
 .functype __wasi_thread_start_C (i32, i32) -> ()
 ^
testsuite/wasi_thread_spawn.S:15:2: error: unknown directive
 .functype wasi_thread_start (i32, i32) -> ()
 ^
testsuite/wasi_thread_spawn.S:19:2: error: invalid instruction mnemonic 'local.get'
 local.get 1 # start_arg
 ^~~~~~~~~
testsuite/wasi_thread_spawn.S:20:2: error: invalid instruction mnemonic 'i32.load'
 i32.load 0 # stack
 ^~~~~~~~
testsuite/wasi_thread_spawn.S:21:2: error: invalid instruction mnemonic 'global.set'
 global.set __stack_pointer
 ^~~~~~~~~~
testsuite/wasi_thread_spawn.S:24:2: error: invalid instruction mnemonic 'local.get'
 local.get 0 # tid
 ^~~~~~~~~
testsuite/wasi_thread_spawn.S:25:2: error: invalid instruction mnemonic 'local.get'
 local.get 1 # start_arg
 ^~~~~~~~~
testsuite/wasi_thread_spawn.S:28:2: error: invalid instruction mnemonic 'end_function'
 end_function
 ^~~~~~~~~~~~

It seems like there are some specific environment requirements for these scripts to work that we should document. I can figure these out separately (unless @loganek or @yamt you know them off hand?) but I just wanted to record the issue.

loganek commented 1 year ago

Thanks Andrew for the report. I think the default compiler configuration in build.sh is far from being perfect; this is what we have in the wasi-testsuite repo to build it: https://github.com/WebAssembly/wasi-testsuite/blob/main/scripts/update-proposal-tests.sh#L26 I hope that solves your problem for now, but I'll keep the ticket open to update build.sh with some default parameters to simplify the command (and/or at least update the readme file).

abrown commented 1 year ago

Interesting; with wasi-sdk-20+threads is the "build wasi-libc again" step necessary any more?

On a slightly different note, I want to propose a perhaps controversial opinion: why don't we just build this specific test, either with a script or a Dockerfile, and check in the compiled WebAssembly? My reasoning is that this checked-in artifact should only be necessary for this test or some small subset of future C tests and it is much more convenient on the "just run the tests" side to not have to worry about building any artifacts. E.g., in https://github.com/bytecodealliance/wasmtime/pull/5907 I ended up just running the WAT files directly instead of converting them to binary form (and this *.c test ends up getting ignored).

Here's another dimension to this: in Wasmtime, e.g., most of the test infrastructure is Rust-based, so I built a simple test runner in Rust. This avoids a Python dependency when a user runs cargo test. I would propose that the https://github.com/WebAssembly/wasi-testsuite repository add some documentation defining what the test suite layout should look like (the JSON spec format, the WAT/Wasm naming conventions), how one should run the tests (e.g., compare the exit code with the JSON one, etc.), and then note that the Python infrastructure is just one way to run the test suite. (I can volunteer to add all this documentation if you think this is a good idea). If we make it clear that one can easily port the test infrastructure to other languages and we make the test artifacts immediately available when someone clones the repository (no extra build step), then it makes it easier to use this test suite in more places.

@loganek, let me know what you think!

AlexEne commented 1 year ago

I know this wasn't a question for me :) but I like the idea of having a way to run things easier.

loganek commented 1 year ago

Hi, sorry for late reply.

Interesting; with wasi-sdk-20+threads is the "build wasi-libc again" step necessary any more?

Most likely not, I'll try to upgrade it (update: https://github.com/WebAssembly/wasi-testsuite/pull/69)

why don't we just build this specific test, either with a script or a Dockerfile, and check in the compiled WebAssembly?

We already do that. Please see https://github.com/WebAssembly/wasi-testsuite/tree/prod/testsuite-base for tests for existing snapshot and https://github.com/WebAssembly/wasi-testsuite/tree/prod/testsuite-all for all tests (including proposals, that are updated daily). The reason we push it to separate branches is because we've got feedback from people living in locations where internet connection isn't great that it will affect their development process. I personally don't know if that's really a big problem, but at the same time pushing those test binaries to separate branch isn't a big deal from maintenance perspective, so we keep it like that for now.

I would propose that the WebAssembly/wasi-testsuite repository add some documentation defining what the test suite layout should look like

Yes, python test runner is considered a reference test runner, but other runners can co-exist as well. I'll work on the documentation in the upcoming weeks. After that we can add more test runner implementations if needed. One note though - we'll have to figure out how to add tests to for the runner itself to make sure all implementations work fine.

abrown commented 1 year ago

I checked this issue again with your suggestion but this is the error I see:

$ CC="$WASI_SDK_DIR/bin/clang -pthread --sysroot=$WASI_SDK_DIR/share/wasi-sysroot -Wl,--import-memory --target=wasm32-wasi-threads" ./build.sh 
Compiling testsuite/thread_spawn-simple.c
testsuite/thread_spawn-simple.c:1:10: fatal error: 'assert.h' file not found
#include <assert.h>
         ^~~~~~~~~~
1 error generated.
loganek commented 1 year ago

I wasn't able to reproduce your issue using wasi-sdk-20. What's the SDK version you use? Also, could you check if assert.h is in your sysroot (should be in =$WASI_SDK_DIR/share/wasi-sysroot/include/assert.h? It looks to me like the sysroot is somehow corrupted rather than problem with the test itself.

abrown commented 1 year ago

Huh, this is weird: /opt/wasi-sdk/wasi-sdk-20.0/share/wasi-sysroot/include/assert.h is definitely a file and it looks like it has all the right C code. [fiddles around] Ok, I think I see the problem. Somehow I mistakenly set WASI_SDK_DIR to either the wrong thing or the wrong name... something like that. When I write that path out things seem to work!

$ CC="/opt/wasi-sdk/wasi-sdk-20.0/bin/clang -pthread --sysroot=/opt/wasi-sdk/wasi-sdk-20.0/share/wasi-sysroot -Wl,--import-memory --target=wasm32-wasi-threads" ./build.sh
Compiling testsuite/thread_spawn-simple.c

My mistake! Ok, so how do we resolve this issue? Do we set the default CC in build-c.sh to some default path? Or do we add the flags part to build-c.sh and then document that the user of build.sh should run CC="path/to/clang --sysroot=path/to/wasi-sysroot" ./build.sh? What do you think?

g0djan commented 1 year ago

@abrown @loganek It's compiling like this but it was not enough to make it run on wasmtime

wasmtime --wasm-features=threads --wasi-modules=experimental-wasi-threads testsuite/thread_spawn-simple.wasm
Error: failed to run main module `testsuite/thread_spawn-simple.wasm`

Caused by:
    0: failed to invoke command default
    1: error while executing at wasm backtrace:
           0: 0x32b0 - <unknown>!__wasi_args_sizes_get
           1: 0x31eb - <unknown>!__main_void
           2:  0x2af - <unknown>!_start
    2: missing required memory export

To make it run on wasmtime I also added -Wl,--export-memory to CC=/opt/.... Now it crashes a bit further on wasmtime and everything works fine on WAMR

Error: failed to run main module `testsuite/thread_spawn-simple.wasm`

Caused by:
    0: failed to invoke command default
    1: error while executing at wasm backtrace:
           0: 0x2ca6 - <unknown>!dlfree
           1: 0x29ff - <unknown>!free
           2:  0xcc1 - <unknown>!main
           3: 0x323b - <unknown>!__main_void
           4:  0x2b8 - <unknown>!_start
    2: memory fault at wasm address 0x2db4a81c in linear memory of size 0x20000
    3: wasm trap: out of bounds memory access
loganek commented 1 year ago

@g0djan have you defined stack size in the command? Is it possible that the test goes beyond the stack? Also, what's the max memory defined?

g0djan commented 1 year ago

$WASI_SDK_DIR/bin/clang --sysroot $WASI_SDK_DIR/share/wasi-sysroot \ --target=wasm32-wasi-threads -pthread \ -Wl,--import-memory,--export-memory,--shared-memory,--max-memory=67108864,--export=wasi_thread_start \ -z stack-size=32768 testsuite/thread_spawn-simple.c -o thread_spawn-simple.wasm Didn't help, maybe wamr lacks a check for that