wasi: Add UVWASI interface #792
Conversation
tools/wasi/uvwasi.cpp
Outdated
| uvwasi_t m_state{}; | ||
|
|
||
| public: | ||
| ~UVWASIImpl() final { uvwasi_destroy(&m_state); } |
There was a problem hiding this comment.
Apparently we'll need a flag to check if uvwasi_init was already called sucessfully.
Codecov Report
@@ Coverage Diff @@
## master #792 +/- ##
==========================================
- Coverage 99.23% 99.14% -0.09%
==========================================
Files 79 80 +1
Lines 12394 12443 +49
==========================================
+ Hits 12299 12337 +38
- Misses 95 106 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| auto module = parse(wasm_binary); | ||
| auto imports = resolve_imported_functions(*module, wasi_functions); | ||
| return instantiate(std::move(module), std::move(imports), {}, {}, {}, MaxMemoryPagesLimit); | ||
| } |
There was a problem hiding this comment.
We had an assert(instance != nullptr) in the old place. Worth keeping here or unique_ptr ensures it?
There was a problem hiding this comment.
unique_ptr doesn't care, but instantiate reports errors by throwing, so I don't think it's useful.
tools/wasi/wasi.hpp
Outdated
| @@ -26,6 +31,18 @@ std::optional<bytes> load_file(std::string_view file, std::ostream& err) noexcep | |||
| /// @todo Make noexcept. | |||
| bool load_and_run(int argc, const char** argv, std::ostream& err); | |||
|
|
|||
| /// Instantiates a module that imports WASI functions. | |||
| std::unique_ptr<Instance> instantiate(bytes_view wasm_binary); | |||
There was a problem hiding this comment.
What is the reason behind splitting?
There was a problem hiding this comment.
To access instance in tests after run.
gumb0
force-pushed
the
wasi-instance
branch
2 times, most recently
from
8739786
to
d43a350
Compare
tools/wasi/wasi.cpp
Outdated
| const uvwasi_errno_t uvwasi_err = uvwasi_init(&state, &options); | ||
| bool run(Instance& instance, int argc, const char* argv[], std::ostream& err) | ||
| { | ||
| if (!uvwasi) |
There was a problem hiding this comment.
I don't think this is good idea, for many reasons (e.g. not thread-safe, hidden dependencies). Why just not add UVWASI& param?
There was a problem hiding this comment.
This is a good idea.
Do you want also to pass it into WASI functions via host_context to make it thread-safe?
There was a problem hiding this comment.
I don't see why this would be needed. This object will be bound to the imported functions, right?
By "not-thread-safe" I mean the initialization. I think this should be propagated up to the main() function which should invoke create_uvwasi(). And fizzy-unittest-wasi will execute create_mocked_uvwasi() if needed.
There was a problem hiding this comment.
I don't see why this would be needed. This object will be bound to the imported functions, right?
What do you mean by bound? The way to bind it in current API is to pass it via host_context.
See the latest version.
| public: | ||
| ~UVWASIImpl() final | ||
| { | ||
| if (m_inited) |
There was a problem hiding this comment.
Also suggested a change to uvwasi to handle this nodejs/uvwasi#163
But it won't help the case with calling init twice, so we need a flag anyway.
| @@ -13,10 +13,20 @@ class UVWASIImpl final : public UVWASI | |||
| { | |||
| /// UVWASI state. | |||
| uvwasi_t m_state{}; | |||
| bool m_inited = false; | |||
There was a problem hiding this comment.
Perhaps m_initialised is more correct, but fine either way.
Pulled out of #790