JumpIfFalse skips over PopEnvironment instruction

[HalidOdat]

Describe the bug
We do not handle pushing and popping of environments correctly in the bytecompiler.

To Reproduce

let i = 0;
while (i < 10) {
    if (i == 3) {
        break;
    }
    i++;
}

The following instructions are generated:

----------------------Compiled Output: '<main>'-----------------------
Location  Count   Opcode                     Operands

000000    0000    PushZero
000001    0001    DefInitLet                 0000: 'i'
000006    0002    LoopStart
000007    0003    LoopContinue
000008    0004    GetName                    0000: 'i'
000013    0005    PushInt8                   10
000015    0006    LessThan
000016    0007    JumpIfFalse                78
000021    0008    PushDeclarativeEnvironment 0, 1
000030    0009    GetName                    0000: 'i'
000035    0010    PushInt8                   3
000037    0011    Eq
000038    0012    JumpIfFalse                58
000043    0013    PushDeclarativeEnvironment 0, 0
000052    0014    Jump                       78
000057    0015    PopEnvironment
000058    0016    GetName                    0000: 'i'
000063    0017    IncPost
000064    0018    RotateRight                2
000066    0019    SetName                    0000: 'i'
000071    0020    Pop
000072    0021    PopEnvironment
000073    0022    Jump                       7
000078    0023    LoopEnd

Literals:
    <empty>

Bindings:
    0000: i

Functions:
    <empty>

Actual behavior
The 0038: JumpIfFalse instruction is jumping to instruction 58, skipping the 0057: PopEnvironment, and the plain 0073: Jump also skips it by jumping to the end of the function.

Expected behavior
The PopEnvironment instruction should be executed.

Build environment (please complete the following information):

• OS: Arch Linux
• Version: Rolling Release (kernel version 5.15.73-1-lts)
• Target triple: x86_64-unknown-linux-gnu
• Rustc version: rustc 1.65.0 (897e37553 2022-11-02)

Additional context

Here is the flow graphs generated by #2422 :

We can see that there is a PopEnvironment that is never executed (since there is nothing that flows into it).

[HalidOdat]

On closer inspection even this code reproduces this type of bug:

while (true) {
     break;
}

Compiled and trace output

----------------------Compiled Output: ''-----------------------
Location  Count   Opcode                     Operands

000000    0000    LoopStart
000001    0001    LoopContinue
000002    0002    PushTrue
000003    0003    JumpIfFalse                28
000008    0004    PushDeclarativeEnvironment 0, 0
000017    0005    Jump                       28
000022    0006    PopEnvironment
000023    0007    Jump                       1
000028    0008    LoopEnd

Literals:
    

Bindings:
    

Functions:
    


----------------------------------------- Call Frame -----------------------------------------
Time          Opcode                     Operands                   Top Of Stack

7μs           LoopStart                                             
2μs           LoopContinue                                          
6μs           PushTrue                                              true
3μs           JumpIfFalse                28                         
16μs          PushDeclarativeEnvironment 0, 0                       
2μs           Jump                       28                         
3μs           LoopEnd                                               

Stack:
    


undefined

We can see that it never calls the PopEnvironment instruction

Flowgraph

So the problem seems to be the way we handle break statements.

[nekevss]

So I was playing around with this and I think it may have something to do with how the ByteCompiler is handling PopEnvironment in general when loops are involved.

Below is the trace for

for (let i = 0; i < 10; i++) {
    if (i == 3) {
        break;
    }
}

----------------------Compiled Output: '<main>'-----------------------
Location  Count   Opcode                     Operands

000000    0000    PushDeclarativeEnvironment 1, 2
000009    0001    PushZero                   
000010    0002    DefInitLet                 0000: 'i'
000015    0003    LoopStart                  
000016    0004    Jump                       36
000021    0005    LoopContinue               
000022    0006    GetName                    0000: 'i'
000027    0007    IncPost                    
000028    0008    RotateRight                2
000030    0009    SetName                    0000: 'i'
000035    0010    Pop                        
000036    0011    GetName                    0000: 'i'
000041    0012    PushInt8                   10
000043    0013    LessThan                   
000044    0014    JumpIfFalse                92
000049    0015    PushDeclarativeEnvironment 0, 1
000058    0016    GetName                    0000: 'i'
000063    0017    PushInt8                   2
000065    0018    Eq                         
000066    0019    JumpIfFalse                86
000071    0020    PushDeclarativeEnvironment 0, 0
000080    0021    Jump                       92
000085    0022    PopEnvironment             
000086    0023    PopEnvironment             
000087    0024    Jump                       21
000092    0025    LoopEnd                    
000093    0026    PopEnvironment             

Literals:
    <empty>

Bindings:
    0000: i

Functions:
    <empty>


----------------------------------------- Call Frame -----------------------------------------
Time          Opcode                     Operands                   Top Of Stack

14╬╝s          PushDeclarativeEnvironment 1, 2                       <empty>
1╬╝s           PushZero                                              0
4╬╝s           DefInitLet                 0000: 'i'                  <empty>
3╬╝s           LoopStart                                             <empty>
1╬╝s           Jump                       36                         <empty>
2╬╝s           GetName                    0000: 'i'                  0
0╬╝s           PushInt8                   10                         10
3╬╝s           LessThan                                              true
0╬╝s           JumpIfFalse                92                         <empty>
2╬╝s           PushDeclarativeEnvironment 0, 1                       <empty>
1╬╝s           GetName                    0000: 'i'                  0
0╬╝s           PushInt8                   2                          2
3╬╝s           Eq                                                    false
0╬╝s           JumpIfFalse                86                         <empty>
0╬╝s           PopEnvironment                                        <empty>
0╬╝s           Jump                       21                         <empty>
0╬╝s           LoopContinue                                          <empty>
0╬╝s           GetName                    0000: 'i'                  0
4╬╝s           IncPost                                               0
4╬╝s           RotateRight                2                          1
3╬╝s           SetName                    0000: 'i'                  0
0╬╝s           Pop                                                   <empty>
0╬╝s           GetName                    0000: 'i'                  1
0╬╝s           PushInt8                   10                         10
2╬╝s           LessThan                                              true
0╬╝s           JumpIfFalse                92                         <empty>
2╬╝s           PushDeclarativeEnvironment 0, 1                       <empty>
0╬╝s           GetName                    0000: 'i'                  1
0╬╝s           PushInt8                   2                          2
0╬╝s           Eq                                                    false
0╬╝s           JumpIfFalse                86                         <empty>
0╬╝s           PopEnvironment                                        <empty>
0╬╝s           Jump                       21                         <empty>
0╬╝s           LoopContinue                                          <empty>
0╬╝s           GetName                    0000: 'i'                  1
0╬╝s           IncPost                                               1
0╬╝s           RotateRight                2                          2
1╬╝s           SetName                    0000: 'i'                  1
0╬╝s           Pop                                                   <empty>
0╬╝s           GetName                    0000: 'i'                  2
0╬╝s           PushInt8                   10                         10
0╬╝s           LessThan                                              true
0╬╝s           JumpIfFalse                92                         <empty>
2╬╝s           PushDeclarativeEnvironment 0, 1                       <empty>
0╬╝s           GetName                    0000: 'i'                  2
0╬╝s           PushInt8                   2                          2
0╬╝s           Eq                                                    true
0╬╝s           JumpIfFalse                86                         <empty>
1╬╝s           PushDeclarativeEnvironment 0, 0                       <empty>
0╬╝s           Jump                       92                         <empty>
2╬╝s           LoopEnd                                               <empty>
0╬╝s           PopEnvironment                                        <empty>

There seems to be a PopEnvironment compiled at 85 that looks like it's never reached. We can also fix the above example of by adding self.emit_opcode(Opcode::PopEnvironment); to compile_while_loop, but I'm not entirely sure how it works with other loops still, and I'm trying to find where the PopEnvironment is being emitted from. Hence the comment 😄

[HalidOdat]

boa/boa_engine/src/bytecompiler/mod.rs

Line 2181 in fdac8ec

self.pop_loop_control_info();

The problem is here where we handle breaks, every loop has a block statement (if it has { }) which emits a push and pop env instruction, but when we patch the breaks (which are inside the {}), we don't account for the unpopped environments, we jump past them.

The initial way I would solve this is by adding a Break instruction that would have two operands n (number of environments to be popped) and address, to jump after the environments popping. We would have to keep track of how many push environment instructions have been omitted for that loop.

If you have a better approach I'd be happy to hear :)

[nekevss]

I was sort of thinking that it was an issue with handling Break in the pop_loop_control_info (that or compile_block in general). I'm still kind of playing around with the ByteCompiler and the above sounds like the correct approach to me.

I hesitated earlier today when taking a look at this because I wasn't sure whether we want block to be adding a PopEnivonment that is never reached.