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

[Parser] Parse wast scripts #6581

Merged
merged 2 commits into from
May 13, 2024
Merged

[Parser] Parse wast scripts #6581

merged 2 commits into from
May 13, 2024

Conversation

tlively
Copy link
Member

@tlively tlively commented May 10, 2024

The spec tests use an extension of the standard text format that includes
various commands and assertions used to test WebAssembly implementations. Add a
utility to parse this extended WebAssembly script format and use it in
wasm-shell to check that it parses our spec tests without error. Fix a few
errors the new parser found in our spec tests.

A future PR will rewrite wasm-shell to interpret the results of the new parser,
but for now to keep the diff smaller, do not do anything with the new parser
except check for errors.

The spec tests use an extension of the standard text format that includes
various commands and assertions used to test WebAssembly implementations. Add a
utility to parse this extended WebAssembly script format and use it in
wasm-shell to check that it parses our spec tests without error. Fix a few
errors the new parser found in our spec tests.

A future PR will rewrite wasm-shell to interpret the results of the new parser,
but for now to keep the diff smaller, do not do anything with the new parser
except check for errors.
@tlively tlively requested a review from kripken May 10, 2024 20:30
Copy link
Member Author

tlively commented May 10, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @tlively and the rest of your teammates on Graphite Graphite

;; (assert_return (invoke "tuple-global-get") (tuple.make 2 (i32.const 0) (i64.const 0)))
;; (assert_return (invoke "tuple-global-set"))
;; (assert_return (invoke "tuple-global-get") (tuple.make 2 (i32.const 42) (i64.const 7)))
;; (assert_return (invoke "tail-call") (tuple.make 2 (i32.const 42) (i64.const 7)))
Copy link
Member

Choose a reason for hiding this comment

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

Are these not valid? (If so why not remove them?)

Copy link
Member Author

Choose a reason for hiding this comment

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

The wast parser does not parse tuple.make.

Copy link
Member

Choose a reason for hiding this comment

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

Naively I'd expect the wast parser to parse a superset of the wat parser's input, which I guess is related to the other question about sharing logic between them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since the goal is to run the upstream spec tests, it shouldn't be necessary to parse tuple.make to test multivalue. Rather than doing a bunch of work to support parsing tuple.make constants anyway, it seemed better to comment this out for now. I didn't delete them entirely because when we switch to the upstream spec tests, we will want to check that we regain this coverage.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. I'd have hoped to see tuple.make get parsed properly by reusing some wat parser logic, but as that's not the goal here it doesn't matter.

return in.err("expected end of v128.const");
}
return vec;
}
Copy link
Member

Choose a reason for hiding this comment

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

There seems to be duplication here of the core parser logic for these constants etc. - can that be shared somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

I initially implemented this by calling parseExpression, then translating the parsed constant Expression* into a literal. I forget why reimplementing the parsing ended up being easier, so I'll try that again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, this reuses the existing parser now. PTAL.

;; (assert_return (invoke "tuple-global-get") (tuple.make 2 (i32.const 0) (i64.const 0)))
;; (assert_return (invoke "tuple-global-set"))
;; (assert_return (invoke "tuple-global-get") (tuple.make 2 (i32.const 42) (i64.const 7)))
;; (assert_return (invoke "tail-call") (tuple.make 2 (i32.const 42) (i64.const 7)))
Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. I'd have hoped to see tuple.make get parsed properly by reusing some wat parser logic, but as that's not the goal here it doesn't matter.

@tlively tlively merged commit 924533f into main May 13, 2024
13 checks passed
@tlively tlively deleted the parser-wast branch May 13, 2024 21:18
@gkdn gkdn mentioned this pull request Aug 31, 2024
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.

2 participants