Bug 198761 – [WASM-References] Add support for Table.size, grow and fill instructions

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

[Justin Michaud]

Add support for Table.size, grow and fill

[Justin Michaud]

Created attachment 372141 [details]
Patch

[Justin Michaud]

Comment on attachment 372141 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmInstance.cpp:94
> +    // TODO doSet

Once the Funcref patch lands, we can do this.

[EWS Watchlist]

This patch modifies one of the wasm.json files. Please ensure that any changes in one have been mirrored to the other. You can find the wasm.json files at "Source/JavaScriptCore/wasm/wasm.json" and "JSTests/wasm/wasm.json".

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/wasm/generateWasmOpsHeader.py:68:  [<lambda>] Undefined variable 'isUnary'  [pylint/E0602] [5]
ERROR: Source/JavaScriptCore/wasm/generateWasmOpsHeader.py:68:  [<lambda>] Undefined variable 'isBinary'  [pylint/E0602] [5]
Total errors found: 2 in 16 files


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

[EWS Watchlist]

Comment on attachment 372141 [details]
Patch

Attachment 372141 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12477300

New failing tests:
wasm.yaml/wasm/references/anyref_table.js.wasm-no-call-ic
wasm.yaml/wasm/references/anyref_table_import.js.wasm-no-cjit-yes-tls-context
wasm.yaml/wasm/references/anyref_table.js.wasm-collect-continuously
wasm.yaml/wasm/references/anyref_table_import.js.wasm-no-call-ic
wasm.yaml/wasm/references/anyref_table_import.js.default-wasm
wasm.yaml/wasm/references/anyref_table_import.js.wasm-no-tls-context
wasm.yaml/wasm/references/anyref_table_import.js.wasm-eager-jettison
wasm.yaml/wasm/references/anyref_table.js.wasm-no-cjit-yes-tls-context
wasm.yaml/wasm/references/anyref_table_import.js.wasm-collect-continuously
wasm.yaml/wasm/references/anyref_table.js.wasm-no-tls-context
wasm.yaml/wasm/references/anyref_table.js.wasm-slow-memory
wasm.yaml/wasm/references/anyref_table.js.wasm-eager-jettison
wasm.yaml/wasm/references/anyref_table.js.default-wasm
wasm.yaml/wasm/references/anyref_table_import.js.wasm-slow-memory
apiTests

[Justin Michaud]

Created attachment 372156 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/wasm/generateWasmOpsHeader.py:68:  [<lambda>] Undefined variable 'isUnary'  [pylint/E0602] [5]
ERROR: Source/JavaScriptCore/wasm/generateWasmOpsHeader.py:68:  [<lambda>] Undefined variable 'isBinary'  [pylint/E0602] [5]
Total errors found: 2 in 16 files


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

[Yusuke Suzuki]

Comment on attachment 372156 [details]
Patch

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

r=me

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:307
> +        switch ((ExtTableOpType) extOp) {

Use `static_cast<ExtTableOpType>(extOp)`.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:90
> +    if (!newSize || *newSize == oldSize)
> +        return -1;

delta = 0, returning -1, I think this is a spec bug. So, can we have a FIXME comment here with an url pointing bugzilla issue about the spec bug?

> Source/JavaScriptCore/wasm/WasmInstance.cpp:107
> +    unsigned i = offset;
> +    unsigned n = count;

Can we use the names `offset` and `count` for the local variables instead? Using `offset` and `count` is better than `i` and `n`.
Like, putting `unsafeOffset` and `unsafeCount` as parameters, validate with the above if-statement, and copy them to the `unsigned` offset and count.

> Source/JavaScriptCore/wasm/WasmInstance.cpp:110
> +    if (i >= instance->table()->length() || i + n > instance->table()->length())
> +        return false;

I think only `i + n > instance->table()->length()` is OK. The maximum value of i and n is INT32_MAX. INT32_MAX + INT32_MAX does not exceed UINT32_MAX, so we do not need to consider about overflow too.

> Source/JavaScriptCore/wasm/WasmTable.cpp:130
> +    for (uint32_t i = m_length; i < allocatedLength(newLength); ++i)
> +        m_jsValues.get()[i].setWithoutWriteBarrier(jsNull());
> +

This part is already fixed in https://trac.webkit.org/changeset/246487/webkit, so it is not necessary.

[Justin Michaud]

Comment on attachment 372156 [details]
Patch

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

>> Source/JavaScriptCore/wasm/WasmInstance.cpp:110
>> +        return false;
> 
> I think only `i + n > instance->table()->length()` is OK. The maximum value of i and n is INT32_MAX. INT32_MAX + INT32_MAX does not exceed UINT32_MAX, so we do not need to consider about overflow too.

If n=0, I=table->length then it should not succeed

[Justin Michaud]

Created attachment 372397 [details]
Patch

[Justin Michaud]

Created attachment 372398 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/wasm/generateWasmOpsHeader.py:68:  [<lambda>] Undefined variable 'isUnary'  [pylint/E0602] [5]
ERROR: Source/JavaScriptCore/wasm/generateWasmOpsHeader.py:68:  [<lambda>] Undefined variable 'isBinary'  [pylint/E0602] [5]
Total errors found: 2 in 14 files


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

[WebKit Commit Bot]

Comment on attachment 372398 [details]
Patch

Clearing flags on attachment: 372398

Committed r246577: <https://trac.webkit.org/changeset/246577>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51875848>