Bug 218644 – WebAssembly: opcodes for table.grow and table.size are mixed up

• Status: RESOLVED
• Resolution: FIXED
• Component: WebAssembly

[Sergey Rubanov]

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

[Sergey Rubanov]

Created attachment 413392 [details]
Patch

[EWS Watchlist]

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]

Created attachment 413393 [details]
Patch

[Keith Miller]

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]

(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]

Created attachment 413480 [details]
Patch

[Sergey Rubanov]

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]

Created attachment 413506 [details]
Patch

[Yusuke Suzuki]

(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]

<rdar://problem/71202695>

[Sergey Rubanov]

Tests are blocked by https://bugs.webkit.org/show_bug.cgi?id=218744

[Sergey Rubanov]

Created attachment 414034 [details]
Patch

[Sergey Rubanov]

jsc-armv7-test seem to be stuck :(

[Yusuke Suzuki]

Comment on attachment 414034 [details]
Patch

Perfect! Thanks, r=me.

[EWS]

Committed r269790: <https://trac.webkit.org/changeset/269790>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414034 [details].