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

Adding tests for wat compatibility #33

Closed
ktran opened this issue Mar 5, 2020 · 6 comments
Closed

Adding tests for wat compatibility #33

ktran opened this issue Mar 5, 2020 · 6 comments

Comments

@ktran
Copy link
Collaborator

ktran commented Mar 5, 2020

We would like to validate that the wat output generated by the wasmparser is wat compatible for DevTools.

Would it make sense to you to include running wat2wasm's parseWat for validation? At the moment parseWat is already used in some tests in test.ts to generate the wasm binary, before disassembling it again and testing the output.

An option would be to run parseWat on the test/*out files and confirm that no error was thrown. However, in this case a few out files (4 in total, if pull request #32 is accepted) need to be excluded, as wat2wasm in some cases requires different wat format, although the format in the wasmparser is valid according to the specification (e.g. because of different naming of operators for SIMD -- i32.atomic.rmw8.add_u vs i32.atomic.rmw8_u.add, etc.). It would also be possible to adapt the output to be completely compatible with wat2wasm.

What do you think?

@bmeurer
Copy link
Collaborator

bmeurer commented Mar 19, 2020

I'd vote for wasmparser to be 100% compatible with wabt.

@sunfishcode
Copy link

wasmparser should follow the spec, which was changed to use the i32.atomic.rmw8.add_u names. wabt also follows this spec, and is already updated for the new names, so this will be compatible.

Note that https://webassembly.github.io/threads/ still uses the i32.atomic.rmw8_u.add names; I filed WebAssembly/threads#152 for that.

@ktran
Copy link
Collaborator Author

ktran commented Mar 24, 2020

Thanks Benedikt and Dan for the comments!

Dan, thanks for linking the updated threads spec.

Following these specs (threads, bulk-memory, and core spec) there seem to be a few things that are different in the wasmparser:

  • using i32.atomic.rmw8.add_u
  • table.copy arguments
  • table.init arguments
  • elem arguments (this might require a bigger change)

I'll have a look into these issues.

bmeurer added a commit to bmeurer/wasmparser that referenced this issue May 13, 2020
In https://webassembly.github.io/spec/core/text/instructions.html#variable-instructions
the variable instructions are specified as `local.get` instead of `get_local`,
and so on. This changes the disassembler (and the internal names) to match the
standard.

Ref: wasdk#33
Bug: https://crbug.com/1043031
bmeurer added a commit to bmeurer/wasmparser that referenced this issue May 13, 2020
In https://webassembly.github.io/spec/core/text/instructions.html#variable-instructions
the variable instructions are specified as `local.get` instead of `get_local`,
and so on. This changes the disassembler (and the internal names) to match the
standard.

Ref: wasdk#33
Bug: https://crbug.com/1043031
@duongnhn
Copy link
Contributor

Thanks Benedikt and Dan for the comments!

Dan, thanks for linking the updated threads spec.

Following these specs (threads, bulk-memory, and core spec) there seem to be a few things that are different in the wasmparser:

  • using i32.atomic.rmw8.add_u
  • table.copy arguments
  • table.init arguments
  • elem arguments (this might require a bigger change)

I'll have a look into these issues.

@ktran What's about "elem arguments"? Could you elaborate :)

@ktran
Copy link
Collaborator Author

ktran commented May 27, 2020

My comment on elem does not seem to be up to date anymore; it happened when parsing the memory_bulk.0.wasm file, which seems fine now.

@duongnhn
Copy link
Contributor

My comment on elem does not seem to be up to date anymore; it happened when parsing the memory_bulk.0.wasm file, which seems fine now.

No worries. I think all the issues here are resolved now :)

@bmeurer bmeurer closed this as completed May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants