Bug 198761

Summary: [WASM-References] Add support for Table.size, grow and fill instructions
Product: WebKit Reporter: Justin Michaud <justin_michaud>
Component: JavaScriptCoreAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch none

Justin Michaud
Reported 2019-06-11 11:12:04 PDT
Add support for Table.size, grow and fill
Attachments
Patch (34.26 KB, patch)
2019-06-14 14:20 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Patch (34.50 KB, patch)
2019-06-14 17:30 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Patch (37.14 KB, patch)
2019-06-18 15:56 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Patch (37.13 KB, patch)
2019-06-18 15:58 PDT, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Justin Michaud
Comment 1 2019-06-14 14:20:15 PDT
Created attachment 372141 [details] Patch
Justin Michaud
Comment 2 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.
EWS Watchlist
Comment 3 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".
EWS Watchlist
Comment 4 2019-06-14 14:22:45 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 5 2019-06-14 16:12:15 PDT Comment hidden (obsolete)
Justin Michaud
Comment 6 2019-06-14 17:30:41 PDT
Created attachment 372156 [details] Patch
EWS Watchlist
Comment 7 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.
Yusuke Suzuki
Comment 8 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.
Justin Michaud
Comment 9 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
Justin Michaud
Comment 10 2019-06-18 15:56:53 PDT
Created attachment 372397 [details] Patch
Justin Michaud
Comment 11 2019-06-18 15:58:11 PDT
Created attachment 372398 [details] Patch
EWS Watchlist
Comment 12 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.
WebKit Commit Bot
Comment 13 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>
WebKit Commit Bot
Comment 14 2019-06-18 18:18:50 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 15 2019-06-18 18:19:17 PDT
<rdar://problem/51875848>
Note You need to log in before you can comment on or make changes to this bug.