Bug 170740

Summary: WebAssembly: import GCC torture tests
Product: WebKit Reporter: JF Bastien <jfbastien>
Component: JavaScriptCoreAssignee: JF Bastien <jfbastien>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, fpizlo, jfbastien, keith_miller, mark.lam, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 159775, 170970    
Attachments:
Description Flags
patch
none
patch
none
patch
saam: review+, saam: commit-queue-
patch
commit-queue: commit-queue-
patch none

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.