Bug 165716 – WebAssembly: implement the table section and table import

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

Created attachment 296889 [details]
WIP

Most of the JS API is implemented I think. I just need to write the JSWebAssemblyTablePrototype functions now.
I haven't tested anything yet, either, so it could be broken somewhere.

[Saam Barati]

Created attachment 296898 [details]
WIP

More is done. Basically, the main API is done. I now need to teach call_indirect about tables, and then also teach our test builder about tables.

[Saam Barati]

Created attachment 296942 [details]
WIP

It turns out we misread the spec a bit on call_indirect. It only calls into the table index space. So I've changed that to be the case here. I think the entire system is set up to use tables, I'm going to implement testing now.

[Saam Barati]

Created attachment 296949 [details]
WIP

Maybe everything is working. I've written some basic table test. I now need to remove old call_indirect tests that were based on a misinterpretation of the spec.

[Saam Barati]

Created attachment 296966 [details]
patch

[WebKit Commit Bot]

Attachment 296966 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:674:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:679:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/wasm/WasmFunctionParser.h:395:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:237:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 296966 [details]
patch

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

> JSTests/wasm/Builder.js:329
> +        } else {
> +            assert.truthy(typeof signature === "number");
> +            const typeSection = builder._getSection("Type");
> +            assert.truthy(!!typeSection);
> +            assert.truthy(signature < typeSection.data.length);
> +            type = signature;
> +            signature = typeSection.data[signature];
> +            assert.hasObjectProperty(signature, "params", `Expect function signature to be an object with a "params" field, got "${signature}"`);
> +            params = signature.params;
> +        }

This now allows us to provide an index into the type section instead of the {params: ..., ret: ...} object.

> JSTests/wasm/Builder.js:429
>                  // FIXME Implement table https://bugs.webkit.org/show_bug.cgi?id=164135

This should be removed.

[Saam Barati]

Created attachment 296969 [details]
patch

[WebKit Commit Bot]

Attachment 296969 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:674:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:679:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/wasm/WasmFunctionParser.h:395:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:237:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Comment on attachment 296966 [details]
patch

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

> JSTests/wasm/Builder.js:324
> +            assert.truthy(signature < typeSection.data.length);

The above two should only fire in checked mode: they're fundamentally a breakage of the Module binary format's invariants, but they're a well-formed Module which we can assemble in this builder. We therefore want to be able to assemble such an invalid Module, but only in unchecked mode.

> JSTests/wasm/Builder_WebAssemblyBinary.js:32
> +function putResizableLimits(bin, initial, maximum) {

Arrow-func *all* the things!

> JSTests/wasm/function-tests/table-basic-2.js:19
> +            .Function("foo", 0 /*['i32', 'i32'] => 'i32'*/)

Does this not get uniqued if you provide it explicitly?

> JSTests/wasm/function-tests/table-basic.js:51
> +// FIXME: make this work cross module. The reason it doesn't

https://bugs.webkit.org/show_bug.cgi?id=165511

> JSTests/wasm/js-api/table.js:16
> +}

That's:
assert.throws(() => new WebAssembly.Module(bin), WebAssembly.CompileError, `Expected message`);

> Source/JavaScriptCore/wasm/WasmFormat.h:154
> +        ASSERT(!*this);

Why?

> Source/JavaScriptCore/wasm/WasmFormat.h:163
> +        ASSERT(*this);

Why?

> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:341
> +    static constexpr int8_t anyfuncType = -16;

This should come from the auto-generated code.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyTable.cpp:60
> +    m_jsFunctions = MallocPtr<WriteBarrier<WebAssemblyFunction>>::malloc(sizeof(WriteBarrier<WebAssemblyFunction>) * m_size);

What happens if allocation fails? We use the try* things elsewhere to fail gracefully if a silly user gives us huge numbers.
We should also check that these multiplications can't overflow beforehand (otherwise we'd under-allocate, and then write over some other memory). That check should probably be elsewhere though (since e.g. grow needs it too).

> Source/JavaScriptCore/wasm/js/JSWebAssemblyTable.cpp:101
> +    m_jsFunctions.realloc(sizeof(WriteBarrier<WebAssemblyFunction>) * newSize);

It aborts on failure?

> Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:144
> +            // This should be specified in the JS API, but isn't.

Issue number on GitHub?

> Source/JavaScriptCore/wasm/js/WebAssemblyTableConstructor.cpp:63
> +    // FIXME: this function is really close to WebAssembly.Memory construcotr, we could probably

Typo.

> Source/JavaScriptCore/wasm/js/WebAssemblyTablePrototype.cpp:189
> +    JSC_NATIVE_FUNCTION_WITHOUT_TRANSITION("set", webAssemblyTableProtoFuncSet, DontEnum, 2);

Can't you register all of these using the .lut.h files?

[Saam Barati]

Comment on attachment 296966 [details]
patch

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

>> JSTests/wasm/Builder.js:324
>> +            assert.truthy(signature < typeSection.data.length);
> 
> The above two should only fire in checked mode: they're fundamentally a breakage of the Module binary format's invariants, but they're a well-formed Module which we can assemble in this builder. We therefore want to be able to assemble such an invalid Module, but only in unchecked mode.

I agree with this, but it looks like we depend on this having real values below. I'm not quite sure how to make the two things compatible.

>> JSTests/wasm/Builder_WebAssemblyBinary.js:32
>> +function putResizableLimits(bin, initial, maximum) {
> 
> Arrow-func *all* the things!

Why?

>> JSTests/wasm/function-tests/table-basic-2.js:19
>> +            .Function("foo", 0 /*['i32', 'i32'] => 'i32'*/)
> 
> Does this not get uniqued if you provide it explicitly?

It does.

>> JSTests/wasm/js-api/table.js:16
>> +}
> 
> That's:
> assert.throws(() => new WebAssembly.Module(bin), WebAssembly.CompileError, `Expected message`);

I'm using indexOf, so I don't think it's the same. I guess I could move to full error messages and use this.

>> Source/JavaScriptCore/wasm/WasmFormat.h:154
>> +        ASSERT(!*this);
> 
> Why?

To assert something we depend on.

>> Source/JavaScriptCore/wasm/WasmFormat.h:163
>> +        ASSERT(*this);
> 
> Why?

Ditto

>> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:341
>> +    static constexpr int8_t anyfuncType = -16;
> 
> This should come from the auto-generated code.

Yeah I thought this might. How do I get this number from auto generated code?

>> Source/JavaScriptCore/wasm/js/JSWebAssemblyTable.cpp:60
>> +    m_jsFunctions = MallocPtr<WriteBarrier<WebAssemblyFunction>>::malloc(sizeof(WriteBarrier<WebAssemblyFunction>) * m_size);
> 
> What happens if allocation fails? We use the try* things elsewhere to fail gracefully if a silly user gives us huge numbers.
> We should also check that these multiplications can't overflow beforehand (otherwise we'd under-allocate, and then write over some other memory). That check should probably be elsewhere though (since e.g. grow needs it too).

I'm pretty sure bmalloc will crash if its allocation is too large.

Yeah, I was assuming the multiplications won't overflow because this is only on 64 bit platforms. But I'm actually not sure if the compiler will emit a 64bit or 32bit multiplication here.

>> Source/JavaScriptCore/wasm/js/JSWebAssemblyTable.cpp:101
>> +    m_jsFunctions.realloc(sizeof(WriteBarrier<WebAssemblyFunction>) * newSize);
> 
> It aborts on failure?

I think this crashes on failure.

>> Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:144
>> +            // This should be specified in the JS API, but isn't.
> 
> Issue number on GitHub?

Sorry I should remove this FIXME. My question was answered. It should only the the current size, not the "initial" size. This also means we have a bug for Memory imports where we just check the initialization start size, not the dynamic size.

>> Source/JavaScriptCore/wasm/js/WebAssemblyTableConstructor.cpp:63
>> +    // FIXME: this function is really close to WebAssembly.Memory construcotr, we could probably
> 
> Typo.

I'm going to remove this FIXME, it's different enough to stay separate.

[JF Bastien]

(In reply to comment #11)
> Comment on attachment 296966 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296966&action=review
> 
> >> JSTests/wasm/Builder.js:324
> >> +            assert.truthy(signature < typeSection.data.length);
> > 
> > The above two should only fire in checked mode: they're fundamentally a breakage of the Module binary format's invariants, but they're a well-formed Module which we can assemble in this builder. We therefore want to be able to assemble such an invalid Module, but only in unchecked mode.
> 
> I agree with this, but it looks like we depend on this having real values
> below. I'm not quite sure how to make the two things compatible.

Could you file a FIXME for it? I can do that later, agreed it's not the most important.


> >> JSTests/wasm/Builder_WebAssemblyBinary.js:32
> >> +function putResizableLimits(bin, initial, maximum) {
> > 
> > Arrow-func *all* the things!
> 
> Why?

Because that's the style around these files :=>)
(that's an arrow fun smilie)

> >> JSTests/wasm/function-tests/table-basic-2.js:19
> >> +            .Function("foo", 0 /*['i32', 'i32'] => 'i32'*/)
> > 
> > Does this not get uniqued if you provide it explicitly?
> 
> It does.

I'd use that syntax then, instead of explicit 0.


> >> Source/JavaScriptCore/wasm/WasmFormat.h:154
> >> +        ASSERT(!*this);
> > 
> > Why?

Ah sorry, I misread and thought it was an assert that this wasn't non-null! That would have been UB :-)


> >> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:341
> >> +    static constexpr int8_t anyfuncType = -16;
> > 
> > This should come from the auto-generated code.
> 
> Yeah I thought this might. How do I get this number from auto generated code?

It's just "Anyfunc", from the auto-gem WasmOps.h.


> >> Source/JavaScriptCore/wasm/js/JSWebAssemblyTable.cpp:60
> >> +    m_jsFunctions = MallocPtr<WriteBarrier<WebAssemblyFunction>>::malloc(sizeof(WriteBarrier<WebAssemblyFunction>) * m_size);
> > 
> > What happens if allocation fails? We use the try* things elsewhere to fail gracefully if a silly user gives us huge numbers.
> > We should also check that these multiplications can't overflow beforehand (otherwise we'd under-allocate, and then write over some other memory). That check should probably be elsewhere though (since e.g. grow needs it too).
> 
> I'm pretty sure bmalloc will crash if its allocation is too large.

I'd rather use the try* variants, or file a bug to do so.


> Yeah, I was assuming the multiplications won't overflow because this is only
> on 64 bit platforms. But I'm actually not sure if the compiler will emit a
> 64bit or 32bit multiplication here.

Yeah we should check that, overflow would be a Bad Thing, and it can't go above 32-bit in a useful way anyways.

[Saam Barati]

Comment on attachment 296966 [details]
patch

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

>>>> JSTests/wasm/Builder.js:324
>>>> +            assert.truthy(signature < typeSection.data.length);
>>> 
>>> The above two should only fire in checked mode: they're fundamentally a breakage of the Module binary format's invariants, but they're a well-formed Module which we can assemble in this builder. We therefore want to be able to assemble such an invalid Module, but only in unchecked mode.
>> 
>> I agree with this, but it looks like we depend on this having real values below. I'm not quite sure how to make the two things compatible.
> 
> Could you file a FIXME for it? I can do that later, agreed it's not the most important.

done.

>>>> JSTests/wasm/Builder_WebAssemblyBinary.js:32
>>>> +function putResizableLimits(bin, initial, maximum) {
>>> 
>>> Arrow-func *all* the things!
>> 
>> Why?
> 
> Because that's the style around these files :=>)
> (that's an arrow fun smilie)

ok.

>>>> JSTests/wasm/function-tests/table-basic-2.js:19
>>>> +            .Function("foo", 0 /*['i32', 'i32'] => 'i32'*/)
>>> 
>>> Does this not get uniqued if you provide it explicitly?
>> 
>> It does.
> 
> I'd use that syntax then, instead of explicit 0.

I need to test my new code that handles indices.

>>> JSTests/wasm/js-api/table.js:16
>>> +}
>> 
>> That's:
>> assert.throws(() => new WebAssembly.Module(bin), WebAssembly.CompileError, `Expected message`);
> 
> I'm using indexOf, so I don't think it's the same. I guess I could move to full error messages and use this.

I'm going to skip doing this since the error message I get is tied to the exact code evaluated that produces the error. That seems unnecessarily fragile to me. However, I'll strengthen my assertion here to test `e instanceof WebAssembly.CompileError`

[Saam Barati]

Created attachment 296981 [details]
patch

[WebKit Commit Bot]

Attachment 296981 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmFunctionParser.h:395:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 296981 [details]
patch

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

> Source/JavaScriptCore/wasm/WasmFormat.h:224
> +    explicit operator bool() const { return !code && !signature; }

This is wrong. I'll remove this I don't think I use it anywhere.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyHelpers.h:34
> +uint32_t ToNonWrappingUint32(ExecState*, JSValue);

I'll delete this line

> Source/JavaScriptCore/wasm/js/JSWebAssemblyTable.cpp:108
> +    // FIXME: we should protect ourselves from crazy sizes here.

I'll remove this since we now protect ourselves

> Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:140
> +                return JSValue::encode(throwException(exec, throwScope, createTypeError(exec, ASCIILiteral("Table import is not an instance of WebAssembly.Table"))));

I'll add a test for this I don't think I have one.

> Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:155
> +                        throwException(exec, throwScope, createTypeError(exec, ASCIILiteral("Table imports 'maximum' is larger than the module's expected 'maximum'"))));

Should be "import's" not "imports"

> Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:234
> +            instance->setTable(vm,

This should be over two lines so we check the exception. Alternatively we can assert that this doesn't throw since we check that the initial size is valid during parsing.

[Keith Miller]

Comment on attachment 296981 [details]
patch

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

r=me.

> JSTests/wasm/Builder.js:115
> +    return (module, field, tableDescription) => {

I think we should start having a comment saying what properties we want in the tableDescription. If you're motivated, ditto for memory description above. Figuring out exactly what properties we need / can optionally have is a pain otherwise.

> JSTests/wasm/Builder.js:326
> +            // FIXME: we should allow indices that exceed this to be able to
> +            // test JSCs validator is correct. https://bugs.webkit.org/show_bug.cgi?id=165786
> +            assert.truthy(signature < typeSection.data.length);

Can't you just do:

if (builder._checked)
    assert.truthy(signature < typeSection.data.length);

> JSTests/wasm/function-tests/table-basic-2.js:1
> +import Builder from '../Builder.js'

I don't know if these tests should be in function-tests. Maybe we should have a different directory but function tests was originally just for the jsc.cpp testWasmFunction tests.

[Saam Barati]

Comment on attachment 296981 [details]
patch

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

>> JSTests/wasm/Builder.js:115
>> +    return (module, field, tableDescription) => {
> 
> I think we should start having a comment saying what properties we want in the tableDescription. If you're motivated, ditto for memory description above. Figuring out exactly what properties we need / can optionally have is a pain otherwise.

I'll use destructuring to indicate which properties we're expecting and which properties we forward into a new object.

>> JSTests/wasm/Builder.js:326
>> +            assert.truthy(signature < typeSection.data.length);
> 
> Can't you just do:
> 
> if (builder._checked)
>     assert.truthy(signature < typeSection.data.length);

No because the builder below expects type and params to be real things. In the future, we should make the code be able to just do this.

[Saam Barati]

Created attachment 297034 [details]
patch for landing

[WebKit Commit Bot]

Attachment 297034 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmFunctionParser.h:395:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

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