Bug 218644 - WebAssembly: opcodes for table.grow and table.size are mixed up
Summary: WebAssembly: opcodes for table.grow and table.size are mixed up
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebAssembly (show other bugs)
Version: Safari 14
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 218876
Blocks:
  Show dependency treegraph
 
Reported: 2020-11-05 20:16 PST by Sergey Rubanov
Modified: 2020-11-13 13:42 PST (History)
9 users (show)

See Also:


Attachments
Patch (4.06 KB, patch)
2020-11-05 20:25 PST, Sergey Rubanov
no flags Details | Formatted Diff | Diff
Patch (7.44 KB, patch)
2020-11-05 20:37 PST, Sergey Rubanov
no flags Details | Formatted Diff | Diff
Patch (10.92 KB, patch)
2020-11-06 14:11 PST, Sergey Rubanov
no flags Details | Formatted Diff | Diff
Patch (12.09 KB, patch)
2020-11-06 20:03 PST, Sergey Rubanov
no flags Details | Formatted Diff | Diff
Patch (14.76 KB, patch)
2020-11-13 05:38 PST, Sergey Rubanov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].