Rewrite wasm-shell to use new wast parser #6601
Conversation
Use the new wast parser to parse a full script up front, then traverse the parsed script data structure and execute the commands. wasm-shell had previously used the new wat parser for top-level modules, but it now uses the new parser for module assertions as well. Fix various bugs this uncovered. After this change, wasm-shell supports all the assertions used in the upstream spec tests (although not new kinds of assertions introduced in any proposals). Uncomment various `assert_exhaustion` tests that we can now execute. Other kinds of assertions remain commented out in our tests: wasm-shell now supports `assert_unlinkable`, but the interpreter does not eagerly check for the existence of imports, so those tests do not pass. Tests that check for NaNs also remain commented out because they do not yet use the standard syntax that wasm-shell now supports for canonical and arithmetic NaN results, and our interpreter would not pass all of those tests even if they did use the standard syntax.
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
@aheejin, as far as I can tell by looking at the documentation for old exceptions, the new parser is correct in accepting the modules in the changed tests here. In both cases, the delegate ends up targeting the function scope in the IR. Does that seem right to you?
There was a problem hiding this comment.
I think it is correct. I wonder why was it being marked as assert_invalid passing, given that this PR does not change the logic related to delegate?
Also delegate can target any blocks per spec, but Binaryen didn't support it internally. Binaryen supported the special "function scope" target though.
There was a problem hiding this comment.
Until this change, modules inside assert_invalid assertions were still being parsed with the old parser. Only top-level modules were being parsed with the new parser.
There was a problem hiding this comment.
Yeah but the code should've been valid with the old parser as well, no? Because we handled the function level delegate correctly before.
There was a problem hiding this comment.
But syntactically these delegates do not directly target the top level. It's only by searching for the next valid target scope that the parser ends up making them target the top level. The old parser did not do that search.
| @@ -391,24 +389,30 @@ Result<WASTCommand> command(Lexer& in) { | |||
| return *mod; | |||
| } | |||
|
|
|||
| #pragma GCC diagnostic push | |||
| #pragma GCC diagnostic ignored "-Wmaybe-uninitialized" | |||
There was a problem hiding this comment.
Hmm, this keeps coming up... 😄 Oh well, this seems like the best solution, we don't want to disable this globally.
| @@ -4103,7 +4103,7 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> { | |||
| // can use it (refactor?) | |||
| Literals callFunctionInternal(Name name, Literals arguments) { | |||
| if (callDepth > maxDepth) { | |||
| externalInterface->trap("stack limit"); | |||
| hostLimit("stack limit"); | |||
There was a problem hiding this comment.
Now that we support assert_exhaustion, we actually have test coverage for this!
| std::cerr << i << ' '; | ||
| Colors::normal(std::cerr); |
There was a problem hiding this comment.
| std::cerr << i << ' '; | |
| Colors::normal(std::cerr); | |
| std::cerr << i++ << ' '; |
Nit
| } | ||
| ++i; |
|
Oops, sorry for jumping the gun, @aheejin! I'd be happy to address any other comments you have in follow-ups as well. |
Use the new wast parser to parse a full script up front, then traverse the
parsed script data structure and execute the commands. wasm-shell had previously
used the new wat parser for top-level modules, but it now uses the new parser
for module assertions as well. Fix various bugs this uncovered.
After this change, wasm-shell supports all the assertions used in the upstream
spec tests (although not new kinds of assertions introduced in any proposals).
Uncomment various
assert_exhaustiontests that we can now execute.Other kinds of assertions remain commented out in our tests: wasm-shell now
supports
assert_unlinkable, but the interpreter does not eagerly check for theexistence of imports, so those tests do not pass. Tests that check for NaNs also
remain commented out because they do not yet use the standard syntax that
wasm-shell now supports for canonical and arithmetic NaN results, and our
interpreter would not pass all of those tests even if they did use the standard
syntax.