Summary: | [WASM-References] Add support for Table.size, grow and fill instructions | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Justin Michaud <justin_michaud> | ||||||||||
Component: | JavaScriptCore | Assignee: | Justin Michaud <justin_michaud> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Justin Michaud
2019-06-11 11:12:04 PDT
Created attachment 372141 [details]
Patch
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. 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". 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.
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 Created attachment 372156 [details]
Patch
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 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 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 Created attachment 372397 [details]
Patch
Created attachment 372398 [details]
Patch
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 on attachment 372398 [details] Patch Clearing flags on attachment: 372398 Committed r246577: <https://trac.webkit.org/changeset/246577> All reviewed patches have been landed. Closing bug. |