wasm2c: run multi-memory tests #1834
Conversation
426e286 to
9aeea2c
Compare
src/tools/wasm2c.cc
Outdated
| @@ -94,6 +94,7 @@ static void ParseOptions(int argc, char** argv) { | |||
| exit(1); | |||
| } | |||
| s_features.disable_bulk_memory(); | |||
| s_features.enable_multi_memory(); | |||
There was a problem hiding this comment.
I'm not sure we want to enable this in add cases. The tests that need it can/should probably enable it on the command line (see test/spec/multi-memory/data.txt for example)
There was a problem hiding this comment.
Our thinking here was that since wasm2c only supports a fixed set of WebAssembly features (https://github.com/WebAssembly/wabt/blob/main/src/tools/wasm2c.cc#L91-L95) and there's no way (currently) to enable/disable a feature on the command-line, basically we just wanted to add multi-memory to that fixed set.
We can add a command-line switch to enable multi-memory in wasm2c (and leave it off by default), and plumb that through run-spec-wasm2c.py, but this would be the first "selectable" feature that wasm2c supports. Please lmk if that's what you had in mind -- it seems doable so maybe that's the right future direction.
test/run-spec-wasm2c.py
Outdated
| @@ -386,6 +386,7 @@ def main(args): | |||
| error_cmdline=options.error_cmdline) | |||
| wast2json.verbose = options.print_cmd | |||
| wast2json.AppendOptionalArgs({'-v': options.verbose}) | |||
| wast2json.AppendArg('--enable-multi-memory') | |||
There was a problem hiding this comment.
Again, this should probably go in the test file
There was a problem hiding this comment.
Can do (if I understood your comment above correctly) -- we'll also have to plumb through the selectable features in run-spec-wasm2c.py. It's not as sophisticated as the interpreter runner and I don't think it supports command-line enable/disable arguments in the test file yet.
There was a problem hiding this comment.
I think it probably makes sense to pass features through to wasm2c, yes. Mostly because all the other wabt tools do that.
9aeea2c to
2c52c10
Compare
src/tools/wasm2c.cc
Outdated
| @@ -82,13 +89,15 @@ static void ParseOptions(int argc, char** argv) { | |||
|
|
|||
| // TODO(binji): currently wasm2c doesn't support any non-default feature | |||
| // flags. | |||
This runs and passes 5 of the 9 multi-memory tests (although one of them, binary.wast, is a nop for wasm2c itself). The other 4 tests would require reference types or bulk memory: imports.wast load.wast memory-multi.wast store.wast
2c52c10 to
31889d1
Compare
This PR is sequenced behind #1810 and #1829.PR #1751 added support for multi-memory and ran the multi-memory tests with
run-interp-spec. This PR also runs (some of) the multi-memory tests under wasm2c, withrun-spec-wasm2c. Only 5 of the 9 multi-memory tests are supported by wasm2c right now; the others require reference types, multi-table, or bulk memory.