Bug 163267 – Basic WebAssembly testing

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Bastien]

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.

[JF Bastien]

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/.

[Keith Miller]

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.

[JF Bastien]

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.

[JF Bastien]

Created attachment 291321 [details]
patch

A random % appeared in my code. I have no idea where it came from. It is now gone.

[JF Bastien]

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.

[Keith Miller]

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.

[JF Bastien]

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.

[Keith Miller]

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.

[JF Bastien]

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.

[WebKit Commit Bot]

Comment on attachment 291661 [details]
patch

Clearing flags on attachment: 291661

Committed r207363: <http://trac.webkit.org/changeset/207363>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.