Closed Bug 1253137 Opened 4 years ago Closed 4 years ago

Baldr: update binary format to match BinaryEncoding.md

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: luke, Assigned: luke)

References

()

Details

Attachments

(26 files, 1 obsolete file)

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
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.
Attached patch version-changeSplinter Review
Attachment #8726036 - Flags: review?(sunfish)
Attached patch section-header-1Splinter Review
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)
Attached patch section-header-2Splinter Review
This switches the section id to be length+bytes instead of null-terminated.
Attachment #8726039 - Flags: review?(sunfish)
Attached patch memory-change-1Splinter Review
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)
Attachment #8726036 - Flags: review?(sunfish) → review+
Attachment #8726039 - Flags: review?(sunfish) → review+
Attachment #8726042 - Flags: review?(sunfish) → review+
Attachment #8726037 - Flags: review?(sunfish) → review+
Whiteboard: [leave open]
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)
Attached patch memory-change-2Splinter Review
:(  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+
Attached patch unknown-sectionsSplinter Review
Unknown-section checking was too lenient.
Attachment #8726596 - Flags: review?(sunfish)
Attached patch section-idsSplinter Review
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)
Attachment #8726596 - Flags: review?(sunfish) → review+
Attachment #8726597 - Flags: review?(sunfish) → review+
Attached patch rm-expected-typeSplinter Review
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+
Attached patch bottom-up-typesSplinter Review
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)
Attached patch change-func-bodySplinter Review
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)
Attachment #8726986 - Flags: review?(sunfish) → review+
Attachment #8726991 - Flags: review?(sunfish) → review+
Attached patch locals-in-body (obsolete) — Splinter Review
This patch moves locals into the body which then makes it natural to local-types vector out of FunctionBytecode.
Attachment #8727036 - Flags: review?(sunfish)
Attached patch uint8-arraySplinter Review
This matches v8 and it's a better design anyway since a view lets you subset a bigger buffer.
Attachment #8727039 - Flags: review?(sunfish)
Attached patch locals-in-bodySplinter Review
Attachment #8727036 - Attachment is obsolete: true
Attachment #8727036 - Flags: review?(sunfish)
Attachment #8727103 - Flags: review?(sunfish)
Attached patch value-opcodesSplinter Review
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)
Attachment #8727039 - Flags: review?(sunfish) → review+
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+
Attached patch group-localsSplinter Review
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)
Attached patch rename-to-bytesSplinter Review
This patch does some simple renaming/refactoring in preparation for the next patch.
Attachment #8727190 - Flags: review?(sunfish)
Attached patch byte-stringsSplinter Review
This is the last remaining change that I know of \o/
Attachment #8727191 - Flags: review?(sunfish)
Attachment #8727143 - Flags: review?(sunfish) → review+
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+
Attached patch sleb128Splinter Review
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+
Attached patch br-valueSplinter Review
Updating to match sexpr-wasm for now.  Easy enough to change to an immediate arity later.
Attachment #8727475 - Flags: review?(sunfish)
Attached patch exports-objSplinter Review
V8 has apparently already made this change.
Attachment #8727568 - Flags: review?(sunfish)
Attached patch br-table-syntaxSplinter Review
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)
Attachment #8727475 - Flags: review?(sunfish) → review+
Attachment #8727568 - Flags: review?(sunfish) → review+
Attachment #8727582 - Flags: review?(sunfish) → review+
Attached patch func-body-countSplinter Review
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+
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.
Attached patch fix-ifSplinter Review
To match ml-proto/Binaryen updates.
Attachment #8727719 - Flags: review?(sunfish)
Attachment #8727719 - Flags: review?(sunfish) → review+
Attached patch br-table-immSplinter Review
(Probably want to re-visit and re-question the motivations for this later...)
Attachment #8728124 - Flags: review?(sunfish)
From design/#592: encode alignment as power-of-2 up to natural alignment.
Attachment #8728127 - Flags: review?(sunfish)
Attachment #8728124 - Flags: review?(sunfish) → review+
Attachment #8728127 - Flags: review?(sunfish) → review+
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)
Attachment #8728541 - Flags: review?(sunfish) → review+
This adds the rotate and eqz operators.
Attachment #8728768 - Flags: review?(luke)
Attachment #8728768 - Flags: review?(luke) → review+
Looks like the bits have stabilized and we're done here :)
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/b401844a547f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.