WASI tests #790

gumb0 · 2021-04-21T18:09:26Z

No description provided.

codecov · 2021-04-22T10:40:01Z

Codecov Report

Merging #790 (7b700be) into master (c341e3a) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 7b700be differs from pull request most recent head 145f2d2. Consider uploading reports for the commit 145f2d2 to get more accurate results

@@            Coverage Diff             @@
##           master     #790      +/-   ##
==========================================
+ Coverage   99.10%   99.12%   +0.02%     
==========================================
  Files          86       85       -1     
  Lines       13017    13010       -7     
==========================================
- Hits        12900    12896       -4     
+ Misses        117      114       -3

Flag	Coverage Δ
rust	`99.90% <0.00%> (+0.29%)`	⬆️
spectests	`89.98% <0.00%> (ø)`
unittests	`98.94% <0.00%> (ø)`
unittests-32	`99.03% <0.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
bindings/rust/src/constnonnull.rs
bindings/rust/src/lib.rs	`99.90% <0.00%> (+<0.01%)`	⬆️

chfast · 2021-04-22T15:36:12Z

If this way is easier, I'm fine with this. Do you have estimate how much uvwasi methods are we are going to need?

gumb0 · 2021-04-23T08:47:08Z

If this way is easier, I'm fine with this. Do you have estimate how much uvwasi methods are we are going to need?

It seems it's about one uvwasi funciton per one WASI function + some serialization functions that we may or may not want to mock.

tools/wasi/CMakeLists.txt

gumb0 · 2021-05-04T11:18:43Z

Obeservations about serizaliation part of uvwasi:

Deserialization functions for fd_write / fd_read check passed pointers against the bounds passed in end parameter:
https://github.com/nodejs/uvwasi/blob/adda15515cbb5a478d0ddb85602d987278a43384/src/wasi_serdes.c#L191-L192

So I think passing the memory size here is correct:

fizzy/tools/wasi/wasi.cpp

Line 45 in 2ea01bc

// TODO: not sure what to pass as end, passing memory size...

It doesn't check bounds when deserializing "buffer pointer" and "buffer size" values themselves:
https://github.com/nodejs/uvwasi/blob/adda15515cbb5a478d0ddb85602d987278a43384/src/wasi_serdes.c#L188-L189
~~We could check these pointers ourselves at our side of fd_write / fd_read.~~
On our side of fd_write / fd_read we could check at least that pointers we pass to uvwasi (iov_ptr, nread_ptr, nwritten_ptr) are inside memory bounds.

chfast

This looks good.

Some additional questions:

This seems not using GMock. Is the include even needed?
We considered before to create separate unit tests executable for wasi, e.g. fizzy-unittests-wasi to decouple this subproject and its dependencies. This would also allow us to create separate directory for wasi unit tests and split these into more files.

chfast · 2021-05-04T11:24:09Z

tools/wasi/uvwasi.hpp

+class UVWASI
+{
+public:
+    virtual ~UVWASI();


= default.

This produces error: 'UVWASI' has no out-of-line virtual method definitions; its vtable will be emitted in every translation unit [clang-diagnostic-weak-vtables,-warnings-as-errors]

https://app.circleci.com/pipelines/github/wasmx/fizzy/6667/workflows/39a77e98-002f-42e8-a916-19acec64fcbc/jobs/58758/parallel-runs/0/steps/0-111

chfast · 2021-05-04T11:27:04Z

tools/wasi/wasi.hpp

@@ -13,6 +13,9 @@ struct Instance;

 namespace wasi
 {
+class UVWASI;
+extern UVWASI* uvwasi;


Is this needed for mocking?

Yes.

Alternatively maybe It could be declared in wasi_test.cpp and another time in wasi.cpp

gumb0 · 2021-05-19T14:05:22Z

@axic You've merged #796 here, I have removed the merge commit, the last commit is now a single commit from #796.

axic · 2022-05-20T10:41:47Z

@gumb0 can you rebase this?

gumb0 · 2022-05-20T13:59:49Z

Rebased.

axic · 2022-05-21T09:55:06Z

test/unittests/wasi_test.cpp

+
+    EXPECT_TRUE(mock_uvwasi->init_called);
+    ASSERT_TRUE(mock_uvwasi->write_fd.has_value());
+    EXPECT_EQ(*mock_uvwasi->write_fd, 1);


Based on the tests, write_fd should be a vector too.

Not sure what you mean, WASI fd_write calls uvwasi_fd_write only once, with one fd argument, and this is what the mock saves in write_fd and the test checks.

In the current tests, but eventually can imagine multiple calls.

Then it would rather be vector like vector<pair<fd, write_data>> to record all calls... not sure it's worth to complicate it now.

test/unittests/wasi_test.cpp

axic · 2022-05-21T09:57:20Z

test/unittests/wasi_test.cpp

+    /* wat2wasm
+      (func (import "wasi_snapshot_preview1" "fd_write") (param i32 i32 i32 i32) (result i32))
+      (memory (export "memory") 1)
+      (data (i32.const 0)    "\00\00\01\00")    ;; buf ptr - out of memory bounds


So this only checks the offset against pages?

I think the test tries to check that error value returned from uvwasi is correctly propagated. I don't think we should test all possible errors that uvwasi returns.

I meant this only checks against exceeding memory pages?

The test tries to get the data to write from the address out of memory bounds (0x10000 = 65536), so this is just one possible way to make fd_write fail.

test/unittests/wasi_test.cpp

axic

Okay for now I think.

gumb0 force-pushed the uvwasi_interface branch 2 times, most recently from cbf1c6a to b6b0fa5 Compare April 22, 2021 10:24

gumb0 force-pushed the uvwasi_interface branch from b6b0fa5 to e1b08e6 Compare April 22, 2021 11:06

gumb0 force-pushed the uvwasi_interface branch 19 times, most recently from fff0787 to 8c58abc Compare April 28, 2021 17:38

gumb0 commented May 3, 2021

View reviewed changes

tools/wasi/CMakeLists.txt Outdated Show resolved Hide resolved

gumb0 force-pushed the uvwasi_interface branch 2 times, most recently from 21dd342 to 3d9fa22 Compare May 4, 2021 10:51

chfast reviewed May 4, 2021

View reviewed changes

chfast approved these changes May 19, 2021

View reviewed changes

gumb0 force-pushed the uvwasi_interface branch 2 times, most recently from 13995b6 to 72e7826 Compare May 19, 2021 13:54

axic requested a review from chfast May 20, 2022 10:41

gumb0 force-pushed the uvwasi_interface branch 2 times, most recently from 767a55e to 39f0c06 Compare May 20, 2022 13:37

gumb0 force-pushed the uvwasi_interface branch from 39f0c06 to a2f67b0 Compare May 20, 2022 16:37

chfast approved these changes May 20, 2022

View reviewed changes

axic reviewed May 21, 2022

View reviewed changes

test/unittests/wasi_test.cpp Outdated Show resolved Hide resolved

axic reviewed May 21, 2022

View reviewed changes

test/unittests/wasi_test.cpp Outdated Show resolved Hide resolved

axic approved these changes May 23, 2022

View reviewed changes

gumb0 added 8 commits May 23, 2022 15:11

test: WASI tests for proc_exit and fd_write with mocked UVWASI

b60180f

test: More tests for fd_write

03f1f66

test: WASI tests for fd_read

1bac224

test: WASI test for fd_prestat_get

c36682d

test: WASI test for fd_prestat_dir_name (check it's not implemented)

13df69a

test: WASI test for environ_sizes_get (check it always returns 0s)

c7fc915

test: WASI test for environ_get (check it's not implemented)

c792d24

cmake: Split WASI unit tests into a separate executable

145f2d2

gumb0 force-pushed the uvwasi_interface branch from a2f67b0 to 145f2d2 Compare May 23, 2022 13:11

axic merged commit ee99be7 into master May 23, 2022

axic deleted the uvwasi_interface branch May 23, 2022 13:36

axic mentioned this pull request May 23, 2022

WASI interface #789

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WASI tests #790

WASI tests #790

gumb0 commented Apr 21, 2021 •

edited

Loading

codecov bot commented Apr 22, 2021 •

edited

Loading

chfast commented Apr 22, 2021

gumb0 commented Apr 23, 2021

gumb0 commented May 4, 2021 •

edited

Loading

chfast left a comment

chfast May 4, 2021

gumb0 May 4, 2021

chfast May 4, 2021

gumb0 May 4, 2021

gumb0 commented May 19, 2021 •

edited

Loading

axic commented May 20, 2022

gumb0 commented May 20, 2022

axic May 21, 2022

gumb0 May 23, 2022

axic May 23, 2022

gumb0 May 23, 2022

axic May 21, 2022

gumb0 May 23, 2022

axic May 23, 2022

gumb0 May 23, 2022

axic left a comment

WASI tests #790

WASI tests #790

Conversation

gumb0 commented Apr 21, 2021 • edited Loading

codecov bot commented Apr 22, 2021 • edited Loading

Codecov Report

chfast commented Apr 22, 2021

gumb0 commented Apr 23, 2021

gumb0 commented May 4, 2021 • edited Loading

chfast left a 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

gumb0 commented May 19, 2021 • edited Loading

axic commented May 20, 2022

gumb0 commented May 20, 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

axic left a comment

Choose a reason for hiding this comment

gumb0 commented Apr 21, 2021 •

edited

Loading

codecov bot commented Apr 22, 2021 •

edited

Loading

gumb0 commented May 4, 2021 •

edited

Loading

gumb0 commented May 19, 2021 •

edited

Loading