From d8990fdc764a1967b277b0b0e1d620a75381f45d Mon Sep 17 00:00:00 2001 From: Benedikt Meurer Date: Wed, 28 Sep 2022 11:07:29 +0200 Subject: [PATCH] [debug] Remove statement position from spreads in array literals. Following up on https://crrev.com/c/3916453, we also remove the confusing breakable and steppable positions from spreads in array literals. These positions provide no meaningful advdantage for developers, but just makes it annoying to step through code that contains spreads. Drive-by: Add similar inspector tests to ensure that the positions in the stack are correctly inferred when stopped in the Symbol.iterator or the next methods. Before: https://imgur.com/jVf2JeB.png After: https://imgur.com/u8SfNhy.png Fixed: chromium:1368971 Change-Id: Ibf791167936c1ed28ac3240acb7c0846b11ebecb Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3925200 Auto-Submit: Benedikt Meurer Commit-Queue: Leszek Swirski Commit-Queue: Benedikt Meurer Reviewed-by: Leszek Swirski Cr-Commit-Position: refs/heads/main@{#83469} --- src/interpreter/bytecode-generator.cc | 3 +- test/debugger/debug/es6/debug-stepnext-for.js | 106 +++++++++++++++--- .../debugger/array-spread-expected.txt | 75 +++++++++++++ test/inspector/debugger/array-spread.js | 90 +++++++++++++++ .../ArrayLiterals.golden | 2 +- .../CallAndSpread.golden | 2 +- .../bytecode_expectations/NewAndSpread.golden | 2 +- .../SuperCallAndSpread.golden | 2 +- 8 files changed, 260 insertions(+), 22 deletions(-) create mode 100644 test/inspector/debugger/array-spread-expected.txt create mode 100644 test/inspector/debugger/array-spread.js diff --git a/src/interpreter/bytecode-generator.cc b/src/interpreter/bytecode-generator.cc index d85bdbffd2..96e0aa2a1c 100644 --- a/src/interpreter/bytecode-generator.cc +++ b/src/interpreter/bytecode-generator.cc @@ -3484,8 +3484,7 @@ void BytecodeGenerator::BuildCreateArrayLiteral( Expression* subexpr = *current; if (subexpr->IsSpread()) { RegisterAllocationScope scope(this); - builder()->SetExpressionAsStatementPosition( - subexpr->AsSpread()->expression()); + builder()->SetExpressionPosition(subexpr->AsSpread()->expression()); VisitForAccumulatorValue(subexpr->AsSpread()->expression()); builder()->SetExpressionPosition(subexpr->AsSpread()->expression()); IteratorRecord iterator = BuildGetIteratorRecord(IteratorType::kNormal); diff --git a/test/debugger/debug/es6/debug-stepnext-for.js b/test/debugger/debug/es6/debug-stepnext-for.js index c3c68423cd..b6c93fc974 100644 --- a/test/debugger/debug/es6/debug-stepnext-for.js +++ b/test/debugger/debug/es6/debug-stepnext-for.js @@ -98,35 +98,109 @@ print("log:\n"+ JSON.stringify(log)); // based on other values. var expected = [ // Entry - "a2", + 'a2', // Empty for-in-var: get enumerable - "c16", + 'c16', // Empty for-in: get enumerable - "d12", + 'd12', // For-in-var: get enumerable, assign, body, assign, body, ... - "e16","e11","E4","e11","E4","e11","E4","e11", + 'e16', + 'e11', + 'E4', + 'e11', + 'E4', + 'e11', + 'E4', + 'e11', // For-in: get enumerable, assign, body, assign, body, ... - "f12","f7","F4","f7","F4","f7","F4","f7", + 'f12', + 'f7', + 'F4', + 'f7', + 'F4', + 'f7', + 'F4', + 'f7', // For-in-let: get enumerable, next, body, next, ... - "g16","g11","G4","g11","G4","g11","G4","g11", + 'g16', + 'g11', + 'G4', + 'g11', + 'G4', + 'g11', + 'G4', + 'g11', // For-of-var: [Symbol.iterator](), next(), body, next(), body, ... - "h16","h11","H4","h11","H4","h11","H4","h11", + 'h16', + 'h11', + 'H4', + 'h11', + 'H4', + 'h11', + 'H4', + 'h11', // For-of: [Symbol.iterator](), next(), body, next(), body, ... - "i12","i7","I4","i7","I4","i7","I4","i7", + 'i12', + 'i7', + 'I4', + 'i7', + 'I4', + 'i7', + 'I4', + 'i7', // For-of-let: [Symbol.iterator](), next(), body, next(), ... - "j18","j11","J4","j11","J4","j11","J4","j11", + 'j18', + 'j11', + 'J4', + 'j11', + 'J4', + 'j11', + 'J4', + 'j11', // For-var: init, condition, body, next, condition, body, ... - "k15","k20","K4","k26","k20","K4","k26","k20","K4","k26","k20", + 'k15', + 'k20', + 'K4', + 'k26', + 'k20', + 'K4', + 'k26', + 'k20', + 'K4', + 'k26', + 'k20', // For: init, condition, body, next, condition, body, ... - "l7","l16","L4","l22","l16","L4","l22","l16","L4","l22","l16", + 'l7', + 'l16', + 'L4', + 'l22', + 'l16', + 'L4', + 'l22', + 'l16', + 'L4', + 'l22', + 'l16', // For-let: init, condition, body, next, condition, body, ... - "m15","m20","M4","m26","m20","M4","m26","m20","M4","m26","m20", + 'm15', + 'm20', + 'M4', + 'm26', + 'm20', + 'M4', + 'm26', + 'm20', + 'M4', + 'm26', + 'm20', // For-of, empty: [Symbol.iterator](), next() once - "n16", "n11", - // Spread: expression statement, spread - "o2", "o9", + 'n16', + 'n11', + // Spread: expression statement + 'o2', // Exit. - "y0","z0", + 'y0', + 'z0', ] print("expected:\n"+ JSON.stringify(expected)); diff --git a/test/inspector/debugger/array-spread-expected.txt b/test/inspector/debugger/array-spread-expected.txt new file mode 100644 index 0000000000..948974885c --- /dev/null +++ b/test/inspector/debugger/array-spread-expected.txt @@ -0,0 +1,75 @@ +Tests breakable locations in array spread. + +Running test: testBreakLocations + +function testFunction() { + var a = |_|[...iterable]; + var b = [...|_|a, ...iterable, ...a]; +|R|} + +const iterable = |_|{ + [Symbol.iterator]() { + const it = |_|[1, 2].|C|values(); + |_|return {next() { |_|return it.|C|next();|R| }};|R| + } +}; +|R| + + +Running test: testStepping +Execution paused in testFunction: +function testFunction() { + var a = #[...iterable]; + var b = [...a, ...iterable, ...a]; + +Execution paused in [Symbol.iterator]: + [Symbol.iterator]() { + const it = #[1, 2].values(); + return {next() { return it.next(); }}; + +Called from testFunction: +function testFunction() { + var a = [...#iterable]; + var b = [...a, ...iterable, ...a]; + +Execution paused in next: + const it = [1, 2].values(); + return {next() { #return it.next(); }}; + } + +Called from testFunction: +function testFunction() { + var a = [...#iterable]; + var b = [...a, ...iterable, ...a]; + +Execution paused in testFunction: + var a = [...iterable]; + var b = [...#a, ...iterable, ...a]; +} + +Execution paused in [Symbol.iterator]: + [Symbol.iterator]() { + const it = #[1, 2].values(); + return {next() { return it.next(); }}; + +Called from testFunction: + var a = [...iterable]; + var b = [...a, ...#iterable, ...a]; +} + +Execution paused in next: + const it = [1, 2].values(); + return {next() { #return it.next(); }}; + } + +Called from testFunction: + var a = [...iterable]; + var b = [...a, ...#iterable, ...a]; +} + +Execution paused in testFunction: + var b = [...a, ...iterable, ...a]; +#} + + +Resuming and finishing... diff --git a/test/inspector/debugger/array-spread.js b/test/inspector/debugger/array-spread.js new file mode 100644 index 0000000000..ce4d5d3589 --- /dev/null +++ b/test/inspector/debugger/array-spread.js @@ -0,0 +1,90 @@ +// Copyright 2022 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. + +let {session, contextGroup, Protocol} = + InspectorTest.start('Tests breakable locations in array spread.'); + +const source = ` +function testFunction() { + var a = [...iterable]; + var b = [...a, ...iterable, ...a]; +} + +const iterable = { + [Symbol.iterator]() { + const it = [1, 2].values(); + return {next() { return it.next(); }}; + } +}; +`; + +const url = 'test.js'; +contextGroup.addScript(source, 0, 0, url); +session.setupScriptMap(); + +InspectorTest.runAsyncTestSuite([ + async function testBreakLocations() { + let [, , {params: {scriptId}}] = await Promise.all([ + Protocol.Runtime.enable(), + Protocol.Debugger.enable(), + Protocol.Debugger.onceScriptParsed(), + ]); + let {result: {locations}} = await Protocol.Debugger.getPossibleBreakpoints( + {start: {lineNumber: 0, columnNumber: 0, scriptId}}); + await session.logBreakLocations(locations); + await Promise.all([ + Protocol.Debugger.disable(), + Protocol.Runtime.disable(), + ]); + }, + + async function testStepping() { + let [, , {params: {scriptId}}] = await Promise.all([ + Protocol.Runtime.enable(), + Protocol.Debugger.enable(), + Protocol.Debugger.onceScriptParsed(), + ]); + const {breakpointId} = await Protocol.Debugger.setBreakpoint({ + location: { + scriptId, + lineNumber: 2, + } + }); + const evalPromise = + Protocol.Runtime.evaluate({expression: 'testFunction()'}); + for (;;) { + const {method, params} = await Promise.race([ + evalPromise, + Protocol.Debugger.oncePaused(), + ]); + if (method !== 'Debugger.paused') { + break; + } + const callFrames = params.callFrames.filter( + callFrame => callFrame.location.scriptId === scriptId); + if (callFrames.length === 0) { + InspectorTest.log('Resuming and finishing...'); + await Protocol.Debugger.resume(); + } else { + const [{functionName, location}, ...callerFrames] = callFrames; + InspectorTest.log(`Execution paused in ${functionName}:`); + await session.logSourceLocation(location); + for (const {location, functionName} of callerFrames) { + InspectorTest.log(`Called from ${functionName}:`); + await session.logSourceLocation(location); + } + if (functionName === 'testFunction') { + await Protocol.Debugger.stepInto(); + } else { + await Protocol.Debugger.stepOut(); + } + } + } + await Promise.all([ + Protocol.Debugger.removeBreakpoint({breakpointId}), + Protocol.Debugger.disable(), + Protocol.Runtime.disable(), + ]); + } +]); diff --git a/test/unittests/interpreter/bytecode_expectations/ArrayLiterals.golden b/test/unittests/interpreter/bytecode_expectations/ArrayLiterals.golden index 28eb0aaa8b..c93d81af30 100644 --- a/test/unittests/interpreter/bytecode_expectations/ArrayLiterals.golden +++ b/test/unittests/interpreter/bytecode_expectations/ArrayLiterals.golden @@ -145,7 +145,7 @@ bytecodes: [ /* 52 S> */ B(CreateArrayLiteral), U8(1), U8(1), U8(37), B(Star1), B(LdaSmi), I8(1), - /* 67 S> */ B(Star2), + B(Star2), /* 67 E> */ B(GetIterator), R(0), U8(2), U8(4), B(Star4), B(GetNamedProperty), R(4), U8(2), U8(6), diff --git a/test/unittests/interpreter/bytecode_expectations/CallAndSpread.golden b/test/unittests/interpreter/bytecode_expectations/CallAndSpread.golden index 16ca804e77..cfcf6aa5f3 100644 --- a/test/unittests/interpreter/bytecode_expectations/CallAndSpread.golden +++ b/test/unittests/interpreter/bytecode_expectations/CallAndSpread.golden @@ -75,7 +75,7 @@ bytecodes: [ B(Star2), B(LdaSmi), I8(1), B(Star3), - /* 49 S> */ B(CreateArrayLiteral), U8(3), U8(5), U8(37), + /* 49 E> */ B(CreateArrayLiteral), U8(3), U8(5), U8(37), B(Star6), /* 49 E> */ B(GetIterator), R(6), U8(6), U8(8), B(Star5), diff --git a/test/unittests/interpreter/bytecode_expectations/NewAndSpread.golden b/test/unittests/interpreter/bytecode_expectations/NewAndSpread.golden index 07182e59a7..46a4f12b03 100644 --- a/test/unittests/interpreter/bytecode_expectations/NewAndSpread.golden +++ b/test/unittests/interpreter/bytecode_expectations/NewAndSpread.golden @@ -106,7 +106,7 @@ bytecodes: [ B(Star2), B(LdaSmi), I8(1), B(Star3), - /* 101 S> */ B(CreateArrayLiteral), U8(4), U8(1), U8(37), + /* 101 E> */ B(CreateArrayLiteral), U8(4), U8(1), U8(37), B(Star6), /* 101 E> */ B(GetIterator), R(6), U8(2), U8(4), B(Star5), diff --git a/test/unittests/interpreter/bytecode_expectations/SuperCallAndSpread.golden b/test/unittests/interpreter/bytecode_expectations/SuperCallAndSpread.golden index 568f777024..3ed6406af6 100644 --- a/test/unittests/interpreter/bytecode_expectations/SuperCallAndSpread.golden +++ b/test/unittests/interpreter/bytecode_expectations/SuperCallAndSpread.golden @@ -102,7 +102,7 @@ bytecodes: [ /* 140 S> */ B(CreateArrayLiteral), U8(0), U8(0), U8(37), B(Star6), B(LdaSmi), I8(1), - /* 152 S> */ B(Star7), + B(Star7), /* 152 E> */ B(GetIterator), R(3), U8(1), U8(3), B(Star9), B(GetNamedProperty), R(9), U8(1), U8(5),