Bug 164134

Created attachment 296451 [details] WIP I need to hook it up to Instance now.

Created attachment 296469 [details] WIP Need to write stuff in the builder now to test that it works.

Comment on attachment 296469 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=296469&action=review Quick review. > Source/JavaScriptCore/jsc.cpp:2629 > + if (!!plan.getModuleInformation()->memory) { I'm not a fan of having operator bool for this. It's not clear what it means w.r.t. wasm semantics. > Source/JavaScriptCore/testWasm.cpp:306 > + /* I think all the ones without memory can be removed separately? Or are those not in the builder yet? And this patch can remove all the memory ones. > Source/JavaScriptCore/runtime/VM.h:297 > + uint32_t wasmMemorySize; I'd like use to call it "size in pages" so it's unambiguously not bytes (or use "size in bytes"... maybe bytes is better?). > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:212 > + if (!!memory) { Ditto on operator bool, I'd spell out the meaning. > Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:29 > +#include "WasmCallingConvention.h" I think we usually put this include under the #if ENABLE. > Source/JavaScriptCore/wasm/WasmMemoryInformation.h:62 > + explicit operator bool() const { return !!m_minSize && !!m_maxSize; } I would make this an explicit function. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:327 > + PageCount maxSize = PageCount::max(); IMO it's worth distinguishing between explicitly getting max from the user, versus getting an unspecified value. > Source/JavaScriptCore/wasm/WasmPageCount.h:36 > + : m_pageCount(UINT_MAX) I'd rather use a typedef for uint32_t here and below, and / or do: std::numeric_limits<typeof(m_pageCount)>::max() > Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:62 > + createTypeError(exec, ASCIILiteral("WebAssembly.Memory expects the 'initial' property to be an integer in the range: [0, 2^32 - 1]"))); That's too big: we can only address 32-bit, so 2^32 pages exceeds that number.

Comment on attachment 296469 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=296469&action=review Thanks for the comments. >> Source/JavaScriptCore/runtime/VM.h:297 >> + uint32_t wasmMemorySize; > > I'd like use to call it "size in pages" so it's unambiguously not bytes (or use "size in bytes"... maybe bytes is better?). This is in bytes. My solution for this problem was adding Wasm::PageCount to indicate we're talking about a quantity of pages. Any other memory related size is an uint32 and represents bytes. That said, it's probably worth adding "bytes" to the end of this name to prevent any confusion. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:212 >> + if (!!memory) { > > Ditto on operator bool, I'd spell out the meaning. What name would you give this? The first thing that comes to my mind is "isValid" and that doesn't seem much better than "operator book". In some ways, what we want is an Optional<MemoryInformation>, but I thought I'd just build in the Optional bit into the MemoryInformation itself. >> Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:29 >> +#include "WasmCallingConvention.h" > > I think we usually put this include under the #if ENABLE. Will fix. >> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:327 >> + PageCount maxSize = PageCount::max(); > > IMO it's worth distinguishing between explicitly getting max from the user, versus getting an unspecified value. Yeah you're right. I was mimicking what the old code was doing, but we definitely want to be able to tell when a module specifies a maximum memory size versus not specifying one. I just now need to decide what to do inside Memory construction when given an invalid maximum page count. >> Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:62 >> + createTypeError(exec, ASCIILiteral("WebAssembly.Memory expects the 'initial' property to be an integer in the range: [0, 2^32 - 1]"))); > > That's too big: we can only address 32-bit, so 2^32 pages exceeds that number. This is just performing the toUint32t operation the spec defines. There is a nicer error below when you specify a page count that is too large. Do you think it's worth specifiying this error to just say it expects a range up to PageCount::max()?

Created attachment 296580 [details] WIP The Memory API works enough to be able to run a simple test on it.

Created attachment 296612 [details] WIP rebased

Created attachment 296617 [details] WIP More tests and more validation.

Created attachment 296623 [details] WIP I think this is just about done.

Comment on attachment 296623 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=296623&action=review > JSTests/wasm/Builder.js:108 > + assert.isString(field, `Import function field should be a string, got "${field}"`); Both above should be "Import memory". > JSTests/wasm/Builder.js:109 > + // FIXME: what validation should I do here? I'd just fix that later, separate bug. > JSTests/wasm/Builder.js:482 > + const functionIndexSpaceOffset = 0; Derp. > JSTests/wasm/Builder_WebAssemblyBinary.js:64 > + assert.truthy(typeof initial === "number", "Execpt a number of undefined for 'maximum'"); assert.isA(foo, "number", "what what"); does that for you. Also, typo "Execpt". > JSTests/wasm/Builder_WebAssemblyBinary.js:71 > + assert.truthy(typeof maximum === "undefined", "Execpt a number of undefined for 'maximum'"); Ditto. > JSTests/wasm/Builder_WebAssemblyBinary.js:73 > + put(bin, "varuint32", hasMaximum); This is a varuint1. > JSTests/wasm/js-api/test_memory.js:3 > +function assert(b) { The other tests just import assert.js. > JSTests/wasm/js-api/test_memory.js:12 > + let threw = false; The assert module has a helper for throwing. See test_BuilderJSON.js: (function CustomSectionInvalidByte() { const u = (new Builder()).Unknown("custom section"); assert.throws(() => u.Byte(0xFF + 1), Error, `Not the same: "0" and "256": Unknown section expected byte, got: "256"`); })(); > JSTests/wasm/js-api/test_memory.js:150 > + } This test passing makes me happy. > Source/JavaScriptCore/runtime/VM.h:300 > + uint32_t wasmMemorySize; I'd call these "top" as well, since we can have multiple. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:357 > + // We only allow one memory for now. Can you quote the spec / design here? Makes it easier to map back if it changes IMO. > Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:66 > + // so we guarantee its lifecylce. Typo. > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:166 > + throwException(exec, throwScope, createTypeError(exec, ASCIILiteral("Memory imports 'maximum' is larger than the module's expected 'maximum")))); No "s" on "import" as above.

Comment on attachment 296623 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=296623&action=review >> JSTests/wasm/Builder.js:482 >> + const functionIndexSpaceOffset = 0; > > Derp. I need to come up with the actual solution for this. I'll figure it out before uploading the patch for review.

Created attachment 296675 [details] patch

Attachment 296675 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jsc.cpp:72: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryPrototype.cpp:57: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.h:50: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:197: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 6 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 296675 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=296675&action=review A few nits, then lgtm > JSTests/wasm/js-api/test_basic_api.js:78 > + new WebAssembly.Memory({initial: 20}); We should add a FIXME to test more of the Memory API's properties. > JSTests/wasm/js-api/test_memory.js:1 > +import Builder from '../Builder.js'; I would use assert.js here. It does much of the assert / throw stuff below. > Source/JavaScriptCore/wasm/WasmMemory.cpp:43 > + RELEASE_ASSERT(!maximum || maximum >= initial); // This should be gauaranteed by our caller. Typo.

Created attachment 296685 [details] patch

Attachment 296685 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jsc.cpp:72: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryPrototype.cpp:57: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.h:50: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:197: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 6 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 296685 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=296685&action=review r=me. > JSTests/wasm/Builder.js:106 > + return (module, field, {initial, maximum}) => { Why destructure this?

Comment on attachment 296685 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=296685&action=review >> JSTests/wasm/Builder.js:106 >> + return (module, field, {initial, maximum}) => { > > Why destructure this? Probably when I was debugging this code. I'll remove that.

landed in: https://trac.webkit.org/changeset/209630