1496582 - Missing target memory/table index immediate for bulk memory operations

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P2)

Description

[Luke Wagner [:luke]]

As our first producer for this new feature (\o/), Alex noticed we're currently deviating from the spec (at least overview.md) in not having any memory/table index immediates.  Right now things just work b/c there's only 1 memory/table, but in the multi-memory/table future, we need the index to say which one.  Practically, we'll always just encode 0 and fail validation if it's not 0 (or even if it is zero and there is no memory/table section).

Comment 1

[Alex Crichton [:acrichto]]

Another slight devation I think is that the spec says (for memory.init): "The instruction also has three i32 operands: an offset into the source segment, an offset into the target memory, and a length to copy"

I think in Spidermonkey though right now the order of arguments is [destination offset, source offset, length] instead of [source, destination, length]?

Comment 2

[Alex Crichton [:acrichto]]

(Also FWIW the debugger seems to crash or at least always show a completely blank pane for trying to look at the disassembly of wasm which contains a memory.init/memory.drop instruction)

Comment 3

[Julian Seward [:jseward]]

(In reply to Alex Crichton [:acrichto] from comment #1)
> I think in Spidermonkey though right now the order of arguments is
> [destination offset, source offset, length] instead of [source, destination,
> length]?

The spec was changed so as to consistently have destinations as the
leftmost argument, a la C memcpy.  See 
https://github.com/WebAssembly/bulk-memory-operations/issues/24

Comment 4

[Alex Crichton [:acrichto]]

Aha, perfect!

Comment 5

[Lars T Hansen [:lth]]

Ah, that was an oversight.  In bug 1494231 I added memory indices to "active" data and element segments; this is the other side of that coin, where instructions that process passive segments must themselves hold the index.

Comment 6

[Lars T Hansen [:lth]]

Attachment: bug1496582-mem-table-flag.patch

Simple patch: encode a zero byte when translating from text; read a byte and check that it is zero when processing binary; adjust a test case for the new binary encoding.

Comment 7

[Julian Seward [:jseward]]

Comment on attachment 9017824 [details] [diff] [review]
bug1496582-mem-table-flag.patch

Looks OK to me.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe962bfc351a
add required flags to bulk memory/table operations.  r=jseward

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/fe962bfc351a