wabt icon indicating copy to clipboard operation
wabt copied to clipboard

Provide backward compatible support for assert_return_arithmetic_nan

Open binji opened this issue 5 years ago • 5 comments
trafficstars

See #1275 where these were removed. The upstream spec was changed to use a more generic syntax:

(assert_return_canonical_nan (invoke "..."))

becomes

(assert_return (invoke "...") (nan:canonical))

(And similarly for assert_return_arithmetic_nan and nan:arithmetic).

But in general, wabt tries to be backward compatible, so it could continue to support this old syntax too.

See https://github.com/WebAssembly/bulk-memory-operations/issues/144.

binji avatar Mar 23 '20 19:03 binji

I don't think we should do this. I think wabt should support all the testsuite features that are present in the amalgamated testsuite repo (https://github.com/WebAssembly/testsuite) and no others.

If we do decide we need this behaviour then I think the way to achieve it would be to first include all tests from all proposals in the amalgamated repo rather then limiting the mirroring to only tests with upstream changes. i.e. we should let the testsuite repo drive out testing/coverage.

sbc100 avatar Mar 23 '20 19:03 sbc100

Fair point. My main concern is that there are existing .wast files that we may want to continue to support, especially those that have been copied from the spec testsuite at an earlier date, which seems to be common. I don't know that we want to say everything should work, but it would be nice to avoid breaking code if it's not too much work (which I think it wouldn't be, in this case).

binji avatar Mar 23 '20 19:03 binji

I don't have a strong opinion on this but the reason I suggested keeping backwards compatible behavior was that my spectests suddenly stopped working when i upgraded the version of wabt that comes with my distro, and it was an annoying experience.

rianhunter avatar Mar 23 '20 19:03 rianhunter

Can the wasm 1.0 tests from WebAssembly/spec, branch wg_v1 be updated to the need syntax? It looks there are only 4 cases of the old syntax:

find ../wasm-spec/test/core -name '*.wast' -exec wast2json {} \;                                                                                                                   ✔  14:13:49  
../wasm-spec/test/core/float_misc.wast:572:1: error: unexpected token (, expected EOF.
(assert_return_canonical_nan (invoke "f64.sqrt" (f64.const -0x1.360e8d0032adp...
^
../wasm-spec/test/core/f64.wast:51:1: error: unexpected token (, expected EOF.
(assert_return_canonical_nan (invoke "add" (f64.const -0x0p+0) (f64.const -na...
^
../wasm-spec/test/core/f32.wast:51:1: error: unexpected token (, expected EOF.
(assert_return_canonical_nan (invoke "add" (f32.const -0x0p+0) (f32.const -na...
^
../wasm-spec/test/core/float_exprs.wast:49:1: error: unexpected token (, expected EOF.
(assert_return_arithmetic_nan (invoke "f32.no_fold_add_zero" (f32.const nan:0...
^
../wasm-spec/test/core/conversions.wast:357:1: error: unexpected token (, expected EOF.
(assert_return_canonical_nan (invoke "f64.promote_f32" (f32.const nan)))
^

chfast avatar Aug 07 '20 12:08 chfast

Upgrading the branch seems plausible, but we'd need to make sure that there are no other clients of the 1.0 test suite that we'd break with that.

rossberg avatar Aug 10 '20 12:08 rossberg

Closing as stale -- it doesn't appear that a lot of people need WABT to support this syntax (even if it was in the spec testsuite at the time of 1.0?).

keithw avatar Mar 15 '23 08:03 keithw