Conversation
| return Building.run_binaryen_command('wasm-opt', *args, **kwargs) | ||
|
|
||
| @staticmethod | ||
| def do_wasm2c(infile): |
There was a problem hiding this comment.
I don't like adding stuff to shared.py like this because:
(a) this is not shared.. it has exactly one call site
(b) shared.py get included absolutely everywhere.. we should try to keep it minimal.
I know that we are long way from (b) and we have history of throwing stuff in shared.py .. but I don't think thats good reason to make it worse. I still like the idea of wasm2c.py which exactly one function in it.
There was a problem hiding this comment.
Yeah, I think I see what you mean. I moved it to a separate file now, let me know what you think.
| @@ -0,0 +1,111 @@ | |||
| # Copyright 2020 The Emscripten Authors. All rights reserved. | |||
There was a problem hiding this comment.
I don't care much about the location of the new file, but a benefit to wasm2c/__init__.py is that it won't mess up autocomplete (which having both wasm2c.py and wasm2c/*.c would do).
There was a problem hiding this comment.
It think just having tools/wasm2c.py is fine.
I don't think we should worry too much about auto-complete when naming files.
There was a problem hiding this comment.
Ok, renamed as you requested.
|
ping @sbc100 |
|
|
||
| extern int (*Z_do_bad_thingZ_ii)(int); | ||
|
|
||
| extern int (*Z_twiceZ_ii)(int); |
There was a problem hiding this comment.
I find it clearer to just use C ABI for these kinds of tests to avoid the name mangling.
There was a problem hiding this comment.
That's name mangling that wasm2c does. cc @binji
The Z_ prefix might make it look too much like C++ mangling, which can be confusing. I wonder if it would be worth considering W2C_ or such?
| #include <stdio.h> | ||
|
|
||
| EMSCRIPTEN_KEEPALIVE | ||
| int twice(int x) { |
There was a problem hiding this comment.
This file is a called ".c" but the name mangling looks like its being compiled as C++?
| js_engines += wasm_engines | ||
| if self.get_setting('WASM2C'): | ||
| # the "engine" to run wasm2c builds is clang that compiles the c | ||
| js_engines += [[CLANG_CC]] |
There was a problem hiding this comment.
This seems odd. Maybe "js_engines" needs to be renamed?
There was a problem hiding this comment.
Good idea, I'll look into that as a followup.
| configure_args=configure_args, | ||
| cache_name_extra=configure_commands[0]) | ||
|
|
||
| @also_with_standalone_wasm |
There was a problem hiding this comment.
test_hello_world is extremely basic, and we test far more interesting things, so it's redundant.
also, we use test_hello_world in a few places as a sanity check. it can be confusing to have complicated things like standalone happen there.
There was a problem hiding this comment.
I like the idea of being able to test simple things... now if I want to test the most basic standalone program I need to look for a more complex test?
What do you think about make also_with_standalone_wasm expand to @parameterized and turn the tests into two different tests?
Then test_hello_world_standalone can fail separately to test_hello_world_standalone (I recently made the parameterized thing handle empty string as the name of the variant correctly).
In any case this change is unrelated to wasm2c and doesn't really belong in this PR.
There was a problem hiding this comment.
Ok, I restored that test now.
We could also use parametrized, yeah. I can look into that.
| return 0; | ||
| }); | ||
|
|
||
| IMPORT_IMPL(u32, Z_wasi_snapshot_preview1Z_fd_readZ_iiiii, (u32 fd, u32 iov, u32 iovcnt, u32 pnum), { |
There was a problem hiding this comment.
Is this name mangling part of the way wasm2c works?
| configure_args=configure_args, | ||
| cache_name_extra=configure_commands[0]) | ||
|
|
||
| @also_with_standalone_wasm |
There was a problem hiding this comment.
I like the idea of being able to test simple things... now if I want to test the most basic standalone program I need to look for a more complex test?
What do you think about make also_with_standalone_wasm expand to @parameterized and turn the tests into two different tests?
Then test_hello_world_standalone can fail separately to test_hello_world_standalone (I recently made the parameterized thing handle empty string as the name of the variant correctly).
In any case this change is unrelated to wasm2c and doesn't really belong in this PR.
2 participants
This new option makes us run wabt's wasm2c, combine the output
with an emscripten
main()file + runtime in C, and emits a finalsingle C file of all that stuff. If that is built to a native executable it
does the same thing as the wasm(+js) would have done.
This has several potentially interesting use cases:
we can distribute those to users as an alternative for people that
can't use the emsdk builds for some reason. For example if binaryen
upgrades to c++20 but someone is on a linux distro that is too old for
our standard builds, and doesn't have a c++20 compiler, then giving
them a C file may help them.
building on the 3 major OSes but it takes a bunch of work. A C build
could be generated on a developer's machine very easily and run
on all targets. This is the same as making a build that runs in node.js,
which also would have those benefits. A C build has different
tradeoffs, the main benefit being that it may be faster and in particular
faster to start up (in theory node with wasm code caching could be
similar, but that doesn't work atm - hopefully it will be possible eventually).
a simple way.
Note that this PR uses an emscripten build of wasm2c, running on
node, instead of us needing to package normal binaries of wabt. That was
super-easy to do, and maybe we can consider it for binaryen or llvm
eventually (but for performance reasons, a wasm2c build may be
preferable there). It's an example of how the second item in that
bullet list can be helpful!