Closed
Bug 1522081
Opened 6 years ago
Closed 6 years ago
Update memory.init and table.init encodings
Categories
(Core :: JavaScript: WebAssembly, enhancement, P3)
Core
JavaScript: WebAssembly
Tracking
()
RESOLVED
FIXED
mozilla66
| Tracking | Status | |
|---|---|---|
| firefox66 | --- | fixed |
People
(Reporter: lth, Assigned: lth)
References
Details
Attachments
(1 file)
|
(deleted),
patch
|
jseward
:
review+
|
Details | Diff | Splinter Review |
The current spec puts the data/elem segment index before the target memory/table index, and makes both fields mandatory, and does not leave space for any flags. So we should rearrange the order in which we read the fields and we read them unconditionally. Otherwise there should be no functional change.
|
Assignee | |
Comment 1•6 years ago
|
||
Attachment #9038484 -
Flags: review?(jseward)
|
|
Assignee | |
Comment 2•6 years ago
|
||
I should mention, this sits on top of my patch for bug 1521780.
|
||
Comment 3•6 years ago
|
||
Comment on attachment 9038484 [details] [diff] [review]
bug1522081-encode-memory-and-table-init.patch
Review of attachment 9038484 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/wasm/WasmTextToBinary.cpp
@@ +6350,5 @@
> return EncodeExpr(e, s.dst()) && EncodeExpr(e, s.src()) &&
> EncodeExpr(e, s.len()) &&
> e.writeOp(s.isMem() ? MiscOp::MemInit : MiscOp::TableInit) &&
> + e.writeVarU32(s.segIndex()) &&
> + e.writeVarU32(s.target().index());
A query that's more about the spec than the implementation. From this patch it's obvious that the intended type of both fields is "varuint32". But the spec has
memory.init = FC 08 segment:varuint32 memory:0x00
table.init = FC 0C segment:varuint32 table:0x00
and it's unclear (to me, at least) whether the value 0x00 uniquely implies a type of varuint32. How do we know for example that it isn't a uint8 ? I'd be happier if the spec indicated varuint32 for both fields.
Attachment #9038484 -
Flags: review?(jseward) → review+
|
|
Assignee | |
Comment 4•6 years ago
|
||
Good point. I'll ask for spec clarification.
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f632c68b2052
Update instruction encodings for memory.init / table.init. r=jseward
|
||
Comment 6•6 years ago
|
||
| bugherder | ||
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
|
||
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•