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

Upstream initial test suite from wasm-tools #85

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

abrown
Copy link
Collaborator

@abrown abrown commented Oct 15, 2024

This change starts this proposal's test suite by adding encoding, decoding, and validation tests for the new instructions. This is a wholesale copy of the current state of the wasm-tools tests. Some things to note:

  • some of the tests were generated mechanically to attempt to "cover all possibilities;" not all proposals do this, though, so it's unclear whether we want to here (it's verbose, but hopefully more complete)
  • these tests include the component model built-ins, which are not executable in many engines; it's unclear whether to keep that file (the proposal speaks of this) or not
  • these tests are a start, not the end state: we're missing anything to do with "thread locals," pause, and possibly some other bits.

This change starts this proposal's test suite by adding encoding,
decoding, and validation tests for the new instructions. This is a
wholesale copy of the current state of the `wasm-tools` [tests]. Some
things to note:
- some of the tests were generated mechanically to attempt to "cover all
  possibilities;" not all proposals do this, though, so it's unclear
  whether we want to here (it's verbose, but hopefully more complete)
- these tests include the component model built-ins, which are not
  executable in many engines; it's unclear whether to keep that file
  (the proposal speaks of this) or not
- these tests are a start, not the end state: we're missing anything to
  do with "thread locals," `pause`, and possibly some other bits.

[tests]: https://github.com/bytecodealliance/wasm-tools/tree/5a83828/tests/local/shared-everything-threads
@abrown
Copy link
Collaborator Author

abrown commented Oct 15, 2024

@rossberg, looks like we don't even have CI turned on here...

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

I do think it makes sense to inlude mechanically generated tests to improve coverage, and we might as well include the component model tests in case they are useful to more folks. We can always remove things later once we have an interpreter implementation and CI running.

(table shared (ref null (shared func)) (elem (ref.null (shared func))))
)

;; Note that shared elements can live within an unshared table.
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to test that the reverse is invalid. Also, we can test the interaction with shared and unshared globals used in the table initializers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would be good to test that the reverse is invalid.

We do; they're just a few lines down. I'll move those closer to this assertion.

Also, we can test the interaction with shared and unshared globals used in the table initializers.

Were you thinking of variations on this kind of thing?

(module
  (type $f (shared (func)))
  (global $g (shared (ref null $f)) (ref.null $f))
  (table $t shared 1 (ref null $f) (ref.null $f))
  (elem (global.get $g))
)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, although that test could be simplified by removing the table entirely and just keeping the element segment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Really? Both wasm-tools and wabt seem to expect an elem to have a preceding table definition or we get an error. E.g., (module (elem (i32.const 0) funcref)) is invalid in wat2wasm v1.0.33.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

...but see 740c560 (a lot more notation is necessary than I might have expected to convince wasm-tools it is valid).

Copy link
Member

Choose a reason for hiding this comment

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

Really? Both wasm-tools and wabt seem to expect an elem to have a preceding table definition or we get an error. E.g., (module (elem (i32.const 0) funcref)) is invalid in wat2wasm v1.0.33.

That module is invalid because it defines an active segment, which needs a table to be initialized into. Get rid of the (i32.const 0) offset and it should validate.

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

Successfully merging this pull request may close these issues.

3 participants