Closed Bug 1332691 Opened 6 years ago Closed 6 years ago

wasm: Run shared test suite in Gecko


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




Tracking Status
firefox53 --- affected
firefox54 --- fixed


(Reporter: bbouvier, Assigned: bbouvier)




(8 files, 3 obsolete files)

Attached patch poc.patch (obsolete) — Splinter Review
      No description provided.
Priority: -- → P1
OK, after some recent discussions with jgraham and older ones with luke, my plan is the following:

- clone the wasm spec repo into the tests directory, and check that in, so we can modify tests easily. The .git directory is not checked in.
- add a script to build the entire test suite (generate JS tests from wast + generate WPT tests from JS)
- put the generated tests into the right places: JS tests go to /js/src/jit-tests/tests/wasm/spec/, and WPT go to a new /testing/web-platform/mozilla/tests/wasm directory
- the script should make an update of the manifest

A few convenient tooling things:
- jit-tests should inject a testharness.js shim before running the wasm/spec pure JS tests.
- provide a way to manually run the wasm WPT tests in CLI mode (not to be used in automation; automation will use the standard WPT way, so this is transparent).
- provide a way to create WPT expectations files (indicating whether a failure in gecko is known or not) and put them into the right directory.
Attached patch 1.utils.patch (obsolete) — Splinter Review
James, selecting you as a reviewer, but feel free to bounce to somebody else if you're not the right person for this review.

This patch contains a small utility script that allows one to run WPT tests from the JS dir, called

` --run /objdir/dist/bin/firefox` allow to run web platform tests; it's equivalent to `mach wpt`, but it defaults to the wasm WPT test cases if no other arguments are provided, and it generates an expectation file in a temporary directory, to be used below.

Note that wasm WPT tests will live under /testing/web-platform/mozilla/tests/, although at the moment there are none.

Since the wasm WPT test cases will be auto-generated from another repository, I've also added a simple way to update the manifest (equivalent of `mach wpt-manifest-update`, here done with ` --manifest`) and a simple way to update expectations files (` --expectations`), using the expectation file generated during a --run.

It's also possible to add all the same CLI arguments that each of the `mach` aliased commands would expect: all the arguments after -- will be passed to the subsequent command. For example, one can run the WPT cases in /testing/web-platform/tests/js/builtins WPT with the following:

./ --run objdir/dist/bin/firefox -- js/builtins

(which is effectively aliasing `mach wpt js/builtins`)

This script is intended to be run by humans, not in automation. Also, later patches in this bug will add other python script which may call functions from this one.

Any comments will be appreciated, thanks! (and sorry for the long post)
Attachment #8828907 - Attachment is obsolete: true
Attachment #8838161 - Flags: review?(james)
This just removes the current spec tests directory, as well as wast.js, the wast interpreter in JS that we've implemented.
Attachment #8838610 - Flags: review?(luke)
This patch adds a feature to the ability to include a file before the script is run, with a directive.

All the wasm test cases (or so) import the script libdir/wasm.js. With the directives.txt file that Lars introduced for testing the wasm baseline compiler, we can have an entire directory include this file by default, instead of explicitly doing it in every single file.
Attachment #8838612 - Flags: review?(sphink)
This polyfills the testharness.js APIs for use in Spidermonkey. These APIs are used in the generated JS tests (wast + pure JS), and this file will be automagically included for each generated spec test (thanks to the previous patch).
Attachment #8838614 - Flags: review?(luke)
Attached patch 5.makefile.patch (obsolete) — Splinter Review
This adds a Makefile in js/src/wasm/ for doing common tasks related to the shared test suite.

We initially talked about checking in the github WebAssembly/spec repository directly in Gecko. Eventually, I've thought that we can do even better: just ignore it (hence the directive in the toplevel .hgignore), and locally use a symbolic link to our own clone of the spec repo (alternatively, the Makefile will clone the spec repository itself).

I thought about putting this Makefile into js/src/jit-tests/tests/wasm first, but this would have been noisy: because we want the spec git repo around, all the JS files included within it would have been interpreted as tests by This is why I've put it into /js/src/wasm instead.

Principal commands are documented in the changeset message. This depends on patch 1 (for wpt/ and

Pulling tests into Gecko would be as simple as:
make update
BROWSER=/path/to/firefox/bin/firefox make expectations

Next commits will illustrate these two commands.
Attachment #8838617 - Flags: review?(luke)
Asking for rs? here; these are the JS tests generated from the spec repo, included in /js/src/jit-tests/tests/wasm/spec.
Attachment #8838618 - Flags: review?(luke)
This is the metadata for running wasm tests.

James, asking you for review also to give you notice that the WPT test cases for wasm are to be included under /testing/web-platform/mozilla/tests/wasm. Please let us know if that's OK that we (JS peers) manage this and the metadata directories ourselves.
Attachment #8838619 - Flags: review?(james)
Asking for rs? here too: these are the generated WPT test cases, with an update to the MANIFEST.json file that the WPT harness wants.

Patches 6 + 8 are the result of the `make update` command.
Attachment #8838620 - Flags: review?(luke)
Handle known failures:
- for the JS tests, we have to do this manually, by commenting out lines.
- for the WPT tests, `make expectations` generate meta files describing which tests are known to fail, and then it's just a matter of letting hg know about these files.
Attachment #8838623 - Flags: review?(luke)
Attachment #8838161 - Attachment is obsolete: true
Attachment #8838161 - Flags: review?(james)
Attached patch 5.makefile.patchSplinter Review
Actually, since we have a Makefile that calls external commands, why wouldn't it just call the mach command for us, with the right parameters? Let's do this instead, and remove the wpt/ script entirely, which was really just a proxy anyway. Less code FTW.
Attachment #8838617 - Attachment is obsolete: true
Attachment #8838617 - Flags: review?(luke)
Attachment #8838666 - Flags: review?(luke)
Depends on: 1333490
Depends on: 1338002
Comment on attachment 8838612 [details] [diff] [review]

Review of attachment 8838612 [details] [diff] [review]:

Hm. I'm ok with this, but it's another reminder that we have two redundant test harnesses. With, this would just be placing wasm.js into the test directory, using the name 'shell.js'. I don't know enough about the two harnesses to understand the relative advantages/disadvantages of each, but if one of the advantages of jit-tests is that you can just run the .js file directly, then this breaks that for the wasm subdirectory.

::: js/src/tests/lib/
@@ +311,5 @@
>          # We may have specified '-a' or '-d' twice: once via --jitflags, once
>          # via the "|jit-test|" line.  Remove dups because they are toggles.
>          cmd = prefix + ['--js-cache', JitTest.CacheDir]
>          cmd += list(set(self.jitflags)) + ['-e', expr]
> +        cmd += [y for x in self.other_includes for y in ['-f', libdir + x]]

personally, I'd go for

  for inc in self.other_includes:
    cmd += ['-f', libdir + inc]
Attachment #8838612 - Flags: review?(sphink) → review+
Attachment #8838619 - Flags: review?(james) → review+
Attachment #8838610 - Flags: review?(luke) → review+
Attachment #8838614 - Flags: review?(luke) → review+
Attachment #8838618 - Flags: review?(luke) → review+
Attachment #8838620 - Flags: review?(luke) → review+
Comment on attachment 8838623 [details] [diff] [review]

Review of attachment 8838623 [details] [diff] [review]:

IIUC, this patch disappears with the fixes in the blocking bug 1333490?
Comment on attachment 8838666 [details] [diff] [review]

Review of attachment 8838666 [details] [diff] [review]:

Could you copy explanation in the commit summary into Makefile comments?
Attachment #8838666 - Flags: review?(luke) → review+
Attachment #8838623 - Flags: review?(luke) → review+
Depends on: 1341090
Depends on: 1342045
This try build looks much better:

The only error still happening is the max memory bug, which is handled in bug 1342045.

Known failures after this:
- a conversion error, which is bug 1336139, and as it might take more time, I'd be willing to disable the test for some time.
- `new Uint8Array('hi')` makes us fail before we enter a test. So it's breaking at the WPT harness level, for jsapi.js.
Pushed by
Remove wasm/spec tests; r=luke
Allow including files with a jit-test directive; r=sfink
Make a testharness.js polyfill for Spidermonkey; r=luke
Add script to regenerate tests, update the WPT manifest and expectations; r=luke
Generate wasm JavaScript tests from the shared test suite; r=luke
Add the metadata directory for the wasm WPT directory; r=jgraham
Generate wasm Web Platform test cases; r=luke
Handle known failures in wasm WPT and JS test cases; r=luke
Pushed by
Skip conversions.wast.js.html for unexpected successes on x86; r=bustage
Pushed by
Disable wasm WPT nop/resizing test cases; r=bustage
Depends on: 1342956
You need to log in before you can comment on or make changes to this bug.