Bug 170740 - WebAssembly: import GCC torture tests
Summary: WebAssembly: import GCC torture tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords:
Depends on:
Blocks: 159775 170970
  Show dependency treegraph
 
Reported: 2017-04-11 10:59 PDT by JF Bastien
Modified: 2017-04-19 11:02 PDT (History)
8 users (show)

See Also:


Attachments
patch (9.22 KB, patch)
2017-04-18 15:54 PDT, JF Bastien
no flags Details | Formatted Diff | Diff
patch (9.22 KB, patch)
2017-04-18 15:58 PDT, JF Bastien
no flags Details | Formatted Diff | Diff
patch (9.24 KB, patch)
2017-04-18 16:15 PDT, JF Bastien
sbarati: review+
sbarati: commit-queue-
Details | Formatted Diff | Diff
patch (9.26 KB, patch)
2017-04-19 09:50 PDT, JF Bastien
commit-queue: commit-queue-
Details | Formatted Diff | Diff
patch (9.26 KB, patch)
2017-04-19 09:56 PDT, JF Bastien
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2017-04-11 10:59:24 PDT
As in http://wasm-stat.us
Comment 1 JF Bastien 2017-04-18 15:54:43 PDT
Created attachment 307429 [details]
patch
Comment 2 Build Bot 2017-04-18 15:56:39 PDT
Attachment 307429 [details] did not pass style-queue:


ERROR: Tools/Scripts/update-wasm-gcc-torture.py:10:  trailing whitespace  [pep8/W291] [5]
ERROR: Tools/Scripts/update-wasm-gcc-torture.py:13:  trailing whitespace  [pep8/W291] [5]
ERROR: Tools/Scripts/update-wasm-gcc-torture.py:148:  indentation is not a multiple of four  [pep8/E111] [5]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 JF Bastien 2017-04-18 15:58:46 PDT
Created attachment 307430 [details]
patch

Fix style.
Comment 4 JF Bastien 2017-04-18 16:15:38 PDT
Created attachment 307432 [details]
patch

Rebase.
Comment 5 Mark Lam 2017-04-18 16:21:30 PDT
Comment on attachment 307432 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307432&action=review

> Tools/Scripts/run-jsc-stress-tests:1212
> +    prepareExtraRelativeFiles([Pathname('..') + wasm], $collection)

Should this be "[(Pathname('..') + wasm).to_s]" instead of "[Pathname('..') + wasm]"?
Comment 6 JF Bastien 2017-04-18 16:47:15 PDT
(In reply to Mark Lam from comment #5)
> Comment on attachment 307432 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307432&action=review
> 
> > Tools/Scripts/run-jsc-stress-tests:1212
> > +    prepareExtraRelativeFiles([Pathname('..') + wasm], $collection)
> 
> Should this be "[(Pathname('..') + wasm).to_s]" instead of "[Pathname('..')
> + wasm]"?

What I have now works because, IIUC, `Pathname + String => Pathname` and prepareExtraRelativeFiles expects at least the extra files or the destination to be a Pathname. As long as one of them is a Pathname we're fine: it concatenates both!
Comment 7 JF Bastien 2017-04-18 22:34:47 PDT
Emscripten was being noisy in its print error log. This fix should help:
  https://github.com/kripken/emscripten/pull/5144
Comment 8 Saam Barati 2017-04-19 09:41:53 PDT
Comment on attachment 307432 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307432&action=review

Nice. r=me

> Tools/ChangeLog:3
> +        WebAssembly: import GCC torture tests

Nit: I'd make the bug title reflect what the patch actually does.

> Tools/Scripts/update-wasm-gcc-torture.py:96
> +def waterfall_known_failures(args):

What determines if something is a failure? If it's just that v8 fails that test, perhaps that's not the metric we want to use.
Comment 9 JF Bastien 2017-04-19 09:50:54 PDT
Created attachment 307485 [details]
patch

> > Tools/ChangeLog:3
> > +        WebAssembly: import GCC torture tests
> 
> Nit: I'd make the bug title reflect what the patch actually does.

Fixed.

> > Tools/Scripts/update-wasm-gcc-torture.py:96
> > +def waterfall_known_failures(args):
> 
> What determines if something is a failure? If it's just that v8 fails that
> test, perhaps that's not the metric we want to use.

The waterfall runs all these tests on V8, spec interpreter, and jsc. The expected failures file lists which test flavor it expects to fail and on which engine. The Mac waterfall turns red if jsc tests start failing.

We just happen to be on par with V8 :)
The line `if 'emwasm' not in attributes` is the one that distinguishes this particular build of the torture tests. IMO the other two flavors (bare and musl) are kinda obsolete now and have served their prior purpose wells I just ignored them here. If jsc and V8 diverged then the attributes would also say "jsc" or "V8", but then we'd need to differentiate which engine fails on which build flavor. Let's just not diverge instead ;)
Comment 10 WebKit Commit Bot 2017-04-19 09:53:19 PDT
Comment on attachment 307485 [details]
patch

Rejecting attachment 307485 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 307485, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Tools/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/3564373
Comment 11 JF Bastien 2017-04-19 09:56:25 PDT
Created attachment 307487 [details]
patch

One day I won't forget to update the OOPS. Today is not that day.
Comment 12 WebKit Commit Bot 2017-04-19 11:02:07 PDT
Comment on attachment 307487 [details]
patch

Clearing flags on attachment: 307487

Committed r215519: <http://trac.webkit.org/changeset/215519>
Comment 13 WebKit Commit Bot 2017-04-19 11:02:09 PDT
All reviewed patches have been landed.  Closing bug.