• Benedikt Meurer's avatar
    [debug] Don't put a source position on internal `Return`s. · 06359f74
    Benedikt Meurer authored
    Be explicit about source positions for `Return`s in the
    BytecodeGenerator, and only do self-healing explicitly in the
    `ReturnStatement` translation, where an end position of
    `kNoSourcePosition` is turned into the return position of the
    function literal.
    
    This allows us to reason more easily about which `Return`s actually
    receive a meaningful source position, and in particular it allows us
    to construct the internal `Return`s for `yield` and `yield*` with no
    source position attached to them. Previously they'd get the source
    position for the implicit (final) return attached to it, which confused
    the debugger and led to breakpoints being set in the completely wrong
    spot.
    
    Considering the simplified example
    
    ```
    function* foo(){
      var a = 1;
    }
    ```
    
    this would previously generate the following bytecode
    
    ```
            0 : SwitchOnGeneratorState r0, [0], [1] { 0: @20 }
            4 : Mov <closure>, r2
            7 : Mov <this>, r3
     13 E> 10 : InvokeIntrinsic [_CreateJSGeneratorObject], r2-r3
           14 : Star0
     13 E> 15 : SuspendGenerator r0, r0-r1, [0]
           20 : ResumeGenerator r0, r0-r1
           24 : Star2
           25 : InvokeIntrinsic [_GeneratorGetResumeMode], r0-r0
           29 : SwitchOnSmiNoFeedback [1], [2], [0] { 0: @39, 1: @36 }
           33 : Ldar r2
     13 E> 35 : Throw
           36 : Ldar r2
     30 S> 38 : Return    <=========================== internal Return
     27 S> 39 : LdaSmi [1]
           41 : Star1
           42 : LdaUndefined
     30 S> 43 : Return
    ```
    
    where everything between offset 4 and 42 corresponds to the implicit
    yield at the beginning of every generator function, in particular the
    code between 20 and 42 corresponds to that initial yields resumption
    logic. Notice how the internal Return at offset 38 gets assigned the
    source position of the function literal (the same as the implicit
    return at the end). This confuses the debugger quite a bit when trying
    to set a breakpoint on the closing brace, since it's going in bytecode
    order and will thus discover the `Return` at offset 38 first (matching
    the source position 30 it's currently looking for) and setting the
    breakpoint there. This `Return` bytecode however is only executed when
    the generator is resumed via `GeneratorPrototype.return()`, and it'll
    not hit when the developer uses the generator normally, which is not
    the desired behavior and extremely confusing (especially since stepping
    on the other hand works as expected).
    
    With this patch, we no longer slap a source position (and in particular
    not the function literal's return position) onto these internal
    `Return`s as you can see from the generated bytecode below:
    
    ```
           0 : SwitchOnGeneratorState r0, [0], [1] { 0: @20 }
           4 : Mov <closure>, r2
           7 : Mov <this>, r3
    13 E> 10 : InvokeIntrinsic [_CreateJSGeneratorObject], r2-r3
          14 : Star0
    13 E> 15 : SuspendGenerator r0, r0-r1, [0]
          20 : ResumeGenerator r0, r0-r1
          24 : Star2
          25 : InvokeIntrinsic [_GeneratorGetResumeMode], r0-r0
          29 : SwitchOnSmiNoFeedback [1], [2], [0] { 0: @39, 1: @36 }
          33 : Ldar r2
    13 E> 35 : Throw
          36 : Ldar r2
          38 : Return
    27 S> 39 : LdaSmi [1]
          41 : Star1
          42 : LdaUndefined
    30 S> 43 : Return
    ```
    
    This also allows us to remove the break position finding hack that was
    kept in BreakIterator::BreakIndexFromPosition() for generators and
    modules.
    
    Fixed: chromium:901819
    Change-Id: If19a6b26e2622d49b6b5e54bf7a162747543f970
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2727820Reviewed-by: 's avatarYang Guo <yangguo@chromium.org>
    Reviewed-by: 's avatarLeszek Swirski <leszeks@chromium.org>
    Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#73119}
    06359f74
bytecode-array-builder.h 28.8 KB