Bug 218644

Summary: WebAssembly: opcodes for table.grow and table.size are mixed up
Product: WebKit Reporter: Sergey Rubanov <chi187>
Component: WebAssemblyAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, smoley, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: Safari 14   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 218876    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Sergey Rubanov
Reported 2020-11-05 20:16:42 PST
table.grow has extendedOp equal 16 and table.size has extendedOp equal 15, but it should be the other way around. https://github.com/WebAssembly/reference-types/issues/29#issuecomment-463608507 https://webassembly.github.io/reference-types/core/binary/instructions.html#table-instructions
Attachments
Patch (4.06 KB, patch)
2020-11-05 20:25 PST, Sergey Rubanov 		no flags
DetailsFormatted DiffDiff
Patch (7.44 KB, patch)
2020-11-05 20:37 PST, Sergey Rubanov 		no flags
DetailsFormatted DiffDiff
Patch (10.92 KB, patch)
2020-11-06 14:11 PST, Sergey Rubanov 		no flags
DetailsFormatted DiffDiff
Patch (12.09 KB, patch)
2020-11-06 20:03 PST, Sergey Rubanov 		no flags
DetailsFormatted DiffDiff
Patch (14.76 KB, patch)
2020-11-13 05:38 PST, Sergey Rubanov 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Sergey Rubanov
Comment 1 2020-11-05 20:25:35 PST
Created attachment 413392 [details] Patch
EWS Watchlist
Comment 2 2020-11-05 20:26:26 PST
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".
Sergey Rubanov
Comment 3 2020-11-05 20:37:41 PST
Created attachment 413393 [details] Patch
Keith Miller
Comment 4 2020-11-06 07:51:28 PST
Comment on attachment 413393 [details] Patch Looks like there's a layout test relying on this. Can you fix that test? Also, could you import the spec tests for the table grow instructions?
Sergey Rubanov
Comment 5 2020-11-06 11:51:00 PST
(In reply to Keith Miller from comment #4) > Comment on attachment 413393 [details] > Patch > > Looks like there's a layout test relying on this. Can you fix that test? > Also, could you import the spec tests for the table grow instructions? will do 👍
Sergey Rubanov
Comment 6 2020-11-06 14:11:43 PST
Created attachment 413480 [details] Patch
Sergey Rubanov
Comment 7 2020-11-06 14:31:42 PST
I didn't find test for table.grow and table.size in current spec and in reference types proposal repos. There could be tests in V8 and SpiderMonkey repo as mentioned in https://bugs.webkit.org/show_bug.cgi?id=218331#c0, but it would be better to import all reference types tests at once IMO. My patch will have a conflict with the patch from #218331 (mentioned above). Either of these two need to be updated after another one will land.
Sergey Rubanov
Comment 8 2020-11-06 20:03:28 PST
Created attachment 413506 [details] Patch
Yusuke Suzuki
Comment 9 2020-11-08 14:10:50 PST
(In reply to Sergey Rubanov from comment #7) > I didn't find test for table.grow and table.size in current spec and in > reference types proposal repos. There could be tests in V8 and SpiderMonkey > repo as mentioned in https://bugs.webkit.org/show_bug.cgi?id=218331#c0, but > it would be better to import all reference types tests at once IMO. > > My patch will have a conflict with the patch from #218331 (mentioned above). > Either of these two need to be updated after another one will land. Oh... Can you write a test for table.size and table.grow in JSTests/wasm/stress? You can write a test with wabt by using wabt-wrapper.js? The example using wabt is `JSTests/wasm/stress/top-most-enclosing-stack.js`.
Radar WebKit Bug Importer
Comment 10 2020-11-09 11:46:51 PST
<rdar://problem/71202695>
Sergey Rubanov
Comment 11 2020-11-12 13:59:03 PST
Tests are blocked by https://bugs.webkit.org/show_bug.cgi?id=218744
Sergey Rubanov
Comment 12 2020-11-13 05:38:51 PST
Created attachment 414034 [details] Patch
Sergey Rubanov
Comment 13 2020-11-13 10:53:01 PST
jsc-armv7-test seem to be stuck :(
Yusuke Suzuki
Comment 14 2020-11-13 13:08:30 PST
Comment on attachment 414034 [details] Patch Perfect! Thanks, r=me.
EWS
Comment 15 2020-11-13 13:40:29 PST
Committed r269790: <https://trac.webkit.org/changeset/269790> All reviewed patches have been landed. Closing bug and clearing flags on attachment 414034 [details].
Note You need to log in before you can comment on or make changes to this bug.