684384 - IonMonkey: Compile break-to-labeled-scope

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Add JSOP_LABEL

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

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Add JSOP_LABEL v2

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.

Comment 3

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/49b9c222a890

Please don't close this bug, I still have to fix the IonMonkey part.

Comment 4

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/49b9c222a890

Comment 5

[Alon Zakai (:azakai)]

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)?

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Patch

Alon, can you try this patch and see if it works on other scripts?

Comment 7

[Alon Zakai (:azakai)]

Comment on attachment 710099 [details] [diff] [review]
Patch

Looks good on the emscripten test suite.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Patch

Thanks! Attaching the same patch but with an extra assert.

decoder and gary, would you guys mind fuzzing this for a while?

Comment 9

[Christian Holler (:decoder)]

I'm on it :)

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Me too.

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 711212 [details] [diff] [review]
Patch

Tentatively marking feedback+ - I didn't find anything immediately wrong after a few hours' of fuzzing.

Comment 12

[Christian Holler (:decoder)]

Comment on attachment 711212 [details] [diff] [review]
Patch

No bugs found so far :)

Comment 13

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7dd9c9b874f5

Try: https://tbpl.mozilla.org/?tree=Try&rev=7860ea2b03eb

Thanks for the fuzzing/review!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7dd9c9b874f5