Skip to content

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
@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.

3 participants