Closed
Bug 1253137
Opened 8 years ago
Closed 8 years ago
Baldr: update binary format to match BinaryEncoding.md
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
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.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8726036 -
Flags: review?(sunfish)
Assignee | ||
Comment 2•8 years ago
|
||
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•8 years ago
|
||
This switches the section id to be length+bytes instead of null-terminated.
Attachment #8726039 -
Flags: review?(sunfish)
Assignee | ||
Comment 4•8 years ago
|
||
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•8 years ago
|
Attachment #8726036 -
Flags: review?(sunfish) → review+
Updated•8 years ago
|
Attachment #8726039 -
Flags: review?(sunfish) → review+
Updated•8 years ago
|
Attachment #8726042 -
Flags: review?(sunfish) → review+
Updated•8 years ago
|
Attachment #8726037 -
Flags: review?(sunfish) → review+
Assignee | ||
Updated•8 years ago
|
Whiteboard: [leave open]
https://hg.mozilla.org/integration/mozilla-inbound/rev/e68e5d3f9101 https://hg.mozilla.org/integration/mozilla-inbound/rev/196acfbcffbb https://hg.mozilla.org/integration/mozilla-inbound/rev/6705f67ab798 https://hg.mozilla.org/integration/mozilla-inbound/rev/13ce4c5281ad
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/7972152f5978 https://hg.mozilla.org/integration/mozilla-inbound/rev/184d1803b4e2 https://hg.mozilla.org/integration/mozilla-inbound/rev/6fcdd3dfb29c https://hg.mozilla.org/integration/mozilla-inbound/rev/fcae5c32f48f
Comment 7•8 years ago
|
||
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=22920510&repo=mozilla-inbound
Flags: needinfo?(luke)
Assignee | ||
Comment 8•8 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)
https://hg.mozilla.org/integration/mozilla-inbound/rev/53cb9d108ec5 https://hg.mozilla.org/integration/mozilla-inbound/rev/56a5e1a49781 https://hg.mozilla.org/integration/mozilla-inbound/rev/6257b3a68cfd https://hg.mozilla.org/integration/mozilla-inbound/rev/029e7b380dac
Assignee | ||
Comment 10•8 years ago
|
||
:( But tentative agreement to switch back after demo milestone.
Attachment #8726581 -
Flags: review?(sunfish)
Comment 11•8 years ago
|
||
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•8 years ago
|
||
Unknown-section checking was too lenient.
Attachment #8726596 -
Flags: review?(sunfish)
Assignee | ||
Comment 13•8 years ago
|
||
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•8 years ago
|
Attachment #8726596 -
Flags: review?(sunfish) → review+
Updated•8 years ago
|
Attachment #8726597 -
Flags: review?(sunfish) → review+
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/53cb9d108ec5 https://hg.mozilla.org/mozilla-central/rev/56a5e1a49781 https://hg.mozilla.org/mozilla-central/rev/6257b3a68cfd https://hg.mozilla.org/mozilla-central/rev/029e7b380dac
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/374edef94056 https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd419f5a9d0 https://hg.mozilla.org/integration/mozilla-inbound/rev/e3d938a82d2c
Assignee | ||
Comment 17•8 years ago
|
||
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 18•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
Attachment #8726986 -
Flags: review?(sunfish) → review+
Updated•8 years ago
|
Attachment #8726991 -
Flags: review?(sunfish) → review+
Comment 21•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cf0ffedcb77 https://hg.mozilla.org/integration/mozilla-inbound/rev/46b17b64ec46 https://hg.mozilla.org/integration/mozilla-inbound/rev/d0c4157a7fc6
Assignee | ||
Comment 22•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
Attachment #8727036 -
Attachment is obsolete: true
Attachment #8727036 -
Flags: review?(sunfish)
Attachment #8727103 -
Flags: review?(sunfish)
Assignee | ||
Comment 25•8 years ago
|
||
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•8 years ago
|
Attachment #8727039 -
Flags: review?(sunfish) → review+
Updated•8 years ago
|
Attachment #8727103 -
Flags: review?(sunfish) → review+
Comment 26•8 years ago
|
||
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+
Comment 27•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/60fb79d430d8 https://hg.mozilla.org/integration/mozilla-inbound/rev/939b709ea842 https://hg.mozilla.org/integration/mozilla-inbound/rev/004f09562d01
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/374edef94056 https://hg.mozilla.org/mozilla-central/rev/6bd419f5a9d0 https://hg.mozilla.org/mozilla-central/rev/e3d938a82d2c https://hg.mozilla.org/mozilla-central/rev/da1eeb4a0dd0 https://hg.mozilla.org/mozilla-central/rev/3cf0ffedcb77 https://hg.mozilla.org/mozilla-central/rev/46b17b64ec46 https://hg.mozilla.org/mozilla-central/rev/d0c4157a7fc6
Assignee | ||
Comment 29•8 years ago
|
||
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)
Comment 30•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/60fb79d430d8 https://hg.mozilla.org/mozilla-central/rev/939b709ea842 https://hg.mozilla.org/mozilla-central/rev/004f09562d01
Assignee | ||
Comment 31•8 years ago
|
||
This patch does some simple renaming/refactoring in preparation for the next patch.
Attachment #8727190 -
Flags: review?(sunfish)
Assignee | ||
Comment 32•8 years ago
|
||
This is the last remaining change that I know of \o/
Attachment #8727191 -
Flags: review?(sunfish)
Updated•8 years ago
|
Attachment #8727143 -
Flags: review?(sunfish) → review+
Updated•8 years ago
|
Attachment #8727190 -
Flags: review?(sunfish) → review+
Comment 33•8 years ago
|
||
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+
Comment 34•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/57e9d3626218 https://hg.mozilla.org/integration/mozilla-inbound/rev/5eabc3a7368a https://hg.mozilla.org/integration/mozilla-inbound/rev/0d9926dd10ea
Assignee | ||
Comment 35•8 years ago
|
||
Actually, I just noticed that BinaryEncoding.md already has SLEB128 for i32.const and i64.const.
Attachment #8727295 -
Flags: review?(sunfish)
Comment 36•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/57e9d3626218 https://hg.mozilla.org/mozilla-central/rev/5eabc3a7368a https://hg.mozilla.org/mozilla-central/rev/0d9926dd10ea
Comment 37•8 years ago
|
||
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 38•8 years ago
|
||
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•8 years ago
|
||
Updating to match sexpr-wasm for now. Easy enough to change to an immediate arity later.
Attachment #8727475 -
Flags: review?(sunfish)
Assignee | ||
Comment 41•8 years ago
|
||
V8 has apparently already made this change.
Attachment #8727568 -
Flags: review?(sunfish)
Assignee | ||
Comment 42•8 years ago
|
||
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•8 years ago
|
Attachment #8727475 -
Flags: review?(sunfish) → review+
Updated•8 years ago
|
Attachment #8727568 -
Flags: review?(sunfish) → review+
Updated•8 years ago
|
Attachment #8727582 -
Flags: review?(sunfish) → review+
Assignee | ||
Comment 43•8 years ago
|
||
Attachment #8727630 -
Flags: review?(sunfish)
Comment 44•8 years ago
|
||
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•8 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.
Comment 46•8 years ago
|
||
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•8 years ago
|
||
To match ml-proto/Binaryen updates.
Attachment #8727719 -
Flags: review?(sunfish)
Updated•8 years ago
|
Attachment #8727719 -
Flags: review?(sunfish) → review+
Comment 48•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/970c0c8816a5 https://hg.mozilla.org/integration/mozilla-inbound/rev/ca09c67724cb https://hg.mozilla.org/integration/mozilla-inbound/rev/8fa44c40bd00 https://hg.mozilla.org/integration/mozilla-inbound/rev/b2ec1e1d2fe9 https://hg.mozilla.org/integration/mozilla-inbound/rev/bf186476cb90
Assignee | ||
Comment 49•8 years ago
|
||
(Probably want to re-visit and re-question the motivations for this later...)
Attachment #8728124 -
Flags: review?(sunfish)
Assignee | ||
Comment 50•8 years ago
|
||
From design/#592: encode alignment as power-of-2 up to natural alignment.
Attachment #8728127 -
Flags: review?(sunfish)
Comment 51•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c2b008a0c7cc
Updated•8 years ago
|
Attachment #8728124 -
Flags: review?(sunfish) → review+
Updated•8 years ago
|
Attachment #8728127 -
Flags: review?(sunfish) → review+
Comment 52•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeed1a47818e https://hg.mozilla.org/integration/mozilla-inbound/rev/21d121d42c75
Comment 53•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/970c0c8816a5 https://hg.mozilla.org/mozilla-central/rev/ca09c67724cb https://hg.mozilla.org/mozilla-central/rev/8fa44c40bd00 https://hg.mozilla.org/mozilla-central/rev/b2ec1e1d2fe9 https://hg.mozilla.org/mozilla-central/rev/bf186476cb90 https://hg.mozilla.org/mozilla-central/rev/eeed1a47818e https://hg.mozilla.org/mozilla-central/rev/21d121d42c75
Assignee | ||
Comment 54•8 years ago
|
||
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•8 years ago
|
Attachment #8728541 -
Flags: review?(sunfish) → review+
Comment 56•8 years ago
|
||
This adds the rotate and eqz operators.
Attachment #8728768 -
Flags: review?(luke)
Comment 57•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/99dfc9704fce
Assignee | ||
Updated•8 years ago
|
Attachment #8728768 -
Flags: review?(luke) → review+
Assignee | ||
Comment 58•8 years ago
|
||
Looks like the bits have stabilized and we're done here :)
Whiteboard: [leave open]
Comment 60•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b401844a547f
Status: ASSIGNED → RESOLVED
Closed: 8 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.
Description
•