[debugger] Don't attach source positions to implicit returns.

Previously we'd attach source positions to implicit returns that are
generated when leaving an async function with a promise rejection. This
was due to the use of `kNoSourcePosition` on the `end_position` in the
`ReturnStatement` nodes as indicator to pick the return position from
the function literal, instead of really not putting a source position on
that specific `Return` bytecode.

This CL adds a dedicated marker to `ReturnStatement` to express that the
`BytecodeGenerator` should put the return position from the function
literal there instead of overloading the meaning of `kNoSourcePosition`.

Bug: chromium:901819, chromium:782461
Fixed: chromium:1199919, chromium:1201706
Change-Id: I3647e0c3d711e9c3d6ae44606b70ec92ad82e1cf
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2859945
Auto-Submit: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74301}

src/ast/ast.h

@@ -629,6 +629,11 @@ class ReturnStatement final : public JumpStatement {
     return type() == kSyntheticAsyncReturn;
   }
 
+  // This constant is used to indicate that the return position
+  // from the FunctionLiteral should be used when emitting code.
+  static constexpr int kFunctionLiteralReturnPosition = -2;
+  STATIC_ASSERT(kFunctionLiteralReturnPosition == kNoSourcePosition - 1);
+
   int end_position() const { return end_position_; }
 
  private:
@@ -2857,20 +2862,22 @@ class AstNodeFactory final {
     return zone_->New<BreakStatement>(target, pos);
   }
 
-  ReturnStatement* NewReturnStatement(Expression* expression, int pos,
-                                      int end_position = kNoSourcePosition) {
+  ReturnStatement* NewReturnStatement(
+      Expression* expression, int pos,
+      int end_position = ReturnStatement::kFunctionLiteralReturnPosition) {
     return zone_->New<ReturnStatement>(expression, ReturnStatement::kNormal,
                                        pos, end_position);
   }
 
-  ReturnStatement* NewAsyncReturnStatement(
-      Expression* expression, int pos, int end_position = kNoSourcePosition) {
+  ReturnStatement* NewAsyncReturnStatement(Expression* expression, int pos,
+                                           int end_position) {
     return zone_->New<ReturnStatement>(
         expression, ReturnStatement::kAsyncReturn, pos, end_position);
   }
 
   ReturnStatement* NewSyntheticAsyncReturnStatement(
-      Expression* expression, int pos, int end_position = kNoSourcePosition) {
+      Expression* expression, int pos,
+      int end_position = ReturnStatement::kFunctionLiteralReturnPosition) {
     return zone_->New<ReturnStatement>(
         expression, ReturnStatement::kSyntheticAsyncReturn, pos, end_position);
   }

src/interpreter/bytecode-generator.cc

@@ -6,6 +6,7 @@
 
 #include "src/api/api-inl.h"
 #include "src/ast/ast-source-ranges.h"
+#include "src/ast/ast.h"
 #include "src/ast/scopes.h"
 #include "src/builtins/builtins-constructor.h"
 #include "src/codegen/compiler.h"
@@ -1774,7 +1775,7 @@ void BytecodeGenerator::VisitReturnStatement(ReturnStatement* stmt) {
   builder()->SetStatementPosition(stmt);
   VisitForAccumulatorValue(stmt->expression());
   int return_position = stmt->end_position();
-  if (return_position == kNoSourcePosition) {
+  if (return_position == ReturnStatement::kFunctionLiteralReturnPosition) {
     return_position = info()->literal()->return_position();
   }
   if (stmt->is_async_return()) {

src/parsing/parser-base.h

@@ -1452,8 +1452,9 @@ class ParserBase {
 
   // Convenience method which determines the type of return statement to emit
   // depending on the current function type.
-  inline StatementT BuildReturnStatement(ExpressionT expr, int pos,
-                                         int end_pos = kNoSourcePosition) {
+  inline StatementT BuildReturnStatement(
+      ExpressionT expr, int pos,
+      int end_pos = ReturnStatement::kFunctionLiteralReturnPosition) {
     if (impl()->IsNull(expr)) {
       expr = factory()->NewUndefinedLiteral(kNoSourcePosition);
     } else if (is_async_generator()) {

src/parsing/parser.cc

@@ -751,8 +751,8 @@ void Parser::ParseWrapped(Isolate* isolate, ParseInfo* info,
       kNoSourcePosition, FunctionSyntaxKind::kWrapped, LanguageMode::kSloppy,
       arguments_for_wrapped_function);
 
-  Statement* return_statement = factory()->NewReturnStatement(
-      function_literal, kNoSourcePosition, kNoSourcePosition);
+  Statement* return_statement =
+      factory()->NewReturnStatement(function_literal, kNoSourcePosition);
   body->Add(return_statement);
 }
 
@@ -2011,8 +2011,8 @@ void Parser::ParseAndRewriteAsyncGeneratorFunctionBody(
 
     Expression* reject_call = factory()->NewCallRuntime(
         Runtime::kInlineAsyncGeneratorReject, reject_args, kNoSourcePosition);
-    catch_block = IgnoreCompletion(
-        factory()->NewReturnStatement(reject_call, kNoSourcePosition));
+    catch_block = IgnoreCompletion(factory()->NewReturnStatement(
+        reject_call, kNoSourcePosition, kNoSourcePosition));
   }
 
   {
@@ -2878,8 +2878,8 @@ Block* Parser::BuildRejectPromiseOnException(Block* inner_block,
     reject_promise = factory()->NewCallRuntime(
         Runtime::kInlineAsyncFunctionReject, args, kNoSourcePosition);
   }
-  Block* catch_block = IgnoreCompletion(
-      factory()->NewReturnStatement(reject_promise, kNoSourcePosition));
+  Block* catch_block = IgnoreCompletion(factory()->NewReturnStatement(
+      reject_promise, kNoSourcePosition, kNoSourcePosition));
 
   // Treat the exception for REPL mode scripts as UNCAUGHT. This will
   // keep the corresponding JSMessageObject alive on the Isolate. The

test/cctest/interpreter/bytecode_expectations/AsyncModules.golden

@@ -67,7 +67,7 @@ bytecodes: [
                 B(Star6),
                 B(Mov), R(0), R(4),
                 B(InvokeIntrinsic), U8(Runtime::k_AsyncFunctionReject), R(4), U8(3),
-  /*   10 S> */ B(Return),
+                B(Return),
 ]
 constant pool: [
   Smi [20],
@@ -142,7 +142,7 @@ bytecodes: [
                 B(Star6),
                 B(Mov), R(0), R(4),
                 B(InvokeIntrinsic), U8(Runtime::k_AsyncFunctionReject), R(4), U8(3),
-  /*   21 S> */ B(Return),
+                B(Return),
 ]
 constant pool: [
   Smi [20],
@@ -223,7 +223,7 @@ bytecodes: [
                 B(Star7),
                 B(Mov), R(0), R(5),
                 B(InvokeIntrinsic), U8(Runtime::k_AsyncFunctionReject), R(5), U8(3),
-  /*   54 S> */ B(Return),
+                B(Return),
 ]
 constant pool: [
   Smi [27],
@@ -305,7 +305,7 @@ bytecodes: [
                 B(Star7),
                 B(Mov), R(0), R(5),
                 B(InvokeIntrinsic), U8(Runtime::k_AsyncFunctionReject), R(5), U8(3),
-  /*   49 S> */ B(Return),
+                B(Return),
 ]
 constant pool: [
   Smi [30],

test/cctest/interpreter/bytecode_expectations/ForAwaitOf.golden

@@ -141,7 +141,7 @@ bytecodes: [
                 B(Star8),
                 B(Mov), R(0), R(6),
                 B(InvokeIntrinsic), U8(Runtime::k_AsyncFunctionReject), R(6), U8(3),
-  /*   57 S> */ B(Return),
+                B(Return),
 ]
 constant pool: [
   Smi [85],
@@ -303,7 +303,7 @@ bytecodes: [
                 B(Star8),
                 B(Mov), R(0), R(6),
                 B(InvokeIntrinsic), U8(Runtime::k_AsyncFunctionReject), R(6), U8(3),
-  /*   68 S> */ B(Return),
+                B(Return),
 ]
 constant pool: [
   Smi [85],
@@ -469,7 +469,7 @@ bytecodes: [
                 B(Star8),
                 B(Mov), R(0), R(6),
                 B(InvokeIntrinsic), U8(Runtime::k_AsyncFunctionReject), R(6), U8(3),
-  /*  114 S> */ B(Return),
+                B(Return),
 ]
 constant pool: [
   Smi [85],
@@ -599,7 +599,7 @@ bytecodes: [
                 B(Star6),
                 B(Mov), R(0), R(4),
                 B(InvokeIntrinsic), U8(Runtime::k_AsyncFunctionReject), R(4), U8(3),
-  /*   96 S> */ B(Return),
+                B(Return),
 ]
 constant pool: [
   OBJECT_BOILERPLATE_DESCRIPTION_TYPE,

test/cctest/interpreter/bytecode_expectations/ForOfLoop.golden

@@ -726,7 +726,7 @@ bytecodes: [
                 B(Star9),
                 B(Mov), R(0), R(7),
                 B(InvokeIntrinsic), U8(Runtime::k_AsyncFunctionReject), R(7), U8(3),
-  /*   60 S> */ B(Return),
+                B(Return),
 ]
 constant pool: [
   ONE_BYTE_INTERNALIZED_STRING_TYPE ["next"],
@@ -850,7 +850,7 @@ bytecodes: [
                 B(Star8),
                 B(Mov), R(0), R(6),
                 B(InvokeIntrinsic), U8(Runtime::k_AsyncFunctionReject), R(6), U8(3),
-  /*   54 S> */ B(Return),
+                B(Return),
 ]
 constant pool: [
   Smi [88],

test/cctest/interpreter/bytecode_expectations/StandardForLoop.golden

@@ -394,7 +394,7 @@ bytecodes: [
                 B(Star7),
                 B(Mov), R(0), R(5),
                 B(InvokeIntrinsic), U8(Runtime::k_AsyncFunctionReject), R(5), U8(3),
-  /*   67 S> */ B(Return),
+                B(Return),
 ]
 constant pool: [
   SCOPE_INFO_TYPE,
@@ -462,7 +462,7 @@ bytecodes: [
                 B(Star6),
                 B(Mov), R(0), R(4),
                 B(InvokeIntrinsic), U8(Runtime::k_AsyncFunctionReject), R(4), U8(3),
-  /*   61 S> */ B(Return),
+                B(Return),
 ]
 constant pool: [
   Smi [42],

test/inspector/debugger/break-locations-await-expected.txt

@@ -6,7 +6,7 @@ function testFunction() {
   async function f1() {
     for (let x = |_|0; x |_|< 1; ++|_|x) |_|await x;
     |_|return |_|await Promise.|C|resolve(2);|R|
-  |R|}
+  }
 
   async function f2() {
     let r = |_|await |C|f1() + |_|await |C|f1();
@@ -17,7 +17,7 @@ function testFunction() {
     let p = |_|Promise.|C|resolve(42);
     |_|await p;
     |_|return r;|R|
-  |R|}
+  }
 
   return |C|f2();|R|
 }

test/inspector/debugger/break-locations-var-init-expected.txt

@@ -21,7 +21,7 @@ function testFunction() {
   |_|(async function asyncF() {
     let r = |_|await Promise.|C|resolve(42);
     |_|return r;|R|
-  |R|})|C|();
+  })|C|();
   |_|return promise;|R|
 }
 

test/inspector/debugger/get-possible-breakpoints-master-expected.txt

@@ -236,7 +236,7 @@ async function testPromiseAsyncWithCode() {
   |R|}
   |C|main();
   |_|return testPromise;|R|
-|R|}
+}
 
 function returnFunction() {
   |_|return returnObject;|R|
@@ -249,7 +249,7 @@ async function testPromiseComplex() {
     async function foo() {
       |_|await Promise.|C|resolve();
       |_|return 42;|R|
-    |R|}
+    }
     var x = |_|1;
     var y = |_|2;
     |C|returnFunction(|C|emptyFunction(), x++, --y, x => 2 |_|* x|R|, |C|returnCall())|C|().a = |_|await |C|foo((a => 2 |_|*a|R|)|C|(5));
@@ -257,7 +257,7 @@ async function testPromiseComplex() {
   |R|}
   |C|main();
   |_|return testPromise;|R|
-|R|}
+}
 
 function twiceDefined() {
   |_|return a + b;|R|

test/inspector/regress/regress-crbug-1199919-expected.txt

+Regression test for crbug/1199919
+
+Running test: testDefaultParameter
+defaultParameter (v8://test.js:2:2)
+(anonymous) (:0:0)
+
+Running test: testDestructuringParameter
+destructuringParameter (v8://test.js:6:2)
+(anonymous) (:0:0)

test/inspector/regress/regress-crbug-1199919.js

+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+const {session, contextGroup, Protocol} =
+  InspectorTest.start('Regression test for crbug/1199919');
+
+const source = `
+async function defaultParameter(x = 1) {
+  return x;
+}
+
+async function destructuringParameter({x}) {
+  return x;
+}
+`;
+const url = 'v8://test.js';
+
+contextGroup.addScript(source, 0, 0, url);
+session.setupScriptMap();
+
+InspectorTest.runAsyncTestSuite([
+  async function testDefaultParameter() {
+    await Promise.all([Protocol.Runtime.enable(), Protocol.Debugger.enable()]);
+    const {result: {breakpointId}} = await Protocol.Debugger.setBreakpointByUrl({lineNumber: 2, url});
+    const evalPromise = Protocol.Runtime.evaluate({expression: 'defaultParameter()'});
+    const {params: {callFrames}} = await Protocol.Debugger.oncePaused();
+    session.logCallFrames(callFrames);
+    await Protocol.Debugger.removeBreakpoint({breakpointId});
+    await Promise.all([Protocol.Debugger.resume(), evalPromise]);
+    await Promise.all([Protocol.Runtime.disable(), Protocol.Debugger.disable()]);
+  },
+
+  async function testDestructuringParameter() {
+    await Promise.all([Protocol.Runtime.enable(), Protocol.Debugger.enable()]);
+    const {result: {breakpointId}} = await Protocol.Debugger.setBreakpointByUrl({lineNumber: 6, url});
+    const evalPromise = Protocol.Runtime.evaluate({expression: 'destructuringParameter({x: 5})'});
+    const {params: {callFrames}} = await Protocol.Debugger.oncePaused();
+    session.logCallFrames(callFrames);
+    await Protocol.Debugger.removeBreakpoint({breakpointId});
+    await Promise.all([Protocol.Debugger.resume(), evalPromise]);
+    await Promise.all([Protocol.Runtime.disable(), Protocol.Debugger.disable()]);
+  }
+]);