Closed Bug 684384 Opened 9 years ago Closed 7 years ago

IonMonkey: Compile break-to-labeled-scope

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: dvander, Assigned: jandem)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [ion:t])

Attachments

(2 files, 2 obsolete files)

From bug 684037:

We can't compile ecma_2/Statements/while-004.js because it contains something like:

>  label: {
>      while (cond) {
>          break label;
>      }
>      A;
>  }
>  B;

We precompute the successors of loops and use that to detect nesting of breaks. However, in this case, the break does not go to a loop but a statement outside of the loop. The control-flow is still structured, but we didn't recognize the structure ahead of time.

The best fix would be to introduce a new opcode indicating that we are introducing a label and what the successor of the label is.
Assignee: general → jandemooij
Status: NEW → ASSIGNED
Attached patch Add JSOP_LABEL (obsolete) — Splinter Review
This patch adds the JSOP_LABEL and JSOP_LABELX opcodes. I'd like to land this on mozilla-central first to make merges a bit easier.

For this snippet:
--
label:
while(true) {
    break label;
}
return 0;
--
We generate the following bytecode:

00000:  label 17 (+17)
00003:  goto 13 (+10)
00006:  trace 0
00009:  goto 17 (+8)
00012:  nullblockchain
00013:  true
00014:  ifne 6 (-8)
00017:  zero
00018:  return
Attachment #570788 - Flags: review?(dvander)
Attachment #570788 - Flags: review?(dvander) → review+
Just noticed that in this case the JSOP_GOTO for the "break L" points to the JSOP_NOP for the end brace:
--
L: {
    break L;
}
--
The JSOP_LABEL instruction points to the instruction following the JSOP_NOP. The simplest fix is to patch JSOP_LABEL before we emit the JSOP_NOP.
Attachment #570788 - Attachment is obsolete: true
Attachment #570985 - Flags: review?(dvander)
Attachment #570985 - Flags: review?(dvander) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/49b9c222a890

Please don't close this bug, I still have to fix the IonMonkey part.
Whiteboard: [ion:t]
Blocks: 837832
As mentioned in bug 837832 that jandem just linked, implementing this could allow emscripten to generate smaller code, by labeling ifs instead of adding new loops with labels, which would make code smaller and less nested. Is this still planned (I see the last activity here is over a year ago)?
Attached patch Patch (obsolete) — Splinter Review
Alon, can you try this patch and see if it works on other scripts?
Attachment #710099 - Flags: feedback?(azakai)
Comment on attachment 710099 [details] [diff] [review]
Patch

Looks good on the emscripten test suite.
Attachment #710099 - Flags: feedback?(azakai) → feedback+
Attached patch PatchSplinter Review
Thanks! Attaching the same patch but with an extra assert.

decoder and gary, would you guys mind fuzzing this for a while?
Attachment #710099 - Attachment is obsolete: true
Attachment #711212 - Flags: review?(dvander)
Attachment #711212 - Flags: feedback?(gary)
Attachment #711212 - Flags: feedback?(choller)
I'm on it :)
Comment on attachment 711212 [details] [diff] [review]
Patch

Tentatively marking feedback+ - I didn't find anything immediately wrong after a few hours' of fuzzing.
Attachment #711212 - Flags: feedback?(gary) → feedback+
Comment on attachment 711212 [details] [diff] [review]
Patch

No bugs found so far :)
Attachment #711212 - Flags: feedback?(choller) → feedback+
Attachment #711212 - Flags: review?(dvander) → review+
https://hg.mozilla.org/mozilla-central/rev/7dd9c9b874f5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.