Missing target memory/table index immediate for bulk memory operations

RESOLVED FIXED in Firefox 64

Status

()

P2
normal
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: luke, Assigned: lth)

Tracking

unspecified
mozilla64
Points:
---

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 months ago
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).
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]?
(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)
(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
QA Contact: ajones
Aha, perfect!
(Assignee)

Comment 5

5 months ago
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.
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Priority: -- → P3
(Assignee)

Updated

5 months ago
Blocks: 1450263
(Assignee)

Updated

5 months ago
Priority: P3 → P2
(Assignee)

Comment 6

5 months ago
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.
Attachment #9017824 - Flags: review?(jseward)
Comment on attachment 9017824 [details] [diff] [review]
bug1496582-mem-table-flag.patch

Looks OK to me.
Attachment #9017824 - Flags: review?(jseward) → review+

Comment 8

5 months ago
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe962bfc351a
add required flags to bulk memory/table operations.  r=jseward

Comment 9

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fe962bfc351a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.