Skip to content
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

Rename `wasm2asm` to `wasm2js`, emit ESM by default #1642

Merged
merged 12 commits into from Aug 30, 2018

Conversation

@alexcrichton
Copy link
Contributor

@alexcrichton alexcrichton commented Aug 24, 2018

This commit updates the wasm2asm tool by renaming it to wasm2js and then tweaks it to emit an ES module by default. The old behavior is preserved for tests and internal usage, but external usage will now (in theory) effectively replace a wasm file with an ES module implemented without wasm that has the same behavior (roughly).

I'm not convinced that the integration into wasm2js.h is now the best, I suspect there's better ways to create JS ASTs! Feedback on that would be much appreciated!

@alexcrichton alexcrichton force-pushed the alexcrichton:wasm2js branch from 8e7add2 to 372e276 Aug 24, 2018
Copy link
Member

@kripken kripken left a comment

Nice, thanks!


using namespace cashew;
using namespace wasm;

int main(int argc, const char *argv[]) {
Wasm2AsmBuilder::Flags builderFlags;
Options options("wasm2asm", "Transform .wast files to asm.js");
Wasm2JsBuilder::Flags builderFlags;

This comment has been minimized.

@kripken

kripken Aug 25, 2018
Member

Regarding capitalization, how about Wasm2JS (capitalizing the S as well)?

Element* root;
Module wasm;
Ref asmjs;

try {
if (options.debug) std::cerr << "s-parsing..." << std::endl;

This comment has been minimized.

@kripken

kripken Aug 25, 2018
Member

for reading the file, please include wasm-io.h, and ModuleReader there will read either a binary or a text file automatically for you (see e.g. tools/wasm-opt.cpp).

This comment has been minimized.

@alexcrichton

alexcrichton Aug 27, 2018
Author Contributor

Oh nice! This sort of plays into the question though I think of what to do with all the old output. In the wast mode it supports the --allow-asserts option which parses the remaining s-expressions and needs the original parser (e.g. can't use ModuleReader).

I wasn't really sure what to do with all the old tests that actually exercise wasm2asm, the new ESM output isn't compatible with spidermonkey I think and is only compatible with experimental node.js, so it makes it a bit more difficult to test (but perhaps not impossible!).

Do you have a preferred method of how this is handled? At the very least it seems definitely worthwhile to have a comment here explaining what's going on, but you may know of a better way to handle all this!

This comment has been minimized.

@kripken

kripken Aug 27, 2018
Member

Hmm - I think we just run in node.js on the bots anyhow, so spidermonkey not supporting ESM should not be a problem? (although I thought it did..? maybe behind a flag)

Options::Arguments::Zero,
[&](Options* o, const std::string& argument) {
builderFlags.onlyAsmjsWrapper = true;
})
.add_positional("INFILE", Options::Arguments::One,

This comment has been minimized.

@kripken

kripken Aug 25, 2018
Member

is preserving the old output just for testing purposes? If so I guess it makes this PR more incremental, but I lean towards just changing to the new JS output, to have less complexity overall. Or are there other benefits I'm missing?

This comment has been minimized.

@alexcrichton

alexcrichton Aug 27, 2018
Author Contributor

Oh hm I'm not sure I quite understand this comment, can you elaborate? I did want to be sure to emit the new ESM output by default, though!

This comment has been minimized.

@kripken

kripken Aug 27, 2018
Member

Sorry for not being clear. I meant, how about not adding this flag, and only doing the default behavior of emitting the ESM output? That seems like all we really want, and it's simpler to not add an option?

This comment has been minimized.

@kripken

kripken Aug 27, 2018
Member

Oh, I see in the later responses that the issue is that testing ESM is not that easy, so keeping the old mode is convenient for that reason.

I think testing in node is enough, as I said later down - but if we can't get that to work, then yeah, it does seem reasonable to keep an option to not emit ESM, for now (with a comment explaining why).

This comment has been minimized.

@alexcrichton

alexcrichton Aug 27, 2018
Author Contributor

Ah ok, makes sense!

Do you know of a good way to blacklist test for wasm2js in the test suite? I remember now after doing this again that the first failure I hit was:

executing:  bin/wasm2js test/empty_imported_table.wast
Fatal: non-function imports aren't supported yet

which makes sense in that wasm2js doesn't work with an imported function table right now, but that test is explicitly testing an imported function table!

This comment has been minimized.

@kripken

kripken Aug 28, 2018
Member

Do you mean to skip it, for now? There's no special mechanism, but some of the tests ignore certain inputs by just having a list of those in the python code. For example in the spec tests in check.py,

      # skip checks for some tests
      if os.path.basename(wast) in ['linking.wast', 'nop.wast', 'stack.wast', 'typecheck.wast', 'unwind.wast']:  # FIXME
        continue
@@ -82,8 +82,17 @@ static uint64_t constOffset(Table::Segment &segment) {
return c->value.getInteger();
}

static uint64_t constOffset(Memory::Segment &segment) {

This comment has been minimized.

@kripken

kripken Aug 25, 2018
Member

can actually use a c++ template for this and the other constOffset,

template<typename T>
static uint64_t constOffset(const T& segment) {
  ..

This comment has been minimized.

@alexcrichton

alexcrichton Aug 27, 2018
Author Contributor

Sounds good to me! Unfortunately though I'm pretty unfamiliar with C++ templates, and that change generates:

FAILED: /usr/bin/clang++    -Isrc -std=c++11 -Wall -Werror -Wextra -Wno-unused-parameter -fno-omit-frame-pointer -fPIC -fcolor-diagnostics -O3 -DNDEBUG -O2 -UNDEBUG   -std=gnu++11 -MD -MT CMakeFiles/wasm2js.dir/src/tools/wasm2js.cpp.o -MF CMakeFiles/wasm2js.dir/src/tools/wasm2js.cpp.o.d -o CMakeFiles/wasm2js.dir/src/tools/wasm2js.cpp.o -c src/tools/wasm2js.cpp
In file included from src/tools/wasm2js.cpp:25:
src/wasm2js.h:78:29: error: use 'template' keyword to treat 'dynCast' as a dependent template name
  auto* c = segment.offset->dynCast<Const>();
                            ^
                            template 
1 error generated.

do you know how that could be resolved?

This comment has been minimized.

@kripken

kripken Aug 27, 2018
Member

Hmm, yeah - sorry, this is some C++ unpleasantness ;) We are using a template in a template. To fix it, replace ->dynCast with ->template dynCast.

@@ -328,8 +340,12 @@ Ref Wasm2AsmBuilder::processWasm(Module* wasm, Name funcName) {
#endif

Ref ret = ValueBuilder::makeToplevel();
if (!flags.onlyAsmjsWrapper)

This comment has been minimized.

@kripken

kripken Aug 25, 2018
Member

{, } if if body is not on the same line

@@ -370,7 +370,7 @@ def update_wasm2asm_tests():
asserts_expected_file = os.path.join('test', asserts)
traps_expected_file = os.path.join('test', traps)

cmd = WASM2ASM + [os.path.join(wasm2asm_dir, wasm), '--allow-asserts']
cmd = WASM2JS + [os.path.join(wasm2js_dir, wasm), '--allow-asserts', '--only-asmjs-wrapper']

This comment has been minimized.

@kripken

kripken Aug 25, 2018
Member

I think I'm missing something - I see outputs without --only-asmjs-wrapper are included in this PR (which is great!) but I don't see the code to emit them?

This comment has been minimized.

@alexcrichton

alexcrichton Aug 27, 2018
Author Contributor

Oh this was the if (!flags.onlyAsmjsWrapper) checks added to src/wasm2asm.h. Basically this is keeping compatibility with the old test suite by ensuring that all the tests disable the ESM output

@alexcrichton alexcrichton force-pushed the alexcrichton:wasm2js branch from 807f06f to 5c6b14b Aug 27, 2018
@alexcrichton
Copy link
Contributor Author

@alexcrichton alexcrichton commented Aug 28, 2018

Ok should be updated!

Wasm2JSBuilder wasm2js(builderFlags);
asmjs = wasm2js.processWasm(&wasm);

// Otherwise assume it's a `*.wast` file and go from there

This comment has been minimized.

@kripken

kripken Aug 29, 2018
Member

maybe add a comment about why we can't just use ModuleReader everywhere, adding "because we need to process the assertions too"?

flattenAppend(ret, module);

// 64-bit numbers get a different ABI w/ wasm2asm, and in general you can't
// Ref lib = ValueBuilder::makeObject();

This comment has been minimized.

@kripken

kripken Aug 29, 2018
Member

can all this commented-out code be removed?

@kripken
Copy link
Member

@kripken kripken commented Aug 29, 2018

Great, thanks! Looks good aside from two tiny comments above, and also I wanted to ask where memasmFunc etc. come from - seems like an odd name, why isn't it just mem?

@alexcrichton
Copy link
Contributor Author

@alexcrichton alexcrichton commented Aug 30, 2018

Updated!

It was originally called mem yeah but with the tests some of them instantiate multiple wasm modules which means that the mem needs tob e named separately, and for now it was just easiest to append the function name (which was already unique) onto the end.

@kripken
Copy link
Member

@kripken kripken commented Aug 30, 2018

Sounds good, thanks! lgtm. Looks like some test failures though, maybe need to run the test output updater?

@alexcrichton
Copy link
Contributor Author

@alexcrichton alexcrichton commented Aug 30, 2018

Er oops, updated!

@kripken
Copy link
Member

@kripken kripken commented Aug 30, 2018

Hmm, sorry about this, looks like those failing tests are recent breakage on master, and unrelated to this PR. But this PR is good to land as soon as we figure that out.

@kripken
Copy link
Member

@kripken kripken commented Aug 30, 2018

Master should be fixed now - can you please merge it to here?

alexcrichton added 12 commits Aug 16, 2018
This commit performs a relatively simple rename of the `wasm2asm` tool to
`wasm2js`. The functionality of the tool doesn't change just yet but it's
intended that we'll start generating an ES module instead of just an `asm.js`
function soon.
Previously `wasm2js` only supported `*.wast` files but to make it a bit easier
to use in tooling pipelines this commit adds support for reading in a `*.wasm`
file directly. Determining which parser to use depends on the input filename,
where the binary parser is used with `*.wasm` files and the wast parser is used
for all other files.
This commit adds a new argument, `--only-asmjs-wrapper`, to the `wasm2js` tool.
While probably not going to be widely used it's intended for this argument to be
used by the test suite to continue operating as it does today. When not passed
the behavior of the `wasm2js` tool will be to emit an ES module by default (to
be added after this commit).
This commit alters the default behavior of `wasm2js` to emit an ESM by default,
either importing items from the environment or exporting. Items like
initialization of memory are also handled here.
And port the tests to work with it!
@alexcrichton alexcrichton force-pushed the alexcrichton:wasm2js branch from 76fca8f to 9443bc4 Aug 30, 2018
@alexcrichton
Copy link
Contributor Author

@alexcrichton alexcrichton commented Aug 30, 2018

Sure thing!

@kripken
Copy link
Member

@kripken kripken commented Aug 30, 2018

All green, merging.

@kripken kripken merged commit f109f3c into WebAssembly:master Aug 30, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jakirkham
Copy link

@jakirkham jakirkham commented Sep 16, 2018

Was asm2wasm suppose to be renamed to js2wasm too?

@kripken
Copy link
Member

@kripken kripken commented Sep 17, 2018

It's not quite symmetrical - asm2wasm does actually translate asm.js, and not arbitrary JS. So I think it's ok to leave the current name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.