baldr: require br etc. to be followed by end or else

RESOLVED FIXED in Firefox 52

Status

()

Core
JavaScript Engine: JIT
P3
enhancement
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

unspecified
mozilla53
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

Attachments

(5 attachments, 9 obsolete attachments)

62.61 KB, patch
Details | Diff | Splinter Review
32.69 KB, patch
luke
: review+
Details | Diff | Splinter Review
34.41 KB, patch
Details | Diff | Splinter Review
34.76 KB, patch
Details | Diff | Splinter Review
245.94 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

11 months ago
Created attachment 8819335 [details] [diff] [review]
br-end.patch

Attached is a patch which implement's Luke's unreachable-code proposal in design #894 [0]. This will depend on a decision being made in that PR.

This patch also updates everything in the testsuite accordingly, except the spec tests.

I initially implemented this in the manner of my Terminators prototype, where the code that handles Br/BrTable/Return/Unreachable just immediately calls the End code, inserting extra code to consume and validate the End opcode at the appropriate place. However, with the current proposal we still have to handle both Else and End, so it's less pretty. So instead, I just added a peekOp to check for Else/End in the appropriate place, which is uncomplicated and doesn't appear to be measurably slower.

[0] https://github.com/WebAssembly/design/pull/894
Currently marking P3. Feel free to update if you know when this has to land.
Priority: -- → P3

Comment 2

11 months ago
Thanks for the patch!  Can you confirm that, with the patch, we are able to validate, e.g., webassembly.org/demo and http://files.unity3d.com/jonas/benchmarks/5.5-wasm-release?
(Assignee)

Comment 3

11 months ago
webassembly.org/demo validates with this patch, however 5.5-wasm-release currently does not. In several places there are instructions between `return` and `unreachable` and the subsequent `end`.
(Assignee)

Comment 4

11 months ago
It seems likely that binaryen could be made to remove such instructions automatically. I briefly investigated, and filed https://github.com/WebAssembly/binaryen/issues/862.

Comment 5

11 months ago
Created attachment 8825573 [details] [diff] [review]
br-end.patch

(Rebased)
Attachment #8819335 - Attachment is obsolete: true
(Assignee)

Comment 6

11 months ago
As an aside, this patch just implements the proposed restriction. What it doesn't yet do is remove the full "unreachable mode" from the code. Doing that should produce a bunch of simplifications.
(Assignee)

Comment 7

11 months ago
Created attachment 8825835 [details] [diff] [review]
remove-the-mode.patch

And, here's a patch removing the unreachable mode, and associated simplifications. Interesting bits include the code in enterUnreachableCode() which pushes a type to satisfy the block signature, and WasmIonCompile's handling of calls, which needed to be reorganized a little so that we call readCallArgsEnd even in "dead code", because we now need to keep the stack up to date even in what Ion considers to be "dead code".

As with the first patch, this patch updates the tests, but not the spec tests.
(Assignee)

Comment 8

11 months ago
Created attachment 8825909 [details] [diff] [review]
baseline.patch

And, here's a patch that removes the "deadCode_" mode from RaBaldr. With this, RaBaldr just generates code for everything it sees, which is a considerable simplification. The patch adds a new "undefined" stack entry kind which gets pushed after a br/etc. so that code after a block can pop it and doesn't need to care if it's actually reachable or not.

Comment 9

11 months ago
(In reply to Dan Gohman [:sunfish] from comment #8)
> Created attachment 8825909 [details] [diff] [review]
> baseline.patch
> 
> And, here's a patch that removes the "deadCode_" mode from RaBaldr. With
> this, RaBaldr just generates code for everything it sees, which is a
> considerable simplification.

The simplification of if-then-else is particularly welcome...
(Assignee)

Comment 10

10 months ago
Created attachment 8826721 [details] [diff] [review]
br-end.patch

(Rebased)
Attachment #8825573 - Attachment is obsolete: true
(Assignee)

Comment 11

10 months ago
Created attachment 8826722 [details] [diff] [review]
remove-the-mode.patch

(Rebased)
Attachment #8825835 - Attachment is obsolete: true
(Assignee)

Comment 12

10 months ago
Created attachment 8826726 [details] [diff] [review]
baseline.patch

(Rebased)
Attachment #8825909 - Attachment is obsolete: true
(Assignee)

Comment 13

10 months ago
Created attachment 8827737 [details] [diff] [review]
br-end.patch

Requesting review for the first patch, that just adds the rule that br/br_table/return/unreachable must be immediately followed by end/else. Design #894 is still open, but for now, this patch has helped find one case where a production tool was missing opportunities to eliminate trivially unreachable code already, so checking it in may help find more.

This patch doesn't yet update the spec tests, which have many instances of trivially unreachable instructions. For now, it just disables those tests.
Attachment #8826721 - Attachment is obsolete: true
Attachment #8827737 - Flags: review?(luke)

Comment 14

10 months ago
Comment on attachment 8827737 [details] [diff] [review]
br-end.patch

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

Thanks Dan and agreed.  I think the reasons to land this are as follows:
 - what we are shipping now is way more permissive than any of the proposals in design/#894, so we have to switch to *something*
 - we have to make this change ASAP (due to upcoming release and to avoid any accidental long-term dependencies) so we can't simply wait for design/#894 to be resolved
 - this is the most conservative possible validation rule (of the set being considered), allowing backward-compatible relaxation to any of the other options in the future
 - Chakra and JSC seem in favor of this validation rule
 - landing this now allows us to vet for the group that this is indeed a viable option for design/#894

The patch is also super-minimal and easy to back out in the case that the entire group resolves design/#894 in favor of the non-transitive "unreachable mode".
Attachment #8827737 - Flags: review?(luke) → review+

Comment 15

10 months ago
Alon: this patch will require Binaryen's -O0 to enable DCE to produce wasm that validates in FF.

Comment 16

10 months ago
Pushed by dgohman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/338d4cec4ffd
BaldrMonkey: Require non-fallthrough instructions to be followed by end or else r=luke

Comment 17

10 months ago
Comment on attachment 8827737 [details] [diff] [review]
br-end.patch

Approval Request Comment
[Feature/Bug causing the regression]: spec changing
[User impact if declined]: overly permissive validation predicate
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: not yet enabled, and makes validation more restrictive

Note: when applying the cset to mozilla-aurora, ignore the failure to patch js/src/jit-test/tests/wasm/regress/baseline-joinreg.js: it's a test file that simply doesn't exist on mozilla-aurora.
Attachment #8827737 - Flags: approval-mozilla-aurora?

Comment 18

10 months ago
(In reply to Luke Wagner [:luke] from comment #15)

> Alon: this patch will require Binaryen's -O0 to enable DCE to produce wasm
> that validates in FF.

And we must recompile the embenchen programs, because those all fail in a current shell.

Comment 19

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/338d4cec4ffd
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8827737 [details] [diff] [review]
br-end.patch

wasm update for spec change, aurora52+
Attachment #8827737 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This needs to be rebased around bug 1316554.
status-firefox52: --- → affected
Flags: needinfo?(sunfish)
(Assignee)

Comment 22

10 months ago
Created attachment 8828999 [details] [diff] [review]
aurora.patch

Attached is the patch ported to mozilla-aurora.
Flags: needinfo?(sunfish)

Comment 23

10 months ago
Created attachment 8829002 [details] [diff] [review]
br-end.patch (rebased for aurora)

Oops, I tried applying the patch but not building it :P

The fix is to copy over two missing trivial helpers (peekOp() and rollbackPosition()).  Now it builds (and even passes tests).

I don't know if I'm supposed to carry forward a+ so I'll ni?RyanVM
Flags: needinfo?(ryanvm)

Comment 24

10 months ago
Hah, race condition.

Updated

10 months ago
Attachment #8829002 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8828999 - Attachment is obsolete: true
(Assignee)

Comment 25

10 months ago
Created attachment 8829006 [details] [diff] [review]
br-end.patch (ported to aurora)

Sorry for the mixup there. Here's the updated patch for mozilla-aurora.
(Assignee)

Comment 26

10 months ago
Created attachment 8829008 [details] [diff] [review]
br-end.patch (ported to aurora)

And, now updated with the correct bug number.
Attachment #8829006 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/releases/mozilla-aurora/rev/d1138edcbc38
status-firefox52: affected → fixed
Flags: in-testsuite+
(Assignee)

Comment 28

10 months ago
Created attachment 8832279 [details] [diff] [review]
baseline.patch

(Rebased)
Attachment #8826726 - Attachment is obsolete: true
(Assignee)

Comment 29

10 months ago
Created attachment 8832301 [details] [diff] [review]
spec-tests-for-br-end.patch

(And for completeness, here's a patch which updates the spec tests, in case anyone is curious about what that would look like. I aimed to preserve as much of the original intent of tests as possible. In many cases, I duplicated the tests, one with an assert_invalid testing that the original code is rejected, and one where branches etc. are wrapped in blocks so they retain their original meaning.)
(Assignee)

Updated

10 months ago
Blocks: 1336837
You need to log in before you can comment on or make changes to this bug.