Bug 163267

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

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.

Created attachment 291321 [details] patch A random % appeared in my code. I have no idea where it came from. It is now gone.

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

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 on attachment 291661 [details] patch r=me. I'm not CQ+ing the patch since you said Saam wanted to look at it first.

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 on attachment 291661 [details] patch Clearing flags on attachment: 291661 Committed r207363: <http://trac.webkit.org/changeset/207363>

All reviewed patches have been landed. Closing bug.