Add some low level tests #19

yamt · 2022-12-22T00:12:50Z

(venv) spacetanuki% TOYWASM=~/git/toywasm/b/toywasm python3 test-runner/wasi_test_runner.py -t ~/git/wasm/wasi-threads/test/testsuite -r ~/git/toywasm/test/wasi-testsuite-adapter.py
Test wasi_threads_exit_nonmain_wasi passed
Test wasi_threads_exit_main_busy passed
Test wasi_threads_exit_main_wasi passed
Test wasi_threads_exit_nonmain_busy passed
Test wasi_threads_spawn passed
Test wasi_threads_exit_main_block passed
Test wasi_threads_exit_nonmain_block passed

===== Test results =====
Runtime: toywasm v0.0
Suite: WASI threads proposal
  Total: 7
  Passed:  7
  Failed:  0

Test suites: 1 passed, 0 total
Tests:       7 passed, 0 total
(venv) spacetanuki%

abrown · 2022-12-22T01:02:17Z

I'm interested in reviewing this but it may have to be in the new year (I'll be out for a bit). @yamt, you might also be interested in the tests I added over in WebAssembly/wasi-libc#369. I think it is a good idea to have tests in both places: in the spec to check that and over in wasi-libc to check our pthreads implementation. What I would like to think through more clearly in January is whether there is a clean way to enable multiple engines to run both sets of tests (and wasi-testsuite too, @loganek!). I see you have a Python driver of some kind... perhaps there is a need for something like eshost but for Wasm engines that lets us easily run all these tests.

yamt · 2022-12-22T02:28:01Z

I'm interested in reviewing this but it may have to be in the new year (I'll be out for a bit). @yamt, you might also be interested in the tests I added over in WebAssembly/wasi-libc#369.

i will take a look.

I think it is a good idea to have tests in both places: in the spec to check that and over in wasi-libc to check our pthreads implementation.

i agree.

What I would like to think through more clearly in January is whether there is a clean way to enable multiple engines to run both sets of tests (and wasi-testsuite too, @loganek!). I see you have a Python driver of some kind... perhaps there is a need for something like eshost but for Wasm engines that lets us easily run all these tests.

wasi-testsuite has simple wrappers (called "runtime adapter" there) for engines. there are implementations of wasmtime, wamr, toywasm, at least. i feel the current adapter api is too simple though. cf. WebAssembly/wasi-testsuite#39

i was not aware of eshost, but its purpose looks similar, sure.

yamt · 2022-12-23T07:48:46Z

test/testsuite/wasi_threads_exit_main_block.wat

+ call $proc_exit
+ unreachable
+ )
+ (memory 1 1 shared)


these assume the "inherit semantics" described in WebAssembly/wasi-libc#369 (comment).

for "import semantics" described in WebAssembly/wasi-libc#369 (comment), this needs to be changed to an imported memory.

i adapted these tests to the latter.

loganek · 2022-12-23T11:23:55Z

Same here, I'm on holidays till January (in fact, I'm ooto since mid December so my responses are a bit slow right now) so I'll catch up on all the changes then.

What I would like to think through more clearly in January is whether there is a clean way to enable multiple engines to run both sets of tests

We already have a framework for that, an the next step is to pull tests from different repos (proposals, even wasi-libc) an run them with runtimes that are onboarded to the framework. We can talk more about this next year.

i feel the current adapter api is too simple though

Yes, the API is intentionally simple so we can quickly get something up and running. I expect some changes there in the future although leaning towards keeping it rather simple to reduce onboarding effort (rather than asking runtimes to implement complex debug/test interfaces).

loganek · 2023-01-04T13:31:22Z

test/testsuite/wasi_threads_exit_main_block.wat

@@ -0,0 +1,39 @@
+(module
+ (memory (import "foo" "bar") 1 1 shared)


Should there also be an exported memory? From the doc:

Regardless of the kind, all modules accessing WASI APIs also export a linear memory with the name memory.

Should there also be an exported memory? From the doc:

Regardless of the kind, all modules accessing WASI APIs also export a linear memory with the name memory.

sure. i will update tomorrow.

I'm not sure about this. Firstly, that doc marked as legacy. Secondly, if this doc were current I would recommend that it be updated as part of wasi-threads to specify that memory but either be exported or imported.

IIUC wasi-threads should require that memory be imported. The extra requirement to redundantly re-export the memory back to the runtime seems like a wasmtime specific thing that I'm not sure we want to spec.

i guess the original motivation of memory export in wasi was to explicitly allow the host to examine the linear memory for given pointers, especially for polyfilling implementations. in that regard, you are right when the memory is imported from the host, there is little point to export it back.

on the other hand, it might still make sense to have a well-known name (ie. "memory") to indicate which memory is used for wasi pointers in case of multi-memory.

i personally has no strong opinions either ways.

Should there also be an exported memory? From the doc:

Regardless of the kind, all modules accessing WASI APIs also export a linear memory with the name memory.

sure. i will update tomorrow.

postponed as it seems a bit more controversial than i thought.

an old discussion: WebAssembly/WASI#48 (comment)

i'd suggest not to block this PR for the memory export/import issue.
it isn't something to decide in this PR.
#22

Should there also be an exported memory? From the doc:

Regardless of the kind, all modules accessing WASI APIs also export a linear memory with the name memory.

sure. i will update tomorrow.

postponed as it seems a bit more controversial than i thought.

i added export as well to increase the chances for these to work on more runtimes.

sbc100 · 2023-01-11T12:01:07Z

test/build-wat.sh

@@ -0,0 +1,6 @@
+#! /bin/sh


Should this file be just be called build.sh or build-wasm.sh (build-wat reads like the output of the script, would be wat, but its actually the input.. maybe campile-wat.sh if you want to keep wat in the name?)

there is already build.sh, which build wasm from c.
i meant build from wat.

Ah I see. In that case perhaps we can replace the two different shell scripts with a single Makefile that by default converts all .c and .wat files?

No need to block this PR on that, but it might be nicer that creating a bunch of different "make" scripts.

yes, i agree it's nice to have a unformed way to build tests.
at least it would help WebAssembly/wasi-testsuite#41 .

or merge those two build scripts into a single build.sh

or merge those two build scripts into a single build.sh

eventually, maybe.

at this moment C tests and WAT tests are worked on by different people.
merging them now just increases chances to conflict with eg. #25

yamt · 2023-01-18T06:24:10Z

is there any blocker on this?

abrown

Ok, I've had some more time to look at this and have an opinion:

I think it's great to expand the test coverage for this proposal, so that's a plus!
I still don't really understand how to run these in Wasmtime, e.g., but maybe that can should is already explained somewhere else?
I have had to write WAT tests in the past and they can be very... unpleasant to work with. It is difficult to understand "what is being tested here?" at a glance. I think two things would improve this PR: 1) a short story at the top explaining the sequence of events (e.g., "we start two threads, the main one exits with code X and we ensure that that is the code observed") and 2) making the WAT easier to read, e.g., by using names ($foo in locals, parameters) and "infix" notation (if (i32.le_s (i32.const 0)) ...). I get that this is pretty subjective but if you all have had to write and debug much WAT by hand in the past you may know what I mean.

I don't want to block progress so I'm fine with merging this PR but I do think it could be improved, either here or in the future to make things easier to understand. Thanks @yamt for the PR!

loganek · 2023-01-20T01:08:00Z

I still don't really understand how to run these in Wasmtime, e.g., but maybe that can should is already explained somewhere else?

I'm going to open a PR to update https://github.com/WebAssembly/wasi-threads/blob/main/test/README.md tomorrow or beginning of next week so that should be clear then.

yamt · 2023-01-20T01:58:29Z

Ok, I've had some more time to look at this and have an opinion:

* I think it's great to expand the test coverage for this proposal, so that's a plus!

* I still don't really understand how to run these in Wasmtime, e.g., but maybe that can should is already explained somewhere else?

run build-wat.sh to build tests.
follow the instructions in wasi-testsuite.

depending on the way how the feature is enabled in wasmtime, (i don't know)
you might need to modify the wasmtime adapter script, which is currently in wasi-testsuite repo.

* I have had to write WAT tests in the past and they can be very... unpleasant to work with. It is difficult to understand "what is being tested here?" at a glance. I think two things would improve this PR: 1) a short story at the top explaining the sequence of events (e.g., "we start two threads, the main one exits with code X and we ensure that that is the code observed") and 2) making the WAT easier to read, e.g., by using names (`$foo` in locals, parameters) and "infix" notation (`if (i32.le_s (i32.const 0)) ...`). I get that this is pretty subjective but if you all have had to write and debug much WAT by hand in the past you may know what I mean.

for me, the "infix" notation is more difficult to read/write than the current style.
other points make sense.

I don't want to block progress so I'm fine with merging this PR but I do think it could be improved, either here or in the future to make things easier to understand. Thanks @yamt for the PR!

why don't you approve then? :-)

loganek · 2023-01-20T16:27:33Z

I think having a brief description at the beginning of each test file would really help. I'd also prefer to have single command for building all tests (we can merge both .sh files into one, I'm not that concerned about potential conflicts).

Other than that, the PR looks good. Many thanks for doing that!

abrown · 2023-01-20T21:38:47Z

why don't you approve then? :-)

Because I was too lazy to actually decipher the WAT!

yamt · 2023-01-23T05:00:02Z

I think having a brief description at the beginning of each test file would really help.

done.

I'd also prefer to have single command for building all tests (we can merge both .sh files into one, I'm not that concerned about potential conflicts).

done.
this conflicts with #25 .

loganek · 2023-01-23T17:26:23Z

test/testsuite/wasi_threads_exit_main_block.wat

@@ -0,0 +1,45 @@
+;; When the main thread calls proc_exit, it should terminate
+;; a thread blocking in `memory.atomic.wait32` opecode.


Suggested change

;; a thread blocking in `memory.atomic.wait32` opecode.

;; a thread blocking in `memory.atomic.wait32` opcode.

loganek · 2023-01-23T17:31:21Z

test/testsuite/wasi_threads_exit_nonmain_block.wat

@@ -0,0 +1,45 @@
+;; When a non-main thread calls proc_exit, it should terminate
+;; the main thread blocking in `memory.atomic.wait32` opecode.


Suggested change

;; the main thread blocking in `memory.atomic.wait32` opecode.

;; the main thread blocking in `memory.atomic.wait32` opcode.

yamt · 2023-01-23T22:51:26Z

I think having a brief description at the beginning of each test file would really help.

done.

I'd also prefer to have single command for building all tests (we can merge both .sh files into one, I'm not that concerned about potential conflicts).

done. this conflicts with #25 .

and as you merged #25, i now have to resolve conflicts. sigh.

``` (venv) spacetanuki% TOYWASM=~/git/toywasm/b/toywasm python3 test-runner/wasi_test_runner.py -t ~/git/wasm/wasi-threads/test/testsuite -r ~/git/toywasm/test/wasi-testsuite-adapter.py Test wasi_threads_exit_nonmain_wasi passed Test wasi_threads_exit_main_busy passed Test wasi_threads_exit_main_wasi passed Test wasi_threads_exit_nonmain_busy passed Test wasi_threads_spawn passed Test wasi_threads_exit_main_block passed Test wasi_threads_exit_nonmain_block passed ===== Test results ===== Runtime: toywasm v0.0 Suite: WASI threads proposal Total: 7 Passed: 7 Failed: 0 Test suites: 1 passed, 0 total Tests: 7 passed, 0 total (venv) spacetanuki% ```

as suggested in WebAssembly/wasi-libc#369 the corresponding toywasm change: yamt/toywasm@4d81846 ``` spacetanuki% TOYWASM=~/git/toywasm/b/toywasm python3 ~/git/wasm/wasi-testsuite/test-runner/wasi_test_runner.py -t ./test/testsuite -r ~/git/toywasm/test/wasi-testsuite-adapter.py Test wasi_threads_exit_nonmain_wasi passed Test wasi_threads_exit_main_busy passed Test wasi_threads_exit_main_wasi passed Test wasi_threads_exit_nonmain_busy passed Test wasi_threads_spawn passed Test wasi_threads_exit_main_block passed Test wasi_threads_exit_nonmain_block passed ===== Test results ===== Runtime: toywasm v0.0 Suite: WASI threads proposal Total: 7 Passed: 7 Failed: 0 Test suites: 1 passed, 0 total Tests: 7 passed, 0 total spacetanuki% ```

While it's a bit redundant to both import and export a memory, it's what WASI implementations expect. Emscripten, toywasm: Import alone is fine. But export wouldn't hurt. wasm-micro-runtime: Export is checked. Nothing actually seems to rely on it though. wasmtime: Export is necessary? References: https://github.com/WebAssembly/WASI/blob/main/legacy/application-abi.md#current-unstable-abi WebAssembly#22

Also add some comments

While I suspect wait and notify are memory barriers for most implementations, it's safer to issue explicit fences.

cf. WebAssembly#26 WebAssembly/wasi-libc#387

yamt · 2023-02-06T08:57:36Z

@abrown any blockers?

abrown · 2023-02-16T19:32:10Z

Ok, I'm merging this; thanks @yamt for your patience as we veeery slowly reviewed this. I think any additional nits can be worked out in future commits and this moves things forward as-is.

yamt mentioned this pull request Dec 22, 2022

Add some wasi-threads tests WebAssembly/wasi-testsuite#37

Closed

yamt mentioned this pull request Dec 23, 2022

Add example with multiple threads creation bytecodealliance/wasm-micro-runtime#1826

Closed

yamt commented Dec 23, 2022

View reviewed changes

yamt mentioned this pull request Dec 24, 2022

test: add tests for pthread API WebAssembly/wasi-libc#369

Draft

loganek reviewed Jan 4, 2023

View reviewed changes

This was referenced Jan 5, 2023

should memory be exported as well? #22

Open

Update simple test #25

Merged

yamt force-pushed the wat-tests branch from 00ff533 to 40e5a44 Compare January 11, 2023 07:20

sbc100 reviewed Jan 11, 2023

View reviewed changes

abrown reviewed Jan 19, 2023

View reviewed changes

yamt force-pushed the wat-tests branch from 40e5a44 to 67c2b3f Compare January 23, 2023 04:58

yamt force-pushed the wat-tests branch from 11bd530 to c6be70c Compare January 23, 2023 08:45

loganek reviewed Jan 23, 2023

View reviewed changes

loganek approved these changes Jan 23, 2023

View reviewed changes

yamt added 5 commits January 24, 2023 07:53

Add a short summary on each wat files

d6206dd

Rename build.sh to build-c.sh

0f6a4fd

yamt added 5 commits January 24, 2023 07:53

Add build.sh which invokes build-*.sh

3b4f5b7

Move build-*.sh into a separate directory

d12f784

Give names to some locals and params in wat

43aaad2

Also add some comments

wasi_threads_spawn.wat: sprinkle a few memory fences

276bd47

While I suspect wait and notify are memory barriers for most implementations, it's safer to issue explicit fences.

opecode -> opcode in comments

7164d4b

yamt force-pushed the wat-tests branch from c6be70c to 7164d4b Compare January 23, 2023 22:55

eloparco mentioned this pull request Jan 29, 2023

Interrupt blocking instructions bytecodealliance/wasm-micro-runtime#1921

Closed

Rename thread_spawn to thread-spawn in wat tests

9082bca

cf. WebAssembly#26 WebAssembly/wasi-libc#387

abrown merged commit b278cc6 into WebAssembly:main Feb 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some low level tests #19

Add some low level tests #19

yamt commented Dec 22, 2022

abrown commented Dec 22, 2022

yamt commented Dec 22, 2022

yamt Dec 23, 2022

yamt Dec 24, 2022

loganek commented Dec 23, 2022

loganek Jan 4, 2023

yamt Jan 4, 2023

sbc100 Jan 4, 2023

yamt Jan 4, 2023

yamt Jan 5, 2023

yamt Jan 5, 2023

yamt Jan 11, 2023

yamt Jan 11, 2023

sbc100 Jan 11, 2023

yamt Jan 11, 2023

sbc100 Jan 11, 2023

yamt Jan 11, 2023

loganek Jan 11, 2023 •

edited

Loading

yamt Jan 11, 2023

yamt commented Jan 18, 2023

abrown left a comment

loganek commented Jan 20, 2023

yamt commented Jan 20, 2023

loganek commented Jan 20, 2023

abrown commented Jan 20, 2023

yamt commented Jan 23, 2023

loganek Jan 23, 2023

yamt Jan 23, 2023

loganek Jan 23, 2023

yamt Jan 23, 2023

yamt commented Jan 23, 2023

yamt commented Feb 6, 2023

abrown commented Feb 16, 2023

		@@ -0,0 +1,39 @@
		(module
		(memory (import "foo" "bar") 1 1 shared)

		@@ -0,0 +1,45 @@
		;; When the main thread calls proc_exit, it should terminate
		;; a thread blocking in `memory.atomic.wait32` opecode.

		@@ -0,0 +1,45 @@
		;; When a non-main thread calls proc_exit, it should terminate
		;; the main thread blocking in `memory.atomic.wait32` opecode.

Add some low level tests #19

Add some low level tests #19

Conversation

yamt commented Dec 22, 2022

abrown commented Dec 22, 2022

yamt commented Dec 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loganek commented Dec 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loganek Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamt commented Jan 18, 2023

abrown left a comment

Choose a reason for hiding this comment

loganek commented Jan 20, 2023

yamt commented Jan 20, 2023

loganek commented Jan 20, 2023

abrown commented Jan 20, 2023

yamt commented Jan 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamt commented Jan 23, 2023

yamt commented Feb 6, 2023

abrown commented Feb 16, 2023

loganek Jan 11, 2023 •

edited

Loading