WASI testing #713

chfast · 2021-02-01T20:21:29Z

Closes #708

TODO

Separate wasm file loading from execution.
Consider separate unittest executable: fizzy-wasi-unittests. This relaxes WASI and TESTING CMake options dependencies. (resolution: It is fine for now; less CMake option combinations is good for development and nobody reported issues with compiling libuvwasi so far; the CMake option can always be added later).
Consider different location for fizzy::wasi library (resolution: it is fine where it is).
~~Create interface for WASI implementation / uvwasi. This allows mocking the implementation.~~ (To be done in a separate PR later)

codecov · 2021-02-01T20:30:29Z

Codecov Report

Merging #713 (e550933) into master (a1280d6) will decrease coverage by 0.02%.
The diff coverage is 68.18%.

@@            Coverage Diff             @@
##           master     #713      +/-   ##
==========================================
- Coverage   99.26%   99.24%   -0.03%     
==========================================
  Files          74       76       +2     
  Lines       11462    11482      +20     
==========================================
+ Hits        11378    11395      +17     
- Misses         84       87       +3

Flag	Coverage Δ
rust	`99.86% <ø> (ø)`
spectests	`90.55% <ø> (ø)`
unittests	`99.20% <68.18%> (-0.03%)`	⬇️

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

Impacted Files	Coverage Δ
tools/wasi/wasi.cpp	`59.57% <64.70%> (+2.27%)`	⬆️
tools/wasi/main.cpp	`66.66% <66.66%> (ø)`
test/unittests/wasi_test.cpp	`100.00% <100.00%> (ø)`

gumb0 · 2021-02-02T18:06:51Z

tools/wasi/CMakeLists.txt

-set(fizzy_include_dir ${PROJECT_SOURCE_DIR}/lib/fizzy)
+add_library(wasi)
+add_library(fizzy::wasi ALIAS wasi)
+target_compile_features(wasi PUBLIC cxx_std_17)


Not needed, fizzy:fizzy has it.

The fizzy::fizzy-internal is private dependency: the fizzy::wasi will be built with -std=c++17, but users of fizzy::wasi (e.g. fizzy-wasi tool) will not have this requirement.

Here it make sense to declare fizzy::wasi with C++17 because it uses a C++17 feature in the main header.

This is not relevant any more because fizzy::wasi now links fizzy::fizzy-internal publicly.

This is not relevant any more because fizzy::wasi now links fizzy::fizzy-internal publicly.

Why does it?

Because now Fizzy is included in the wasi.hpp.

chfast

Update: all necessary changes are done to allow writing "top-down" unit tests - you can provide input as wat2wasm comment and check result and error string. For full testability we need WASI interface abstraction, but this should be done in a separate PR.

Without WASI abstraction we still have some integration tests in CTest. These are also nice so we know that the basic features of libuvwasi are working fine.

Finally, some unit tests truly accessing filesystem can be done in GTest provided it has some helpers creating/deleting temporary files.

Also see updated description.

chfast · 2021-02-23T17:23:19Z

tools/wasi/wasi.cpp


-bool run(int argc, const char** argv)
+std::optional<bytes> load_wasm_binary(const std::filesystem::path& path, std::ostream& err)


This function has pretty bad API, but I'm not sure how exactly improve it right now.
Also current error reporting is in some middle ground: it provides some additional but not complete set of diagnostics. And finally, the std::ifstream can sometimes be in "fail" state but it can also throw an exception in other cases...

Not sure how filesystem::path works but maybe this function should take a string (provided by user) and create filesytem::path inside, handling the errors like invalid symbols in path.

I considered the following options and used the one which is the easiest to use at the moment.

const char* - only literals and argv[] work really,

const std::string& - specific string type used, conversion from const char* is needed.

std::string_view - does not work with std::ifstream - conversion to std::string is needed.

std::filesystem::path - works with everything, emphasize that this is path.

I agree that invalid path format should be also handled inside. In the some time, there are other error cases that are not handled explicitly.

So switch to some other type?

(I imagined it like taking string_view then constucting path out of it, and then ifstream out of this path)

I'm fine to leave it like this for now. At some point exceptions thrown by path constructor will need to be handled (when we address @todo Make noexcept.)

chfast · 2021-02-23T17:38:26Z

Lastly, this has triggered a know bug in stdlibc++'s std::ostringstream implementation: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97415.

I think we should disable GCC sanitizer for GCC 10 and wait for GCC 11 release (or use experimental Debian package).

chfast · 2021-02-24T10:56:57Z

I think we should disable GCC sanitizer for GCC 10 and wait for GCC 11 release (or use experimental Debian package).

One ASan runtime option is disabled for current GCC runs.

tools/wasi/wasi.hpp

gumb0 · 2021-02-24T15:09:36Z

tools/wasi/wasi.cpp


-bool run(int argc, const char** argv)
+std::optional<bytes> load_wasm_binary(const std::filesystem::path& path, std::ostream& err)


Not sure how filesystem::path works but maybe this function should take a string (provided by user) and create filesytem::path inside, handling the errors like invalid symbols in path.

tools/wasi/wasi.hpp

tools/wasi/wasi.cpp

gumb0

Looks good!

axic · 2021-03-09T20:08:24Z

tools/wasi/wasi.cpp

-    auto imports = fizzy::resolve_imported_functions(*module, wasi_functions);
-    auto instance = fizzy::instantiate(
-        std::move(module), std::move(imports), {}, {}, {}, fizzy::MemoryPagesValidationLimit);
+    auto module = parse(wasm_binary);


I actually liked having the explicit fizzy prefix on things here, but I'm fine either way.

It does not make sense when we are in fizzy::wasi.

tools/wasi/wasi.cpp

axic · 2021-03-09T20:09:48Z

tools/wasi/wasi.cpp

 // SPDX-License-Identifier: Apache-2.0

+#include "wasi.hpp"
 #include "execute.hpp"
 #include "limits.hpp"
 #include "parser.hpp"


Is the <iostream> header still needed here (since std::cerr is moved to main.cpp)?

axic · 2021-03-09T20:13:29Z

tools/wasi/wasi.cpp

@@ -99,7 +99,7 @@ ExecutionResult environ_sizes_get(Instance& instance, const Value* args, int)
 }
 }  // namespace

-bool run(int argc, const char** argv)
+bool run(int argc, const char** argv, std::ostream& err)


I know you say to make this noexcept, but if not noexcept we could consider just outputting error text as part of an exception? Or we want to pass stdout/stdin here too?

This is to be able to inspect the error messages, one step from using std::cerr implicitly inside. Further modifications are possible, but I'm not sure what's best.

tools/wasi/CMakeLists.txt

axic · 2021-03-09T20:16:14Z

tools/wasi/CMakeLists.txt

 target_include_directories(wasi PUBLIC ${CMAKE_CURRENT_SOURCE_DIR})
-target_link_libraries(wasi PRIVATE fizzy::fizzy-internal uvwasi::uvwasi)
+target_link_libraries(wasi PUBLIC fizzy::fizzy-internal PRIVATE uvwasi::uvwasi)


Does this change below to the load_file commit?

Yes, because with this commit wasi.hpp started including fizzy headers.

tools/wasi/wasi.cpp

axic · 2021-03-09T20:19:21Z

tools/wasi/wasi.hpp

 /// @param  err     Error output stream.
 /// @return         False in case of error.
 ///
 /// @todo Make noexcept.
-bool run(int argc, const char** argv, std::ostream& err);
+bool load_and_run(int argc, const char** argv, std::ostream& err);


Why do you want to do this separation?

Why do we have load_and_run() combo? There are some uses of it, previously run().

axic

You may want to address some of these comments, but I am also okay if not.

Downgrade GCC ASan detect_invalid_pointer_pairs check from 2 (comparisons/subtractions against null pointers are also invalid) to 1.

axic · 2021-03-10T23:32:03Z

We forgot one thing: the uvwasi_state must be moved into some context and can't stay as a static global now that this is librarized.

chfast · 2021-03-11T09:16:13Z

This will be done with mocking.

chfast requested review from axic and gumb0 February 1, 2021 20:25

gumb0 reviewed Feb 2, 2021

View reviewed changes

chfast force-pushed the wasi_testing branch from a084bf5 to f1659dc Compare February 19, 2021 13:34

chfast commented Feb 23, 2021

View reviewed changes

chfast marked this pull request as ready for review February 23, 2021 17:39

chfast requested a review from gumb0 February 23, 2021 17:39

gumb0 reviewed Feb 24, 2021

View reviewed changes

chfast force-pushed the wasi_testing branch 2 times, most recently from 9d76188 to 64f408d Compare March 1, 2021 13:45

gumb0 approved these changes Mar 1, 2021

View reviewed changes

axic reviewed Mar 9, 2021

View reviewed changes

tools/wasi/wasi.cpp Show resolved Hide resolved

axic reviewed Mar 9, 2021

View reviewed changes

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

axic reviewed Mar 9, 2021

View reviewed changes

tools/wasi/wasi.cpp Show resolved Hide resolved

axic reviewed Mar 9, 2021

View reviewed changes

axic approved these changes Mar 9, 2021

View reviewed changes

chfast added 3 commits March 10, 2021 20:38

ci: Downgrade GCC ASan pointer checks

38af7c9

Downgrade GCC ASan detect_invalid_pointer_pairs check from 2 (comparisons/subtractions against null pointers are also invalid) to 1.

wasi: Put WASI related stuff in fizzy::wasi namespace

6b9ff50

wasi: Move fizzy-wasi main() to separate file

57c29dc

chfast added 6 commits March 10, 2021 20:44

wasi: Add error stream param

faacc57

cmake: Create wasi library

808df1f

cmake: Force WASI when TESTING

b2e2f2c

test: Add WASI unit tests

05842f8

wasi: Separate load_file() procedure

3781dd2

wasi: Split run() procedure

e550933

chfast force-pushed the wasi_testing branch from 64f408d to e550933 Compare March 10, 2021 19:45

chfast merged commit 07f87db into master Mar 10, 2021

chfast deleted the wasi_testing branch March 10, 2021 22:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WASI testing #713

WASI testing #713

chfast commented Feb 1, 2021 •

edited

Loading

codecov bot commented Feb 1, 2021 •

edited

Loading

gumb0 Feb 2, 2021

chfast Feb 19, 2021

chfast Feb 24, 2021

gumb0 Feb 24, 2021

chfast Feb 25, 2021

chfast left a comment

chfast Feb 23, 2021

gumb0 Feb 24, 2021

chfast Feb 25, 2021

gumb0 Feb 25, 2021

chfast Mar 1, 2021

chfast commented Feb 23, 2021

chfast commented Feb 24, 2021

gumb0 Feb 24, 2021

gumb0 left a comment

axic Mar 9, 2021

chfast Mar 10, 2021

axic Mar 9, 2021

axic Mar 9, 2021

chfast Mar 10, 2021

axic Mar 9, 2021 •

edited

Loading

chfast Mar 10, 2021

axic Mar 9, 2021

chfast Mar 10, 2021

axic left a comment

axic commented Mar 10, 2021

chfast commented Mar 11, 2021


		bool run(int argc, const char** argv)
		std::optional<bytes> load_wasm_binary(const std::filesystem::path& path, std::ostream& err)

WASI testing #713

WASI testing #713

Conversation

chfast commented Feb 1, 2021 • edited Loading

codecov bot commented Feb 1, 2021 • edited Loading

Codecov Report

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

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

Choose a reason for hiding this comment

chfast commented Feb 23, 2021

chfast commented Feb 24, 2021

Choose a reason for hiding this comment

gumb0 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

Choose a reason for hiding this comment

axic Mar 9, 2021 • edited Loading

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

axic commented Mar 10, 2021

chfast commented Mar 11, 2021

chfast commented Feb 1, 2021 •

edited

Loading

codecov bot commented Feb 1, 2021 •

edited

Loading

axic Mar 9, 2021 •

edited

Loading