WebAssembly JS API: implement importing and defining Memory

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[JF Bastien]

https://github.com/WebAssembly/design/blob/master/JS.md#webassemblymemory-constructor

Also allow importing / exporting them.

[Saam Barati]

Created attachment 296451 [details]
WIP

I need to hook it up to Instance now.

[Saam Barati]

Created attachment 296469 [details]
WIP

Need to write stuff in the builder now to test that it works.

[JF Bastien]

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.

[Saam Barati]

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()?

[Saam Barati]

Created attachment 296580 [details]
WIP

The Memory API works enough to be able to run a simple test on it.

[Saam Barati]

Created attachment 296612 [details]
WIP

rebased

[Saam Barati]

Created attachment 296617 [details]
WIP

More tests and more validation.

[Saam Barati]

Created attachment 296623 [details]
WIP

I think this is just about done.

[JF Bastien]

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.

[Saam Barati]

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.

[Saam Barati]

Created attachment 296675 [details]
patch

[WebKit Commit Bot]

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.

[JF Bastien]

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.

[Saam Barati]

Created attachment 296685 [details]
patch

[WebKit Commit Bot]

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.

[Keith Miller]

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?

[Saam Barati]

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.

[Saam Barati]

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