• 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
Name
Last commit
Last update
..
api Loading commit data...
asmjs Loading commit data...
ast Loading commit data...
base Loading commit data...
baseline Loading commit data...
builtins Loading commit data...
codegen Loading commit data...
common Loading commit data...
compiler Loading commit data...
compiler-dispatcher Loading commit data...
d8 Loading commit data...
date Loading commit data...
debug Loading commit data...
deoptimizer Loading commit data...
diagnostics Loading commit data...
execution Loading commit data...
extensions Loading commit data...
flags Loading commit data...
handles Loading commit data...
heap Loading commit data...
ic Loading commit data...
init Loading commit data...
inspector Loading commit data...
interpreter Loading commit data...
json Loading commit data...
libplatform Loading commit data...
libsampler Loading commit data...
logging Loading commit data...
numbers Loading commit data...
objects Loading commit data...
parsing Loading commit data...
profiler Loading commit data...
protobuf Loading commit data...
regexp Loading commit data...
roots Loading commit data...
runtime Loading commit data...
sanitizer Loading commit data...
snapshot Loading commit data...
strings Loading commit data...
tasks Loading commit data...
third_party Loading commit data...
torque Loading commit data...
tracing Loading commit data...
trap-handler Loading commit data...
utils Loading commit data...
wasm Loading commit data...
zone Loading commit data...
DEPS Loading commit data...
DIR_METADATA Loading commit data...
OWNERS Loading commit data...