1452588 - Import WebAssembly reference tests for import/export of mutable globals

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P1)

Description

[Lars T Hansen [:lth]]

We need to be running the reference tests in SpiderMonkey before the proposal can advance in the Wasm CG.

Comment 1

[Lars T Hansen [:lth]]

Blocks on this PR on the proposal repo: https://github.com/WebAssembly/mutable-global/pull/7

Comment 2

[Lars T Hansen [:lth]]

Blocks on this issue on the proposal repo: https://github.com/WebAssembly/mutable-global/issues/10

Comment 3

[Lars T Hansen [:lth]]

The bug in comment 2 was fixed; the bug in comment 1 is controversial.

Comment 4

[Julian Seward [:jseward]]

Attachment: bug1452588-WAG-import-spec-tests-1.diff

Requesting comment.

Whilst writing the commit message, I realised that it was probably a mistake
to have simply overwritten js/src/jit-test/tests/wasm/spec/jsapi.js with the
imported version.  Instead I should have left that unchanged and put the
imported jsapi.js in the new proposal_mutable_global/, so as to maintain the
invariant that all of the tests that require mutable-global support actually
live in proposal_mutable_global/.  So I'm requesting comment on the above
observation, too.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8980330 [details] [diff] [review]
bug1452588-WAG-import-spec-tests-1.diff

Review of attachment 8980330 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. I think that if we decide that it's time for WebAssembly.Global to be promoted and shipped, then we could land this directly in the main directory and not under the proposal sub repository. I would rather do this like that, rather than having two different copies of the jsapi.wast.js file in two directories, that would get merged back later.

(I didn't look at the test cases, which I assume come from the spec repository.)

::: js/src/jit-test/lib/wasm-testharness.js
@@ +12,5 @@
> +//
> +// That is, it has a fixed offset relative to what we need.  So we can
> +// simply do this:
> +
> +let specdir = libdir + "../tests/wasm/spec/";

nit: can even concatenate "harness/" to this, and make the load() statements simpler below.

@@ +60,5 @@
>      throw new Error(`unreachable:\n${description}`);
>  }
> +
> +// For defining assert_not_equals, unfortunately there doesn't seem to be an
> +// opposite-sense version of assertEq we can use.

let caught = false; try { assertEq(actual, expected); } catch (e) { caught = true; } assertEq(caught, true, description);

Either this, or you can add an assertNotEq in js/src/tests/shell.js, using SameValue that's implemented there.

(For what it's worth, your impl is correct and matches https://github.com/w3c/web-platform-tests/blob/master/resources/testharness.js#L981, which this file is imitating)

Comment 6

[Julian Seward [:jseward]]

Attachment: bug1452588-WAG-import-spec-tests-2.diff

With very minor changes per review comments in comment 5.

> nit: can even concatenate "harness/" to this

Done.

> let caught = false; try { assertEq(actual, expected); } 

I used a modified version of this, in the end, because I couldn't
get a version that defines assertNotEq to work.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8980561 [details] [diff] [review]
bug1452588-WAG-import-spec-tests-2.diff

Review of attachment 8980561 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks good to me.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/775566443136
Import WebAssembly reference tests for import/export of mutable globals.  r=bbouvier.

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/775566443136