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

WASM Specification Testsuite #32

Merged
merged 2 commits into from
Sep 24, 2024

Conversation

george-cosma
Copy link
Collaborator

Pull Request Overview

This pull request adds the necessary bits of code to be able the interpret the .wast files present in the WASM Specification Testsuite. Core code has also been modified so that interpretation of the wast directives is possible.

Note: parsing of the .wast files is done by the crate with the same name.

TODO or Help Wanted

This pull request is still very much work-in-progress.

One of the main issues I would like to discuss is if the code changes to the validation & execution part of our interpreter could cause an issue. To be more precise, to be able to invoke any function, the parameters and results of the function must implement the trait InteropValueList. However, after wast parsing I end up with the types wast::WastArg and wast::WastRet which are the equivelent to our Value enum. The trait is impossible to implement for a vector of any of these two types because the wasm types are unknown at compile-time. For each variation of (Argument Types, Return Types) a new invoke_func is created. However, since these arguments are read at runtime, there is no way to assign a constant value to the TYS "paramater" of the InteropValueList.

The TYS constant is used in invoke_func to verify (at runtime) if the called wasm function takes the same parameters and emits the same results as the parameters for invoke_func. Switching the check for the parameters from a constant to a function was trivial for the parameters and (likely?) has the same effect. However, the verification for the result types had to be removed. Instead of removing it, an alternative would have been to pass all expected return types as an argument, but I had issues with this. This inherently makes the interpreter less safe.

I've gated these changes behind a feature flag, but that brings me to the core of my problem: if i gate these changes behind a feature flag, then the binary tested with the .wast files is different from that which will be released. This means that a bug in the release version could be missed, as it is not checked for/it doesn't happen in the version meant for the specification test.

I would like to hear your opinion on this, especially since the change I have described above is (probably) not the only one I will have to make in order to get .wast files working.

Formatting

  • Ran cargo fmt
  • Ran cargo check
  • Ran cargo build
  • Ran nix fmt
  • Ran treefmt

Github Issue

This pull request is linked to #9

Author

Signed-off-by: George Cosma [email protected]

@george-cosma
Copy link
Collaborator Author

I've had a second shot at the spec testsuite. I've built this attempt on top of #54. As it stands, this works with the notable mention that only the dummy.wast file doesn't error out immediately. The rest (which is to say, the official test suite) all fail instantly due to unimplemented op codes.

I've kept thinking if I should modify the verification to "skip" functions with invalid op codes, making us able to test at least the functioning functions/op codes. However, this feature will be made pointless in the (far) future when we have all the op codes we are interested in implementing.

The actual output of the reports is ugly: spec_log.txt. I will be looking for a generic tool to be able to show this off in a browser, especially with line-by-line errors being shown in files.

@george-cosma george-cosma force-pushed the feature/spec-tests branch 2 times, most recently from 369a259 to 28fefdd Compare August 23, 2024 13:05
Copy link

codecov bot commented Aug 23, 2024

Codecov Report

Attention: Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/execution/mod.rs 94.44% 1 Missing ⚠️
Files with missing lines Coverage Δ
src/execution/mod.rs 96.39% <94.44%> (+0.31%) ⬆️

... and 15 files with indirect coverage changes

@george-cosma george-cosma marked this pull request as ready for review September 2, 2024 17:37
@george-cosma
Copy link
Collaborator Author

I've spend some time to refactor this so that working on it in the future won't be a nightmare. Still not perfect, but good enough imho.

The magic question of how we want to integrate this into code coverage still remains. I've locked the testsuite behind a feature flag (because running it takes a bit of time) but it's not yet integrated into the coverage report procedures. @wucke13 I think we should collaborate on this issue, I'll need your help ^-^

There is still much left to be done. Part of it is we have to implement some base features (e.g. importing/exporting, the fuel mechanic for assert_trap, references, etc.), but assert_invalid and assert_malformed could be implemented with our current interpreter. Though, I'd like to get those done in a different PR.

One quick observation: there is a run.py script inside of the testsuite, but it seems very useless. All it does is to call the interpreter with the input filepath, and it also requires it to be able to do binary to wat and wat to binary conversions. This does not seem like something relevant for us, but I mentioned it regardless since it is technically part of the testsuite. Here is the link to it: https://github.com/WebAssembly/spec/blob/main/test/core/run.py

And one more final issue: representation of output. For now, the program prints a detailed report to stdout. Here's a snippet:

Report for ./tests/specification/dummy.wast:
------ ./tests/specification/dummy.wast ------
✅ ./tests/specification/dummy.wast:12 -> assert_return (invoke "add" (i32.const 1) (i32.const 1)) (i32.const 2))
✅ ./tests/specification/dummy.wast:13 -> assert_return (invoke "add" (i32.const 1) (i32.const 0)) (i32.const 1))
✅ ./tests/specification/dummy.wast:14 -> assert_return (invoke "add" (i32.const -1) (i32.const -1)) (i32.const -2))
✅ ./tests/specification/dummy.wast:15 -> assert_return (invoke "add" (i32.const -1) (i32.const 1)) (i32.const 0))
✅ ./tests/specification/dummy.wast:16 -> assert_return (invoke "add" (i32.const 0x7fffffff) (i32.const 1)) (i32.const 0x80000000))
✅ ./tests/specification/dummy.wast:17 -> assert_return (invoke "add" (i32.const 0x80000000) (i32.const -1)) (i32.const 0x7fffffff))
✅ ./tests/specification/dummy.wast:18 -> assert_return (invoke "add" (i32.const 0x80000000) (i32.const 0x80000000)) (i32.const 0))
✅ ./tests/specification/dummy.wast:19 -> assert_return (invoke "add" (i32.const 0x3fffffff) (i32.const 1)) (i32.const 0x40000000))
✅ ./tests/specification/dummy.wast:21 -> assert_return (invoke "mul" (i32.const 1) (i32.const 1)) (i32.const 1))
✅ ./tests/specification/dummy.wast:22 -> assert_return (invoke "mul" (i32.const 1) (i32.const 0)) (i32.const 0))

...

Execution finished. Passed: 79, Failed: 0, Total: 79
~~~~~~~~~~~~~~~~

...
❌ ./tests/specification/testsuite/f64.wast:305 -> assert_return (invoke "add" (f64.const inf) (f64.const -0x0.0000000000001p-1022)) (f64.const inf))
    Error: assert_eq failed: left: [F64(F64(1.8442240474082181e19))], right: [F64(F64(9.218868437227405e18))]
...

Report for ./tests/specification/testsuite/exports.wast:
------ ./tests/specification/testsuite/exports.wast ------
⚠ Compilation Failed ⚠
Context: main module validation failed
Error: An invalid instruction opcode was found: `0xf`
~~~~~~~~~~~~~~~~

...

Tests: 1 Passed, 20 Failed, 305 Compilation Errors

If we do find a tool that can help represent this type of information somewhat more visually, I am more than happy to mold this output into something that can be understood by said tool.

wucke13
wucke13 previously approved these changes Sep 11, 2024
Copy link
Collaborator

@wucke13 wucke13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

There are some things that I'm not 100% sure about, but nothing that would lead me to delay this any further to be merged!

@george-cosma george-cosma force-pushed the feature/spec-tests branch 2 times, most recently from fc23809 to 7354825 Compare September 12, 2024 11:54
@george-cosma
Copy link
Collaborator Author

Alright, I tackled most issues. The reason why the Requirement Preview deployment fails is because it is trying to make a push from outside this repo (from my george-cosma/wasm-interpreter repo). It should be fine when we merge.

@florianhartung
Copy link
Collaborator

I'm gonna go ahead and resolve the last conversation. This should be ready to merge then?

@george-cosma
Copy link
Collaborator Author

Yep, should be ready

@george-cosma george-cosma added this pull request to the merge queue Sep 24, 2024
Merged via the queue into DLR-FT:main with commit 6b0827f Sep 24, 2024
20 of 21 checks passed
@george-cosma george-cosma deleted the feature/spec-tests branch September 24, 2024 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants