Conversation
| if (str[7] == 'g') return makeHost(s, HostOp::MemoryGrow); | ||
| abort_on(str); | ||
| } | ||
|
|
There was a problem hiding this comment.
Most of the changes so far are naive find-and-replaces with case sensitivity. I've fixed a few spots where I found that it made things incorrect. e.g. memory.size() vs memory_size() but there might be others.
This was one such spot, where I needed to change the parser itself because it assumed that anything with a dot was i32|i64|f32|f64 operations.
There was a problem hiding this comment.
Makes sense, yeah, that's what I was thinking before I saw this comment.
In general I'd say if the test suite passes we should be good - we have good coverage of this stuff.
|
Should we keep support for the old instruction names in wat/wast parsers? FWIW wabt did |
|
Personally I don't think it's important for us to support the old names - I think wabt is what most people use for reading wat files, so makes more sense there, while here our text parsing isn't spec-compliant for other reasons anyhow. |
|
About the spec tests, as mentioned in the other PR, I'd be fine to replace the git submodule with just a directory of the files in their current state, then modify them for changes like this. (For the same reason as not supporting the old notation - our text format kind of diverged anyhow; may make sense one day to harmonize them, but really we are focused on the binary format here, and just use spec tests for test purposes.) |
|
@kripken cool. I had started to go down the road of finding and fixing them but I just keep finding more divergences. What I'm gonna do is comment out the ones that binaryen does not support, with a comment--sound good? |
|
btw I noticed one example in the spec tests where I think mixing AST/stack syntaxes can be ambiguous. This may not be news to everyone else, but I thought it was interesting. Binaryen parses the first one fine, but it fails validation because in AST form it's an empty return, whereas in parsers that support mixed AST/stack form like wabt, the block's value is used as the return operand but if and only if the function has a result signature expecting one, otherwise it's treated as an empty return ignoring the block value. 🤯If I'm wrong, lmk. |
Yeah, sounds good. And yeah, you're right about that example - it's pretty weird I think that in wat format you can write |
|
@kripken Made more progress but I can't seem to figure out this latest error. It's not clear where these fail functions come from or why this particular test is failing. I manually ran it and I believe it's returning Curious if something immediately jumps out at you? (See Travis or below) function $14(i) { // 16u_good5
i = i | 0;
return HEAPU16[(i + 25 | 0) >> 1] | 0 | 0;
}
// ... etc ...
function check15() {
return (retasmFunc.$16u_good5(0 | 0) | 0 | 0) == (122 | 0) | 0;
}
if (!check15()) fail15();(func (export "16u_good5") (param $i i32) (result i64)
(i64.load16_u offset=25 align=2 (get_local $i)) ;; 122 'z\0'
)
;; ... etc ...
(assert_return (invoke "16u_good5" (i32.const 0)) (i32.const 122)) |
|
Do you see the failure locally too? In the past I saw an odd failure in those tests that was fixed by upgrading node. But likely unrelated. It might be best to do a separate PR for "flattening" the spec tests, starting with changing nothing but removing the submodule. Then the PR for renaming memory operations would be a lot smaller (just the actual renaming + a few spec test changes) and we could be sure it isn't something that went wrong with the spec test changes. |
|
Happens locally too. Saw that other ticket mentioning node 10. I tried various versions of node, including latest. I’ll make a separate PR for the submodule—it’s already in a separate commit so it’s easy 🤙 |
2 participants
NOT READY FOR REVIEW
To align with the latest spec we need to rename the memory instructions
grow_memory->memory.growcurrent_memory->memory.sizeThis PR is a very early WIP towards that goal, however as changes to the upstream spec testsuite binaryen uses has changed in other ways as well this PR will likely need to contain other fixes to accommodate. I'll list them as I find them, if any. Numerous spec tests now mix stack/sexprs which binaryen doesn't support so I'm attempting to modify them upstream to be 100% sexprs.
This PR also will eventually unblock #1644 which is also waiting on a spec testsuite update