-
Notifications
You must be signed in to change notification settings - Fork 702
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wasm2c: uvwasi support #2002
base: main
Are you sure you want to change the base?
wasm2c: uvwasi support #2002
Conversation
This looks quite interesting! Big-picture questions:
Smaller stuff:
|
Hi Keith, Thanks for the quick feedback and thoughtful comments!
People who like running code that makes system calls =) On several current projects students needed to be able to run existing libraries and applications, but couldn't, because of the lack of a WASI support in wasm2c (e.g. because said libraries/applications needed to access the filesystem). I went looking for a WASI implementation and came across wasm2native, so I forked an older version that still supported (an older version of) wasm2c, made some small changes, and students have been making use of that. Being able to just compile and run existing code with wasi-libc and wasi makes life so much easier, and most other wasm toolchains (e.g. WAMR, Wasmtime) already supports this. uvwasi seems to be a reasonable wasi implementation, node.js uses it, WAMR uses it, and its a third party dependency in wasm2c already.
I think there is plenty of stuff one might want to sandbox today with RLBox where you need some WASI support, and this is a reasonable solution for that.
Just started looking at that yesterday. There are a couple of wasi test suites out there in various stages of usefulness. Some of that discussion is here: There doesn't seem to be one answer that folks have converged on. node.js seems to have a decent little collection of tests but the driver seems a little more complicated then I what I want, since I'm just testing a binding, and I don't think wasm2c wants to depend on node. My first thought was So, not 100% clear on the path forward (I have a couple other ideas), but plan to have that worked out soon.
Yup, that sounds good. Removing pre-processor stuff and hard coding names 👍 . Moving wasm2native into /scripts and out of examples probably makes sense, then it can just be
I would be glad to know. I reached out to @vshymanskyy a couple days ago by email but haven't heard back yet. wasm2native abandoned wasm2c support sometime back in favor of w2c2, so I'm guessing its not a priority.
👍
Sure, that seems like a good suggestion.
Honestly, it was one of the few things that was there when I grabbed this code where I was like, huh, that's gross, I should probably look into that at some point, and moved on. There is probably some backstory there I just grepped through wasi-libc and it looks like its only using the preview1 symbols. I would be happy to rip out that extra pre-processor cruft.
None of those flags are needed for this simple example, they just got carried along with the boilerplate from a previous makefile I used for this =/
I think taking out the stuff that builds .c => wasm makes sense for the example as it eliminates that dependency on the wasi-sdk. I think making this simpler and keeping it as the wasi example makes sense, and then turning build-standalone into wasm2native as a script also sounds good. |
Co-authored-by: Yuhan Deng <[email protected]>
…ebAssembly#1999) also: - cleanup handling of newlines - "init_memory"/"init_table" -> "init_memories"/"init_tables"
b3e7b63
to
e9cd029
Compare
@@ -395,6 +397,10 @@ IF (NOT WIN32) | |||
endif () | |||
endif () | |||
|
|||
if (BUILD_UVWASI) | |||
add_subdirectory("third_party/uvwasi" ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a WITH_WASI
flag .. is there some reason we can't use use that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seemed to me like enabling the mostly not working wasi support in the interpreter, and allowing people to build their standalone apps with uvwasi were logically separate things. I'm not attached around the issue of wether this is enabled by default, so whatever makes sense just lmk.
Can you rebase now that the bulk memory change has landed? |
cp ${ORIGIN_DIR}/$1 ./input.wasm | ||
make input.c | ||
|
||
make input.elf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this build-standalone
idea. The mixing of a build.sh script and the Makefile file a find a little confusing, but I think we can iterate on it over time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure, it was just a quick way to get something working, if we want to promote to a first class feature I can just replace it with a stand-alone script.
|
||
//force pre-processor expansion of m_name | ||
#define __module_init(m_name) Z_## m_name ##_init_module() | ||
#define module_init(m_name) __module_init(m_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also agree with @keithw that it would be nice to avoid these macros and assume that wasm2c was run with the correct -m
flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure, they have been removed in 9703be9.
Sure. |
@sbc100 sorry about the delayed response, I was filtering my git notifications into a lower priority folder because of a time in the past when there was a lot of traffic. I have responded to all of @keithw's comments, except for the need for a test suite. I'm a little unsure of where to put this, since its a little bit out of step with the rest of the wabt tests. The wasm-c-api test script looks good, but those tests live in a third_part repo. If you have thoughts on where you would be happy with a directory of .c tests, that would be helpful, or I can put them in a submodule, whatever seems right. I think aside from this, adding some security checks to uvwasi-rt.c, and updating the README is all I have left on my list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could hello.wasm be included as a wat file (instead of checking the .wasm into Git)?
@@ -0,0 +1,607 @@ | |||
;;; TOOL: run-spec-wasm2c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is probably a Git issue (recreating the old-spec/bulk-memory-operations/imports.txt
file along with the other three files in this directory). It might be best to rebase your commits on top of the main branch (rather than using a merge commit), which will make the GitHub "Files changed" view a lot simpler.
On the testsuite, I think anything that gives confidence that the code works as intended (and will be robust to well-meaning but unintentionally buggy refactors) sounds fine to me. Maybe some options are:
Probably the tests should include some "evil" WASI commands that try to put naughty parameters in (to make sure these are correctly caught). |
If we could avoid using wat, and instead just have a C test suite that would be much less work for me to write, and https://github.com/nodejs/node/tree/main/test/wasi/c Each test could be stand-alone, consisting of a set of calls and some assertions For the purposes of testing with malicous parameters, we could certain add tests |
That sounds fine to me -- I guess the plan would be that the testsuite first compiles all these .c files to .wasm by using emscripten? If there's a pre-existing collection of WASI tests in C, I agree that's way better than writing them anew. I don't think it's a problem to put these tests in this repo in some subdir (maybe (Separately, somehow we do have to deal with the build failures in CI to get this merged...) |
Cool, sounds good. I think just using the clang/llvm compiler that comes as part of the wasi-sdk will be simpler. It seems like adding it as a dependency to the README would be the simplest way to include it.
Cool, will do.
I was thinking to just copy and lightly modify wabt/test/run-c-api-examples.py e.g. run-uvwasi-tests.py,
For sure. |
wasi-sdk is great too, but, where do we pull that in from for the CI tests? (With emscripten I know there's an emsdk:latest Docker repo that wabt already uses...) |
Cool, I think I would just stick with the clang that comes with the wasi-sdk. No need to get involved with emscripten.
|
I'd rather not depend on emscripten or wasi-sdk here. I think having them checked in here as wat files makes more sense, perhaps with script that can be used to update them using wasi-sdk? Also, I don't think its important to run the entire wasi test suite, at least not initially. Just one or two basic tests to start with I think should be fine. Once we are happy with the approach we can consider expanding to a larger wasi test suite. When we do this we should co-ordinate the wasm-interp which can also run wasi and it would be good to test that method using the same test suite. |
Hey! I'm not opposed to this at all. Of course a proper attribution should be included. Let me know if you have any questions! |
This all sounds good to me. |
Hi friends, @keithw @sbc100
Here is a first cut at providing uvwasi bindings for wasm2c.
Inspiration and code started as part of wasm2native.
wasm2c/examples/hello-wasi - shows how we can use this with a simple application that exercises wasi.
wasm2c/examples/build-standalone - provides a script that takes an arbitrary wasm file and builds it with wasi bindings so it can run as a normal stand-alone binary.
There are a few things that still need to be done, security checks on arguments to wasi calls, some documentation, but I think it's at the point where feedback would be helpful.