Bug 198761 - [WASM-References] Add support for Table.size, grow and fill instructions
Summary: [WASM-References] Add support for Table.size, grow and fill instructions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Michaud
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-11 11:12 PDT by Justin Michaud
Modified: 2019-06-18 18:19 PDT (History)
9 users (show)

See Also:


Attachments
Patch (34.26 KB, patch)
2019-06-14 14:20 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (34.50 KB, patch)
2019-06-14 17:30 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (37.14 KB, patch)
2019-06-18 15:56 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (37.13 KB, patch)
2019-06-18 15:58 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Michaud 2019-06-11 11:12:04 PDT
Add support for Table.size, grow and fill
Comment 1 Justin Michaud 2019-06-14 14:20:15 PDT
Created attachment 372141 [details]
Patch
Comment 2 Justin Michaud 2019-06-14 14:20:48 PDT
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.
Comment 3 Build Bot 2019-06-14 14:22:31 PDT
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".
Comment 4 Build Bot 2019-06-14 14:22:45 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2019-06-14 16:12:15 PDT Comment hidden (obsolete)
Comment 6 Justin Michaud 2019-06-14 17:30:41 PDT
Created attachment 372156 [details]
Patch
Comment 7 Build Bot 2019-06-14 17:33:39 PDT
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.
Comment 8 Yusuke Suzuki 2019-06-17 19:41:46 PDT
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.
Comment 9 Justin Michaud 2019-06-18 15:32:54 PDT
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
Comment 10 Justin Michaud 2019-06-18 15:56:53 PDT
Created attachment 372397 [details]
Patch
Comment 11 Justin Michaud 2019-06-18 15:58:11 PDT
Created attachment 372398 [details]
Patch
Comment 12 Build Bot 2019-06-18 16:01:23 PDT
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.
Comment 13 WebKit Commit Bot 2019-06-18 18:18:49 PDT
Comment on attachment 372398 [details]
Patch

Clearing flags on attachment: 372398

Committed r246577: <https://trac.webkit.org/changeset/246577>
Comment 14 WebKit Commit Bot 2019-06-18 18:18:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2019-06-18 18:19:17 PDT
<rdar://problem/51875848>