wasm: Run shared test suite in Gecko

RESOLVED FIXED in Firefox 54

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 affected, firefox54 fixed)

Details

Attachments

(8 attachments, 3 obsolete attachments)

Assignee

Description

2 years ago
Posted patch poc.patch (obsolete) — Splinter Review
No description provided.
Assignee

Updated

2 years ago
Priority: -- → P1
Assignee

Comment 1

2 years ago
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.
Assignee

Comment 2

2 years ago
Posted 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.py.

`run.py --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 `run.py --manifest`) and a simple way to update expectations files (`run.py --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.py --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)
Assignee

Comment 3

2 years ago
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)
Assignee

Comment 4

2 years ago
This patch adds a feature to jit_tests.py: 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)
Assignee

Comment 5

2 years ago
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)
Assignee

Comment 6

2 years ago
Posted 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 jit_tests.py. 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/run.py) and https://github.com/WebAssembly/spec/pull/419.

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)
Assignee

Comment 7

2 years ago
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)
Assignee

Comment 8

2 years ago
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)
Assignee

Comment 9

2 years ago
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)
Assignee

Comment 10

2 years ago
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)
Assignee

Updated

2 years ago
Attachment #8838161 - Attachment is obsolete: true
Attachment #8838161 - Flags: review?(james)
Assignee

Comment 12

2 years ago
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/run.py 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)
Assignee

Updated

2 years ago
Depends on: 1333490
Assignee

Updated

2 years ago
Depends on: 1338002
Comment on attachment 8838612 [details] [diff] [review]
3.include-directive.patch

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 jstests.py, 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/jittests.py
@@ +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+

Updated

2 years ago
Attachment #8838610 - Flags: review?(luke) → review+

Updated

2 years ago
Attachment #8838614 - Flags: review?(luke) → review+

Updated

2 years ago
Attachment #8838618 - Flags: review?(luke) → review+

Updated

2 years ago
Attachment #8838620 - Flags: review?(luke) → review+
Comment on attachment 8838623 [details] [diff] [review]
9.known-failures.patch

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]
5.makefile.patch

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

Could you copy explanation in the commit summary into Makefile comments?
Attachment #8838666 - Flags: review?(luke) → review+

Updated

2 years ago
Attachment #8838623 - Flags: review?(luke) → review+
Assignee

Updated

2 years ago
Depends on: 1341090
Assignee

Updated

2 years ago
Depends on: 1342045
Assignee

Comment 16

2 years ago
This try build looks much better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f33366b8184a82f5dcd8383ae7b3e8a230711e4b

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.

Comment 17

2 years ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c9427dde77e
Remove wasm/spec tests; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/07b697bc921f
Allow including files with a jit-test directive; r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/f98c8a722582
Make a testharness.js polyfill for Spidermonkey; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/a75ac6014699
Add script to regenerate tests, update the WPT manifest and expectations; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/f675a1c2d70e
Generate wasm JavaScript tests from the shared test suite; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/69b7861e868c
Add the metadata directory for the wasm WPT directory; r=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/82c003b7106a
Generate wasm Web Platform test cases; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5a8190c6f7b
Handle known failures in wasm WPT and JS test cases; r=luke

Comment 18

2 years ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0028f7c1c00
Skip conversions.wast.js.html for unexpected successes on x86; r=bustage

Comment 19

2 years ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8e97754bd0f
Disable wasm WPT nop/resizing test cases; r=bustage
Assignee

Updated

2 years ago
Depends on: 1342956
You need to log in before you can comment on or make changes to this bug.