Baldr: update binary format to match BinaryEncoding.md

RESOLVED FIXED in Firefox 48

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(URL)

Attachments

(26 attachments, 1 obsolete attachment)

2.32 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
29.17 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
6.20 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
19.50 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
13.69 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
5.83 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
27.64 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
67.79 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
37.67 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
16.87 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
19.78 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
23.12 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
63.17 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
14.46 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
35.32 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
20.81 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
10.16 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
6.33 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
3.75 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
5.42 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
2.52 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
18.71 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
4.73 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
16.31 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
52.43 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
14.47 KB, patch
luke
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
BinaryEncoding.md is now getting close enough to the target we can start trying to match it exactly.  Mostly just a lot of small modifications to what's already landed.
(Assignee)

Comment 1

2 years ago
Created attachment 8726036 [details] [diff] [review]
version-change
Attachment #8726036 - Flags: review?(sunfish)
(Assignee)

Comment 2

2 years ago
Created attachment 8726037 [details] [diff] [review]
section-header-1

Puts the string after and "inside" the section instead of before.  This actually makes the decoding code a bit uglier, I think it's fine for now.
Attachment #8726037 - Flags: review?(sunfish)
(Assignee)

Comment 3

2 years ago
Created attachment 8726039 [details] [diff] [review]
section-header-2

This switches the section id to be length+bytes instead of null-terminated.
Attachment #8726039 - Flags: review?(sunfish)
(Assignee)

Comment 4

2 years ago
Created attachment 8726042 [details] [diff] [review]
memory-change-1

Drops the extensible field names (which I want to propose adding in the future) and adds max (which is ignored and I would like to make optional in the future, likely in conjunction with the extensible field names).
Attachment #8726042 - Flags: review?(sunfish)

Updated

2 years ago
Attachment #8726036 - Flags: review?(sunfish) → review+

Updated

2 years ago
Attachment #8726039 - Flags: review?(sunfish) → review+

Updated

2 years ago
Attachment #8726042 - Flags: review?(sunfish) → review+

Updated

2 years ago
Attachment #8726037 - Flags: review?(sunfish) → review+
(Assignee)

Updated

2 years ago
Whiteboard: [leave open]
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=22920510&repo=mozilla-inbound
Flags: needinfo?(luke)
(Assignee)

Comment 8

2 years ago
D'oh!  That's what I get for thinking a patch is platform-independent enough not to need to try: the patch intentionally allows zero-sized memory, but for the first time and this was causing VirtualProtect to fail on Win64 when given a 0 length.  Fix is to just not call VirtualProtect when numBytes=0 since it's a nop anyway.
Flags: needinfo?(luke)
(Assignee)

Comment 10

2 years ago
Created attachment 8726581 [details] [diff] [review]
memory-change-2

:(  But tentative agreement to switch back after demo milestone.
Attachment #8726581 - Flags: review?(sunfish)
Comment on attachment 8726581 [details] [diff] [review]
memory-change-2

Review of attachment 8726581 [details] [diff] [review]:
-----------------------------------------------------------------

:( yes. But this looks ok for now.
Attachment #8726581 - Flags: review?(sunfish) → review+
(Assignee)

Comment 12

2 years ago
Created attachment 8726596 [details] [diff] [review]
unknown-sections

Unknown-section checking was too lenient.
Attachment #8726596 - Flags: review?(sunfish)
(Assignee)

Comment 13

2 years ago
Created attachment 8726597 [details] [diff] [review]
section-ids

This syncs up all the section-ids with BinaryEncoding.md except for "functions" which has yet to be split in BinaryEncoding.md.  It also removes some of the generic field support which I think we'll want to add back later.
Attachment #8726597 - Flags: review?(sunfish)

Updated

2 years ago
Attachment #8726596 - Flags: review?(sunfish) → review+

Updated

2 years ago
Attachment #8726597 - Flags: review?(sunfish) → review+
(Assignee)

Comment 17

2 years ago
Created attachment 8726936 [details] [diff] [review]
rm-expected-type

To be able to remove the number-of-expressions from the function body, we need to remove the top-down expected-type propagation in WasmIonCompile.  This is almost trivial (just removing a bunch of asserts) except for If/IfElse which used that expected type to decide whether to push/pop to propagate the result value.  When we add values to br/br_if/br_table, we'll have the same problem and probably reuse the same addJoinPredecessor function added here.
Attachment #8726936 - Flags: review?(sunfish)
Comment on attachment 8726936 [details] [diff] [review]
rm-expected-type

Review of attachment 8726936 [details] [diff] [review]:
-----------------------------------------------------------------

I like how, for the most part, things get simpler :).
Attachment #8726936 - Flags: review?(sunfish) → review+
(Assignee)

Comment 19

2 years ago
Created attachment 8726986 [details] [diff] [review]
bottom-up-types

Here is the symmetric change in Wasm.cpp.  Things do get a bit more complicated, so I would say the top-down formulation was simpler overall, but we're going postorder anyway so this is a step in the right direction.  Some of the error messages change because (1) there were two errors and we find the other one first now, (2) when "unifying" two types bottom up that are different, you get void and then that fails in the parent context with "void"; with top-down you'd push the parent's desired type down to each branch and fail with both specific types.
Attachment #8726986 - Flags: review?(sunfish)
(Assignee)

Comment 20

2 years ago
Created attachment 8726991 [details] [diff] [review]
change-func-body

And with those two previous changes it's now possible to remove the number-of-expressions count from the body and decode things bottom-up using only the body's byte count.
Attachment #8726991 - Flags: review?(sunfish)

Updated

2 years ago
Attachment #8726986 - Flags: review?(sunfish) → review+

Updated

2 years ago
Attachment #8726991 - Flags: review?(sunfish) → review+
(Assignee)

Comment 22

2 years ago
Created attachment 8727036 [details] [diff] [review]
locals-in-body

This patch moves locals into the body which then makes it natural to local-types vector out of FunctionBytecode.
Attachment #8727036 - Flags: review?(sunfish)
(Assignee)

Comment 23

2 years ago
Created attachment 8727039 [details] [diff] [review]
uint8-array

This matches v8 and it's a better design anyway since a view lets you subset a bigger buffer.
Attachment #8727039 - Flags: review?(sunfish)
(Assignee)

Comment 24

2 years ago
Created attachment 8727103 [details] [diff] [review]
locals-in-body
Attachment #8727036 - Attachment is obsolete: true
Attachment #8727036 - Flags: review?(sunfish)
Attachment #8727103 - Flags: review?(sunfish)
(Assignee)

Comment 25

2 years ago
Created attachment 8727104 [details] [diff] [review]
value-opcodes

This patch moves ValType/ExprType to WasmBinary.h (which switches the header dependency order).  The patch also tides up WasmBinary.h a bit.
Attachment #8727104 - Flags: review?(sunfish)

Updated

2 years ago
Attachment #8727039 - Flags: review?(sunfish) → review+

Updated

2 years ago
Attachment #8727103 - Flags: review?(sunfish) → review+
Comment on attachment 8727104 [details] [diff] [review]
value-opcodes

Review of attachment 8727104 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/WasmBinary.h
@@ +44,2 @@
>  {
> +    I32                                  = 0x01,

It'd be good to add a comment mentioning that 0x00 is skipped to leave encoding space in ExprType for Void (while it exists).

::: js/src/jit-test/tests/wasm/binary.js
@@ +34,1 @@
>  const I32x4Code = 4;

F64Code and I32x4Code are both 4 here.
Attachment #8727104 - Flags: review?(sunfish) → review+
(Assignee)

Comment 29

2 years ago
Created attachment 8727143 [details] [diff] [review]
group-locals

Implements the "Local Entry" (https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#local-entry) design (instead of the current simple list of value types).
Attachment #8727143 - Flags: review?(sunfish)
(Assignee)

Comment 31

2 years ago
Created attachment 8727190 [details] [diff] [review]
rename-to-bytes

This patch does some simple renaming/refactoring in preparation for the next patch.
Attachment #8727190 - Flags: review?(sunfish)
(Assignee)

Comment 32

2 years ago
Created attachment 8727191 [details] [diff] [review]
byte-strings

This is the last remaining change that I know of \o/
Attachment #8727191 - Flags: review?(sunfish)

Updated

2 years ago
Attachment #8727143 - Flags: review?(sunfish) → review+

Updated

2 years ago
Attachment #8727190 - Flags: review?(sunfish) → review+
Comment on attachment 8727191 [details] [diff] [review]
byte-strings

Review of attachment 8727191 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/Wasm.cpp
@@ +1155,5 @@
>          return nullptr;
>      }
>  
> +    if (!fieldBytes.append(0))
> +        return false;

It'd be good to memchr for a nul byte in the middle of a string here, so that we can fail gracefully for now rather than a potential mysterious failure.
Attachment #8727191 - Flags: review?(sunfish) → review+
(Assignee)

Comment 35

2 years ago
Created attachment 8727295 [details] [diff] [review]
sleb128

Actually, I just noticed that BinaryEncoding.md already has SLEB128 for i32.const and i64.const.
Attachment #8727295 - Flags: review?(sunfish)
Comment on attachment 8727295 [details] [diff] [review]
sleb128

Review of attachment 8727295 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/AsmJS.cpp
@@ +2819,5 @@
>      Encoder& encoder() { return *encoder_; }
>  
>      MOZ_WARN_UNUSED_RESULT bool writeInt32Lit(int32_t i32) {
>          return encoder().writeExpr(Expr::I32Const) &&
> +               encoder().writeVarS32(i32);

writeInt32Lit is used for some things which I wouldn't expect to use SLEB. For example br_table table indices, which Wasm.cpp still decodes as unsigned LEB.
Attachment #8727295 - Flags: review?(sunfish)
Comment on attachment 8727295 [details] [diff] [review]
sleb128

Review of attachment 8727295 [details] [diff] [review]:
-----------------------------------------------------------------

Ah; I was mistaken. The usage for br_table and so on are for cases where the asm.js lowering is using actual i32.const nodes. LGTM.
Attachment #8727295 - Flags: review+
(Assignee)

Comment 40

2 years ago
Created attachment 8727475 [details] [diff] [review]
br-value

Updating to match sexpr-wasm for now.  Easy enough to change to an immediate arity later.
Attachment #8727475 - Flags: review?(sunfish)
(Assignee)

Comment 41

2 years ago
Created attachment 8727568 [details] [diff] [review]
exports-obj

V8 has apparently already made this change.
Attachment #8727568 - Flags: review?(sunfish)
(Assignee)

Comment 42

2 years ago
Created attachment 8727582 [details] [diff] [review]
br-table-syntax

This update mirrors today's ml-proto br_table merge, which now has this much-simpler syntax:
  https://github.com/WebAssembly/spec/blob/master/ml-proto/test/switch.wast#L16
Attachment #8727582 - Flags: review?(sunfish)

Updated

2 years ago
Attachment #8727475 - Flags: review?(sunfish) → review+

Updated

2 years ago
Attachment #8727568 - Flags: review?(sunfish) → review+

Updated

2 years ago
Attachment #8727582 - Flags: review?(sunfish) → review+
(Assignee)

Comment 43

2 years ago
Created attachment 8727630 [details] [diff] [review]
func-body-count
Attachment #8727630 - Flags: review?(sunfish)
Comment on attachment 8727630 [details] [diff] [review]
func-body-count

Review of attachment 8727630 [details] [diff] [review]:
-----------------------------------------------------------------

It'd be nice to add a comment here pointing out that the count here is redundant, but serves to keep the encoding of lists regular.
Attachment #8727630 - Flags: review?(sunfish) → review+
(Assignee)

Comment 45

2 years ago
I could, but generally we're not explaining the rationale for the spec in the impl; also, one can see from the validation equality test that numFuncBodies is redundant.
Makes sense. I found it surprising, and initially wondered if there was something subtle I was missing, but you're right that we don't need to explain the rationale for the spec here.
(Assignee)

Comment 47

2 years ago
Created attachment 8727719 [details] [diff] [review]
fix-if

To match ml-proto/Binaryen updates.
Attachment #8727719 - Flags: review?(sunfish)

Updated

2 years ago
Attachment #8727719 - Flags: review?(sunfish) → review+
(Assignee)

Comment 49

2 years ago
Created attachment 8728124 [details] [diff] [review]
br-table-imm

(Probably want to re-visit and re-question the motivations for this later...)
Attachment #8728124 - Flags: review?(sunfish)
(Assignee)

Comment 50

2 years ago
Created attachment 8728127 [details] [diff] [review]
memory-immediates

From design/#592: encode alignment as power-of-2 up to natural alignment.
Attachment #8728127 - Flags: review?(sunfish)

Updated

2 years ago
Attachment #8728124 - Flags: review?(sunfish) → review+

Updated

2 years ago
Attachment #8728127 - Flags: review?(sunfish) → review+
(Assignee)

Comment 54

2 years ago
Created attachment 8728541 [details] [diff] [review]
change-expr-encoding-scheme

Mostly pretty simple change.  Unlike LEB128, the prefix scheme does not allow for putting a 1-byte opcode into a 2-byte encoding.  Fortunately, all the patching only cared about 1-byte ops, so a hack could be made.
Attachment #8728541 - Flags: review?(sunfish)

Updated

2 years ago
Attachment #8728541 - Flags: review?(sunfish) → review+
Created attachment 8728768 [details] [diff] [review]
wasm-eqz-rotate.patch

This adds the rotate and eqz operators.
Attachment #8728768 - Flags: review?(luke)
(Assignee)

Updated

2 years ago
Attachment #8728768 - Flags: review?(luke) → review+
(Assignee)

Comment 58

2 years ago
Looks like the bits have stabilized and we're done here :)
Whiteboard: [leave open]

Comment 60

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b401844a547f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.