Wasm: Ship new sign extension opcodes

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: lth, Assigned: lth)

Tracking

unspecified
mozilla62
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments)

These opcodes are nightly-only under ENABLE_WASM_SIGNEXTEND_OPS.  We are blocked on the spec making its way through the stages in the Wasm CG / WG process.

(Currently bundled with the threads spec but that will change: https://github.com/WebAssembly/threads; these are the five opcodes matching i{32,64}.extend{8,16,32}_s, where the first number is larger than the second.)
The spec is now here: https://github.com/WebAssembly/sign-extension-ops

Work items:

- import the spec test cases (in one form or another) from the proposal
  - ideally in some form that does not depend on our wasmTextToBinary()

- remove the ifdeffery that hides the feature /or/ enable the ifdef unconditionally
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Also, once we switch this on we should remove all the wasmSignExtensionSupported() guards in all test cases.
This imports the sign extension tests from the proposal, suitably translated.  We pass all of them.

However, there's a complication:  The proposal embeds these tests in i32.wast and i64.wast.  But it seems we have not imported the spec tests in a long time, and it does not seem appropriate to do that as part of this work item.  (Happy to be proven wrong on that but that's my reading of the logs.)

So I have factored the tests for this proposal into their own files, extend32.wast and extend64.wast, and I include those here in the new src/ subdirectory; the translations emitted by the spec interpreter, prefixed by the appropriate guard, are then placed in wasm/spec with the other test cases.

IMO:
- when we import the spec tests we should also keep the sources, because the
  sources keep changing in the spec repo and there is no longer a correspondence
  between our spec tests and the available sources
- we need to discuss a more regular cadence for spec test import
- we might try to encourage test cases for in-development features not to be
  placed in existing test files while the features are being developed, to
  simplify focussed testing.
Attachment #8960898 - Flags: review?(luke)
So... two more questions about shipping this feature:

- does it require an intent-to-ship or is it too small for that?
- does it need https-only gating or is it too small, or are we not resolved on how we do that yet?
Flags: needinfo?(luke)
Flags: needinfo?(annevk)
It's not entirely clear to me how big this change is, but in general I think it's good practice to note web-observable changes on dev-platform. If we can gate it on secure contexts that'd be good, but it's a trade-off that needs to take into account complexity as well as other shipping implementations.

As for tests, any reason WebAssembly cannot use the web-platform-tests infrastructure too? We have synchronization in place for that.
Flags: needinfo?(annevk)
(In reply to Anne (:annevk) from comment #5)
> It's not entirely clear to me how big this change is, but in general I think
> it's good practice to note web-observable changes on dev-platform. If we can
> gate it on secure contexts that'd be good, but it's a trade-off that needs
> to take into account complexity as well as other shipping implementations.

OK, will await feedback from Luke.

> As for tests, any reason WebAssembly cannot use the web-platform-tests
> infrastructure too? We have synchronization in place for that.

As for our own tests: All the jit-tests are development tests and we run them in the JS shell.  It's a major overhead if we have to build Firefox to do any kind of testing, and the debugging experience in the presence of problems would be horrific, in comparison with what we can do now.

As for the Wasm reference tests: don't know.  Since the JS tests are generated from wasm tests it is probably doable to generate WPT compatible tests.
(In reply to Lars T Hansen [:lth] from comment #3)
> - when we import the spec tests we should also keep the sources, because the
>   sources keep changing in the spec repo and there is no longer a
>   correspondence between our spec tests and the available sources
> - we need to discuss a more regular cadence for spec test import
> - we might try to encourage test cases for in-development features not to be
>   placed in existing test files while the features are being developed, to
>   simplify focussed testing.

Agreed on all points.  For a while we were updating regularly, and then we stopped when we reached the more open-question of wpt integration and what the happy goal state was.  More on this to Anne's point below.

(In reply to Anne (:annevk) from comment #5)
> It's not entirely clear to me how big this change is, but in general I think
> it's good practice to note web-observable changes on dev-platform. If we can
> gate it on secure contexts that'd be good, but it's a trade-off that needs
> to take into account complexity as well as other shipping implementations.
> 
> As for tests, any reason WebAssembly cannot use the web-platform-tests
> infrastructure too? We have synchronization in place for that.

We are moving toward doing this and Dan Ehrenberg is planned to add a bunch more wasm wpts (after pressing JS/Web API tasks slown down).  We also (currently, although I don't think they are updated live from the core spec repo) run these shell .wast tests in a wpt browser harness (to provide *additional* testing that the browser doesn't regress something the shell passes).  The question/hope I had earlier was that we:
 1. set up automatic two-way sync between the https://github.com/webassembly/spec 'test' dir and wpt
 2. point our SM shell testing harness to refer to the two-way-synced wasm wpt dir in mozilla-central (instead of having a local copy)
 3. thereby automatically pull in all new .wast tests via two-way sync, while keeping our ability to test via shell

This is still my goal but it's blocked on a few things.  In the interim, it does make sense to recommence manually updates from the core test subdir.
Flags: needinfo?(luke)
Oh, for the IsSecureContext question: this feature is really tiny and its development almost entirely predates https://blog.mozilla.org/security/2018/01/15/secure-contexts-everywhere/, so I'd be inclined to let this one slip through.
Oh sorry, to the last q: this also seems quite below the intent-to-ship threshold.  E.g., JS doesn't send an intent-to-ship for each tiny language extension; just the big things.
Fair enough.  I raised the secure-context question on the "ship this" issue on github, I'll update that.
Attachment #8960898 - Flags: review?(luke) → review+
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df9c07695a25
Add spec tests for wasm sign extension ops. r=luke
Unless something happens with Secure Contexts this is the patch to enable the feature.  Might as well review now, but won't land until the CG has voted in favor of releasing.
Attachment #8961295 - Flags: review?(luke)
Comment on attachment 8961295 [details] [diff] [review]
bug1429818-ship-wasm-sign-extension.patch

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

Nicely done (with the overall process of gating and releasing this feature).
Attachment #8961295 - Flags: review?(luke) → review+
Feature remains in the implementation phase in the Wasm CG.
Feature has moved to standardization phase in the Wasm CG.
Component: JavaScript Engine: JIT → Javascript: Web Assembly
Keywords: leave-open
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df11dced021e
Make wasm sign extension feature unconditional. r=luke
https://hg.mozilla.org/mozilla-central/rev/df11dced021e
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1466931
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.