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

Description Sergey Rubanov 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
Comment 1 Sergey Rubanov 2020-11-05 20:25:35 PST 
Created attachment 413392 [details]
Patch
Comment 2 EWS Watchlist 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".
Comment 3 Sergey Rubanov 2020-11-05 20:37:41 PST 
Created attachment 413393 [details]
Patch
Comment 4 Keith Miller 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?
Comment 5 Sergey Rubanov 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 👍
Comment 6 Sergey Rubanov 2020-11-06 14:11:43 PST 
Created attachment 413480 [details]
Patch
Comment 7 Sergey Rubanov 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.
Comment 8 Sergey Rubanov 2020-11-06 20:03:28 PST 
Created attachment 413506 [details]
Patch
Comment 9 Yusuke Suzuki 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`.
Comment 10 Radar WebKit Bug Importer 2020-11-09 11:46:51 PST 
<rdar://problem/71202695>
Comment 11 Sergey Rubanov 2020-11-12 13:59:03 PST 
Tests are blocked by https://bugs.webkit.org/show_bug.cgi?id=218744
Comment 12 Sergey Rubanov 2020-11-13 05:38:51 PST 
Created attachment 414034 [details]
Patch
Comment 13 Sergey Rubanov 2020-11-13 10:53:01 PST 
jsc-armv7-test seem to be stuck :(
Comment 14 Yusuke Suzuki 2020-11-13 13:08:30 PST 
Comment on attachment 414034 [details]
Patch

Perfect! Thanks, r=me.
Comment 15 EWS 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].