Bug 163267 - Basic WebAssembly testing
Summary: Basic WebAssembly testing
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: 162870 163404
Blocks: 162706 163420 163421 163422 163424 163425
  Show dependency treegraph
 
Reported: 2016-10-10 21:28 PDT by JF Bastien
Modified: 2016-10-14 17:13 PDT (History)
8 users (show)

See Also:


Attachments
patch (126.65 KB, patch)
2016-10-10 21:58 PDT, JF Bastien
keith_miller: review-
keith_miller: commit-queue-
Details | Formatted Diff | Diff
patch (133.86 KB, patch)
2016-10-11 15:36 PDT, JF Bastien
no flags Details | Formatted Diff | Diff
patch (133.86 KB, patch)
2016-10-11 17:13 PDT, JF Bastien
no flags Details | Formatted Diff | Diff
patch (135.47 KB, patch)
2016-10-11 17:41 PDT, JF Bastien
keith_miller: review+
keith_miller: commit-queue-
Details | Formatted Diff | Diff
patch (142.32 KB, patch)
2016-10-14 13:10 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 2016-10-10 21:28:55 PDT
Add really basic JavaScript DSL to test WebAssembly. Start with being able to describe basic binaries, and then add more capabilities in follow-up patches.
Comment 1 JF Bastien 2016-10-10 21:58:03 PDT
Created attachment 291228 [details]
patch

I'm not sure about the yaml testing bit: it's acting up on my laptop and I'm tired.

This is a pretty limited first step, I'll need to add control ops, immediates, and other binary sections to the builder. That should be easy to do incrementally.

As discussed offline with Keith I moved the wasm tests to JSTests instead of being under JSTests/stress/.
Comment 2 Keith Miller 2016-10-11 09:01:48 PDT
Comment on attachment 291228 [details]
patch

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

Overall, I think it looks pretty good. I think it could use some more documentation on how one writes tests with this API though. I think wasm.js would be better as multiple module files. Yusuke spent all that time implementing it we might as well use it :D. I'll review the rest once that's happened.

> JSTests/wasm/wasm.js:81
> +        _push_8(v) { this._buf[this._used] = v & 0xFF; this._used += 1; }
> +        
> +        // Data types.
> +        uint8(v) { if ((v & 0xFF) >>> 0 !== v) throw new RangeError(`Invalid uint8 ${v}`); this._maybeGrow(1); this._push_8(v); }
> +        uint16(v) { if ((v & 0xFFFF) >>> 0 !== v) throw new RangeError(`Invalid uint16 ${v}`); this._maybeGrow(2); this._push_8(v); this._push_8(v >>> 8); }
> +        uint32(v) { if ((v & 0xFFFFFFFF) >>> 0 !== v) throw new RangeError(`Invalid uint32 ${v}`); this._maybeGrow(4); this._push_8(v); this._push_8(v >>> 8); this._push_8(v >>> 16); this._push_8(v >>> 24); }

These should be multi-line.

> JSTests/wasm/wasm.js:116
> +        _getter_range_check(at, size) { if (0 > at || at + size > this._used) throw new RangeError(`[${at}, ${at + size}) is out of buffer range [0, ${this._used})`); }
> +        get_uint8(at) { this._getter_range_check(at, 1); return this._buf[at]; }
> +        get_uint16(at) { this._getter_range_check(at, 2); return this._buf[at] | (this._buf[at + 1] << 8); }
> +        get_uint32(at) { this._getter_range_check(at, 4); return (this._buf[at] | (this._buf[at + 1] << 8) | (this._buf[at + 2] << 16) | (this._buf[at + 3] << 24)) >>> 0; }

Ditto on the multi-line.
Comment 3 JF Bastien 2016-10-11 15:36:05 PDT
Created attachment 291306 [details]
patch

Use modules, split things up a bunch, move what's now nice at the global scope there, update variable names to follow naming convention.
Comment 4 JF Bastien 2016-10-11 17:13:01 PDT
Created attachment 291321 [details]
patch

A random % appeared in my code. I have no idea where it came from. It is now gone.
Comment 5 JF Bastien 2016-10-11 17:41:39 PDT
Created attachment 291324 [details]
patch

Add varuint7 and use it where the spec says to, instead of using varuint. Get it directly from the JSON file.
Comment 6 Keith Miller 2016-10-13 14:11:36 PDT
Comment on attachment 291324 [details]
patch

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

r=me with some comments.

> JSTests/ChangeLog:8
> +        This DSL can then be used to write small text description of valid and invalid WebAssembly binaries, making testing the JSC implementation much easier.

I think you should have a larger description of what this patch does here. I don't think it's good practice to have patches this large without some context. You could briefly describe how one uses your DSL etc.

> JSTests/wasm/Builder.js:38
> +    case "i64": throw new Error(`Unimplemented: value check for ${type}`); // FIXME

You should link to a bugzilla here.

> JSTests/wasm/LowLevelBinary.js:165
> +        } while ((byte & 0x80) !== 0);

I thought there was a rule in WASM to limit the number of bytes to 5? Since no canonical VarUInt has more than 5 bytes.

> JSTests/wasm/self-test/test_BuilderJSON.js:20
> +    const b = (new Builder()).setPreamble({ "magic number": 1337 });

nit: These should be 42 :P

> JSTests/wasm/self-test/test_BuilderJSON.js:300
> +// FIXME test too many pops.
> +// FIXME test not enough pops (function has non-empty stack at the end).
> +// FIXME test mismatched pop types.
> +// FIXME test mismatched function signature (calling with wrong arg types).
> +// FIXME test invalid function index.
> +// FIXME test function names (both setting and calling them).

I think you should also have a bugzilla for these. Otherwise they just get forgotten.
Comment 7 JF Bastien 2016-10-14 13:10:03 PDT
Created attachment 291661 [details]
patch

I've addressed review comments:
 - README.md
 - Filed a bunch of bugs for future work, and refer to them in FIXME comments.
 - Add tests for LEBs being no more than 5 bytes (this was already the case through the logic of `shift`, but wasn't tested).
 - Ignore comment on magic number because 1337 is so much more elite than 42.

I also imported some of the testing infrastructure from: https://bugs.webkit.org/show_bug.cgi?id=163404
It was a bit silly to test everything modules wanted. I cleaned up how copying occurs to fit the wasm structure and expected module + wasm.json location.
Comment 8 Keith Miller 2016-10-14 16:45:30 PDT
Comment on attachment 291661 [details]
patch

r=me. I'm not CQ+ing the patch since you said Saam wanted to look at it first.
Comment 9 JF Bastien 2016-10-14 16:48:31 PDT
Saam said not to wait for him because he had some other high-priority thing, so he'd look early next week. Will CQ now.
Comment 10 WebKit Commit Bot 2016-10-14 17:13:10 PDT
Comment on attachment 291661 [details]
patch

Clearing flags on attachment: 291661

Committed r207363: <http://trac.webkit.org/changeset/207363>
Comment 11 WebKit Commit Bot 2016-10-14 17:13:15 PDT
All reviewed patches have been landed.  Closing bug.